Re: [PATCH for-7.2 04/10] ppc/pnv: add helpers for pnv-phb user devices

From: Daniel Henrique Barboza <danielhb413@gmail.com>
To: Frederic Barrat <fbarrat@linux.ibm.com>, qemu-devel@nongnu.org
Cc: qemu-ppc@nongnu.org, clg@kaod.org
Subject: Re: [PATCH for-7.2 04/10] ppc/pnv: add helpers for pnv-phb user devices
Date: Mon, 8 Aug 2022 08:04:12 -0300	[thread overview]
Message-ID: <20b12819-4891-1f1a-0e79-f9dcbedddc4e@gmail.com> (raw)
In-Reply-To: <f69358e3-cec5-196e-4b2f-342742b6c5b7@linux.ibm.com>

On 8/5/22 07:41, Frederic Barrat wrote:
> 
> 
> On 03/08/2022 15:44, Daniel Henrique Barboza wrote:
>> pnv_parent_qom_fixup() and pnv_parent_bus_fixup() are versions of the
>> helpers that were reverted by commit 9c10d86fee "ppc/pnv: Remove
>> user-created PHB{3,4,5} devices". They are needed to amend the QOM and
>> bus hierarchies of user created pnv-phbs, matching them with default
>> pnv-phbs.
>>
>> A new helper pnv_phb_user_device_init() is created to handle
>> user-created devices setup. We're going to call it inside
>> pnv_phb_realize() in case we're realizing an user created device. This
>> will centralize all user device realated in a single spot, leaving the
>> realize functions of the phb3/phb4 backends untouched.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/pci-host/pnv_phb.c | 69 +++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 69 insertions(+)
>>
>> diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
>> index 826c0c144e..da779dc298 100644
>> --- a/hw/pci-host/pnv_phb.c
>> +++ b/hw/pci-host/pnv_phb.c
>> @@ -18,6 +18,37 @@
>>   #include "hw/qdev-properties.h"
>>   #include "qom/object.h"
>> +
>> +/*
>> + * Set the QOM parent of an object child. If the device state
>> + * associated with the child has an id, use it as QOM id. Otherwise
>> + * use object_typename[index] as QOM id.
>> + */
>> +static void pnv_parent_qom_fixup(Object *parent, Object *child, int index)
>> +{
>> +    g_autofree char *default_id =
>> +        g_strdup_printf("%s[%d]", object_get_typename(child), index);
>> +    const char *dev_id = DEVICE(child)->id;
>> +
>> +    if (child->parent == parent) {
>> +        return;
>> +    }
>> +
>> +    object_ref(child);
>> +    object_unparent(child);
>> +    object_property_add_child(parent, dev_id ? dev_id : default_id, child);
>> +    object_unref(child);
>> +}
>> +
>> +static void pnv_parent_bus_fixup(DeviceState *parent, DeviceState *child)
>> +{
>> +    BusState *parent_bus = qdev_get_parent_bus(parent);
>> +
>> +    if (!qdev_set_parent_bus(child, parent_bus, &error_fatal)) {
>> +        return;
>> +    }
>> +}
>> +
>>   /*
>>    * Attach a root port device.
>>    *
>> @@ -41,6 +72,36 @@ static void pnv_phb_attach_root_port(PCIHostState *pci)
>>       pci_realize_and_unref(root, pci->bus, &error_fatal);
>>   }
>> +/*
>> + * User created devices won't have the initial setup that default
>> + * devices have. This setup consists of assigning a parent device
>> + * (chip for PHB3, PEC for PHB4/5) that will be the QOM/bus parent
>> + * of the PHB.
>> + */
>> +static void pnv_phb_user_device_init(PnvPHB *phb)
>> +{
>> +    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
>> +    PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
>> +    Object *parent = NULL;
>> +
>> +    if (!chip) {
>> +        error_setg(&error_fatal, "invalid chip id: %d", phb->chip_id);
>> +        return;
>> +    }
>> +
>> +    /*
>> +     * Reparent user created devices to the chip to build
>> +     * correctly the device tree. pnv_xscom_dt() needs every
>> +     * PHB to be a child of the chip to build the DT correctly.
>> +     *
>> +     * TODO: for version 3 we're still parenting the PHB with the
>> +     * chip. We should parent with a (so far not implemented)
>> +     * PHB3 PEC device.
>> +     */
>> +    pnv_parent_qom_fixup(parent, OBJECT(phb), phb->phb_id);
> 
> 
> Here we reparent the phb to NULL. I can see in the following patches why it's done this way and the end result is ok. After this patch, we can't create a user device yet, so no harm can be done. But a (temporary) comment could save the reviewer a heart attack :-)

Nah that was a mistake, I didn't intend to leave the code to be reparenting
stuff to NULL, even if it's not possible to break stuff at this moment.

I'll amend it like this:

+    if (phb->version == 3) {
+        parent = OBJECT(chip);
+    } else {
+        /* phb4 support will be added later */
+        return;
+    }
+

Thanks,

Daniel

> 
>    Fred
> 
> 
>> +    pnv_parent_bus_fixup(DEVICE(chip), DEVICE(phb));
>> +}
>> +
>>   static void pnv_phb_realize(DeviceState *dev, Error **errp)
>>   {
>>       PnvPHB *phb = PNV_PHB(dev);
>> @@ -74,6 +135,14 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp)
>>       object_property_set_uint(phb->backend, "chip-id", phb->chip_id, errp);
>>       object_property_set_link(phb->backend, "phb-base", OBJECT(phb), errp);
>> +    /*
>> +     * Handle user created devices. User devices will not have a
>> +     * pointer to a chip (PHB3) and a PEC (PHB4/5).
>> +     */
>> +    if (!phb->chip && !phb->pec) {
>> +        pnv_phb_user_device_init(phb);
>> +    }
>> +
>>       if (phb->version == 3) {
>>           object_property_set_link(phb->backend, "chip",
>>                                    OBJECT(phb->chip), errp);