Hi,
Thank you for your reply.
On Wednesday, June 19, 2024 1:07:54 PM GMT+5:30 Eugenio Perez Martin wrote:
> [...]
> > "curr" is being updated here, but descs[i].id is always set to id which
> > doesn't change in the loop. So all the descriptors in the chain will have
> > the same id. I can't find anything in the virtio specification [1] that
> > suggests that all descriptors in the chain have the same id. Also, going
> > by the figure captioned "Three chained descriptors available" in the blog
> > post on packed virtqueues [2], it looks like the descriptors in the chain
> > have different buffer ids.
> >
> > The virtio implementation in Linux also reuses the same id value for all
> > the descriptors in a single chain. I am not sure if I am missing
> > something here.
>
> The code is right, the id that identifies the whole chain is just the
> one on the last descriptor. The key is that all the tail descriptors
> of the chains will have a different id, the rest ids are ignored so it
> is easier this way. I got it wrong in a recent mail in the list, where
> you can find more information. Let me know if you cannot find it :).
I found the mail here [1] :)
> In the split vq is different as a chained descriptor can go back and
> forth in the descriptor ring with the next id. So all of them must be
> different. But in the packed vq, the device knows the next descriptor
> is placed at the next entry in the descriptor ring, so the only
> important id is the last one.
Ok, this makes sense now.
> > > + if (++i >= svq->vring_packed.vring.num) {
> > > + i = 0;
> > > + svq->vring_packed.avail_used_flags ^=
> > > + 1 << VRING_PACKED_DESC_F_AVAIL |
> > > + 1 << VRING_PACKED_DESC_F_USED;
> > > + }
> > > + }
> > > +
> > > + if (i <= head) {
> > > + svq->vring_packed.avail_wrap_counter ^= 1;
> > > + }
> > > +
> > > + svq->vring_packed.next_avail_idx = i;
> > > + svq->free_head = curr;
> >
> > Even though the same id is used, curr will not be id+1 here.
>
> curr is not the descriptor index, but the id. They're used in a stack
> format: One available chain pops an id and one used id pushes its id
> in the stack.
>
> Maybe I'm wrong, but I think the main reason is to reuse the same
> memory region of the descriptor state etc so less memory is changed to
> be used in all the operations.
Right, curr is the id. I didn't really understand the popping and pushing
part.
In the implementation, the possible ids are stored in svq.desc_next. And
it's implemented in a way that the next id is (id + 1) % queue_size.
By the following line:
> Even though the same id is used, curr will not be id+1 here.
I meant that if, for example, there is a chain of 3 descriptors and the
current id is 1, then all 3 descriptors will have 1 as their id. If the vring
size is 5, then the value of curr that will be filled in the 4th descriptor
will be 4 instead of 2.
> > > + return true;
> > > +}
> > > +
> > > +static bool vhost_svq_add_packed(VhostShadowVirtqueue *svq,
> > > + const struct iovec *out_sg, size_t
> > > out_num, + const struct iovec *in_sg,
> > > size_t in_num, + unsigned *head)
> > > +{
> > > + bool ok;
> > > + uint16_t head_flags = 0;
> > > + g_autofree hwaddr *sgs = g_new(hwaddr, out_num + in_num);
> >
> > I chose to use out_num+in_num as the size instead of MAX(ount_num,
> > in_num). I found it easier to implement
> > "vhost_svq_vring_write_descs_packed()" like this. Please let me know if
> > this isn't feasible or ideal.
>
> Not a big deal, I picked the MAX just because it is all the
> hwaddresses the function needs at the same time. Addition should work
> too, and AFAIK chains are usually short. We should get rid of this
> dynamic allocation in the future anyway.
Ok, understood.
> [...]
> > In "struct VhostShadowVirtqueue", I rearranged the order in which some
> > members appear. I tried to keep the members common to split and packed
> > virtqueues above the union and the rest below the union. I haven't
> > entirely understood the role of some of the members (for example,
> > VhostShadowVirtqueueOps *ops). I'll change this ordering if need be as I
> > continue to understand them better.
>
> That's fine, but do it in a separate patch for future series, so it is
> easier to review.
Sure, I'll do that.
> ops is used when a kind of device wants specialized handling for the
> descriptors forwarded. vdpa-net uses it when QEMU also needs to
> inspect the descriptors. Feel free to ask more about it, but adding
> packed format to SVQ should not affect the ops member.
Got it. I don't have any other questions related to ops.
> > For the next step, I think I should work on "vhost_svq_start()" which is
> > where members of the struct are actually initialized. At the moment, only
> > the split ring part of the structure is initialized.
>
> Sounds reasonable. My recommendation is to mimic the patches of the
> kernel, doing a git log and following that order. You also need to
> apply the fixes in the history from that moment.
>
> > I think I should also start working on enhancing "vhost_svq_kick()" to
> > actually send the buffers to the device. I think it'll be easier to test
> > these changes once that's done (I am not sure about this though). Would
> > this involve implementing the notification mechanism and event_idx?
> You can start omitting event_idx if you disable it from the device or
> from qemu's commandline with event_idx=off. If you use vdpa_sim, it's
> easier to remove it using qemu's cmdline in my opinion. Also, there is
> a needs_kick boolean that you can set to always true for testing
> purposes, since it is valid to send extra notifications. I think you
> don't need to modify anything else from vhost_svq_kick to test the
> device receives the buffer, but let me know if you find problems.
I'll take a look at the kernel patches. I'll also try testing these changes with
vdpa_sim. I'll check if the device gets the buffers.
Thanks,
Sahil
[1] https://lists.nongnu.org/archive/html/qemu-devel/2024-05/msg01843.html
