diff mbox series

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

Message ID 20230711155230.64277-6-hreitz@redhat.com
State New
Headers show
Series vhost-user: Add suspend/resume | expand

Commit Message

Hanna Czenczek July 11, 2023, 3:52 p.m. UTC 
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(-)

Comments

Stefan Hajnoczi July 18, 2023, 2:50 p.m. UTC | #1 
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
>
Hanna Czenczek July 19, 2023, 2:09 p.m. UTC | #2 
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
>>
Stefan Hajnoczi July 19, 2023, 3:06 p.m. UTC | #3 
On Wed, 19 Jul 2023 at 10:10, 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.
> >>
> >> 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.

vhost (including vDPA and vhost-user) is not a 100% passthrough
solution. The VMM emulates a VIRTIO device (e.g. virtio-fs-pci) that
has some separate state from the vhost back-end, including the Device
Status Register. This is analogous to how passthrough PCI devices
still have emulated PCI registers that are not passed through to the
physical PCI device.

However, just because the vDPA, and now vhost-user with the SET_STATUS
message, back-end is not directly exposed to the guest does not mean
it should diverge from the VIRTIO specification for no reason.

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

While there is no harm in setting those bits, I agree that leaving the
Device Status Register at 0 while the VM is stopped would be nicer.

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

The device initialization sequence could be put into vhost_dev_start():
1. ACKNOWLEDGE | DRIVER
2. FEATURES_OK via vhost_dev_set_features()
3. DRIVER_OK via ->vhost_dev_start()

But note that the ->vhost_dev_start() callback is too late to set
ACKNOWLEDGE | DRIVER because feature negotiation happens earlier.

> (You mentioned the configuration space – is that accessed while between
> vhost_dev_stop and vhost_dev_start?)

I don't think so.

>
> 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
> >>
>
>
Eugenio Perez Martin July 21, 2023, 3:47 p.m. UTC | #4 
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
> >>
>
diff mbox series

Patch

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;