| Message ID | 20191017163859.23184-1-felipe@nutanix.com |
|---|---|
| State | New |
| Headers | show |
| Series | vhost-user-scsi: implement handle_output | expand |
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 >
> 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 >>
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
> 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
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
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
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
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
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
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;
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(-)