Re: [PATCH for-7.2 2/2] ppc/pnv: fix QOM parenting of user creatable root ports

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 8/19/22 04:19, Cédric Le Goater wrote:
> On 8/19/22 00:45, Daniel Henrique Barboza wrote:
>> User creatable root ports are being parented by the 'peripheral' or the
>> 'peripheral-anon' container. This happens because this is the regular
>> QOM schema for sysbus devices that are added via the command line.
>>
>> Let's make this QOM hierarchy similar to what we have with default root
>> ports, i.e. the root port must be parented by the pnv-root-bus. To do
>> that we change the qom and bus parent of the root port during
>> root_port_realize(). The realize() is shared by the default root port
>> code path, so we can remove the code inside pnv_phb_attach_root_port()
>> that was adding the root port as a child of the bus as well.
>>
>> While we're at it, change pnv_phb_attach_root_port() to receive a PCIBus
>> instead of a PCIHostState to make it clear that the function does not
>> make use of the PHB.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/pci-host/pnv_phb.c | 38 +++++++++++++++++++-------------------
>>   1 file changed, 19 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
>> index 4ea33fb6ba..38ec8571b7 100644
>> --- a/hw/pci-host/pnv_phb.c
>> +++ b/hw/pci-host/pnv_phb.c
>> @@ -62,27 +62,11 @@ static bool pnv_parent_fixup(Object *parent, BusState *parent_bus,
>>       return true;
>>   }
>> -/*
>> - * Attach a root port device.
>> - *
>> - * 'index' will be used both as a PCIE slot value and to calculate
>> - * QOM id. 'chip_id' is going to be used as PCIE chassis for the
>> - * root port.
>> - */
>> -static void pnv_phb_attach_root_port(PCIHostState *pci)
>> +static void pnv_phb_attach_root_port(PCIBus *bus)
> 
> Do we still need this pnv_phb_attach_root_port routine ?

Good point. We can just add these 2 lines in the 'defaults_enabled' case
in pnv_phb_realize().

I'll re-send. Thanks,


Daniel



> 
>>   {
>>       PCIDevice *root = pci_new(PCI_DEVFN(0, 0), TYPE_PNV_PHB_ROOT_PORT);
>> -    const char *dev_id = DEVICE(root)->id;
>> -    g_autofree char *default_id = NULL;
>> -    int index;
>> -    index = object_property_get_int(OBJECT(pci->bus), "phb-id", &error_fatal);
>> -    default_id = g_strdup_printf("%s[%d]", TYPE_PNV_PHB_ROOT_PORT, index);
>> -
>> -    object_property_add_child(OBJECT(pci->bus), dev_id ? dev_id : default_id,
>> -                              OBJECT(root));
>> -
>> -    pci_realize_and_unref(root, pci->bus, &error_fatal);
>> +    pci_realize_and_unref(root, bus, &error_fatal);
>>   }
>>   /*
>> @@ -184,7 +168,7 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp)
>>           return;
>>       }
>> -    pnv_phb_attach_root_port(pci);
>> +    pnv_phb_attach_root_port(pci->bus);
>>   }
>>   static const char *pnv_phb_root_bus_path(PCIHostState *host_bridge,
>> @@ -259,6 +243,11 @@ static void pnv_phb_root_port_realize(DeviceState *dev, Error **errp)
>>       Error *local_err = NULL;
>>       int chip_id, index;
>> +    /*
>> +     * 'index' will be used both as a PCIE slot value and to calculate
>> +     * QOM id. 'chip_id' is going to be used as PCIE chassis for the
>> +     * root port.
>> +     */
>>       chip_id = object_property_get_int(OBJECT(bus), "chip-id", &error_fatal);
>>       index = object_property_get_int(OBJECT(bus), "phb-id", &error_fatal);
>> @@ -266,6 +255,17 @@ static void pnv_phb_root_port_realize(DeviceState *dev, Error **errp)
>>       qdev_prop_set_uint8(dev, "chassis", chip_id);
>>       qdev_prop_set_uint16(dev, "slot", index);
>> +    /*
>> +     * User created root ports are QOM parented to one of
>> +     * the peripheral containers but it's already at the right
>> +     * parent bus. Change the QOM parent to be the same as the
>> +     * parent bus it's already assigned to.
>> +     */
>> +    if (!pnv_parent_fixup(OBJECT(bus), BUS(bus), OBJECT(dev),
>> +                          index, errp)) {
>> +        return;
>> +    }
>> +
>>       rpc->parent_realize(dev, &local_err);
>>       if (local_err) {
>>           error_propagate(errp, local_err);
>