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

[Marcel Apfelbaum <marcel@redhat.com>]

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/pci-bridge@0/ethernet@2/ethernet-phy@0
>>>
>>> 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:
>>>
>>>     /pci-root@4/pci-bridge@0/ethernet@2/ethernet-phy@0
>> Hi Laszlo,
>>
>> I applied your patches but I still get
>> /pci@i0cf8/ethernet@5/ethernet-phy@0
>> in the boot list
>>
>> I checked and  the code enters only once in sysbus_get_fw_dev_path
>> for pci@i0cf8 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
>>> "/pci-root@4", which (a) looks better, (b) provides all the necessary
>>> information.
>>>
>>> Cc: Markus Armbruster <armbru@redhat.com>
>>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>
>>> 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;
>>>
>>
>