Re: [PATCH 5/6] vhost-vdpa: Match vhost-user's status reset

[Eugenio Perez Martin]

On Wed, Jul 19, 2023 at 4:10 PM Hanna Czenczek <hreitz@redhat.com> wrote:
>
> On 18.07.23 16:50, Stefan Hajnoczi wrote:
> > On Tue, Jul 11, 2023 at 05:52:27PM +0200, Hanna Czenczek wrote:
> >> vhost-vdpa and vhost-user differ in how they reset the status in their
> >> respective vhost_reset_status implementations: vhost-vdpa zeroes it,
> >> then re-adds the S_ACKNOWLEDGE and S_DRIVER config bits.  S_DRIVER_OK is
> >> then set in vhost_vdpa_dev_start().
> >>
> >> vhost-user in contrast just zeroes the status, and does no re-add any
> >> config bits until vhost_user_dev_start() (where it does re-add all of
> >> S_ACKNOWLEDGE, S_DRIVER, and S_DRIVER_OK).
> >>
> >> There is no documentation for vhost_reset_status, but its only caller is
> >> vhost_dev_stop().  So apparently, the device is to be stopped after
> >> vhost_reset_status, and therefore it makes more sense to keep the status
> >> field fully cleared until the back-end is re-started, which is how
> >> vhost-user does it.  Make vhost-vdpa do the same -- if nothing else it's
> >> confusing to have both vhost implementations handle this differently.
> >>

Ok now I understand better why you moved the call to vhost_vdpa_reset_status.

Maybe we can move the vhost_vdpa_add_status(dev, _S_ACKNOWLEDGE |
_S_DRIVER) to vhost_vdpa_set_features then, and let reset_status call
vhost_vdpa_reset_device directly. But I'm not 100% sure if I'm missing
something, it would need testing.

Thanks!

> >> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
> >> ---
> >>   hw/virtio/vhost-vdpa.c | 6 +++---
> >>   1 file changed, 3 insertions(+), 3 deletions(-)
> > Hi Hanna,
> > The VIRTIO spec lists the Device Initialization sequence including the
> > bits set in the Device Status Register here:
> > https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-1070001
> >
> > ACKNOWLEDGE and DRIVER must be set before FEATURES_OK. DRIVER_OK is set
> > after FEATURES_OK.
> >
> > The driver may read the Device Configuration Space once ACKNOWLEDGE and
> > DRIVER are set.
> >
> > QEMU's vhost code should follow this sequence (especially for vDPA where
> > full VIRTIO devices are implemented).
> >
> > vhost-user is not faithful to the VIRTIO spec here. That's probably due
> > to the fact that vhost-user didn't have the concept of the Device Status
> > Register until recently and back-ends mostly ignore it.
> >
> > Please do the opposite of this patch: bring vhost-user in line with the
> > VIRTIO specification so that the Device Initialization sequence is
> > followed correctly. I think vhost-vdpa already does the right thing.
>
> Hm.  This sounds all very good, but what leaves me lost is the fact that
> we never actually expose the status field to the guest, as far as I can
> see.  We have no set_status callback, and as written in the commit
> message, the only caller of reset_status is vhost_dev_stop().  So the
> status field seems completely artificial in vhost right now.  That is
> why I’m wondering what the flags even really mean.
>
> Another point I made in the commit message is that it is strange that we
> reset the status to 0, and then add the ACKNOWLEDGE and DRIVER while the
> VM is still stopped.  It doesn’t make sense to me to set these flags
> while the guest driver is not operative.
>
> If what you’re saying is that we must set FEATURES_OK only after
> ACKNOWLEDGE and DRIVER, wouldn’t it be still better to set all of these
> flags only in vhost_*_dev_start(), but do it in two separate SET_STATUS
> calls?
>
> (You mentioned the configuration space – is that accessed while between
> vhost_dev_stop and vhost_dev_start?)
>
> Hanna
>
> >> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> >> index f7fd19a203..0cde8b40de 100644
> >> --- a/hw/virtio/vhost-vdpa.c
> >> +++ b/hw/virtio/vhost-vdpa.c
> >> @@ -1294,8 +1294,6 @@ static void vhost_vdpa_reset_status(struct vhost_dev 
> >> *dev)
> >>       }
> >>
> >>       vhost_vdpa_reset_device(dev);
> >> -    vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> >> -                               VIRTIO_CONFIG_S_DRIVER);
> >>       memory_listener_unregister(&v->listener);
> >>   }
> >>
> >> @@ -1334,7 +1332,9 @@ static int vhost_vdpa_dev_start(struct vhost_dev 
> >> *dev, bool started)
> >>           }
> >>           memory_listener_register(&v->listener, dev->vdev->dma_as);
> >>
> >> -        return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> >> +        return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> >> +                                          VIRTIO_CONFIG_S_DRIVER |
> >> +                                          VIRTIO_CONFIG_S_DRIVER_OK);
> >>       }
> >>
> >>       return 0;
> >> --
> >> 2.41.0
> >>
>