Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

virtqueue: add more virtqueue apis for the virtio device side #631

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

virtqueue: add more virtqueue apis for the virtio device side #631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions lib/include/openamp/virtqueue.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,21 @@ struct virtqueue_buf {
int len;
};

/** @brief Virtqueue buffers descriptor. */
struct virtqueue_bufs {
/** The descriptor index of the first available buffer */
uint16_t head;

/** The capacity of the virtqueue buffers */
unsigned int vb_capacity;

/** The real number of the virtqueue buffers */
unsigned int vb_num;

/** The virtqueue buffers */
struct virtqueue_buf vb[0];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have an issue here if we have to know the number of elements.
If the virtio device does not know it, it will lead to memory issues.
One solution would be to define a static table with the number of elements equal to the number of descriptors, but that would introduce extra data and would not simplify the code for the user.

So the scatter-gather approach I suggested seem not reliable and does not go in a right direction. My apologizes for the time spend on this.

I do not have better proposal ,so let's come back to the add of the virtqueue_get_next_avail_buffer API.
Except is someone else as another suggestion.
I tested it with a virtio -I2C device that chain several buffers and it works.

};

/** @brief Vring descriptor extra information for buffer list management. */
struct vq_desc_extra {
/** Pointer to first descriptor. */
Expand Down Expand Up @@ -289,8 +304,20 @@ void *virtqueue_get_buffer(struct virtqueue *vq, uint32_t *len, uint16_t *idx);
*
* @return Pointer to available buffer
*/
void *virtqueue_get_available_buffer(struct virtqueue *vq, uint16_t *avail_idx,
uint32_t *len);
void *virtqueue_get_first_avail_buffer(struct virtqueue *vq, uint16_t *avail_idx,
uint32_t *len);

/**
* @internal
*
* @brief Returns buffer available for use in the VirtIO queue
*
* @param vq Pointer to VirtIO queue control block
* @param vbs Pointer to virtqueue buffers to store available buffers
*
* @return Function status
*/
int virtqueue_get_available_buffer(struct virtqueue *vq, struct virtqueue_bufs *vbs);

/**
* @internal
Expand Down
4 changes: 2 additions & 2 deletions lib/rpmsg/rpmsg_virtio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ static void *rpmsg_virtio_get_tx_buffer(struct rpmsg_virtio_device *rvdev,
*idx = 0;
}
} else if (VIRTIO_ROLE_IS_DEVICE(rvdev->vdev)) {
data = virtqueue_get_available_buffer(rvdev->svq, idx, len);
data = virtqueue_get_first_avail_buffer(rvdev->svq, idx, len);
}

return data;
Expand Down Expand Up @@ -235,7 +235,7 @@ static void *rpmsg_virtio_get_rx_buffer(struct rpmsg_virtio_device *rvdev,

if (VIRTIO_ROLE_IS_DEVICE(rvdev->vdev)) {
data =
virtqueue_get_available_buffer(rvdev->rvq, idx, len);
virtqueue_get_first_avail_buffer(rvdev->rvq, idx, len);
}

/* Invalidate the buffer before returning it */
Expand Down
62 changes: 60 additions & 2 deletions lib/virtio/virtqueue.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ static int vq_ring_must_notify(struct virtqueue *vq);
static void vq_ring_notify(struct virtqueue *vq);
static int virtqueue_nused(struct virtqueue *vq);
static int virtqueue_navail(struct virtqueue *vq);
static void *virtqueue_get_next_avail_buffer(struct virtqueue *vq, uint16_t idx,
uint16_t *next_idx, uint32_t *next_len);

/* Default implementation of P2V based on libmetal */
static inline void *virtqueue_phys_to_virt(struct virtqueue *vq,
Expand Down Expand Up @@ -200,8 +202,8 @@ void virtqueue_free(struct virtqueue *vq)
}
}

void *virtqueue_get_available_buffer(struct virtqueue *vq, uint16_t *avail_idx,
uint32_t *len)
void *virtqueue_get_first_avail_buffer(struct virtqueue *vq, uint16_t *avail_idx,
uint32_t *len)
{
uint16_t head_idx = 0;
void *buffer;
Expand Down Expand Up @@ -231,6 +233,40 @@ void *virtqueue_get_available_buffer(struct virtqueue *vq, uint16_t *avail_idx,
return buffer;
}

int virtqueue_get_available_buffer(struct virtqueue *vq, struct virtqueue_bufs *vbs)
{
unsigned int i;
uint16_t head;
uint16_t idx;
uint32_t len;
void *buf;

buf = virtqueue_get_first_avail_buffer(vq, &head, &len);
if (!buf)
return ERROR_VRING_NO_BUFF;

vbs->head = head;
vbs->vb[0].buf = buf;
vbs->vb[0].len = len;

for (i = 1, idx = head; ; i++) {
buf = virtqueue_get_next_avail_buffer(vq, idx, &idx, &len);
if (!buf)
break;
else if (i >= vbs->vb_capacity) {
metal_log(METAL_LOG_ERROR, "capacity %u is not enough\n",
vbs->vb_capacity);
return ERROR_VQUEUE_INVLD_PARAM;
}

vbs->vb[i].buf = buf;
vbs->vb[i].len = len;
}

vbs->vb_num = i;
return 0;
}

int virtqueue_add_consumed_buffer(struct virtqueue *vq, uint16_t head_idx,
uint32_t len)
{
Expand Down Expand Up @@ -697,3 +733,25 @@ static int virtqueue_navail(struct virtqueue *vq)

return navail;
}

static void *virtqueue_get_next_avail_buffer(struct virtqueue *vq, uint16_t idx,
uint16_t *next_idx, uint32_t *next_len)
{
void *buffer;
uint16_t next;

VRING_INVALIDATE(vq->vq_ring.desc[idx], sizeof(struct vring_desc));
if (!(vq->vq_ring.desc[idx].flags & VRING_DESC_F_NEXT))
return NULL;

next = vq->vq_ring.desc[idx].next;
if (next_idx)
*next_idx = next;

VRING_INVALIDATE(vq->vq_ring.desc[next], sizeof(struct vring_desc));
buffer = virtqueue_phys_to_virt(vq, vq->vq_ring.desc[next].addr);
if (next_len)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, I would add a test on next_len to return an error if it is null. Having a buffer without its length seems dangerous to me.

*next_len = vq->vq_ring.desc[next].len;

return buffer;
}
Loading