Re: [PATCH v2 3/9] ppc/pnv: use dev->parent_bus->parent to get the PHB

[Mark Cave-Ayland]

On 18/06/2022 12:01, Daniel Henrique Barboza wrote:

> It is not advisable to execute an object_dynamic_cast() to poke into
> bus->qbus.parent and follow it up with a C cast into the PnvPHB type we
> think we got.
>
> A better way is to access the PnvPHB object via a QOM macro accessing
> the existing parent links of the DeviceState. For a given
> pnv-phb3/4-root-port 'dev', dev->parent_bus will give us the PHB bus,
> and dev->parent_bus->parent is the PHB. Use the adequate QOM macro to
> assert the type, and keep the NULL check in case we didn't get the
> object we were expecting.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb3.c | 10 +++++++---
>   hw/pci-host/pnv_phb4.c | 10 +++++++---
>   2 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index 4ba660f8b9..5e7f827415 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1139,12 +1139,16 @@ static void pnv_phb3_root_port_realize(DeviceState 
> *dev, Error **errp)
>   {
>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
>       PCIDevice *pci = PCI_DEVICE(dev);
> -    PCIBus *bus = pci_get_bus(pci);
>       PnvPHB3 *phb = NULL;
>       Error *local_err = NULL;
>   
> -    phb = (PnvPHB3 *) object_dynamic_cast(OBJECT(bus->qbus.parent),
> -                                          TYPE_PNV_PHB3);
> +    /*
> +     * dev->parent_bus gives access to the pnv-phb-root bus.
> +     * The PnvPHB3 is the owner (parent) of the bus.
> +     */
> +    if (dev->parent_bus) {

Here dev->parent_bus shouldn't be accessed directly: you should use 
qdev_get_parent_bus() instead.

> +        phb = PNV_PHB3(dev->parent_bus->parent);
> +    }

This one is a bit trickier, since part of the qdev design is that devices should only 
be aware of their immediate bus, and not the device parenting that bus i.e. 
dev->parent_bus->parent shouldn't be allowed.

What is really needed here is to use QOM links (or embed the device as a suitable QOM 
child) to get the PHB reference which I imagine will be changed as part of the 
follow-up series. So I think this can be left as-is for now, and fixed later.

>       if (!phb) {
>           error_setg(errp,
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index ffd9d8a947..a0ee52e820 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1782,12 +1782,16 @@ static void pnv_phb4_root_port_realize(DeviceState 
> *dev, Error **errp)
>   {
>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
>       PCIDevice *pci = PCI_DEVICE(dev);
> -    PCIBus *bus = pci_get_bus(pci);
>       PnvPHB4 *phb = NULL;
>       Error *local_err = NULL;
>   
> -    phb = (PnvPHB4 *) object_dynamic_cast(OBJECT(bus->qbus.parent),
> -                                          TYPE_PNV_PHB4);
> +    /*
> +     * dev->parent_bus gives access to the pnv-phb-root bus.
> +     * The PnvPHB4 is the owner (parent) of the bus.
> +     */
> +    if (dev->parent_bus) {
> +        phb = PNV_PHB4(dev->parent_bus->parent);
> +    }
>   
>       if (!phb) {
>           error_setg(errp, "%s must be connected to pnv-phb4 buses", dev->id);

I've had a quick look over the rest of the series and from what I can see this is 
definitely heading in the right direction :)


ATB,

Mark.