Re: [PATCH v2 11/16] ppc/pnv: add pnv-phb-root-port device

[Daniel Henrique Barboza]

On 6/2/22 05:12, Mark Cave-Ayland wrote:
> On 31/05/2022 22:49, Daniel Henrique Barboza wrote:
>
> > We have two very similar root-port devices, pnv-phb3-root-port and
> > pnv-phb4-root-port. Both consist of a wrapper around the PCIESlot device
> > that, until now, has no additional attributes.
> >
> > The main difference between the PHB3 and PHB4 root ports is that
> > pnv-phb4-root-port has the pnv_phb4_root_port_reset() callback. All
> > other differences can be merged in a single device without too much
> > trouble.
> >
> > This patch introduces the unified pnv-phb-root-port that, in time, will
> > be used as the default root port for the pnv-phb device.
> >
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> >   hw/pci-host/pnv_phb.c | 107 ++++++++++++++++++++++++++++++++++++++----
> >   hw/pci-host/pnv_phb.h |  17 +++++++
> >   2 files changed, 116 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
> > index 321c4e768a..5047e90d3a 100644
> > --- a/hw/pci-host/pnv_phb.c
> > +++ b/hw/pci-host/pnv_phb.c
> > @@ -114,15 +114,106 @@ static void pnv_phb_class_init(ObjectClass *klass, void 
> > *data)
> >       dc->user_creatable = true;
> >   }
> > -static void pnv_phb_register_type(void)
> > +static void pnv_phb_root_port_reset(DeviceState *dev)
> >   {
> > -    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,
> > -    };
> > +    PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
> > +    PnvPHBRootPort *rootport = PNV_PHB_ROOT_PORT(dev);
> > +    PCIDevice *d = PCI_DEVICE(dev);
> > +    uint8_t *conf = d->config;
> > +    rpc->parent_reset(dev);
> > +
> > +    if (rootport->version == 3) {
> > +        return;
> > +    }
> > +
> > +    /* PHB4 and later requires these extra reset steps */
> > +    pci_byte_test_and_set_mask(conf + PCI_IO_BASE,
> > +                               PCI_IO_RANGE_MASK & 0xff);
> > +    pci_byte_test_and_clear_mask(conf + PCI_IO_LIMIT,
> > +                                 PCI_IO_RANGE_MASK & 0xff);
> > +    pci_set_word(conf + PCI_MEMORY_BASE, 0);
> > +    pci_set_word(conf + PCI_MEMORY_LIMIT, 0xfff0);
> > +    pci_set_word(conf + PCI_PREF_MEMORY_BASE, 0x1);
> > +    pci_set_word(conf + PCI_PREF_MEMORY_LIMIT, 0xfff1);
> > +    pci_set_long(conf + PCI_PREF_BASE_UPPER32, 0x1); /* Hack */
> > +    pci_set_long(conf + PCI_PREF_LIMIT_UPPER32, 0xffffffff);
> > +    pci_config_set_interrupt_pin(conf, 0);
> > +}
> > +
> > +static void pnv_phb_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);
> > +    PnvPHB *phb = NULL;
> > +    Error *local_err = NULL;
> > +
> > +    phb = (PnvPHB *) object_dynamic_cast(OBJECT(bus->qbus.parent),
> > +                                          TYPE_PNV_PHB);
>
> Same here too.


This case is a bit different than the previous because there's no ->phb_base in
this device. Doing

PnvPHB *phb = PNV_PHB(PNV_PHB_ROOT_PORT(dev));

Will somehow retrieve a PnvPHB pointer but with zeroed properties. This makes
the slot/chassis code down below to fail when adding a second port.

I checked how other parts of the code handles that and the closer I found so
far is doing something like this:

    PCIDevice *pci = PCI_DEVICE(dev);
    PnvPHB *phb = PNV_PHB(pci_device_root_bus(pci)->qbus.parent);

