| Message ID | 1481633076-24521-1-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Di, 2016-12-13 at 13:44 +0100, Thomas Huth wrote: > When passing through a USB storage device to a pseries guest, it > is currently not possible to automatically boot from the device > if the "bootindex" property has been specified, too (e.g. when using > "-device nec-usb-xhci -device usb-host,hostbus=1,hostaddr=2,bootindex=0" > at the command line). The problem is that QEMU builds a device tree path > like "/pci@800000020000000/usb@0/usb-host@1" and passes it to SLOF > in the /chosen/qemu,boot-list property. SLOF, however, probes the > USB device, recognizes that it is a storage device and thus changes > its name to "storage", and additionally adds a child node for the > SCSI LUN, so the correct boot path in SLOF is something like > "/pci@800000020000000/usb@0/storage@1/disk@101000000000000" instead. > So when we detect an USB mass storage device with SCSI interface, > we've got to adjust the firmware boot-device path properly, so that > SLOF can automatically boot from the device. > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 208ef7b..fe315b5 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2185,6 +2185,15 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus, > } > } > > + if (strcmp("usb-host", qdev_fw_name(dev)) == 0) { > + USBDevice *usbdev = CAST(USBDevice, dev, TYPE_USB_DEVICE); > + > + /* SLOF scans USB storage and adds a "disk" node for the SCSI LUN */ > + if (usb_host_dev_is_scsi_storage(usbdev)) { > + return g_strdup_printf("storage@%s/disk", usbdev->port->path); > + } > + } > + Any reason why this is hidden in ppc/spapr? usb-host could implement the ->fw_name callback instead to create a name depending on the device type ... cheers, Gerd
On 13.12.2016 14:52, Gerd Hoffmann wrote: > On Di, 2016-12-13 at 13:44 +0100, Thomas Huth wrote: >> When passing through a USB storage device to a pseries guest, it >> is currently not possible to automatically boot from the device >> if the "bootindex" property has been specified, too (e.g. when using >> "-device nec-usb-xhci -device usb-host,hostbus=1,hostaddr=2,bootindex=0" >> at the command line). The problem is that QEMU builds a device tree path >> like "/pci@800000020000000/usb@0/usb-host@1" and passes it to SLOF >> in the /chosen/qemu,boot-list property. SLOF, however, probes the >> USB device, recognizes that it is a storage device and thus changes >> its name to "storage", and additionally adds a child node for the >> SCSI LUN, so the correct boot path in SLOF is something like >> "/pci@800000020000000/usb@0/storage@1/disk@101000000000000" instead. >> So when we detect an USB mass storage device with SCSI interface, >> we've got to adjust the firmware boot-device path properly, so that >> SLOF can automatically boot from the device. > >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index 208ef7b..fe315b5 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -2185,6 +2185,15 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus, >> } >> } >> >> + if (strcmp("usb-host", qdev_fw_name(dev)) == 0) { >> + USBDevice *usbdev = CAST(USBDevice, dev, TYPE_USB_DEVICE); >> + >> + /* SLOF scans USB storage and adds a "disk" node for the SCSI LUN */ >> + if (usb_host_dev_is_scsi_storage(usbdev)) { >> + return g_strdup_printf("storage@%s/disk", usbdev->port->path); >> + } >> + } >> + > > Any reason why this is hidden in ppc/spapr? > > usb-host could implement the ->fw_name callback instead to create a name > depending on the device type ... The naming is specific to SLOF ... not sure what happens with other firmware implementations, so spapr.c sounds like the better place to me right now. Thomas
On Tue, Dec 13, 2016 at 01:44:36PM +0100, Thomas Huth wrote: > When passing through a USB storage device to a pseries guest, it > is currently not possible to automatically boot from the device > if the "bootindex" property has been specified, too (e.g. when using > "-device nec-usb-xhci -device usb-host,hostbus=1,hostaddr=2,bootindex=0" > at the command line). The problem is that QEMU builds a device tree path > like "/pci@800000020000000/usb@0/usb-host@1" and passes it to SLOF > in the /chosen/qemu,boot-list property. SLOF, however, probes the > USB device, recognizes that it is a storage device and thus changes > its name to "storage", and additionally adds a child node for the > SCSI LUN, so the correct boot path in SLOF is something like > "/pci@800000020000000/usb@0/storage@1/disk@101000000000000" instead. > So when we detect an USB mass storage device with SCSI interface, > we've got to adjust the firmware boot-device path properly, so that > SLOF can automatically boot from the device. > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1354177 > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/ppc/spapr.c | 9 +++++++++ > hw/usb/host-libusb.c | 29 +++++++++++++++++++++++++++++ > hw/usb/host-stub.c | 5 +++++ > include/hw/usb.h | 1 + > 4 files changed, 44 insertions(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 208ef7b..fe315b5 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2185,6 +2185,15 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus, > } > } I think a chunk of your commit message needs to go into a comment here. Anticipating the guest firmware behaviour is kind of a hack - I think it's worth it in this case, but we do need to make it clear to someone reading the code what the rationale is. > + if (strcmp("usb-host", qdev_fw_name(dev)) == 0) { > + USBDevice *usbdev = CAST(USBDevice, dev, TYPE_USB_DEVICE); > + > + /* SLOF scans USB storage and adds a "disk" node for the SCSI LUN */ > + if (usb_host_dev_is_scsi_storage(usbdev)) { > + return g_strdup_printf("storage@%s/disk", usbdev->port->path); > + } > + } > + > if (phb) { > /* Replace "pci" with "pci@800000020000000" */ > return g_strdup_printf("pci@%"PRIX64, phb->buid); > diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c > index bd81d71..7791c6d 100644 > --- a/hw/usb/host-libusb.c > +++ b/hw/usb/host-libusb.c > @@ -1707,6 +1707,35 @@ static void usb_host_auto_check(void *unused) > timer_mod(usb_auto_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 2000); > } > > +/** > + * Check whether USB host device has a USB mass storage SCSI interface > + */ > +bool usb_host_dev_is_scsi_storage(USBDevice *ud) > +{ > + USBHostDevice *uhd = USB_HOST_DEVICE(ud); > + struct libusb_config_descriptor *conf; > + const struct libusb_interface_descriptor *intf; > + bool is_scsi_storage = false; > + int i; > + > + if (!uhd || libusb_get_active_config_descriptor(uhd->dev, &conf) != 0) { > + return false; > + } > + > + for (i = 0; i < conf->bNumInterfaces; i++) { > + intf = &conf->interface[i].altsetting[ud->altsetting[i]]; > + if (intf->bInterfaceClass == LIBUSB_CLASS_MASS_STORAGE && > + intf->bInterfaceSubClass == 6) { /* 6 means SCSI */ > + is_scsi_storage = true; > + break; > + } > + } > + > + libusb_free_config_descriptor(conf); > + > + return is_scsi_storage; > +} > + > void hmp_info_usbhost(Monitor *mon, const QDict *qdict) > { > libusb_device **devs = NULL; > diff --git a/hw/usb/host-stub.c b/hw/usb/host-stub.c > index 6ba65a1..d0268ba 100644 > --- a/hw/usb/host-stub.c > +++ b/hw/usb/host-stub.c > @@ -46,3 +46,8 @@ USBDevice *usb_host_device_open(USBBus *bus, const char *devname) > { > return NULL; > } > + > +bool usb_host_dev_is_scsi_storage(USBDevice *ud) > +{ > + return false; > +} > diff --git a/include/hw/usb.h b/include/hw/usb.h > index 847c9de..43838c9 100644 > --- a/include/hw/usb.h > +++ b/include/hw/usb.h > @@ -471,6 +471,7 @@ void usb_generic_async_ctrl_complete(USBDevice *s, USBPacket *p); > /* usb-linux.c */ > USBDevice *usb_host_device_open(USBBus *bus, const char *devname); > void hmp_info_usbhost(Monitor *mon, const QDict *qdict); > +bool usb_host_dev_is_scsi_storage(USBDevice *usbdev); > > /* usb ports of the VM */ >
Hi, > >> in the /chosen/qemu,boot-list property. SLOF, however, probes the > >> USB device, recognizes that it is a storage device and thus changes > >> its name to "storage", and additionally adds a child node for the > >> SCSI LUN, so the correct boot path in SLOF is something like > >> "/pci@800000020000000/usb@0/storage@1/disk@101000000000000" instead. Sure this is correct? usb-storage (aka virtual usb stick) creates paths like this: /pci@i0cf8/usb@1,2/storage@1/channel@0/disk@0,0 i.e. there is a extra channel element (probably for host adapters with multiple scsi busses). > >> So when we detect an USB mass storage device with SCSI interface, > >> we've got to adjust the firmware boot-device path properly, so that > >> SLOF can automatically boot from the device. > > Any reason why this is hidden in ppc/spapr? > > > > usb-host could implement the ->fw_name callback instead to create a name > > depending on the device type ... > The naming is specific to SLOF ... not sure what happens with other > firmware implementations, so spapr.c sounds like the better place to me > right now. If usb-host (and usb-redir too btw) create paths identical to the ones created by usb-storage. Existing firmware should cope just fine (assuming bootindex for usb-storage works). cheers, Gerd
On 14.12.2016 10:36, Gerd Hoffmann wrote: > Hi, > >>>> in the /chosen/qemu,boot-list property. SLOF, however, probes the >>>> USB device, recognizes that it is a storage device and thus changes >>>> its name to "storage", and additionally adds a child node for the >>>> SCSI LUN, so the correct boot path in SLOF is something like >>>> "/pci@800000020000000/usb@0/storage@1/disk@101000000000000" instead. > > Sure this is correct? Yes, this is what SLOF is using / expecting. > usb-storage (aka virtual usb stick) creates paths like this: > > /pci@i0cf8/usb@1,2/storage@1/channel@0/disk@0,0 > > i.e. there is a extra channel element (probably for host adapters with > multiple scsi busses). That's true for the default case, but for the pseries machine, there is already code that changes the boot path for virtual usb-storage devices, see the function spapr_get_fw_dev_path() : ... else if (usb) { /* * We use SRP luns of the form 01000000 | (usb-port << 16) | lun * in the top 32 bits of the 64-bit LUN */ unsigned usb_port = atoi(usb->port->path); unsigned id = 0x1000000 | (usb_port << 16) | d->lun; return g_strdup_printf("%s@%"PRIX64, qdev_fw_name(dev), (uint64_t)id << 32); } >>>> So when we detect an USB mass storage device with SCSI interface, >>>> we've got to adjust the firmware boot-device path properly, so that >>>> SLOF can automatically boot from the device. > >>> Any reason why this is hidden in ppc/spapr? >>> >>> usb-host could implement the ->fw_name callback instead to create a name >>> depending on the device type ... > >> The naming is specific to SLOF ... not sure what happens with other >> firmware implementations, so spapr.c sounds like the better place to me >> right now. > > If usb-host (and usb-redir too btw) create paths identical to the ones > created by usb-storage. Existing firmware should cope just fine > (assuming bootindex for usb-storage works). Which firmware can deal with these original device paths with "channel@0" in it? Certainly not SLOF. Maybe OpenBIOS? Does OpenBIOS support USB storage devices? Thomas
Hi, > That's true for the default case, but for the pseries machine, there is > already code that changes the boot path for virtual usb-storage devices, > see the function spapr_get_fw_dev_path() : > > ... else if (usb) { > /* > * We use SRP luns of the form 01000000 | (usb-port << 16) | lun > * in the top 32 bits of the 64-bit LUN > */ > unsigned usb_port = atoi(usb->port->path); > unsigned id = 0x1000000 | (usb_port << 16) | d->lun; > return g_strdup_printf("%s@%"PRIX64, qdev_fw_name(dev), > (uint64_t)id << 32); > } Hmm, I thought the point of using standard open firmware paths was that we don't need hacks like this. Looks like things are not *that* standardized then if some archs need extra tweaks ... But if that is needed for pseries *anyway* it doesn't hurt to do the same for usb-host. > > If usb-host (and usb-redir too btw) create paths identical to the ones > > created by usb-storage. Existing firmware should cope just fine > > (assuming bootindex for usb-storage works). > > Which firmware can deal with these original device paths with "channel@0" > in it? Certainly not SLOF. Maybe OpenBIOS? Does OpenBIOS support USB storage > devices? seabios does, and I expect ovmf too (didn't test though). cheers, Gerd
On Wed, 14 Dec 2016, Thomas Huth wrote: > in it? Certainly not SLOF. Maybe OpenBIOS? Does OpenBIOS support USB storage > devices? No it doesn't. Only keyboard HID devices. Regards, BALATON Zoltan
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 208ef7b..fe315b5 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2185,6 +2185,15 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus, } } + if (strcmp("usb-host", qdev_fw_name(dev)) == 0) { + USBDevice *usbdev = CAST(USBDevice, dev, TYPE_USB_DEVICE); + + /* SLOF scans USB storage and adds a "disk" node for the SCSI LUN */ + if (usb_host_dev_is_scsi_storage(usbdev)) { + return g_strdup_printf("storage@%s/disk", usbdev->port->path); + } + } + if (phb) { /* Replace "pci" with "pci@800000020000000" */ return g_strdup_printf("pci@%"PRIX64, phb->buid); diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c index bd81d71..7791c6d 100644 --- a/hw/usb/host-libusb.c +++ b/hw/usb/host-libusb.c @@ -1707,6 +1707,35 @@ static void usb_host_auto_check(void *unused) timer_mod(usb_auto_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 2000); } +/** + * Check whether USB host device has a USB mass storage SCSI interface + */ +bool usb_host_dev_is_scsi_storage(USBDevice *ud) +{ + USBHostDevice *uhd = USB_HOST_DEVICE(ud); + struct libusb_config_descriptor *conf; + const struct libusb_interface_descriptor *intf; + bool is_scsi_storage = false; + int i; + + if (!uhd || libusb_get_active_config_descriptor(uhd->dev, &conf) != 0) { + return false; + } + + for (i = 0; i < conf->bNumInterfaces; i++) { + intf = &conf->interface[i].altsetting[ud->altsetting[i]]; + if (intf->bInterfaceClass == LIBUSB_CLASS_MASS_STORAGE && + intf->bInterfaceSubClass == 6) { /* 6 means SCSI */ + is_scsi_storage = true; + break; + } + } + + libusb_free_config_descriptor(conf); + + return is_scsi_storage; +} + void hmp_info_usbhost(Monitor *mon, const QDict *qdict) { libusb_device **devs = NULL; diff --git a/hw/usb/host-stub.c b/hw/usb/host-stub.c index 6ba65a1..d0268ba 100644 --- a/hw/usb/host-stub.c +++ b/hw/usb/host-stub.c @@ -46,3 +46,8 @@ USBDevice *usb_host_device_open(USBBus *bus, const char *devname) { return NULL; } + +bool usb_host_dev_is_scsi_storage(USBDevice *ud) +{ + return false; +} diff --git a/include/hw/usb.h b/include/hw/usb.h index 847c9de..43838c9 100644 --- a/include/hw/usb.h +++ b/include/hw/usb.h @@ -471,6 +471,7 @@ void usb_generic_async_ctrl_complete(USBDevice *s, USBPacket *p); /* usb-linux.c */ USBDevice *usb_host_device_open(USBBus *bus, const char *devname); void hmp_info_usbhost(Monitor *mon, const QDict *qdict); +bool usb_host_dev_is_scsi_storage(USBDevice *usbdev); /* usb ports of the VM */
When passing through a USB storage device to a pseries guest, it is currently not possible to automatically boot from the device if the "bootindex" property has been specified, too (e.g. when using "-device nec-usb-xhci -device usb-host,hostbus=1,hostaddr=2,bootindex=0" at the command line). The problem is that QEMU builds a device tree path like "/pci@800000020000000/usb@0/usb-host@1" and passes it to SLOF in the /chosen/qemu,boot-list property. SLOF, however, probes the USB device, recognizes that it is a storage device and thus changes its name to "storage", and additionally adds a child node for the SCSI LUN, so the correct boot path in SLOF is something like "/pci@800000020000000/usb@0/storage@1/disk@101000000000000" instead. So when we detect an USB mass storage device with SCSI interface, we've got to adjust the firmware boot-device path properly, so that SLOF can automatically boot from the device. Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1354177 Signed-off-by: Thomas Huth <thuth@redhat.com> --- hw/ppc/spapr.c | 9 +++++++++ hw/usb/host-libusb.c | 29 +++++++++++++++++++++++++++++ hw/usb/host-stub.c | 5 +++++ include/hw/usb.h | 1 + 4 files changed, 44 insertions(+)