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;
+ 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 */
--
2.36.1On 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.
> + 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.
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.
On 03/06/2022 21:47, Daniel Henrique Barboza wrote:
> 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.
Hmmm that's odd.
> 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.
I think what should happen is that the parent device should instantiate the pnv_phb
root port via object_initialize_child() so it is embedded within it, at which point
the pnv_phb root port device can just use container_of() to obtain the reference to
the parent device state.
But certainly dereferencing the internal qbus object is not the right long-term solution.
>>> + 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.
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 ?
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 */
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 */
>
On 01/06/2022 09:04, Daniel Henrique Barboza wrote:
> 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.
Yeah I'd be slightly nervous about doing this outside of class_init() too. I think my
preferred method would be that used by virtio, which is to write the value to config
space (see virtio_pci_device_plugged()) and then set k->device_id to 0 with an
appropriate comment as per
https://gitlab.com/qemu-project/qemu/-/blob/master/hw/virtio/vhost-user-rng-pci.c#L53. That
makes it quite clear that the 0 value in class_init() is a placeholder for something
else.
> 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 */
ATB,
Mark.
On 6/2/22 09:06, Mark Cave-Ayland wrote:
> On 01/06/2022 09:04, Daniel Henrique Barboza wrote:
>
>> 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.
>
> Yeah I'd be slightly nervous about doing this outside of class_init() too. I think my preferred method would be that used by virtio, which is to write the value to config space (see virtio_pci_device_plugged()) and then set k->device_id to 0 with an appropriate comment as per https://gitlab.com/qemu-project/qemu/-/blob/master/hw/virtio/vhost-user-rng-pci.c#L53. That makes it quite clear that the 0 value in class_init() is a placeholder for something else.
Ah yes. This is a nice trick. It would simplify a lot the models.
Thanks,
C.
>
>> 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 */
>
> ATB,
>
> Mark.
© 2016 - 2026 Red Hat, Inc.