This is how pci_bus_bypass_iommu() from hw/pci/pci.c retrieves a PCIHostState
pointer of a given bus.

Another idea is to simply add a pointer to the parent PnvPHB device that the
root port is attached to. This would trivialize this part and I could also
use the already set PnvPHB->version for the root port logic as well since 
they're
set to be the same.


Thanks,


Daniel



> > +    if (!phb) {
> > +        error_setg(errp,
> > +"pnv_phb_root_port devices must be connected to pnv-phb 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);
>
> Again this is from older code, but we already have dev so these could be:
>
>      qdev_prop_set_uint8(dev, "chassis", phb->chip_id);
>      qdev_prop_set_uint16(dev, "slot", phb->phb_id);
>
> > +    rpc->parent_realize(dev, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +    pci_config_set_interrupt_pin(pci->config, 0);
> > +}
> > +
> > +static void pnv_phb_root_port_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > +    PCIERootPortClass *rpc = PCIE_ROOT_PORT_CLASS(klass);
> > +
> > +    dc->desc     = "IBM PHB PCIE Root Port";
> > +
> > +    device_class_set_parent_realize(dc, pnv_phb_root_port_realize,
> > +                                    &rpc->parent_realize);
> > +
> > +    device_class_set_parent_reset(dc, pnv_phb_root_port_reset,
> > +                                  &rpc->parent_reset);
> > +    dc->reset = &pnv_phb_root_port_reset;
> > +
> > +    dc->user_creatable = true;
> > +
> > +    k->vendor_id = PCI_VENDOR_ID_IBM;
> > +    /* device_id represents the latest PHB root port version supported */
> > +    k->device_id = PNV_PHB5_DEVICE_ID;
> > +    k->revision  = 0;
> > +
> > +    rpc->exp_offset = 0x48;
> > +    rpc->aer_offset = 0x100;
> > +}
> > +
> > +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,
> > +};
> > +
> > +static const TypeInfo pnv_phb_root_port_info = {
> > +    .name          = TYPE_PNV_PHB_ROOT_PORT,
> > +    .parent        = TYPE_PCIE_ROOT_PORT,
> > +    .instance_size = sizeof(PnvPHBRootPort),
> > +    .class_init    = pnv_phb_root_port_class_init,
> > +};
> > +
> > +static void pnv_phb_register_types(void)
> > +{
> >       type_register_static(&pnv_phb_type_info);
> > +    type_register_static(&pnv_phb_root_port_info);
> >   }
> > -type_init(pnv_phb_register_type)
> > +
> > +type_init(pnv_phb_register_types)
> > diff --git a/hw/pci-host/pnv_phb.h b/hw/pci-host/pnv_phb.h
> > index a7cc8610e2..c8eab4b767 100644
> > --- a/hw/pci-host/pnv_phb.h
> > +++ b/hw/pci-host/pnv_phb.h
> > @@ -36,4 +36,21 @@ struct PnvPHB {
> >   #define TYPE_PNV_PHB "pnv-phb"
> >   OBJECT_DECLARE_SIMPLE_TYPE(PnvPHB, PNV_PHB)
> > +/*
> > + * PHB PCIe Root port
> > + */
> > +#define PNV_PHB3_DEVICE_ID         0x03dc
> > +#define PNV_PHB4_DEVICE_ID         0x04c1
> > +#define PNV_PHB5_DEVICE_ID         0x0652
> > +
> > +typedef struct PnvPHBRootPort {
> > +    PCIESlot parent_obj;
> > +
> > +    uint32_t version;
> > +} PnvPHBRootPort;
> > +
> > +#define TYPE_PNV_PHB_ROOT_PORT "pnv-phb-root-port"
> > +#define PNV_PHB_ROOT_PORT(obj) \
> > +    OBJECT_CHECK(PnvPHBRootPort, obj, TYPE_PNV_PHB_ROOT_PORT)
> > +
> >   #endif /* PCI_HOST_PNV_PHB_H */
>
>
> ATB,
>
> Mark.