Re: [PATCH v2 03/16] ppc/pnv: add PnvPHB base/proxy device

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

On 6/7/22 05:44, Frederic Barrat wrote:
> 
> 
> On 07/06/2022 08:42, Cédric Le Goater wrote:
>> On 6/2/22 18:16, Frederic Barrat wrote:
>>>
>>>
>>> On 31/05/2022 23:49, Daniel Henrique Barboza wrote:
>>>> The PnvPHB device is going to be the base device for all other powernv
>>>> PHBs. It consists of a device that has the same user API as the other
>>>> PHB, namely being a PCIHostBridge and having chip-id and index
>>>> properties. It also has a 'backend' pointer that will be initialized
>>>> with the PHB implementation that the device is going to use.
>>>>
>>>> The initialization of the PHB backend is done by checking the PHB
>>>> version via a 'version' attribute that can be set via a global machine
>>>> property.  The 'version' field will be used to make adjustments based on
>>>> the running version, e.g. PHB3 uses a 'chip' reference while PHB4 uses
>>>> 'pec'. To init the PnvPHB bus we'll rely on helpers for each version.
>>>> The version 3 helper is already added (pnv_phb3_bus_init), the PHB4
>>>> helper will be added later on.
>>>>
>>>> For now let's add the basic logic of the PnvPHB object, which consists
>>>> mostly of pnv_phb_realize() doing all the work of checking the
>>>> phb->version set, initializing the proper backend, passing through its
>>>> attributes to the chosen backend, finalizing the backend realize and
>>>> adding a root port in the end.
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> ---
>>>>   hw/pci-host/meson.build |   3 +-
>>>>   hw/pci-host/pnv_phb.c   | 123 ++++++++++++++++++++++++++++++++++++++++
>>>>   hw/pci-host/pnv_phb.h   |  39 +++++++++++++
>>>>   3 files changed, 164 insertions(+), 1 deletion(-)
>>>>   create mode 100644 hw/pci-host/pnv_phb.c
>>>>   create mode 100644 hw/pci-host/pnv_phb.h
>>>>
>>>> diff --git a/hw/pci-host/meson.build b/hw/pci-host/meson.build
>>>> index c07596d0d1..e832babc9d 100644
>>>> --- a/hw/pci-host/meson.build
>>>> +++ b/hw/pci-host/meson.build
>>>> @@ -35,5 +35,6 @@ specific_ss.add(when: 'CONFIG_PCI_POWERNV', if_true: files(
>>>>     'pnv_phb3_msi.c',
>>>>     'pnv_phb3_pbcq.c',
>>>>     'pnv_phb4.c',
>>>> -  'pnv_phb4_pec.c'
>>>> +  'pnv_phb4_pec.c',
>>>> +  'pnv_phb.c',
>>>>   ))
>>>> diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
>>>> new file mode 100644
>>>> index 0000000000..fa8472622f
>>>> --- /dev/null
>>>> +++ b/hw/pci-host/pnv_phb.c
>>>> @@ -0,0 +1,123 @@
>>>> +/*
>>>> + * QEMU PowerPC PowerNV Proxy PHB model
>>>> + *
>>>> + * Copyright (c) 2022, IBM Corporation.
>>>> + *
>>>> + * This code is licensed under the GPL version 2 or later. See the
>>>> + * COPYING file in the top-level directory.
>>>> + */
>>>> +
>>>> +#include "qemu/osdep.h"
>>>> +#include "qemu/log.h"
>>>> +#include "qapi/visitor.h"
>>>> +#include "qapi/error.h"
>>>> +#include "hw/pci-host/pnv_phb.h"
>>>> +#include "hw/pci-host/pnv_phb3.h"
>>>> +#include "hw/pci-host/pnv_phb4.h"
>>>> +#include "hw/ppc/pnv.h"
>>>> +#include "hw/qdev-properties.h"
>>>> +#include "qom/object.h"
>>>> +
>>>> +
>>>> +static void pnv_phb_realize(DeviceState *dev, Error **errp)
>>>> +{
>>>> +    PnvPHB *phb = PNV_PHB(dev);
>>>> +    PCIHostState *pci = PCI_HOST_BRIDGE(dev);
>>>> +    g_autofree char *phb_typename = NULL;
>>>> +    g_autofree char *phb_rootport_typename = NULL;
>>>> +
>>>> +    if (!phb->version) {
>>>> +        error_setg(errp, "version not specified");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    switch (phb->version) {
>>>> +    case 3:
>>>> +        phb_typename = g_strdup(TYPE_PNV_PHB3);
>>>> +        phb_rootport_typename = g_strdup(TYPE_PNV_PHB3_ROOT_PORT);
>>>> +        break;
>>>> +    case 4:
>>>> +        phb_typename = g_strdup(TYPE_PNV_PHB4);
>>>> +        phb_rootport_typename = g_strdup(TYPE_PNV_PHB4_ROOT_PORT);
>>>> +        break;
>>>> +    case 5:
>>>> +        phb_typename = g_strdup(TYPE_PNV_PHB5);
>>>> +        phb_rootport_typename = g_strdup(TYPE_PNV_PHB5_ROOT_PORT);
>>>> +        break;
>>>> +    default:
>>>> +        g_assert_not_reached();
>>>> +    }
>>>> +
>>>> +    phb->backend = object_new(phb_typename);
>>>> +    object_property_add_child(OBJECT(dev), "phb-device", phb->backend);
>>>> +
>>>> +    /* Passthrough child device properties to the proxy device */
>>>> +    object_property_set_uint(phb->backend, "index", phb->phb_id, errp);
>>>> +    object_property_set_uint(phb->backend, "chip-id", phb->chip_id, errp);
>>>> +    object_property_set_link(phb->backend, "phb-base", OBJECT(phb), errp);
>>>> +
>>>> +    if (phb->version == 3) {
>>>> +        object_property_set_link(phb->backend, "chip",
>>>> +                                 OBJECT(phb->chip), errp);
>>>> +    } else {
>>>> +        object_property_set_link(phb->backend, "pec", OBJECT(phb->pec), errp);
>>>> +    }
>>>
>>>
>>> The patch is fine, but it just highlights that we're doing something wrong. I don't believe there's any reason for the chip/pec/phb relationship to be different between P8 and P9/P10. One day, a brave soul could try to unify the models, it would avoid test like that.
>>
>> I think this is the first thing to do: introduce a PEC model to match
>> P8 HW and unify PHB3/4/5. We have been dragging this for too long (2014 ...)
> 
> 
> I agree it's needed, but it's also orthogonal to this series. And I wouldn't force it on Daniel since we're getting into PCI territory.

I don't mind doing it after this series lands.


Thanks,


Daniel

> 
>    Fred
> 
> 
>>
>> It shouldn't be that complex with all the cleanups that have been done.
>>
>> Thanks,
>>
>> C.
>>
>>
>>> It would be a good cleanup series to do if we ever extend the model with yet another version :-)
>>>
>>>
>>>
>>>
>>>> +
>>>> +    if (!qdev_realize(DEVICE(phb->backend), NULL, errp)) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (phb->version == 3) {
>>>> +        pnv_phb3_bus_init(dev, (PnvPHB3 *)phb->backend);
>>>> +    }
>>>> +
>>>> +    pnv_phb_attach_root_port(pci, phb_rootport_typename);
>>>
>>>
>>>
>>> After we've removed the other instances (done in later patches), we could move pnv_phb_attach_root_port() to pnv_phb.c instead of pnv.c. It would be the perfect home for it as it starts looking off in pnv.c
>>>
>>>    Fred
>>>
>>>
>>>
>>>> +}
>>>> +
>>>> +static const char *pnv_phb_root_bus_path(PCIHostState *host_bridge,
>>>> +                                         PCIBus *rootbus)
>>>> +{
>>>> +    PnvPHB *phb = PNV_PHB(host_bridge);
>>>> +
>>>> +    snprintf(phb->bus_path, sizeof(phb->bus_path), "00%02x:%02x",
>>>> +             phb->chip_id, phb->phb_id);
>>>> +    return phb->bus_path;
>>>> +}
>>>> +
>>>> +static Property pnv_phb_properties[] = {
>>>> +        DEFINE_PROP_UINT32("index", PnvPHB, phb_id, 0),
>>>> +        DEFINE_PROP_UINT32("chip-id", PnvPHB, chip_id, 0),
>>>> +        DEFINE_PROP_UINT32("version", PnvPHB, version, 0),
>>>> +
>>>> +        DEFINE_PROP_LINK("chip", PnvPHB, chip, TYPE_PNV_CHIP, PnvChip *),
>>>> +
>>>> +        DEFINE_PROP_LINK("pec", PnvPHB, pec, TYPE_PNV_PHB4_PEC,
>>>> +                         PnvPhb4PecState *),
>>>> +
>>>> +        DEFINE_PROP_END_OF_LIST(),
>>>> +};
>>>> +
>>>> +static void pnv_phb_class_init(ObjectClass *klass, void *data)
>>>> +{
>>>> +    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
>>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>>> +
>>>> +    hc->root_bus_path = pnv_phb_root_bus_path;
>>>> +    dc->realize = pnv_phb_realize;
>>>> +    device_class_set_props(dc, pnv_phb_properties);
>>>> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>>> +    dc->user_creatable = true;
>>>> +}
>>>> +
>>>> +static void pnv_phb_register_type(void)
>>>> +{
>>>> +    static const TypeInfo pnv_phb_type_info = {
>>>> +        .name          = TYPE_PNV_PHB,
>>>> +        .parent        = TYPE_PCIE_HOST_BRIDGE,
>>>> +        .instance_size = sizeof(PnvPHB),
>>>> +        .class_init    = pnv_phb_class_init,
>>>> +    };
>>>> +
>>>> +    type_register_static(&pnv_phb_type_info);
>>>> +}
>>>> +type_init(pnv_phb_register_type)
>>>> diff --git a/hw/pci-host/pnv_phb.h b/hw/pci-host/pnv_phb.h
>>>> new file mode 100644
>>>> index 0000000000..a7cc8610e2
>>>> --- /dev/null
>>>> +++ b/hw/pci-host/pnv_phb.h
>>>> @@ -0,0 +1,39 @@
>>>> +/*
>>>> + * QEMU PowerPC PowerNV Proxy PHB model
>>>> + *
>>>> + * Copyright (c) 2022, IBM Corporation.
>>>> + *
>>>> + * This code is licensed under the GPL version 2 or later. See the
>>>> + * COPYING file in the top-level directory.
>>>> + */
>>>> +
>>>> +#ifndef PCI_HOST_PNV_PHB_H
>>>> +#define PCI_HOST_PNV_PHB_H
>>>> +
>>>> +#include "hw/pci/pcie_host.h"
>>>> +#include "hw/pci/pcie_port.h"
>>>> +#include "qom/object.h"
>>>> +
>>>> +typedef struct PnvChip PnvChip;
>>>> +typedef struct PnvPhb4PecState PnvPhb4PecState;
>>>> +
>>>> +struct PnvPHB {
>>>> +    PCIExpressHost parent_obj;
>>>> +
>>>> +    uint32_t chip_id;
>>>> +    uint32_t phb_id;
>>>> +    uint32_t version;
>>>> +    char bus_path[8];
>>>> +
>>>> +    PnvChip *chip;
>>>> +
>>>> +    PnvPhb4PecState *pec;
>>>> +
>>>> +    /* The PHB backend (PnvPHB3, PnvPHB4 ...) being used */
>>>> +    Object *backend;
>>>> +};
>>>> +
>>>> +#define TYPE_PNV_PHB "pnv-phb"
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(PnvPHB, PNV_PHB)
>>>> +
>>>> +#endif /* PCI_HOST_PNV_PHB_H */
>>