On Mon, Mar 24, 2025 at 3:34 PM Sahil Siddiq <icegambi...@gmail.com> wrote:
>
> Hi,
>
> I had a few more queries here as well.
>
> On 3/24/25 7:29 PM, Sahil Siddiq wrote:
> > Detect when used descriptors are ready for consumption by the guest via
> > packed virtqueues and forward them from the device to the guest.
> >
> > Signed-off-by: Sahil Siddiq <sahil...@proton.me>
> > ---
> > Changes from v4 -> v5:
> > - New commit.
> > - vhost-shadow-virtqueue.c:
> > (vhost_svq_more_used): Split into vhost_svq_more_used_split and
> > vhost_svq_more_used_packed.
> > (vhost_svq_enable_notification): Handle split and packed vqs.
> > (vhost_svq_disable_notification): Likewise.
> > (vhost_svq_get_buf): Split into vhost_svq_get_buf_split and
> > vhost_svq_get_buf_packed.
> > (vhost_svq_poll): Use new functions.
> >
> > hw/virtio/vhost-shadow-virtqueue.c | 121 ++++++++++++++++++++++++++---
> > 1 file changed, 110 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c
> > b/hw/virtio/vhost-shadow-virtqueue.c
> > index 126957231d..8430b3c94a 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -463,7 +463,7 @@ static void
> > vhost_handle_guest_kick_notifier(EventNotifier *n)
> > vhost_handle_guest_kick(svq);
> > }
> >
> > -static bool vhost_svq_more_used(VhostShadowVirtqueue *svq)
> > +static bool vhost_svq_more_used_split(VhostShadowVirtqueue *svq)
> > {
> > uint16_t *used_idx = &svq->vring.used->idx;
> > if (svq->last_used_idx != svq->shadow_used_idx) {
> > @@ -475,6 +475,22 @@ static bool vhost_svq_more_used(VhostShadowVirtqueue
> > *svq)
> > return svq->last_used_idx != svq->shadow_used_idx;
> > }
> >
> > +static bool vhost_svq_more_used_packed(VhostShadowVirtqueue *svq)
> > +{
> > + bool avail_flag, used_flag, used_wrap_counter;
> > + uint16_t last_used_idx, last_used, flags;
> > +
> > + last_used_idx = svq->last_used_idx;
> > + last_used = last_used_idx & ~(1 << VRING_PACKED_EVENT_F_WRAP_CTR);
>
> In the linux kernel, last_used is calculated as:
>
> last_used_idx & ~(-(1 << VRING_PACKED_EVENT_F_WRAP_CTR))
>
> ...instead of...
>
> last_used_idx & ~(1 << VRING_PACKED_EVENT_F_WRAP_CTR)
>
> Isn't the second option good enough if last_used_idx is uint16_t
> and VRING_PACKED_EVENT_F_WRAP_CTR is defined as 15.
>
I think it is good enough with the u16 restrictions but it's just
defensive code.
> > + used_wrap_counter = !!(last_used_idx & (1 <<
> > VRING_PACKED_EVENT_F_WRAP_CTR));
> > +
> > + flags = le16_to_cpu(svq->vring_packed.vring.desc[last_used].flags);
> > + avail_flag = !!(flags & (1 << VRING_PACKED_DESC_F_AVAIL));
> > + used_flag = !!(flags & (1 << VRING_PACKED_DESC_F_USED));
> > +
> > + return avail_flag == used_flag && used_flag == used_wrap_counter;
> > +}
> > +
>
> Also in the implementation of vhost_svq_more_used_split() [1], I haven't
> understood why the following condition:
>
> svq->last_used_idx != svq->shadow_used_idx
>
> is checked before updating the value of "svq->shadow_used_idx":
>
> svq->shadow_used_idx = le16_to_cpu(*(volatile uint16_t *)used_idx)
>
As far as I know this is used to avoid concurrent access to guest's
used_idx, avoiding cache sharing, the memory barrier, and the
potentially costly volatile access.
