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

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

On 6/1/22 02:56, Cédric Le Goater wrote:
> On 5/31/22 23: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);
>> +
>> +    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);
>> +
>> +    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;
> 
> does that mean powernv8 machines will see phb devices as phb5 devices ?


I had something like this in this patch that would set device_id properly:


diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
index 5d66264a96..d468e8d44a 100644
--- a/hw/pci-host/pnv_phb.c
+++ b/hw/pci-host/pnv_phb.c
@@ -144,6 +144,22 @@ static void pnv_phb_root_port_realize(DeviceState *dev, Error **errp)
      PCIBus *bus = pci_get_bus(pci);
      PnvPHB *phb = NULL;
      Error *local_err = NULL;
+    PnvPHBRootPort *phb_rp = PNV_PHB_ROOT_PORT(dev);
+    PCIDeviceClass *k = PCI_DEVICE_GET_CLASS(pci);
+
+    switch (phb_rp->version) {
+    case 3:
+        k->device_id = PNV_PHB3_DEVICE_ID;
+        break;
+    case 4:
+        k->device_id = PNV_PHB4_DEVICE_ID;
+        break;
+    case 5:
+        k->device_id = PNV_PHB5_DEVICE_ID;
+        break;
+    default:
+        g_assert_not_reached();
+    }
  
      phb = (PnvPHB *) object_dynamic_cast(OBJECT(bus->qbus.parent),
                                            TYPE_PNV_PHB);
@@ -166,6 +182,11 @@ static void pnv_phb_root_port_realize(DeviceState *dev, Error **errp)
      pci_config_set_interrupt_pin(pci->config, 0);
  }
  
+static Property pnv_phb_root_port_properties[] = {
+    DEFINE_PROP_UINT32("version", PnvPHBRootPort, version, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
  static void pnv_phb_root_port_class_init(ObjectClass *klass, void *data)
  {
      DeviceClass *dc = DEVICE_CLASS(klass);
@@ -181,6 +202,7 @@ static void pnv_phb_root_port_class_init(ObjectClass *klass, void *data)
                                    &rpc->parent_reset);
      dc->reset = &pnv_phb_root_port_reset;
  
+    device_class_set_props(dc, pnv_phb_root_port_properties);
      dc->user_creatable = true;
  
      k->vendor_id = PCI_VENDOR_ID_IBM;
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 4d2ea405db..13c8753eb2 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -2148,6 +2148,7 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data)
  
      static GlobalProperty phb_compat[] = {
          { TYPE_PNV_PHB, "version", "3" },
+        { TYPE_PNV_PHB_ROOT_PORT, "version", "3" },
      };
  
      mc->desc = "IBM PowerNV (Non-Virtualized) POWER8";
@@ -2173,6 +2174,7 @@ static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
  
      static GlobalProperty phb_compat[] = {
          { TYPE_PNV_PHB, "version", "4" },
+        { TYPE_PNV_PHB_ROOT_PORT, "version", "4" },
      };
  
      mc->desc = "IBM PowerNV (Non-Virtualized) POWER9";
@@ -2199,6 +2201,7 @@ static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
  
      static GlobalProperty phb_compat[] = {
          { TYPE_PNV_PHB, "version", "5" },
+        { TYPE_PNV_PHB_ROOT_PORT, "version", "5" },
      };
  
      mc->desc = "IBM PowerNV (Non-Virtualized) POWER10";


The reason I didn't follow it through is because I wasn't sure if setting k->device_id
during realize() time was acceptable. Everyone else seems to set k->device_id during
class_init() or via an extra attribute 'device_id' that is written directly into
the PCI header.

If this is something that we can do then I'm fine with fixing this up in this patch.


Thanks,


Daniel

> 
> C.
> 
>> +    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 */
>