| Message ID | 20230711155230.64277-6-hreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vhost-user: Add suspend/resume | expand |
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 >
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 >>
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 > >> > >
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 --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;
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(-)