virtqueue: add more virtqueue apis for the virtio device side #631
Conversation
CV-Bowen
force-pushed
the
virtqueue_next_buffer
branch
3 times, most recently
from
7740f3e to
78f2a0f
Compare
|
@CV-Bowen : We usually wait that the CI is pass before having a look at a PR. So for the next time please ensure that CI has passed. That said, an API that allows getting several buffers would seem more generic and flexible than one that gets the next buffer. What about: int virtqueue_get_available_buffer(struct virtqueue *vq, struct vq_buff *buff, unsigned int needed) |
CV-Bowen
force-pushed
the
virtqueue_next_buffer
branch
from
78f2a0f to
cdb4e01
Compare
|
@arnopo Yes, this is what we want to do and I have updated this PR to add a new API: |
CV-Bowen
force-pushed
the
virtqueue_next_buffer
branch
from
cdb4e01 to
76bc7f2
Compare
CV-Bowen
changed the title
lib/virtio/virtqueue.c
Outdated
| @@ -231,6 +231,63 @@ void *virtqueue_get_available_buffer(struct virtqueue *vq, uint16_t *avail_idx, | |||
| return buffer; | |||
| } | |||
|
|
|||
| void *virtqueue_get_next_avail_buffer(struct virtqueue *vq, uint16_t idx, | |||
There was a problem hiding this comment.
do you still need this one? virtqueue_get_available_buffers should replace this function
lib/virtio/virtqueue.c
Outdated
| } | ||
|
|
||
| int virtqueue_get_available_buffers(struct virtqueue *vq, | ||
| struct virtqueue_buf *vb, int vbsize, |
There was a problem hiding this comment.
as mentioned in my previous comment you should return a struct vq_buf that contain the address, the size and the index of the needed buffers instead of just returning the buffer
|
This pull request has been marked as a stale pull request because it has been open (more than) 45 days with no activity. |
In virtio device side, we always need to get the next avaiable buffer based on current buffer index. So add these two APIs for convinience use. For example, virtio blk driver origanize the buffer: +----------+ | Reqeust | (Flags: Read | Next) +----------+ | Buffer | (Flags: Read/Write | Next) +----------+ | Response | (Flags: Write) +----------+ For the virtio blk device size, we need get the Buffer and Response buffer based on the Request buffer index. So modify the virtqueue_get_available_buffer() to support get more than one available buffer. Signed-off-by: Bowen Wang <[email protected]> Signed-off-by: Yongrong Wang <[email protected]>
CV-Bowen
force-pushed
the
virtqueue_next_buffer
branch
from
76bc7f2 to
1cb110f
Compare
|
@arnopo Sorry for later response, I updated this PR according to your suggestions. |
| unsigned int vb_num; | ||
|
|
||
| /** The virtqueue buffers */ | ||
| struct virtqueue_buf vb[0]; |
There was a problem hiding this comment.
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.
|
|
||
| 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) |
There was a problem hiding this comment.
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.
In virtio device side, we always need to get the next avaiable buffer based on current buffer index. So add this API for convinience use.
For example, virtio blk driver origanize the buffer:
For the virtio blk device size, we need get the Buffer and Response buffer based on the Request buffer index.