Re: [PATCH v3 3/8] ppc/pnv: assign pnv-phb-root-port chassis/slot earlier

[Frederic Barrat]

On 21/06/2022 19:34, 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.
>
> In fact this is not needed. There is nothing sophisticated being done
> with the PHB object retrieved during root_port_realize() for both
> PHB3 and PHB4. We're retrieving a PHB reference PHB just to access
> phb->chip_id and phb->phb_id and use them to define the chassis/slot
> of the root port.
>
> phb->phb_id is already being passed to pnv_phb_attach_root_port() via
> the 'index' parameter. Let's also add a 'chip_id' parameter to this
> function and assign chassis and slot right there. This will spare us
> from the hassle of accessing the PHB object inside realize().
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---


Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>

  Fred


>   hw/pci-host/pnv_phb3.c | 18 ++----------------
>   hw/pci-host/pnv_phb4.c | 18 ++----------------
>   hw/ppc/pnv.c           | 15 +++++++++++++--
>   include/hw/ppc/pnv.h   |  3 ++-
>   4 files changed, 19 insertions(+), 35 deletions(-)
>
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index 4ba660f8b9..afe5698167 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1052,7 +1052,8 @@ static void pnv_phb3_realize(DeviceState *dev, Error 
> **errp)
>   
>       pci_setup_iommu(pci->bus, pnv_phb3_dma_iommu, phb);
>   
> -    pnv_phb_attach_root_port(pci, TYPE_PNV_PHB3_ROOT_PORT, phb->phb_id);
> +    pnv_phb_attach_root_port(pci, TYPE_PNV_PHB3_ROOT_PORT,
> +                             phb->phb_id, phb->chip_id);
>   }
>   
>   void pnv_phb3_update_regions(PnvPHB3 *phb)
> @@ -1139,23 +1140,8 @@ 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);
> -
> -    if (!phb) {
> -        error_setg(errp,
> -"pnv_phb3_root_port devices must be connected to pnv-phb3 buses");
> -        return;
> -    }
> -
> -    /* Set unique chassis/slot values for the root port */
> -    qdev_prop_set_uint8(&pci->qdev, "chassis", phb->chip_id);
> -    qdev_prop_set_uint16(&pci->qdev, "slot", phb->phb_id);
> -
>       rpc->parent_realize(dev, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index ffd9d8a947..725b3d740b 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1585,7 +1585,8 @@ static void pnv_phb4_realize(DeviceState *dev, Error 
> **errp)
>       pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>   
>       /* Add a single Root port if running with defaults */
> -    pnv_phb_attach_root_port(pci, pecc->rp_model, phb->phb_id);
> +    pnv_phb_attach_root_port(pci, pecc->rp_model,
> +                             phb->phb_id, phb->chip_id);
>   
>       /* Setup XIVE Source */
>       if (phb->big_phb) {
> @@ -1781,23 +1782,8 @@ static void pnv_phb4_root_port_reset(DeviceState *dev)
>   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);
> -
> -    if (!phb) {
> -        error_setg(errp, "%s must be connected to pnv-phb4 buses", dev->id);
> -        return;
> -    }
> -
> -    /* Set unique chassis/slot values for the root port */
> -    qdev_prop_set_uint8(&pci->qdev, "chassis", phb->chip_id);
> -    qdev_prop_set_uint16(&pci->qdev, "slot", phb->phb_id);
> -
>       rpc->parent_realize(dev, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 40e0cbd84d..c5e63bede7 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1189,8 +1189,15 @@ static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error 
> **errp)
>       }
>   }
>   
> -/* Attach a root port device */
> -void pnv_phb_attach_root_port(PCIHostState *pci, const char *name, int index)
> +/*
> + * 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.
> + */
> +void pnv_phb_attach_root_port(PCIHostState *pci, const char *name,
> +                              int index, int chip_id)
>   {
>       PCIDevice *root = pci_new(PCI_DEVFN(0, 0), name);
>       g_autofree char *default_id = g_strdup_printf("%s[%d]", name, index);
> @@ -1199,6 +1206,10 @@ void pnv_phb_attach_root_port(PCIHostState *pci, const 
> char *name, int index)
>       object_property_add_child(OBJECT(pci->bus), dev_id ? dev_id : default_id,
>                                 OBJECT(root));
>   
> +    /* Set unique chassis/slot values for the root port */
> +    qdev_prop_set_uint8(DEVICE(root), "chassis", chip_id);
> +    qdev_prop_set_uint16(DEVICE(root), "slot", index);
> +
>       pci_realize_and_unref(root, pci->bus, &error_fatal);
>   }
>   
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 033890a23f..b991194223 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -189,7 +189,8 @@ DECLARE_INSTANCE_CHECKER(PnvChip, PNV_CHIP_POWER10,
>                            TYPE_PNV_CHIP_POWER10)
>   
>   PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
> -void pnv_phb_attach_root_port(PCIHostState *pci, const char *name, int index);
> +void pnv_phb_attach_root_port(PCIHostState *pci, const char *name,
> +                              int index, int chip_id);
>   
>   #define TYPE_PNV_MACHINE       MACHINE_TYPE_NAME("powernv")
>   typedef struct PnvMachineClass PnvMachineClass;