| Message ID | 1369726806-9715-1-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 28/05/2013 09:40, Amos Kong ha scritto: > bootindex parameter of scsi device doesn't work, it causes > by wrong pattern in seabios. > > qemu passes the following firmware dev_path to seabios: > /pci@i0cf8/scsi@4/virtio-scsi-device/channel@0/disk@0,0 No, this is another unexpected change due to the virtio refactoring in QEMU. The right fix is in QEMU, by adding a get_fw_dev_path implementation in hw/virtio/virtio-bus.c. We fixed it already for migration paths, it should be easy to do the same for this. Please Cc qemu-stable@nongnu.org when sending the QEMU patch. Thanks, Paolo > Signed-off-by: Amos Kong <akong@redhat.com> > --- > src/boot.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/src/boot.c b/src/boot.c > index c308602..cd9d784 100644 > --- a/src/boot.c > +++ b/src/boot.c > @@ -138,10 +138,11 @@ int bootprio_find_scsi_device(struct pci_device *pci, int target, int lun) > if (!pci) > // support only pci machine for now > return -1; > - // Find scsi drive - for example: /pci@i0cf8/scsi@5/channel@0/disk@1,0 > + /* Find scsi drive - for example: > + /pci@i0cf8/scsi@5/virtio-scsi-device/channel@0/disk@1,0 */ > char desc[256], *p; > p = build_pci_path(desc, sizeof(desc), "*", pci); > - snprintf(p, desc+sizeof(desc)-p, "/*@0/*@%d,%d", target, lun); > + snprintf(p, desc+sizeof(desc)-p, "/*/*@0/*@%d,%d", target, lun); > return find_prio(desc); > } > >
On Tue, May 28, 2013 at 10:06:51AM +0200, Paolo Bonzini wrote: > Il 28/05/2013 09:40, Amos Kong ha scritto: > > bootindex parameter of scsi device doesn't work, it causes > > by wrong pattern in seabios. > > > > qemu passes the following firmware dev_path to seabios: > > /pci@i0cf8/scsi@4/virtio-scsi-device/channel@0/disk@0,0 > > No, this is another unexpected change due to the virtio refactoring in > QEMU. The right fix is in QEMU, by adding a get_fw_dev_path > implementation in hw/virtio/virtio-bus.c. Hi Paolo, We could not fix this by implementing get_fw_dev_path in hw/virtio/virtio-bus.c virtio-bus is the parent bus of scsi-bus, scsibus_get_fw_dev_path() will be called after called virtio_bus_get_fw_dev_path(). str0 = returns of parents of virtio-bus /* eg: /pci@i0cf8/scsi@4 */ str1 = return of virtio_bus_get_fw_dev_path() /* eg: .. */ str2 = scsibus_get_fw_dev_path() /* eg: channel@0/disk@0,0 */ The final fw_dev_path should be $str0/$str1/$str2 | static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size) | { | int l = 0; | | if (dev && dev->parent_bus) { | char *d; | l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size); | d = bus_get_fw_dev_path(dev->parent_bus, dev); | if (d) { | l += snprintf(p + l, size - l, "%s", d); | g_free(d); if we implement virtio_bus_get_fw_dev_path(), the return value will be set to $str1 | } else { | l += snprintf(p + l, size - l, "%s", object_get_typename(OBJECT(dev))); Currently we didn't implement virtio_bus_get_fw_dev_path(), so 'virtio-scsi-device' was set to $str1 | } | However, we need to change the pattern. Amos. > We fixed it already for migration paths, it should be easy to do the > same for this. > > Please Cc qemu-stable@nongnu.org when sending the QEMU patch. > > Thanks, > > Paolo > > > Signed-off-by: Amos Kong <akong@redhat.com> > > --- > > src/boot.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/src/boot.c b/src/boot.c > > index c308602..cd9d784 100644 > > --- a/src/boot.c > > +++ b/src/boot.c > > @@ -138,10 +138,11 @@ int bootprio_find_scsi_device(struct pci_device *pci, int target, int lun) > > if (!pci) > > // support only pci machine for now > > return -1; > > - // Find scsi drive - for example: /pci@i0cf8/scsi@5/channel@0/disk@1,0 > > + /* Find scsi drive - for example: > > + /pci@i0cf8/scsi@5/virtio-scsi-device/channel@0/disk@1,0 */ > > char desc[256], *p; > > p = build_pci_path(desc, sizeof(desc), "*", pci); > > - snprintf(p, desc+sizeof(desc)-p, "/*@0/*@%d,%d", target, lun); > > + snprintf(p, desc+sizeof(desc)-p, "/*/*@0/*@%d,%d", target, lun); > > return find_prio(desc); > > } > > > >
Il 28/05/2013 11:35, Amos Kong ha scritto: > On Tue, May 28, 2013 at 10:06:51AM +0200, Paolo Bonzini wrote: >> Il 28/05/2013 09:40, Amos Kong ha scritto: >>> bootindex parameter of scsi device doesn't work, it causes >>> by wrong pattern in seabios. >>> >>> qemu passes the following firmware dev_path to seabios: >>> /pci@i0cf8/scsi@4/virtio-scsi-device/channel@0/disk@0,0 >> >> No, this is another unexpected change due to the virtio refactoring in >> QEMU. The right fix is in QEMU, by adding a get_fw_dev_path >> implementation in hw/virtio/virtio-bus.c. > > Hi Paolo, > > We could not fix this by implementing get_fw_dev_path in > hw/virtio/virtio-bus.c > > virtio-bus is the parent bus of scsi-bus, scsibus_get_fw_dev_path() > will be called after called virtio_bus_get_fw_dev_path(). > > str0 = returns of parents of virtio-bus /* eg: /pci@i0cf8/scsi@4 */ > str1 = return of virtio_bus_get_fw_dev_path() /* eg: .. */ > str2 = scsibus_get_fw_dev_path() /* eg: channel@0/disk@0,0 */ > > The final fw_dev_path should be $str0/$str1/$str2 > > | static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size) > | { > | int l = 0; > | > | if (dev && dev->parent_bus) { > | char *d; > | l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size); > | d = bus_get_fw_dev_path(dev->parent_bus, dev); > | if (d) { > | l += snprintf(p + l, size - l, "%s", d); > | g_free(d); > > if we implement virtio_bus_get_fw_dev_path(), the return value will be set to $str1 > > | } else { > | l += snprintf(p + l, size - l, "%s", object_get_typename(OBJECT(dev))); > > Currently we didn't implement virtio_bus_get_fw_dev_path(), so 'virtio-scsi-device' was set to $str1 > > | } > | > > However, we need to change the pattern. Turn this: l += snprintf(p + l, size - l, "%s", object_get_typename(OBJECT(dev))); into the default implementation of get_fw_dev_path (in TYPE_BUS), and change qdev_get_fw_dev_path_helper to if (d) { l += snprintf(p + l, size - l, "%s", d); g_free(d); } else { return l; } Then virtio_bus_get_fw_dev_path can just return NULL. Paolo > Amos. > >> We fixed it already for migration paths, it should be easy to do the >> same for this. >> >> Please Cc qemu-stable@nongnu.org when sending the QEMU patch. >> >> Thanks, >> >> Paolo >> >>> Signed-off-by: Amos Kong <akong@redhat.com> >>> --- >>> src/boot.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/src/boot.c b/src/boot.c >>> index c308602..cd9d784 100644 >>> --- a/src/boot.c >>> +++ b/src/boot.c >>> @@ -138,10 +138,11 @@ int bootprio_find_scsi_device(struct pci_device *pci, int target, int lun) >>> if (!pci) >>> // support only pci machine for now >>> return -1; >>> - // Find scsi drive - for example: /pci@i0cf8/scsi@5/channel@0/disk@1,0 >>> + /* Find scsi drive - for example: >>> + /pci@i0cf8/scsi@5/virtio-scsi-device/channel@0/disk@1,0 */ >>> char desc[256], *p; >>> p = build_pci_path(desc, sizeof(desc), "*", pci); >>> - snprintf(p, desc+sizeof(desc)-p, "/*@0/*@%d,%d", target, lun); >>> + snprintf(p, desc+sizeof(desc)-p, "/*/*@0/*@%d,%d", target, lun); >>> return find_prio(desc); >>> } >>> >>>
On 05/28/13 10:06, Paolo Bonzini wrote: > Il 28/05/2013 09:40, Amos Kong ha scritto: >> bootindex parameter of scsi device doesn't work, it causes >> by wrong pattern in seabios. >> >> qemu passes the following firmware dev_path to seabios: >> /pci@i0cf8/scsi@4/virtio-scsi-device/channel@0/disk@0,0 > > No, this is another unexpected change due to the virtio refactoring in > QEMU. The right fix is in QEMU, by adding a get_fw_dev_path > implementation in hw/virtio/virtio-bus.c. > > We fixed it already for migration paths, it should be easy to do the > same for this. > > Please Cc qemu-stable@nongnu.org when sending the QEMU patch. > > Thanks, > > Paolo Ahhh. I was super confused by this patch initially. Amos, when posting a patch to both lists, please add the project name to the bracketed bag-of-tags in the subject, like [SeaBIOS PATCH] boot: fix path pattern of scsi device I saw this message first on qemu-devel, and until I noticed "src/boot.c" I was kind of confused whom you want to adapt to whom, and in what direction Paolo argues against it. So, virtio refactoring in QEMU (care to name a commit or release?) changed the OpenFirmware device path exported for virtio-scsi devices under the boot order fw_cfg key. This patch intended to adapt SeaBIOS to recognize the new OFW devpath. Under this approach I would have to update QemuBootOrder.c in OVMF in parallel, so that it accepts both old and new style OFW devpaths for virtio-scsi. However Paolo says the new style OFW devpath should be fixed (eliminated) in qemu, and consumers shouldn't notice any change in the long term. And I won't have to change QemuBootOrder.c. Right? Thanks! Laszlo
On Tue, May 28, 2013 at 12:26:34PM +0200, Laszlo Ersek wrote: > On 05/28/13 10:06, Paolo Bonzini wrote: > > Il 28/05/2013 09:40, Amos Kong ha scritto: > >> bootindex parameter of scsi device doesn't work, it causes > >> by wrong pattern in seabios. > >> > >> qemu passes the following firmware dev_path to seabios: > >> /pci@i0cf8/scsi@4/virtio-scsi-device/channel@0/disk@0,0 > > > > No, this is another unexpected change due to the virtio refactoring in > > QEMU. The right fix is in QEMU, by adding a get_fw_dev_path > > implementation in hw/virtio/virtio-bus.c. > > > > We fixed it already for migration paths, it should be easy to do the > > same for this. > > > > Please Cc qemu-stable@nongnu.org when sending the QEMU patch. > > > > Thanks, > > > > Paolo > > Ahhh. I was super confused by this patch initially. > > Amos, when posting a patch to both lists, please add the project name to > the bracketed bag-of-tags in the subject, like > > [SeaBIOS PATCH] boot: fix path pattern of scsi device Sorry for the mistiness. > I saw this message first on qemu-devel, and until I noticed "src/boot.c" > I was kind of confused whom you want to adapt to whom, and in what > direction Paolo argues against it. > > So, virtio refactoring in QEMU (care to name a commit or release?) > changed the OpenFirmware device path exported for virtio-scsi devices > under the boot order fw_cfg key. This patch intended to adapt SeaBIOS to > recognize the new OFW devpath. Under this approach I would have to > update QemuBootOrder.c in OVMF in parallel, so that it accepts both old > and new style OFW devpaths for virtio-scsi. > > However Paolo says the new style OFW devpath should be fixed > (eliminated) in qemu, and consumers shouldn't notice any change in the > long term. And I won't have to change QemuBootOrder.c. Right? Just sent a qemu patch to fix this problem. We will keep original style devpath. > Thanks! > Laszlo
diff --git a/src/boot.c b/src/boot.c index c308602..cd9d784 100644 --- a/src/boot.c +++ b/src/boot.c @@ -138,10 +138,11 @@ int bootprio_find_scsi_device(struct pci_device *pci, int target, int lun) if (!pci) // support only pci machine for now return -1; - // Find scsi drive - for example: /pci@i0cf8/scsi@5/channel@0/disk@1,0 + /* Find scsi drive - for example: + /pci@i0cf8/scsi@5/virtio-scsi-device/channel@0/disk@1,0 */ char desc[256], *p; p = build_pci_path(desc, sizeof(desc), "*", pci); - snprintf(p, desc+sizeof(desc)-p, "/*@0/*@%d,%d", target, lun); + snprintf(p, desc+sizeof(desc)-p, "/*/*@0/*@%d,%d", target, lun); return find_prio(desc); }
bootindex parameter of scsi device doesn't work, it causes by wrong pattern in seabios. qemu passes the following firmware dev_path to seabios: /pci@i0cf8/scsi@4/virtio-scsi-device/channel@0/disk@0,0 Signed-off-by: Amos Kong <akong@redhat.com> --- src/boot.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)