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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3091 bytes --]

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.
> 
> 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.

> 
> 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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]