On Wed, Jul 19, 2023 at 4:10 PM Hanna Czenczek <hre...@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 <hre...@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 > >> >