Re: [Qemu-devel] [PATCH v3 6/6] hw/pci-bridge: set explicit OFW unit address for TYPE_PXB_HOST

[Marcel Apfelbaum]

On 06/11/2015 01:26 PM, Laszlo Ersek wrote:
> On 06/11/15 12:21, Marcel Apfelbaum wrote:
> > On 06/11/2015 03:38 AM, Laszlo Ersek wrote:
> > > The PXB implementation doesn't allow firmware (SeaBIOS or OVMF) to boot
> > > off devices behind the PXB. This happens because the
> > > sysbus_get_fw_dev_path() function in "hw/core/sysbus.c" doesn't have
> > > enough information to format a unique identifier for the PXB in question,
> > > and consequently the OpenFirmware device path passed down to the guest
> > > firmware in the "bootorder" fw_cfg file is unusable for identifying the
> > > boot device.
> > >
> > > For example, the command line fragment
> > >
> > >     -device pxb,id=bridge1,bus_nr=4 \
> > >     \
> > >     -netdev user,id=netdev0 \
> > >     -device e1000,netdev=netdev0,bus=bridge1,addr=2,bootindex=0
> > >
> > > results in the following "bootorder" entry:
> > >
> > >     /pci/address@hidden/address@hidden/address@hidden
> > >
> > > The initial "pci" node is formatted by sysbus_get_fw_dev_path(), and the
> > > resultant OpenFirmware device path is independent of bus_nr=4 -- and
> > > therefore it is useless for identifying the device.
> > >
> > > In this patch we change the fw_name device class member of TYPE_PXB_HOST
> > > from "pci" to "pci-root", and set each instance's explicit OFW unit
> > > address to the PXB bus number. The same command line fragment results in
> > > the following OpenFirmware device path in the "bootorder" fw_cfg file:
> > >
> > >     /address@hidden/address@hidden/address@hidden/address@hidden
> > Hi Laszlo,
> >
> > I applied your patches but I still get
> > /address@hidden/address@hidden/address@hidden
> > in the boot list
> >
> > I checked and  the code enters only once in sysbus_get_fw_dev_path
> > for address@hidden and goes for pio branch.
> >
> > Do you know maybe what I missed?
>
> I think so, yes: you added the ...,bootorder=N property to a device that
> is *not* behind a PXB. :) You forgot the ...,bus=bridgeX property.

Actually:
-device pxb,id=bridge1,bus_nr=4 -netdev user,id=u \
-device 
e1000,id=net2,bus=bridge1,netdev=u,addr=0x5,bootindex=0,romfile=../pc-bios/efi-e1000.rom,bus=pci.0

Hmm :(



> Thanks
> Laszlo
>
> > Thanks,
> > Marcel
> >
> >
> > > The original, initial "/pci" fragment has been replaced with
> > > "/address@hidden", which (a) looks better, (b) provides all the necessary
> > > information.
> > >
> > > Cc: Markus Armbruster <address@hidden>
> > > Cc: Marcel Apfelbaum <address@hidden>
> > > Cc: Michael S. Tsirkin <address@hidden>
> > > Signed-off-by: Laszlo Ersek <address@hidden>
> > > ---
> > >
> > > Notes:
> > >       v3:
> > >       - using new sysbus device property, not inserting additional bus
> > >
> > >    hw/pci-bridge/pci_expander_bridge.c | 7 ++++++-
> > >    1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/pci-bridge/pci_expander_bridge.c
> > > b/hw/pci-bridge/pci_expander_bridge.c
> > > index c7a085d..d468670 100644
> > > --- a/hw/pci-bridge/pci_expander_bridge.c
> > > +++ b/hw/pci-bridge/pci_expander_bridge.c
> > > @@ -93,7 +93,7 @@ static void pxb_host_class_init(ObjectClass *class,
> > > void *data)
> > >        DeviceClass *dc = DEVICE_CLASS(class);
> > >        PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
> > >
> > > -    dc->fw_name = "pci";
> > > +    dc->fw_name = "pci-root";
> > >        hc->root_bus_path = pxb_host_root_bus_path;
> > >    }
> > >
> > > @@ -152,6 +152,7 @@ static int pxb_dev_initfn(PCIDevice *dev)
> > >    {
> > >        PXBDev *pxb = PXB_DEV(dev);
> > >        DeviceState *ds, *bds;
> > > +    char *bus_nr_str;
> > >        PCIBus *bus;
> > >        const char *dev_name = NULL;
> > >
> > > @@ -166,6 +167,10 @@ static int pxb_dev_initfn(PCIDevice *dev)
> > >        }
> > >
> > >        ds = qdev_create(NULL, TYPE_PXB_HOST);
> > > +    bus_nr_str = g_strdup_printf("%x", pxb->bus_nr);
> > > +    qdev_prop_set_string(ds, "explicit_ofw_unit_address", bus_nr_str);
> > > +    g_free(bus_nr_str);
> > > +
> > >        bus = pci_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS);
> > >
> > >        bus->parent_dev = dev;