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

[Laszlo Ersek <lersek@redhat.com>]

On 06/11/15 11:15, 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
> Clean
> 
>>
>> 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);
> 
> This is the best approach yet.
> I think it will not affect migration, because on the target site
> it will have the same bus number, so the property doesn't need to be
> passed.

Yes, I thought the same.

Migration should carry guest-alterable state. If, in some device,
"explicit_ofw_unit_address" will ever be changed due to guest actions
(as opposed to deriving it from a constant on the command line, which
management would sync anyway), then that specific device will have to
migrate the underlying state as it sees fit, and update the property in
a post load callback.

> 
> Is my time to see how it will work on Seabios.
> Thanks a lot for your help!!
> 
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks! I hope to hear back from Michael and Markus as well. Until then,
this should suffice to work on the firmware(s).

Laszlo

>> +
>>       bus = pci_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS);
>>
>>       bus->parent_dev = dev;
>>
>