[PATCH] vhost-user-scsi: implement handle_output

[PATCH] vhost-user-scsi: implement handle_output

[Felipe Franciosi]

Originally, vhost-user-scsi did not implement a handle_output callback
as that didn't seem necessary. Turns out it is.

Depending on which other devices are presented to a VM, SeaBIOS may
decide to map vhost-user-scsi devices on the 64-bit range of the address
space. As a result, SeaBIOS will kick VQs via the config space. Those
land on Qemu (not the vhost backend) and are missed, causing the VM not
to boot. This fixes the issue by getting Qemu to post the notification.

Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
 hw/scsi/vhost-user-scsi.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index 6a6c15dd32..13278ed151 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -62,8 +62,9 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
     }
 }
 
-static void vhost_dummy_handle_output(VirtIODevice *vdev, VirtQueue *vq)
+static void vhost_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
+    event_notifier_set(virtio_queue_get_host_notifier(vq));
 }
 
 static void vhost_user_scsi_realize(DeviceState *dev, Error **errp)
@@ -80,9 +81,9 @@ static void vhost_user_scsi_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    virtio_scsi_common_realize(dev, vhost_dummy_handle_output,
-                               vhost_dummy_handle_output,
-                               vhost_dummy_handle_output, &err);
+    virtio_scsi_common_realize(dev, vhost_handle_output,
+                               vhost_handle_output,
+                               vhost_handle_output, &err);
     if (err != NULL) {
         error_propagate(errp, err);
         return;
-- 
2.20.1

Re: [PATCH] vhost-user-scsi: implement handle_output

[Yongji Xie]

On Fri, 18 Oct 2019 at 01:17, Felipe Franciosi <felipe@nutanix.com> wrote:
>
> Originally, vhost-user-scsi did not implement a handle_output callback
> as that didn't seem necessary. Turns out it is.
>
> Depending on which other devices are presented to a VM, SeaBIOS may
> decide to map vhost-user-scsi devices on the 64-bit range of the address
> space. As a result, SeaBIOS will kick VQs via the config space. Those
> land on Qemu (not the vhost backend) and are missed, causing the VM not
> to boot. This fixes the issue by getting Qemu to post the notification.
>
Should we fix this in vhost-user-blk too?

Thanks,
Yongji

> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> ---
>  hw/scsi/vhost-user-scsi.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index 6a6c15dd32..13278ed151 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -62,8 +62,9 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
>      }
>  }
>
> -static void vhost_dummy_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> +static void vhost_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>  {
> +    event_notifier_set(virtio_queue_get_host_notifier(vq));
>  }
>
>  static void vhost_user_scsi_realize(DeviceState *dev, Error **errp)
> @@ -80,9 +81,9 @@ static void vhost_user_scsi_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>
> -    virtio_scsi_common_realize(dev, vhost_dummy_handle_output,
> -                               vhost_dummy_handle_output,
> -                               vhost_dummy_handle_output, &err);
> +    virtio_scsi_common_realize(dev, vhost_handle_output,
> +                               vhost_handle_output,
> +                               vhost_handle_output, &err);
>      if (err != NULL) {
>          error_propagate(errp, err);
>          return;
> --
> 2.20.1
>

Re: [PATCH] vhost-user-scsi: implement handle_output

[Felipe Franciosi]

> On Oct 18, 2019, at 3:59 AM, Yongji Xie <elohimes@gmail.com> wrote:
> 
> On Fri, 18 Oct 2019 at 01:17, Felipe Franciosi <felipe@nutanix.com> wrote:
>> 
>> Originally, vhost-user-scsi did not implement a handle_output callback
>> as that didn't seem necessary. Turns out it is.
>> 
>> Depending on which other devices are presented to a VM, SeaBIOS may
>> decide to map vhost-user-scsi devices on the 64-bit range of the address
>> space. As a result, SeaBIOS will kick VQs via the config space. Those
>> land on Qemu (not the vhost backend) and are missed, causing the VM not
>> to boot. This fixes the issue by getting Qemu to post the notification.
>> 
> Should we fix this in vhost-user-blk too?

I'm not sure vhost-user-blk suffers from the same problem. Certainly
vhost-scsi does, but I'd prefer to tackle that separately because I
can't trivially test it. If it breaks something there, we can revert
it without affecting a valid fix for vhost-user-scsi.

I can send that patch immediately after this is queued (or resend a v2
including both patches separately if maintainers prefer that).

F.

> 
> Thanks,
> Yongji
> 
>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
>> ---
>> hw/scsi/vhost-user-scsi.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>> 
>> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
>> index 6a6c15dd32..13278ed151 100644
>> --- a/hw/scsi/vhost-user-scsi.c
>> +++ b/hw/scsi/vhost-user-scsi.c
>> @@ -62,8 +62,9 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
>>     }
>> }
>> 
>> -static void vhost_dummy_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>> +static void vhost_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>> {
>> +    event_notifier_set(virtio_queue_get_host_notifier(vq));
>> }
>> 
>> static void vhost_user_scsi_realize(DeviceState *dev, Error **errp)
>> @@ -80,9 +81,9 @@ static void vhost_user_scsi_realize(DeviceState *dev, Error **errp)
>>         return;
>>     }
>> 
>> -    virtio_scsi_common_realize(dev, vhost_dummy_handle_output,
>> -                               vhost_dummy_handle_output,
>> -                               vhost_dummy_handle_output, &err);
>> +    virtio_scsi_common_realize(dev, vhost_handle_output,
>> +                               vhost_handle_output,
>> +                               vhost_handle_output, &err);
>>     if (err != NULL) {
>>         error_propagate(errp, err);
>>         return;
>> --
>> 2.20.1
>> 

Re: [PATCH] vhost-user-scsi: implement handle_output

[Yongji Xie]

On Fri, 18 Oct 2019 at 19:14, Felipe Franciosi <felipe@nutanix.com> wrote:
>
>
>
> > On Oct 18, 2019, at 3:59 AM, Yongji Xie <elohimes@gmail.com> wrote:
> >
> > On Fri, 18 Oct 2019 at 01:17, Felipe Franciosi <felipe@nutanix.com> wrote:
> >>
> >> Originally, vhost-user-scsi did not implement a handle_output callback
> >> as that didn't seem necessary. Turns out it is.
> >>
> >> Depending on which other devices are presented to a VM, SeaBIOS may
> >> decide to map vhost-user-scsi devices on the 64-bit range of the address
> >> space. As a result, SeaBIOS will kick VQs via the config space. Those
> >> land on Qemu (not the vhost backend) and are missed, causing the VM not
> >> to boot. This fixes the issue by getting Qemu to post the notification.
> >>
> > Should we fix this in vhost-user-blk too?
>
> I'm not sure vhost-user-blk suffers from the same problem. Certainly

Actually I found vhost-user-blk has the same problem in a mutilple
GPUs passthough environment.

Thanks,
Yongji

Re: [PATCH] vhost-user-scsi: implement handle_output

[Felipe Franciosi]

> On Oct 21, 2019, at 5:01 AM, Yongji Xie <elohimes@gmail.com> wrote:
> 
> On Fri, 18 Oct 2019 at 19:14, Felipe Franciosi <felipe@nutanix.com> wrote:
>> 
>> 
>> 
>>> On Oct 18, 2019, at 3:59 AM, Yongji Xie <elohimes@gmail.com> wrote:
>>> 
>>> On Fri, 18 Oct 2019 at 01:17, Felipe Franciosi <felipe@nutanix.com> wrote:
>>>> 
>>>> Originally, vhost-user-scsi did not implement a handle_output callback
>>>> as that didn't seem necessary. Turns out it is.
>>>> 
>>>> Depending on which other devices are presented to a VM, SeaBIOS may
>>>> decide to map vhost-user-scsi devices on the 64-bit range of the address
>>>> space. As a result, SeaBIOS will kick VQs via the config space. Those
>>>> land on Qemu (not the vhost backend) and are missed, causing the VM not
>>>> to boot. This fixes the issue by getting Qemu to post the notification.
>>>> 
>>> Should we fix this in vhost-user-blk too?
>> 
>> I'm not sure vhost-user-blk suffers from the same problem. Certainly
> 
> Actually I found vhost-user-blk has the same problem in a mutilple
> GPUs passthough environment.

Let's Cc Changpeng for comments. I'm not familiar with that code.

In any case, I still think we should merge this and fix other
implementations separately. That allows us to revert patches
individually if anything else breaks.

F.

> 
> Thanks,
> Yongji

RE: [PATCH] vhost-user-scsi: implement handle_output

[Liu, Changpeng]

There is some logic in vhost_user_blk_handle_output() for now, it's not empty as vhost-user-scsi.
There should be other issue if it can't start from SeaBIOS.

> -----Original Message-----
> From: Felipe Franciosi [mailto:felipe@nutanix.com]
> Sent: Monday, October 21, 2019 4:00 PM
> To: Yongji Xie <elohimes@gmail.com>; Liu, Changpeng
> <changpeng.liu@intel.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>; Alex Williamson
> <alex.williamson@redhat.com>; Dr . David Alan Gilbert <dgilbert@redhat.com>;
> qemu-devel@nongnu.org
> Subject: Re: [PATCH] vhost-user-scsi: implement handle_output
> 
> 
> 
> > On Oct 21, 2019, at 5:01 AM, Yongji Xie <elohimes@gmail.com> wrote:
> >
> > On Fri, 18 Oct 2019 at 19:14, Felipe Franciosi <felipe@nutanix.com> wrote:
> >>
> >>
> >>
> >>> On Oct 18, 2019, at 3:59 AM, Yongji Xie <elohimes@gmail.com> wrote:
> >>>
> >>> On Fri, 18 Oct 2019 at 01:17, Felipe Franciosi <felipe@nutanix.com> wrote:
> >>>>
> >>>> Originally, vhost-user-scsi did not implement a handle_output callback
> >>>> as that didn't seem necessary. Turns out it is.
> >>>>
> >>>> Depending on which other devices are presented to a VM, SeaBIOS may
> >>>> decide to map vhost-user-scsi devices on the 64-bit range of the address
> >>>> space. As a result, SeaBIOS will kick VQs via the config space. Those
> >>>> land on Qemu (not the vhost backend) and are missed, causing the VM not
> >>>> to boot. This fixes the issue by getting Qemu to post the notification.
> >>>>
> >>> Should we fix this in vhost-user-blk too?
> >>
> >> I'm not sure vhost-user-blk suffers from the same problem. Certainly
> >
> > Actually I found vhost-user-blk has the same problem in a mutilple
> > GPUs passthough environment.
> 
> Let's Cc Changpeng for comments. I'm not familiar with that code.
> 
> In any case, I still think we should merge this and fix other
> implementations separately. That allows us to revert patches
> individually if anything else breaks.
> 
> F.
> 
> >
> > Thanks,
> > Yongji

Re: [PATCH] vhost-user-scsi: implement handle_output

[Yongji Xie]

On Mon, 21 Oct 2019 at 16:20, Liu, Changpeng <changpeng.liu@intel.com> wrote:
>
> There is some logic in vhost_user_blk_handle_output() for now, it's not empty as vhost-user-scsi.
> There should be other issue if it can't start from SeaBIOS.
>

No, it's the same issue. We can see the notify is triggered from the
VIRTIO_PCI_CAP_PCI_CFG region in configuration space. And looks like
seabios also support this notify mode:

void vp_init_simple(struct vp_device *vp, struct pci_device *pci)
{
....
vp_cap->mode = (addr > 0xffffffffll) ?
                    VP_ACCESS_PCICFG : VP_ACCESS_MMIO;
....
}

Thanks,
Yongji

Re: [PATCH] vhost-user-scsi: implement handle_output

[Stefan Hajnoczi]

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

On Mon, Oct 21, 2019 at 08:19:59AM +0000, Liu, Changpeng wrote:
> There is some logic in vhost_user_blk_handle_output() for now, it's not empty as vhost-user-scsi.

The purpose of the vhost_user_blk_handle_output() code is to deal with
legacy drivers that violate the VIRTIO spec by accessing virtqueues
before VIRTIO Device Initialization is complete.  It needs to stay.

The patch I posted should work together with
vhost_user_blk_handle_output() though and handle the case that Felipe
discovered.

Stefan

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

Re: [PATCH] vhost-user-scsi: implement handle_output

[Yongji Xie]

On Mon, 21 Oct 2019 at 16:00, Felipe Franciosi <felipe@nutanix.com> wrote:
>
>
>
> > On Oct 21, 2019, at 5:01 AM, Yongji Xie <elohimes@gmail.com> wrote:
> >
> > On Fri, 18 Oct 2019 at 19:14, Felipe Franciosi <felipe@nutanix.com> wrote:
> >>
> >>
> >>
> >>> On Oct 18, 2019, at 3:59 AM, Yongji Xie <elohimes@gmail.com> wrote:
> >>>
> >>> On Fri, 18 Oct 2019 at 01:17, Felipe Franciosi <felipe@nutanix.com> wrote:
> >>>>
> >>>> Originally, vhost-user-scsi did not implement a handle_output callback
> >>>> as that didn't seem necessary. Turns out it is.
> >>>>
> >>>> Depending on which other devices are presented to a VM, SeaBIOS may
> >>>> decide to map vhost-user-scsi devices on the 64-bit range of the address
> >>>> space. As a result, SeaBIOS will kick VQs via the config space. Those
> >>>> land on Qemu (not the vhost backend) and are missed, causing the VM not
> >>>> to boot. This fixes the issue by getting Qemu to post the notification.
> >>>>
> >>> Should we fix this in vhost-user-blk too?
> >>
> >> I'm not sure vhost-user-blk suffers from the same problem. Certainly
> >
> > Actually I found vhost-user-blk has the same problem in a mutilple
> > GPUs passthough environment.
>
> Let's Cc Changpeng for comments. I'm not familiar with that code.
>
> In any case, I still think we should merge this and fix other
> implementations separately. That allows us to revert patches
> individually if anything else breaks.
>

It's OK for me.

Thanks,
Yongji

Re: [PATCH] vhost-user-scsi: implement handle_output

[Stefan Hajnoczi]

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

On Thu, Oct 17, 2019 at 04:39:44PM +0000, Felipe Franciosi wrote:
> Originally, vhost-user-scsi did not implement a handle_output callback
> as that didn't seem necessary. Turns out it is.
> 
> Depending on which other devices are presented to a VM, SeaBIOS may
> decide to map vhost-user-scsi devices on the 64-bit range of the address
> space. As a result, SeaBIOS will kick VQs via the config space. Those
> land on Qemu (not the vhost backend) and are missed, causing the VM not
> to boot. This fixes the issue by getting Qemu to post the notification.
> 
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> ---
>  hw/scsi/vhost-user-scsi.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index 6a6c15dd32..13278ed151 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -62,8 +62,9 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
>      }
>  }
>  
> -static void vhost_dummy_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> +static void vhost_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>  {
> +    event_notifier_set(virtio_queue_get_host_notifier(vq));
>  }

It would be nice to fix this in hw/virtio/virtio.c:virtio_queue_notify()
so that all devices are automatically covered.

I'll send a patch and CC you.

Stefan

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