[PATCH] boot: fix path pattern of scsi device

[Amos Kong]

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 
---
 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);
 }

[Paolo Bonzini]

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 [e-mail] when sending the QEMU patch.

Thanks,

Paolo
> Signed-off-by: Amos Kong
>
> 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);
>
> }

[Qemu-devel] [PATCH] boot: fix path pattern of scsi device

[Amos Kong]

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 [e-mail] when sending the QEMU patch.
>
> Thanks,
>
> Paolo
> > Signed-off-by: Amos Kong
> >
> > 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);
> >
> > }

[Qemu-devel] [PATCH] boot: fix path pattern of scsi device

[Paolo Bonzini]

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 [e-mail] when sending the QEMU patch.
> >
> > Thanks,
> >
> > Paolo
> > > Signed-off-by: Amos Kong
> > >
> > > 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);
> > >
> > > }

[Qemu-devel] [PATCH] boot: fix path pattern of scsi device

[Laszlo Ersek]

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 [e-mail] 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

[Qemu-devel] [PATCH] boot: fix path pattern of scsi device

[Amos Kong]

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 [e-mail] 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