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

[Hanna Czenczek <hreitz@redhat.com>]

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