[PATCH 00/17] ppc/pnv: enable pnv-phb4 user devices

[PATCH 00/17] ppc/pnv: enable pnv-phb4 user devices

[Daniel Henrique Barboza]

Hi,

This series implements pnv-phb4 user devices for the powernv9 machine.
It also includes a couple of pnv-phb3 and pnv-phb3-root-port fixes that
were also applied for the pnv4 equivalents.

During the enablement I had to rollback from the previously added
support for user creatable pnv-phb4-pec devices. The most important
reason is user experience. PEC devices that doesn't spawn the PHB
devices will be just a placeholder to add PHBs, having no use of their
own as far as the user sees it. From this standpoint it makes more sense
to just create all PECs and attach the PHBs the user wants on them.
Patch 14 also describes technical reasons to rollback this support.

The series is rebased using Cedric's 'powernv-6.2' branch [1]i, which
includes the '[PATCH 0/5] ppc/pnv: Preliminary cleanups before user
created PHBs' patches [2].

[1] https://github.com/legoater/qemu/tree/powernv-6.2
[2] https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg03810.html


Daniel Henrique Barboza (17):
  pnv_phb3.c: add unique chassis and slot for pnv_phb3_root_port
  pnv_phb3.c: do not set 'root-bus' as bus name
  pnv_phb3.h: change TYPE_PNV_PHB3_ROOT_BUS name
  pnv_phb4.c: add unique chassis and slot for pnv_phb4_root_port
  pnv.c: simplify pnv_phb_attach_root_port()
  pnv_phb4.c: attach default root port in phb4 realize()
  pnv_phb4.c: check if root port exists in rc_config functions
  pnv_phb4.c: introduce pnv_phb4_set_stack_phb_props()
  pnv_phb4_pec.c: move pnv_pec_phb_offset() to pnv_phb4.c
  pnv_phb4.c: introduce pnv_pec_init_stack_xscom()
  pnv_phb4_pec.c: use pnv_pec_get_phb_id() in pnv_pec_dt_xscom()
  pnv_phb4_pec.c: use 'default_enabled()' to init stack->phb
  pnv_phb4.h: turn phb into a pointer in struct PnvPhb4PecStack
  Revert "ppc/pnv: Introduce support for user created PHB4 devices"
  ppc/pnv: Introduce user creatable pnv-phb4 devices
  pnv_phb4.c: do not set 'root-bus' as bus name
  pnv_phb4.c: change TYPE_PNV_PHB4_ROOT_BUS name

 hw/pci-host/pnv_phb3.c         |  19 +++-
 hw/pci-host/pnv_phb4.c         | 188 +++++++++++++++++++++++++++++++--
 hw/pci-host/pnv_phb4_pec.c     | 107 ++++++-------------
 hw/ppc/pnv.c                   |  31 ++----
 include/hw/pci-host/pnv_phb3.h |   2 +-
 include/hw/pci-host/pnv_phb4.h |  13 ++-
 include/hw/ppc/pnv.h           |   1 +
 7 files changed, 252 insertions(+), 109 deletions(-)

-- 
2.33.1

[PATCH 01/17] pnv_phb3.c: add unique chassis and slot for pnv_phb3_root_port

[Daniel Henrique Barboza]

When creating a pnv_phb3_root_port using the command line, the first
root port is created successfully, but the second fails with the
following error:

qemu-system-ppc64: -device pnv-phb3-root-port,bus=phb3-root.0,id=pcie.3:
Can't add chassis slot, error -16

This error comes from the realize() function of its parent type,
rp_realize() from TYPE_PCIE_ROOT_PORT. pcie_chassis_add_slot() fails
with -EBUSY if there's an existing PCIESlot that has the same
chassis/slot value, regardless of being in a different bus.

One way to prevent this error is simply set chassis and slot values in
the command line. However, since phb3 root buses only supports a single
root port, we can just get an unique chassis/slot value by checking
which root bus the pnv_phb3_root_port is going to be attached, get the
equivalent phb3 device and use its chip-id and index values, which are
guaranteed to be unique.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb3.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index 4e2d680d44..130d392b3e 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -1156,8 +1156,24 @@ static const TypeInfo pnv_phb3_root_bus_info = {
 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);
-- 
2.33.1

[PATCH 02/17] pnv_phb3.c: do not set 'root-bus' as bus name

[Daniel Henrique Barboza]

All pnv-phb3-root-bus buses are being created as 'root-bus'. This
makes it impossible to, for example, add a pnv-phb3-root-port in
a specific root bus, since they all have the same name. By default
the device will be parented by the pnv-phb3 device that precedeced it in
the QEMU command line.

Moreover, this doesn't all for custom bus naming. Libvirt, for instance,
likes to name these buses as 'pcie.N', where 'N' is the index value of
the controller in the domain XML, by using the 'id' command line
attribute. At this moment this is also being ignored - the created root
bus will always be named 'root-bus'.

This patch fixes both scenarios by removing the 'root-bus' name from the
pci_register_root_bus() call. If an "id" is provided, use that.
Otherwise use 'NULL' as bus name. The 'NULL' value will be handled in
qbus_init_internal() and it will defaulted as lowercase bus type + the
global bus_id value.

After this path we can define the bus name by using the 'id' attribute:

qemu-system-ppc64 -m 4G -machine powernv8,accel=tcg \
    -device pnv-phb3,chip-id=0,index=1,id=pcie.0

  dev: pnv-phb3, id "pcie.0"
    index = 1 (0x1)
    chip-id = 0 (0x0)
    x-config-reg-migration-enabled = true
    bypass-iommu = false
    bus: pcie.0
      type pnv-phb3-root-bus

And without an 'id' we will have the following default:

qemu-system-ppc64 -m 4G -machine powernv8,accel=tcg \
    -device pnv-phb3,chip-id=0,index=1

  dev: pnv-phb3, id ""
    index = 1 (0x1)
    chip-id = 0 (0x0)
    x-config-reg-migration-enabled = true
    bypass-iommu = false
    bus: pnv-phb3-root-bus.0
      type pnv-phb3-root-bus

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb3.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index 130d392b3e..f1b1f109a3 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -1064,7 +1064,8 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
     memory_region_init(&phb->pci_mmio, OBJECT(phb), "pci-mmio",
                        PCI_MMIO_TOTAL_SIZE);
 
-    pci->bus = pci_register_root_bus(dev, "root-bus",
+    pci->bus = pci_register_root_bus(dev,
+                                     dev->id ? dev->id : NULL,
                                      pnv_phb3_set_irq, pnv_phb3_map_irq, phb,
                                      &phb->pci_mmio, &phb->pci_io,
                                      0, 4, TYPE_PNV_PHB3_ROOT_BUS);
-- 
2.33.1

[PATCH 03/17] pnv_phb3.h: change TYPE_PNV_PHB3_ROOT_BUS name

[Daniel Henrique Barboza]

The TYPE_PNV_PHB3_ROOT_BUS name is used as the default bus name when
the dev has no 'id'. However, pnv-phb3-root-bus is a bit too long to be
used as a bus name.

Most common QEMU buses and PCI controllers are named based on their bus
type (e.g. pSeries spapr-pci-host-bridge is called 'pci'). The most
common name for a PCIE bus controller in QEMU is 'pcie'. Naming it
'pcie' would break the documented use of the pnv-phb3 device, since
'pcie.0' would now refer to the root bus instead of the first root port.

There's nothing particularly wrong with the 'root-bus' name used before,
aside from the fact that 'root-bus' is being used for pnv-phb3 and
pnv-phb4 created buses, which is not quite correct since these buses
aren't implemented the same way in QEMU - you can't plug a
pnv-phb4-root-port into a pnv-phb3 root bus, for example.

This patch renames it as 'phb3-root', which is a compromise between the
existing and the previously used name. Creating 3 phbs without ID will
result in an "info qtree" output similar to this:

bus: main-system-bus
  type System
  dev: pnv-phb3, id ""
    index = 2 (0x2)
    chip-id = 0 (0x0)
    x-config-reg-migration-enabled = true
    bypass-iommu = false
    bus: phb3-root.2
      type phb3-root
  dev: pnv-phb3, id ""
    index = 1 (0x1)
    chip-id = 0 (0x0)
    x-config-reg-migration-enabled = true
    bypass-iommu = false
    bus: phb3-root.1
      type phb3-root
  dev: pnv-phb3, id ""
    index = 0 (0x0)
    chip-id = 0 (0x0)
    x-config-reg-migration-enabled = true
    bypass-iommu = false
    bus: phb3-root.0
      type phb3-root

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 include/hw/pci-host/pnv_phb3.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/pci-host/pnv_phb3.h b/include/hw/pci-host/pnv_phb3.h
index 2e423c3890..658ee40e13 100644
--- a/include/hw/pci-host/pnv_phb3.h
+++ b/include/hw/pci-host/pnv_phb3.h
@@ -105,7 +105,7 @@ struct PnvPBCQState {
 /*
  * PHB3 PCIe Root port
  */
-#define TYPE_PNV_PHB3_ROOT_BUS "pnv-phb3-root-bus"
+#define TYPE_PNV_PHB3_ROOT_BUS "phb3-root"
 
 #define TYPE_PNV_PHB3_ROOT_PORT "pnv-phb3-root-port"
 
-- 
2.33.1

[PATCH 04/17] pnv_phb4.c: add unique chassis and slot for pnv_phb4_root_port

[Daniel Henrique Barboza]

A similar situation as described previously with pnv_phb3_root_port
devices also happens with pnv_phb4_root_ports.

The solution is the same: assign an unique chassis/slot combo for them.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 23ab3ba594..4554490e51 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1396,8 +1396,23 @@ 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);
-- 
2.33.1

[PATCH 05/17] pnv.c: simplify pnv_phb_attach_root_port()

[Daniel Henrique Barboza]

The root port 'chassis' and 'slot' attributes are being set in the
realize() callback of phb3_root_port and phb4_root_port.

Remove the unneeded 'chassis' and 'slot' setting from
pnv_phb_attach_root_port().

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/pnv.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 1bd84d20c1..605296fab5 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1222,12 +1222,10 @@ static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
 }
 
 /* Attach a root port */
-static void pnv_phb_attach_root_port(PCIHostState *pci, int id, const char *name)
+static void pnv_phb_attach_root_port(PCIHostState *pci, const char *name)
 {
     PCIDevice *root = pci_new(PCI_DEVFN(0, 0), name);
 
-    qdev_prop_set_uint8(&root->qdev, "chassis", id);
-    qdev_prop_set_uint16(&root->qdev, "slot", id);
     pci_realize_and_unref(root, pci->bus, &error_fatal);
 }
 
@@ -1326,7 +1324,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
             return;
         }
 
-        pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), phb->phb_id,
+        pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
                                  TYPE_PNV_PHB3_ROOT_PORT);
     }
 }
@@ -1506,7 +1504,7 @@ static void pnv_chip_power9_pec_realize(PnvChip *chip, Error **errp)
         for (j = 0; j < pec->num_stacks; j++) {
             PnvPHB4 *phb = &pec->stacks[j].phb;
 
-            pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), phb->phb_id,
+            pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
                                      TYPE_PNV_PHB4_ROOT_PORT);
         }
     }
@@ -1754,7 +1752,7 @@ static void pnv_chip_power10_phb_realize(PnvChip *chip, Error **errp)
         for (j = 0; j < pec->num_stacks; j++) {
             PnvPHB4 *phb = &pec->stacks[j].phb;
 
-            pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), phb->phb_id,
+            pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
                                      TYPE_PNV_PHB5_ROOT_PORT);
         }
     }
-- 
2.33.1

[PATCH 06/17] pnv_phb4.c: attach default root port in phb4 realize()

[Daniel Henrique Barboza]

We're adding the default pnv_phb4_root_port in
pnv_chip_power9_pec_realize() by going into each stack, from eack pec,
accessing the stack PHB and adding the port.

This will be an annoyance when trying to implement user creatable PHB4
devices because, when that happens, stack->phb is not guaranteed to be
valid at that time (we'll assign a PHB to its stack in phb4_realize(),
after stk_realize()).

Let's move the attachment of the default root port to pnv_phb4_realize()
instead. This will create all the default root ports we already create
today, and it'll be one less thing to worry about when implementing user
creatable PHB4s.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c |  7 +++++++
 hw/ppc/pnv.c           | 20 +++-----------------
 include/hw/ppc/pnv.h   |  1 +
 3 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 4554490e51..daa468b812 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -22,6 +22,7 @@
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
 #include "qom/object.h"
+#include "sysemu/sysemu.h"
 #include "trace.h"
 
 #define phb_error(phb, fmt, ...)                                        \
@@ -1224,6 +1225,12 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
     pnv_phb4_update_xsrc(phb);
 
     phb->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc, xsrc->nr_irqs);
+
+    /* Add the default pnv-phb4-root-port */
+    if (defaults_enabled()) {
+        pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
+                                 TYPE_PNV_PHB4_ROOT_PORT);
+    }
 }
 
 static const char *pnv_phb4_root_bus_path(PCIHostState *host_bridge,
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 605296fab5..c88fef26cf 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1222,7 +1222,7 @@ static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
 }
 
 /* Attach a root port */
-static void pnv_phb_attach_root_port(PCIHostState *pci, const char *name)
+void pnv_phb_attach_root_port(PCIHostState *pci, const char *name)
 {
     PCIDevice *root = pci_new(PCI_DEVFN(0, 0), name);
 
@@ -1478,7 +1478,7 @@ static void pnv_chip_quad_realize(Pnv9Chip *chip9, Error **errp)
 static void pnv_chip_power9_pec_realize(PnvChip *chip, Error **errp)
 {
     Pnv9Chip *chip9 = PNV9_CHIP(chip);
-    int i, j;
+    int i;
 
     for (i = 0; i < chip->num_pecs; i++) {
         PnvPhb4PecState *pec = &chip9->pecs[i];
@@ -1500,13 +1500,6 @@ static void pnv_chip_power9_pec_realize(PnvChip *chip, Error **errp)
 
         pnv_xscom_add_subregion(chip, pec_nest_base, &pec->nest_regs_mr);
         pnv_xscom_add_subregion(chip, pec_pci_base, &pec->pci_regs_mr);
-
-        for (j = 0; j < pec->num_stacks; j++) {
-            PnvPHB4 *phb = &pec->stacks[j].phb;
-
-            pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
-                                     TYPE_PNV_PHB4_ROOT_PORT);
-        }
     }
 }
 
@@ -1726,7 +1719,7 @@ static void pnv_chip_power10_quad_realize(Pnv10Chip *chip10, Error **errp)
 static void pnv_chip_power10_phb_realize(PnvChip *chip, Error **errp)
 {
     Pnv10Chip *chip10 = PNV10_CHIP(chip);
-    int i, j;
+    int i;
 
     for (i = 0; i < chip->num_pecs; i++) {
         PnvPhb4PecState *pec = &chip10->pecs[i];
@@ -1748,13 +1741,6 @@ static void pnv_chip_power10_phb_realize(PnvChip *chip, Error **errp)
 
         pnv_xscom_add_subregion(chip, pec_nest_base, &pec->nest_regs_mr);
         pnv_xscom_add_subregion(chip, pec_pci_base, &pec->pci_regs_mr);
-
-        for (j = 0; j < pec->num_stacks; j++) {
-            PnvPHB4 *phb = &pec->stacks[j].phb;
-
-            pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
-                                     TYPE_PNV_PHB5_ROOT_PORT);
-        }
     }
 }
 
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 6c48fc62ff..7f2197dcc0 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -196,6 +196,7 @@ DECLARE_INSTANCE_CHECKER(PnvChip, PNV_CHIP_POWER10,
 
 PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
 void pnv_chip_parent_fixup(PnvChip *chip, Object *obj, int index);
+void pnv_phb_attach_root_port(PCIHostState *pci, const char *name);
 
 #define TYPE_PNV_MACHINE       MACHINE_TYPE_NAME("powernv")
 typedef struct PnvMachineClass PnvMachineClass;
-- 
2.33.1

[PATCH 07/17] pnv_phb4.c: check if root port exists in rc_config functions

[Daniel Henrique Barboza]

pnv_phb4_rc_config_read() and pnv_phb4_rc_config_write() are asserting
the existence of the root port. The root port is now optional, and there
will be cases where a pnv-phb4 device won't have a root port attached.

Instead of asserting, check if the root port exists before read/writing
into it.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index daa468b812..6bd907f91a 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -152,7 +152,9 @@ static void pnv_phb4_rc_config_write(PnvPHB4 *phb, unsigned off,
     }
 
     pdev = pci_find_device(pci->bus, 0, 0);
-    assert(pdev);
+    if (!pdev) {
+        return;
+    }
 
     pci_host_config_write_common(pdev, off, PHB_RC_CONFIG_SIZE,
                                  bswap32(val), 4);
@@ -171,7 +173,9 @@ static uint64_t pnv_phb4_rc_config_read(PnvPHB4 *phb, unsigned off,
     }
 
     pdev = pci_find_device(pci->bus, 0, 0);
-    assert(pdev);
+    if (!pdev) {
+        return 0x0;
+    }
 
     val = pci_host_config_read_common(pdev, off, PHB_RC_CONFIG_SIZE, 4);
     return bswap32(val);
-- 
2.33.1

[PATCH 08/17] pnv_phb4.c: introduce pnv_phb4_set_stack_phb_props()

[Daniel Henrique Barboza]

We want to be able to support user creatable pnv-phb4 objects to allow
users to instantiate a powernv9 machine similar to what it is done with
powernv8.

The main difference is that pnv-phb3 devs are attached directly to the
system bus and can be created in the command line. PCI devices such as
root-ports can be explictly connected to them. This allows users to
create the phbs, assign a bus name if desired, then connect devices onto
them.

pnv-phb4 devices on the other hand are created by adding PCI Express
Controllers (PEC) that will create a certain amount of pnv-phb4 buses
depending on the PEC index used. Index 0 will create 1 phb, index 1
creates 2 phbs, index 2 creates 3 phbs. Creating all PECs from the same
chip will create 6 PHBs. This doesn't users to rename the buses, like it
is done with pnv-phb3, because there's no user control over how the
pnv-phb4 are being created - aside from the amount of phbs and in which
chips they'll reside.

This implicit relationship between PEC devices and available buses can
be tolerable for users that knows how the hardware works, but it's
annoying for Libvirt to deal with. Since there's no explicit
relationship, in the command line, between the created buses and the PCI
devices that will connect to them, the domain XML needs to make a lot of
extra assumptions regarding the relationship between regular PCI devices
and the existing PECs.

The first step to allow for user creatable pnv-phb4 devices is to
decouple the pvn-phb logic from the pnv-phb4-pec code. This patch adds a
helper called pnv_phb4_set_stack_phb_props() to remove the code from
pnv_phb4_pec.c that initiates the object properties of pnv-phb4 devices.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c         | 27 +++++++++++++++++++++++++++
 hw/pci-host/pnv_phb4_pec.c     | 14 +-------------
 include/hw/pci-host/pnv_phb4.h |  1 +
 3 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 6bd907f91a..0ea505cc94 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1164,6 +1164,33 @@ static AddressSpace *pnv_phb4_dma_iommu(PCIBus *bus, void *opaque, int devfn)
     return &ds->dma_as;
 }
 
+/*
+ * Set the object properties of a phb in relation with its stack.
+ *
+ * Note: stack->pec must not be NULL.
+ */
+void pnv_phb4_set_stack_phb_props(PnvPhb4PecStack *stack,
+                                  PnvPHB4 *phb)
+{
+    PnvPhb4PecState *pec = stack->pec;
+    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
+    char name[64];
+
+    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb",
+             pec->chip_id, pec->index, stack->stack_no);
+    pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(phb),
+                          &pnv_phb4_xscom_ops, phb, name, 0x40);
+
+    object_property_set_int(OBJECT(phb), "chip-id", pec->chip_id,
+                            &error_fatal);
+    object_property_set_int(OBJECT(phb), "version", pecc->version,
+                            &error_fatal);
+    object_property_set_int(OBJECT(phb), "device-id", pecc->device_id,
+                            &error_fatal);
+    object_property_set_link(OBJECT(phb), "stack", OBJECT(stack),
+                             &error_abort);
+}
+
 static void pnv_phb4_instance_init(Object *obj)
 {
     PnvPHB4 *phb = PNV_PHB4(obj);
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 98e7ff78bd..700ee4b185 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -595,19 +595,7 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
                           PHB4_PEC_PCI_STK_REGS_COUNT);
 
     /* PHB pass-through */
-    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb",
-             pec->chip_id, pec->index, stack->stack_no);
-    pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(&stack->phb),
-                          &pnv_phb4_xscom_ops, &stack->phb, name, 0x40);
-
-    object_property_set_int(OBJECT(&stack->phb), "chip-id", pec->chip_id,
-                            &error_fatal);
-    object_property_set_int(OBJECT(&stack->phb), "version", pecc->version,
-                            &error_fatal);
-    object_property_set_int(OBJECT(&stack->phb), "device-id", pecc->device_id,
-                            &error_fatal);
-    object_property_set_link(OBJECT(&stack->phb), "stack", OBJECT(stack),
-                             &error_abort);
+    pnv_phb4_set_stack_phb_props(stack, &stack->phb);
     if (!sysbus_realize(SYS_BUS_DEVICE(&stack->phb), errp)) {
         return;
     }
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 629db632d9..d7838513f1 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -133,6 +133,7 @@ struct PnvPHB4 {
 
 void pnv_phb4_pic_print_info(PnvPHB4 *phb, Monitor *mon);
 void pnv_phb4_update_regions(PnvPhb4PecStack *stack);
+void pnv_phb4_set_stack_phb_props(PnvPhb4PecStack *stack, PnvPHB4 *phb);
 extern const MemoryRegionOps pnv_phb4_xscom_ops;
 
 /*
-- 
2.33.1

[PATCH 09/17] pnv_phb4_pec.c: move pnv_pec_phb_offset() to pnv_phb4.c

[Daniel Henrique Barboza]

The logic inside pnv_pec_phb_offset() wiil be useful in the next patch
to determine the stack that should contain a PHB4 device.

Move the function to pnv_phb4.c and make it public since there's no
pnv_phb4_pec.h header. While we're at it, add 'stack_index' as a
parameter and make the function return 'phb-id' directly. And rename it
to pnv_pec_get_phb_id() to be even clearer about the function intent.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c         | 17 +++++++++++++++++
 hw/pci-host/pnv_phb4_pec.c     | 15 +--------------
 include/hw/pci-host/pnv_phb4.h |  2 ++
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 0ea505cc94..36c56007ba 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1164,6 +1164,23 @@ static AddressSpace *pnv_phb4_dma_iommu(PCIBus *bus, void *opaque, int devfn)
     return &ds->dma_as;
 }
 
+/*
+ * Return the index/phb-id of a PHB4 that belongs to a
+ * pec->stacks[stack_index] stack.
+ */
+int pnv_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index)
+{
+    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
+    int index = pec->index;
+    int offset = 0;
+
+    while (index--) {
+        offset += pecc->num_stacks[index];
+    }
+
+    return offset + stack_index;
+}
+
 /*
  * Set the object properties of a phb in relation with its stack.
  *
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 700ee4b185..bc2f8bb8b1 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -374,19 +374,6 @@ static void pnv_pec_instance_init(Object *obj)
     }
 }
 
-static int pnv_pec_phb_offset(PnvPhb4PecState *pec)
-{
-    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
-    int index = pec->index;
-    int offset = 0;
-
-    while (index--) {
-        offset += pecc->num_stacks[index];
-    }
-
-    return offset;
-}
-
 static void pnv_pec_realize(DeviceState *dev, Error **errp)
 {
     PnvPhb4PecState *pec = PNV_PHB4_PEC(dev);
@@ -422,7 +409,7 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < pec->num_stacks; i++) {
         PnvPhb4PecStack *stack = &pec->stacks[i];
         Object *stk_obj = OBJECT(stack);
-        int phb_id = pnv_pec_phb_offset(pec) + i;
+        int phb_id =  pnv_pec_get_phb_id(pec, i);
 
         object_property_set_int(stk_obj, "stack-no", i, &error_abort);
         object_property_set_int(stk_obj, "phb-id", phb_id, &error_abort);
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index d7838513f1..0fa88ca3fa 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -15,6 +15,7 @@
 #include "hw/ppc/xive.h"
 #include "qom/object.h"
 
+typedef struct PnvPhb4PecState PnvPhb4PecState;
 typedef struct PnvPhb4PecStack PnvPhb4PecStack;
 typedef struct PnvPHB4 PnvPHB4;
 typedef struct PnvChip PnvChip;
@@ -134,6 +135,7 @@ struct PnvPHB4 {
 void pnv_phb4_pic_print_info(PnvPHB4 *phb, Monitor *mon);
 void pnv_phb4_update_regions(PnvPhb4PecStack *stack);
 void pnv_phb4_set_stack_phb_props(PnvPhb4PecStack *stack, PnvPHB4 *phb);
+int pnv_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index);
 extern const MemoryRegionOps pnv_phb4_xscom_ops;
 
 /*
-- 
2.33.1

[PATCH 10/17] pnv_phb4.c: introduce pnv_pec_init_stack_xscom()

[Daniel Henrique Barboza]

The XSCOM address space of the stack must be populated after the
initialization of its associated PHB4 is completed. At this moment this
is always true because stk_realize() will always succeeds the realize of
stack->phb, but that will not be the case with user creatable pnv-phb4
devices.

Create a helper that can be used later on during pnv-phb4 realize() to
initialize the xscom address space of its stack.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c         | 26 ++++++++++++++++++++++++++
 hw/pci-host/pnv_phb4_pec.c     | 19 +------------------
 include/hw/pci-host/pnv_phb4.h |  1 +
 3 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 36c56007ba..31284e0460 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1164,6 +1164,32 @@ static AddressSpace *pnv_phb4_dma_iommu(PCIBus *bus, void *opaque, int devfn)
     return &ds->dma_as;
 }
 
+/*
+ * Init the xscom address space of the stack. This must be
+ * called after the associated stack->phb is defined.
+ */
+void pnv_pec_init_stack_xscom(PnvPhb4PecStack *stack)
+{
+    PnvPhb4PecState *pec = stack->pec;
+    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
+    PnvChip *chip = pec->chip;
+    uint32_t pec_nest_base = pecc->xscom_nest_base(pec);
+    uint32_t pec_pci_base = pecc->xscom_pci_base(pec);
+
+
+    /* Populate the XSCOM address space. */
+    pnv_xscom_add_subregion(chip,
+                            pec_nest_base + 0x40 * (stack->stack_no + 1),
+                            &stack->nest_regs_mr);
+    pnv_xscom_add_subregion(chip,
+                            pec_pci_base + 0x40 * (stack->stack_no + 1),
+                            &stack->pci_regs_mr);
+    pnv_xscom_add_subregion(chip,
+                            pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
+                            0x40 * stack->stack_no,
+                            &stack->phb_regs_mr);
+}
+
 /*
  * Return the index/phb-id of a PHB4 that belongs to a
  * pec->stacks[stack_index] stack.
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index bc2f8bb8b1..4f6db26633 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -560,10 +560,6 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
 {
     PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(dev);
     PnvPhb4PecState *pec = stack->pec;
-    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
-    PnvChip *chip = pec->chip;
-    uint32_t pec_nest_base;
-    uint32_t pec_pci_base;
     char name[64];
 
     assert(pec);
@@ -587,20 +583,7 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    pec_nest_base = pecc->xscom_nest_base(pec);
-    pec_pci_base = pecc->xscom_pci_base(pec);
-
-    /* Populate the XSCOM address space. */
-    pnv_xscom_add_subregion(chip,
-                            pec_nest_base + 0x40 * (stack->stack_no + 1),
-                            &stack->nest_regs_mr);
-    pnv_xscom_add_subregion(chip,
-                            pec_pci_base + 0x40 * (stack->stack_no + 1),
-                            &stack->pci_regs_mr);
-    pnv_xscom_add_subregion(chip,
-                            pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
-                            0x40 * stack->stack_no,
-                            &stack->phb_regs_mr);
+    pnv_pec_init_stack_xscom(stack);
 }
 
 static Property pnv_pec_stk_properties[] = {
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 0fa88ca3fa..0f6c81c9cb 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -134,6 +134,7 @@ struct PnvPHB4 {
 
 void pnv_phb4_pic_print_info(PnvPHB4 *phb, Monitor *mon);
 void pnv_phb4_update_regions(PnvPhb4PecStack *stack);
+void pnv_pec_init_stack_xscom(PnvPhb4PecStack *stack);
 void pnv_phb4_set_stack_phb_props(PnvPhb4PecStack *stack, PnvPHB4 *phb);
 int pnv_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index);
 extern const MemoryRegionOps pnv_phb4_xscom_ops;
-- 
2.33.1

[PATCH 11/17] pnv_phb4_pec.c: use pnv_pec_get_phb_id() in pnv_pec_dt_xscom()

[Daniel Henrique Barboza]

Relying on stack->phb to write the xscom DT of the PEC is something that
we won't be able to do with user creatable pnv-phb4 devices.

Hopefully, this can be done by using pnv_pec_get_phb_id(), which is
already used by pnv_pec_realize() to set the phb-id of the stack. Use
the same idea in pnv_pec_dt_xscom() to write ibm,phb-index without the
need to accessing stack->phb, since stack->phb is not granted to be !=
NULL when user creatable phbs are introduced.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4_pec.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 4f6db26633..56ffd446ab 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -466,8 +466,7 @@ static int pnv_pec_dt_xscom(PnvXScomInterface *dev, void *fdt,
                       pecc->compat_size)));
 
     for (i = 0; i < pec->num_stacks; i++) {
-        PnvPhb4PecStack *stack = &pec->stacks[i];
-        PnvPHB4 *phb = &stack->phb;
+        int phb_id =  pnv_pec_get_phb_id(pec, i);
         int stk_offset;
 
         name = g_strdup_printf("stack@%x", i);
@@ -477,7 +476,7 @@ static int pnv_pec_dt_xscom(PnvXScomInterface *dev, void *fdt,
         _FDT((fdt_setprop(fdt, stk_offset, "compatible", pecc->stk_compat,
                           pecc->stk_compat_size)));
         _FDT((fdt_setprop_cell(fdt, stk_offset, "reg", i)));
-        _FDT((fdt_setprop_cell(fdt, stk_offset, "ibm,phb-index", phb->phb_id)));
+        _FDT((fdt_setprop_cell(fdt, stk_offset, "ibm,phb-index", phb_id)));
     }
 
     return 0;
-- 
2.33.1

[PATCH 12/17] pnv_phb4_pec.c: use 'default_enabled()' to init stack->phb

[Daniel Henrique Barboza]

The next step before enabling user creatable pnv-phb4 devices is to
decople the init of the stack->phb object from
pnv_pec_stk_instance_init().

First, use 'defaults_enabled()' inside pnv_pec_realize() to create the
stack->phb object, while removing the equivalent object_initiate_child()
call from stk_instance_init(). Create a new "phb" stack property link so
we can assign stack->phb in an idiomatic manner.

Then we need to handle stack->phb->index assignment. The value is
retrieved with pnv_pec_get_phd_id() and, until this patch, this was
being assigned to a 'phb-id' stack link to phb->index. It is simpler to
assign this directly given that now we need to interact with the PnvPHB4
object directly to set its other attributes. Assign phb->index directly
with the value of pnv_pec_get_phb_id(), and remove the now unused link.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4_pec.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 56ffd446ab..031e98f1f4 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -19,6 +19,7 @@
 #include "hw/pci/pci_bus.h"
 #include "hw/ppc/pnv.h"
 #include "hw/qdev-properties.h"
+#include "sysemu/sysemu.h"
 
 #include <libfdt.h>
 
@@ -409,11 +410,29 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < pec->num_stacks; i++) {
         PnvPhb4PecStack *stack = &pec->stacks[i];
         Object *stk_obj = OBJECT(stack);
-        int phb_id =  pnv_pec_get_phb_id(pec, i);
 
         object_property_set_int(stk_obj, "stack-no", i, &error_abort);
-        object_property_set_int(stk_obj, "phb-id", phb_id, &error_abort);
         object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort);
+
+        /* Create and realize the default stack->phb */
+        if (defaults_enabled()) {
+            PnvPHB4 *phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
+            int phb_id =  pnv_pec_get_phb_id(pec, i);
+
+            object_property_set_int(OBJECT(phb), "index",
+                                    phb_id, &error_abort);
+            object_property_set_link(OBJECT(phb), "stack",
+                                     stk_obj, &error_abort);
+
+            pnv_phb4_set_stack_phb_props(stack, phb);
+            if (!sysbus_realize_and_unref(SYS_BUS_DEVICE(phb), errp)) {
+                return;
+            }
+
+            object_property_set_link(stk_obj, "phb", OBJECT(phb),
+                                     &error_abort);
+        }
+
         if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
             return;
         }
@@ -549,10 +568,6 @@ static const TypeInfo pnv_pec_type_info = {
 
 static void pnv_pec_stk_instance_init(Object *obj)
 {
-    PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(obj);
-
-    object_initialize_child(obj, "phb", &stack->phb, TYPE_PNV_PHB4);
-    object_property_add_alias(obj, "phb-id", OBJECT(&stack->phb), "index");
 }
 
 static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
@@ -589,6 +604,8 @@ static Property pnv_pec_stk_properties[] = {
         DEFINE_PROP_UINT32("stack-no", PnvPhb4PecStack, stack_no, 0),
         DEFINE_PROP_LINK("pec", PnvPhb4PecStack, pec, TYPE_PNV_PHB4_PEC,
                          PnvPhb4PecState *),
+        DEFINE_PROP_LINK("phb", PnvPhb4PecStack, phb, TYPE_PNV_PHB4,
+                         PnvPHB4 *),
         DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.33.1

[PATCH 13/17] pnv_phb4.h: turn phb into a pointer in struct PnvPhb4PecStack

[Daniel Henrique Barboza]

At this moment, stack->phb is the plain PnvPHB4 device itself instead
of a pointer to the device. This will present a problem when adding user
creatable devices because we can't deal with this struct and the
realize() callback from the user creatable device.

We can't get rid of this attribute, similar to what we did when enabling
pnv-phb3 user creatable devices, because pnv_phb4_update_regions() needs
to access stack->phb to do its job. This function is called twice in
pnv_pec_stk_update_map(), which is one of the nested xscom write
callbacks (via pnv_pec_stk_nest_xscom_write()). In fact,
pnv_pec_stk_update_map() code comment is explicit about how the order of
the unmap/map operations relates with the PHB subregions.

All of this indicates that this code is tied together in a way that we
either go on a crusade, featuring lots of refactories and redesign and
considerable pain, to decouple stack and phb mapping, or we allow stack
update_map operations to access the associated PHB as it is today even
after introducing pnv-phb4 user devices.

This patch chooses the latter. Instead of getting rid of stack->phb,
turn it into a PHB pointer. This will allow us to assign an user created
PHB to an existing stack later.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c         | 2 +-
 hw/pci-host/pnv_phb4_pec.c     | 2 +-
 include/hw/pci-host/pnv_phb4.h | 7 +++++--
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 31284e0460..5b2f644662 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1564,7 +1564,7 @@ type_init(pnv_phb4_register_types);
 
 void pnv_phb4_update_regions(PnvPhb4PecStack *stack)
 {
-    PnvPHB4 *phb = &stack->phb;
+    PnvPHB4 *phb = stack->phb;
 
     /* Unmap first always */
     if (memory_region_is_mapped(&phb->mr_regs)) {
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 031e98f1f4..3797696e8f 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -592,7 +592,7 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
                           PHB4_PEC_PCI_STK_REGS_COUNT);
 
     /* PHB pass-through */
-    pnv_phb4_set_stack_phb_props(stack, &stack->phb);
+    pnv_phb4_set_stack_phb_props(stack, stack->phb);
     if (!sysbus_realize(SYS_BUS_DEVICE(&stack->phb), errp)) {
         return;
     }
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 0f6c81c9cb..d67e33924b 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -181,8 +181,11 @@ struct PnvPhb4PecStack {
     /* The owner PEC */
     PnvPhb4PecState *pec;
 
-    /* The actual PHB */
-    PnvPHB4 phb;
+    /*
+     * PHB4 pointer. pnv_phb4_update_regions() needs to access
+     * the PHB4 via a PnvPhb4PecStack pointer.
+     */
+    PnvPHB4 *phb;
 };
 
 struct PnvPhb4PecState {
-- 
2.33.1

[PATCH 14/17] Revert "ppc/pnv: Introduce support for user created PHB4 devices"

[Daniel Henrique Barboza]

The upcoming code that allows for user creatable pnv-phb4 devices relies
on finding the correct pnv-phb4-pec controller to associate with. At
this moment the code that added support for user creatable pnv-phb4-pec
devices does not update chip9->pecs[] and pec->chip->num_pecs after
pnv_pec_realize(). This is not trivial to do because chip9-pecs[] is an
array of PEC devices, not an array of pointers to PEC devices.

All of this wasn't a problem back when this commit was introduced
because the pnv-phb4 devices of each pnv-phb4-pec were being created
automatically.  We had a change of heart since then, realizing that
dealing with pnv-phb4-pec is too complicated from the user standpoint.

In theory we could work the code to change chip9->pecs[] to be an array
of pointers and go from there, but in reality this will be a wasted
effort since we're going to backtrack on the user-creatable
pnv-phb4-pec. All PCI Express controllers of all chips will be created
by default. When running with default settings all pnv-phb4 PHBs will be
created, as usual. When running with '-nodefaults' the PECs will be
created without the PHBs, and then the user will be responsible for
adding them by hand in the command line.

Instead of fixing this situation with chip9->pecs[] not being up to date
with each user created pnv-phb4-pec, then work on user creatable pnv-phb4
support, then removing the support for user pnv-phb4-pec, let's remove
user-creatable pnv-phb4-pec right now and spare the extra code.

This reverts commit 7a221a8f6eb04d3e03081b06a89896803554e37d.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4_pec.c | 19 +------------------
 hw/ppc/pnv.c               |  5 ++---
 2 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 3797696e8f..aa93ad3f10 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -382,17 +382,6 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
     char name[64];
     int i;
 
-    /* User created devices */
-    if (!pec->chip) {
-        PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
-
-        pec->chip = pnv_get_chip(pnv, pec->chip_id);
-        if (!pec->chip) {
-            error_setg(errp, "invalid chip id: %d", pec->chip_id);
-            return;
-        }
-    }
-
     if (pec->index >= PNV_CHIP_GET_CLASS(pec->chip)->num_pecs) {
         error_setg(errp, "invalid PEC index: %d", pec->index);
         return;
@@ -400,12 +389,6 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
 
     pec->num_stacks = pecc->num_stacks[pec->index];
 
-    /*
-     * Reparent user created devices to the chip to build correctly
-     * the device tree.
-     */
-    pnv_chip_parent_fixup(pec->chip, OBJECT(pec), pec->index);
-
     /* Create stacks */
     for (i = 0; i < pec->num_stacks; i++) {
         PnvPhb4PecStack *stack = &pec->stacks[i];
@@ -538,7 +521,7 @@ static void pnv_pec_class_init(ObjectClass *klass, void *data)
 
     dc->realize = pnv_pec_realize;
     device_class_set_props(dc, pnv_pec_properties);
-    dc->user_creatable = true;
+    dc->user_creatable = false;
 
     pecc->xscom_nest_base = pnv_pec_xscom_nest_base;
     pecc->xscom_pci_base  = pnv_pec_xscom_pci_base;
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index c88fef26cf..bf2607446a 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1426,9 +1426,8 @@ static void pnv_chip_power9_instance_init(Object *obj)
 
     object_initialize_child(obj, "homer", &chip9->homer, TYPE_PNV9_HOMER);
 
-    if (defaults_enabled()) {
-        chip->num_pecs = pcc->num_pecs;
-    }
+    /* Number of PECs is the chip default */
+    chip->num_pecs = pcc->num_pecs;
 
     for (i = 0; i < chip->num_pecs; i++) {
         object_initialize_child(obj, "pec[*]", &chip9->pecs[i],
-- 
2.33.1

[PATCH 15/17] ppc/pnv: Introduce user creatable pnv-phb4 devices

[Daniel Henrique Barboza]

This patch introduces pnv-phb4 user creatable devices that are created
in a similar manner as pnv-phb3 devices, allowing the user to interact
with the PHBs directly instead of creating PCI Express Controllers that
will create a certain amount of PHBs per controller index.

First thing we need is to discover which stack will host the created
PHB, which is done by the new pnv_phb4_get_stack() function. During
pnv_phb4_realize() we'll inspect phb->stack to see if we're dealing with
an user creatable device or not. When using default settings, the
automatically created PHB4 devices will be realized with phb->stack
already assigned beforehand during PEC realize. In case we're dealing
with an user device, find its stack, set the PHB attributes based on the
stack it belongs and assign the PHB to the stack.

The xscom stack initialization takes place in pnv_pec_stk_realize() when
using default settings, but at that point we aren't aware of any user
PHB4 devices that will belong to the stack. In that case we'll postpone
xscom stack init until the the end of pnv_phb4_realize(), after all the
memory mappings of the PHB are done.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c     | 84 +++++++++++++++++++++++++++++++++++++-
 hw/pci-host/pnv_phb4_pec.c | 12 +++---
 hw/ppc/pnv.c               |  2 +
 3 files changed, 90 insertions(+), 8 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 5b2f644662..7b53c12b7c 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1244,6 +1244,41 @@ static void pnv_phb4_instance_init(Object *obj)
     object_initialize_child(obj, "source", &phb->xsrc, TYPE_XIVE_SOURCE);
 }
 
+static PnvPhb4PecStack *pnv_phb4_get_stack(PnvChip *chip, PnvPHB4 *phb,
+                                           Error **errp)
+{
+    Pnv9Chip *chip9 = NULL;
+    int chip_id = phb->chip_id;
+    int index = phb->phb_id;
+    int i, j;
+
+    if (chip->num_pecs == 0) {
+        /* Something weird happened. Bail out */
+        error_setg(errp, "chip id %d has no PCIE controllers", chip_id);
+        return NULL;
+    }
+
+    chip9 = PNV9_CHIP(chip);
+
+    for (i = 0; i < chip->num_pecs; i++) {
+        /*
+         * For each PEC, check the amount of stacks it supports
+         * and see if the given phb4 index matches a stack.
+         */
+        PnvPhb4PecState *pec = &chip9->pecs[i];
+
+        for (j = 0; j < pec->num_stacks; j++) {
+            if (index == pnv_pec_get_phb_id(pec, j)) {
+                return &pec->stacks[j];
+            }
+        }
+    }
+
+    error_setg(errp, "pnv-phb4 index %d didn't match any existing PEC",
+               chip_id);
+    return NULL;
+}
+
 static void pnv_phb4_realize(DeviceState *dev, Error **errp)
 {
     PnvPHB4 *phb = PNV_PHB4(dev);
@@ -1251,8 +1286,49 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
     XiveSource *xsrc = &phb->xsrc;
     int nr_irqs;
     char name[32];
+    PnvPhb4PecStack *stack = NULL;
+    bool stack_init_xscom = false;
+    Error *local_err = NULL;
 
-    assert(phb->stack);
+    /* User created PHB */
+    if (!phb->stack) {
+        PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
+        PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
+        BusState *s;
+
+        if (!chip) {
+            error_setg(errp, "invalid chip id: %d", phb->chip_id);
+            return;
+        }
+
+        stack = pnv_phb4_get_stack(chip, phb, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+
+        object_property_set_int(OBJECT(phb), "index",
+                                phb->phb_id, &error_abort);
+
+        pnv_phb4_set_stack_phb_props(stack, phb);
+
+        /* Assign the phb to the stack */
+        stack->phb = phb;
+
+        /*
+         * Reparent user created devices to the chip to build
+         * correctly the device tree.
+         */
+        pnv_chip_parent_fixup(chip, OBJECT(phb), phb->phb_id);
+
+        s = qdev_get_parent_bus(DEVICE(chip));
+        if (!qdev_set_parent_bus(DEVICE(phb), s, &local_err)) {
+            error_propagate(errp, local_err);
+            return;
+        }
+
+        stack_init_xscom = true;
+    }
 
     /* Set the "big_phb" flag */
     phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3;
@@ -1305,6 +1381,10 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
         pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
                                  TYPE_PNV_PHB4_ROOT_PORT);
     }
+
+    if (stack_init_xscom) {
+        pnv_pec_init_stack_xscom(stack);
+    }
 }
 
 static const char *pnv_phb4_root_bus_path(PCIHostState *host_bridge,
@@ -1416,7 +1496,7 @@ static void pnv_phb4_class_init(ObjectClass *klass, void *data)
     dc->realize         = pnv_phb4_realize;
     device_class_set_props(dc, pnv_phb4_properties);
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
-    dc->user_creatable  = false;
+    dc->user_creatable  = true;
 
     xfc->notify         = pnv_phb4_xive_notify;
 }
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index aa93ad3f10..27973779c5 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -574,13 +574,13 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
                           &pnv_pec_stk_pci_xscom_ops, stack, name,
                           PHB4_PEC_PCI_STK_REGS_COUNT);
 
-    /* PHB pass-through */
-    pnv_phb4_set_stack_phb_props(stack, stack->phb);
-    if (!sysbus_realize(SYS_BUS_DEVICE(&stack->phb), errp)) {
-        return;
+    /*
+     * There is no guarantee that stack->phb will be available
+     * at this point.
+     */
+    if (stack->phb) {
+        pnv_pec_init_stack_xscom(stack);
     }
-
-    pnv_pec_init_stack_xscom(stack);
 }
 
 static Property pnv_pec_stk_properties[] = {
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index bf2607446a..e93e77cb2e 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -2270,6 +2270,8 @@ static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
                                   pnv_machine_get_endian, pnv_machine_set_endian);
     object_class_property_set_description(oc, "endianness",
                               "Change CPU initial endianness (default is big)");
+
+    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB4);
 }
 
 static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
-- 
2.33.1

[PATCH 16/17] pnv_phb4.c: do not set 'root-bus' as bus name

[Daniel Henrique Barboza]

This change has the same motivation as the one done for pnv-phb3-root-bus
buses previously. Defaulting every bus to 'root-bus' makes it impossible to attach
root ports to specific buses and it doesn't allow for custom bus
naming because we're ignoring the 'id' value when registering the root
bus.

After this patch, creating pnv-phb4 devices with 'id' being set will
result in the following qtree:

qemu-system-ppc64 -m 4G -machine powernv9,accel=tcg \
   -device pnv-phb4,chip-id=0,index=0,id=pcie.0 \
   -device pnv-phb4,chip-id=1,index=4,id=pcie.1

bus: main-system-bus
  type System
  dev: pnv-phb4, id "pcie.1"
    index = 4 (0x4)
    chip-id = 1 (0x1)
    version = 704374636546 (0xa400000002)
    device-id = 1217 (0x4c1)
    x-config-reg-migration-enabled = true
    bypass-iommu = false
    bus: pcie.1
      type pnv-phb4-root-bus
  dev: pnv-phb4, id "pcie.0"
    index = 0 (0x0)
    chip-id = 0 (0x0)
    version = 704374636546 (0xa400000002)
    device-id = 1217 (0x4c1)
    x-config-reg-migration-enabled = true
    bypass-iommu = false
    bus: pcie.0
      type pnv-phb4-root-bus

And without setting any ids:

qemu-system-ppc64 -m 4G -machine powernv9,accel=tcg \
   -device pnv-phb4,chip-id=0,index=0,id=pcie.0 \
   -device pnv-phb4,chip-id=1,index=4,id=pcie.1

bus: main-system-bus
  type System
  dev: pnv-phb4, id ""
    index = 4 (0x4)
    chip-id = 1 (0x1)
    version = 704374636546 (0xa400000002)
    device-id = 1217 (0x4c1)
    x-config-reg-migration-enabled = true
    bypass-iommu = false
    bus: pnv-phb4-root-bus.1
      type pnv-phb4-root-bus
  dev: pnv-phb4, id ""
    index = 0 (0x0)
    chip-id = 0 (0x0)
    version = 704374636546 (0xa400000002)
    device-id = 1217 (0x4c1)
    x-config-reg-migration-enabled = true
    bypass-iommu = false
    bus: pnv-phb4-root-bus.0
      type pnv-phb4-root-bus

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 7b53c12b7c..982a61ebc0 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1354,7 +1354,7 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
     memory_region_init(&phb->pci_mmio, OBJECT(phb), name,
                        PCI_MMIO_TOTAL_SIZE);
 
-    pci->bus = pci_register_root_bus(dev, "root-bus",
+    pci->bus = pci_register_root_bus(dev, dev->id,
                                      pnv_phb4_set_irq, pnv_phb4_map_irq, phb,
                                      &phb->pci_mmio, &phb->pci_io,
                                      0, 4, TYPE_PNV_PHB4_ROOT_BUS);
-- 
2.33.1

[PATCH 17/17] pnv_phb4.c: change TYPE_PNV_PHB4_ROOT_BUS name

[Daniel Henrique Barboza]

Similar to what was happening with pnv-phb3 buses,
TYPE_PNV_PHB4_ROOT_BUS set to "pnv-phb4-root-bus" is a bit too long for
a default root bus name. The usual default name for theses buses in QEMU
are 'pcie', but we want to make a distinction between pnv-phb4 buses and
other PCIE buses, at least as far as default name goes, because not all
PCIE devices are attachable to a pnv-phb4 root-bus type.

Changing the default to 'phb4-root' allow us to have a shorter name
while making this bus distinct, and the user can always set its own bus
naming via the "id" attribute anyway.

This is the 'info qtree' output after this change, using a powernv9
domain with 2 sockets and default settings enabled:

qemu-system-ppc64 -m 4G -machine powernv9,accel=tcg \
     -smp 2,sockets=2,cores=1,threads=1

  dev: pnv-phb4, id ""
    index = 5 (0x5)
    chip-id = 1 (0x1)
    version = 704374636546 (0xa400000002)
    device-id = 1217 (0x4c1)
    x-config-reg-migration-enabled = true
    bypass-iommu = false
    bus: phb4-root.11
      type phb4-root
      dev: pnv-phb4-root-port, id ""
(...)
  dev: pnv-phb4, id ""
    index = 0 (0x0)
    chip-id = 1 (0x1)
    version = 704374636546 (0xa400000002)
    device-id = 1217 (0x4c1)
    x-config-reg-migration-enabled = true
    bypass-iommu = false
    bus: phb4-root.6
      type phb4-root
      dev: pnv-phb4-root-port, id ""
(..)
  dev: pnv-phb4, id ""
    index = 5 (0x5)
    chip-id = 0 (0x0)
    version = 704374636546 (0xa400000002)
    device-id = 1217 (0x4c1)
    x-config-reg-migration-enabled = true
    bypass-iommu = false
    bus: phb4-root.5
      type phb4-root
      dev: pnv-phb4-root-port, id ""
(...)
  dev: pnv-phb4, id ""
    index = 0 (0x0)
    chip-id = 0 (0x0)
    version = 704374636546 (0xa400000002)
    device-id = 1217 (0x4c1)
    x-config-reg-migration-enabled = true
    bypass-iommu = false
    bus: phb4-root.0
      type phb4-root
      dev: pnv-phb4-root-port, id ""

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 include/hw/pci-host/pnv_phb4.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index d67e33924b..cd0714d30b 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -47,7 +47,7 @@ typedef struct PnvPhb4DMASpace {
 /*
  * PHB4 PCIe Root port
  */
-#define TYPE_PNV_PHB4_ROOT_BUS "pnv-phb4-root-bus"
+#define TYPE_PNV_PHB4_ROOT_BUS "phb4-root"
 #define TYPE_PNV_PHB4_ROOT_PORT "pnv-phb4-root-port"
 #define TYPE_PNV_PHB5_ROOT_PORT "pnv-phb5-root-port"
 
-- 
2.33.1

Re: [PATCH 00/17] ppc/pnv: enable pnv-phb4 user devices

[Cédric Le Goater]

Hello Daniel,

On 12/28/21 20:37, Daniel Henrique Barboza wrote:
> Hi,
> 
> This series implements pnv-phb4 user devices for the powernv9 machine.
> It also includes a couple of pnv-phb3 and pnv-phb3-root-port fixes that
> were also applied for the pnv4 equivalents.
> 
> During the enablement I had to rollback from the previously added
> support for user creatable pnv-phb4-pec devices. The most important
> reason is user experience. PEC devices that doesn't spawn the PHB
> devices will be just a placeholder to add PHBs, having no use of their
> own as far as the user sees it. From this standpoint it makes more sense
> to just create all PECs and attach the PHBs the user wants on them.
> Patch 14 also describes technical reasons to rollback this support.
> 
> The series is rebased using Cedric's 'powernv-6.2' branch [1]i, which
> includes the '[PATCH 0/5] ppc/pnv: Preliminary cleanups before user
> created PHBs' patches [2].

It would be easier if you based the patchset on mainline. It's not
a problem to resend patches of another person or/and even rework
them to fit your needs.

Thanks,

C.

Re: [PATCH 01/17] pnv_phb3.c: add unique chassis and slot for pnv_phb3_root_port

[Cédric Le Goater]

On 12/28/21 20:37, Daniel Henrique Barboza wrote:
> When creating a pnv_phb3_root_port using the command line, the first
> root port is created successfully, but the second fails with the
> following error:
> 
> qemu-system-ppc64: -device pnv-phb3-root-port,bus=phb3-root.0,id=pcie.3:
> Can't add chassis slot, error -16
> 
> This error comes from the realize() function of its parent type,
> rp_realize() from TYPE_PCIE_ROOT_PORT. pcie_chassis_add_slot() fails
> with -EBUSY if there's an existing PCIESlot that has the same
> chassis/slot value, regardless of being in a different bus.
> 
> One way to prevent this error is simply set chassis and slot values in
> the command line. However, since phb3 root buses only supports a single
> root port, we can just get an unique chassis/slot value by checking
> which root bus the pnv_phb3_root_port is going to be attached, get the
> equivalent phb3 device and use its chip-id and index values, which are
> guaranteed to be unique.

I guess parent_realize() will fail if we add 2 root port devices under
the same phb ?

Thanks,

C.


> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb3.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index 4e2d680d44..130d392b3e 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1156,8 +1156,24 @@ static const TypeInfo pnv_phb3_root_bus_info = {
>   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);
> 

Re: [PATCH 02/17] pnv_phb3.c: do not set 'root-bus' as bus name

[Cédric Le Goater]

On 12/28/21 20:37, Daniel Henrique Barboza wrote:
> All pnv-phb3-root-bus buses are being created as 'root-bus'. This
> makes it impossible to, for example, add a pnv-phb3-root-port in
> a specific root bus, since they all have the same name. By default
> the device will be parented by the pnv-phb3 device that precedeced it in
> the QEMU command line.
> 
> Moreover, this doesn't all for custom bus naming. Libvirt, for instance,
> likes to name these buses as 'pcie.N', where 'N' is the index value of
> the controller in the domain XML, by using the 'id' command line
> attribute. At this moment this is also being ignored - the created root
> bus will always be named 'root-bus'.
> 
> This patch fixes both scenarios by removing the 'root-bus' name from the
> pci_register_root_bus() call. If an "id" is provided, use that.
> Otherwise use 'NULL' as bus name. The 'NULL' value will be handled in
> qbus_init_internal() and it will defaulted as lowercase bus type + the
> global bus_id value.
> 
> After this path we can define the bus name by using the 'id' attribute:
> 
> qemu-system-ppc64 -m 4G -machine powernv8,accel=tcg \
>      -device pnv-phb3,chip-id=0,index=1,id=pcie.0
> 
>    dev: pnv-phb3, id "pcie.0"
>      index = 1 (0x1)
>      chip-id = 0 (0x0)
>      x-config-reg-migration-enabled = true
>      bypass-iommu = false
>      bus: pcie.0
>        type pnv-phb3-root-bus
> 
> And without an 'id' we will have the following default:
> 
> qemu-system-ppc64 -m 4G -machine powernv8,accel=tcg \
>      -device pnv-phb3,chip-id=0,index=1
> 
>    dev: pnv-phb3, id ""
>      index = 1 (0x1)
>      chip-id = 0 (0x0)
>      x-config-reg-migration-enabled = true
>      bypass-iommu = false
>      bus: pnv-phb3-root-bus.0
>        type pnv-phb3-root-bus
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> ---
>   hw/pci-host/pnv_phb3.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index 130d392b3e..f1b1f109a3 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1064,7 +1064,8 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
>       memory_region_init(&phb->pci_mmio, OBJECT(phb), "pci-mmio",
>                          PCI_MMIO_TOTAL_SIZE);
>   
> -    pci->bus = pci_register_root_bus(dev, "root-bus",
> +    pci->bus = pci_register_root_bus(dev,
> +                                     dev->id ? dev->id : NULL,
>                                        pnv_phb3_set_irq, pnv_phb3_map_irq, phb,
>                                        &phb->pci_mmio, &phb->pci_io,
>                                        0, 4, TYPE_PNV_PHB3_ROOT_BUS);
> 

Re: [PATCH 02/17] pnv_phb3.c: do not set 'root-bus' as bus name

[Cédric Le Goater]

On 12/28/21 20:37, Daniel Henrique Barboza wrote:
> All pnv-phb3-root-bus buses are being created as 'root-bus'. This
> makes it impossible to, for example, add a pnv-phb3-root-port in
> a specific root bus, since they all have the same name. By default
> the device will be parented by the pnv-phb3 device that precedeced it in

preceded

> the QEMU command line.
> 
> Moreover, this doesn't all for custom bus naming. Libvirt, for instance,
> likes to name these buses as 'pcie.N', where 'N' is the index value of
> the controller in the domain XML, by using the 'id' command line

This sentence is difficult to understand ^.


> attribute. At this moment this is also being ignored - the created root
> bus will always be named 'root-bus'.
> 
> This patch fixes both scenarios by removing the 'root-bus' name from the
> pci_register_root_bus() call. If an "id" is provided, use that.
> Otherwise use 'NULL' as bus name. The 'NULL' value will be handled in
> qbus_init_internal() and it will defaulted as lowercase bus type + the
> global bus_id value.
> 
> After this path we can define the bus name by using the 'id' attribute:
> 
> qemu-system-ppc64 -m 4G -machine powernv8,accel=tcg \
>      -device pnv-phb3,chip-id=0,index=1,id=pcie.0
> 
>    dev: pnv-phb3, id "pcie.0"
>      index = 1 (0x1)
>      chip-id = 0 (0x0)
>      x-config-reg-migration-enabled = true
>      bypass-iommu = false
>      bus: pcie.0
>        type pnv-phb3-root-bus
> 
> And without an 'id' we will have the following default:
> 
> qemu-system-ppc64 -m 4G -machine powernv8,accel=tcg \
>      -device pnv-phb3,chip-id=0,index=1
> 
>    dev: pnv-phb3, id ""
>      index = 1 (0x1)
>      chip-id = 0 (0x0)
>      x-config-reg-migration-enabled = true
>      bypass-iommu = false
>      bus: pnv-phb3-root-bus.0
>        type pnv-phb3-root-bus
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb3.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index 130d392b3e..f1b1f109a3 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1064,7 +1064,8 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
>       memory_region_init(&phb->pci_mmio, OBJECT(phb), "pci-mmio",
>                          PCI_MMIO_TOTAL_SIZE);
>   
> -    pci->bus = pci_register_root_bus(dev, "root-bus",
> +    pci->bus = pci_register_root_bus(dev,
> +                                     dev->id ? dev->id : NULL,
>                                        pnv_phb3_set_irq, pnv_phb3_map_irq, phb,
>                                        &phb->pci_mmio, &phb->pci_io,
>                                        0, 4, TYPE_PNV_PHB3_ROOT_BUS);
> 

Re: [PATCH 03/17] pnv_phb3.h: change TYPE_PNV_PHB3_ROOT_BUS name

[Cédric Le Goater]

On 12/28/21 20:37, Daniel Henrique Barboza wrote:
> The TYPE_PNV_PHB3_ROOT_BUS name is used as the default bus name when
> the dev has no 'id'. However, pnv-phb3-root-bus is a bit too long to be
> used as a bus name.
> 
> Most common QEMU buses and PCI controllers are named based on their bus
> type (e.g. pSeries spapr-pci-host-bridge is called 'pci'). The most
> common name for a PCIE bus controller in QEMU is 'pcie'. Naming it
> 'pcie' would break the documented use of the pnv-phb3 device, since
> 'pcie.0' would now refer to the root bus instead of the first root port.
> 
> There's nothing particularly wrong with the 'root-bus' name used before,
> aside from the fact that 'root-bus' is being used for pnv-phb3 and
> pnv-phb4 created buses, which is not quite correct since these buses
> aren't implemented the same way in QEMU - you can't plug a
> pnv-phb4-root-port into a pnv-phb3 root bus, for example.
> 
> This patch renames it as 'phb3-root', which is a compromise between the
> existing and the previously used name. Creating 3 phbs without ID will
> result in an "info qtree" output similar to this:
> 
> bus: main-system-bus
>    type System
>    dev: pnv-phb3, id ""
>      index = 2 (0x2)
>      chip-id = 0 (0x0)
>      x-config-reg-migration-enabled = true
>      bypass-iommu = false
>      bus: phb3-root.2
>        type phb3-root
>    dev: pnv-phb3, id ""
>      index = 1 (0x1)
>      chip-id = 0 (0x0)
>      x-config-reg-migration-enabled = true
>      bypass-iommu = false
>      bus: phb3-root.1
>        type phb3-root
>    dev: pnv-phb3, id ""
>      index = 0 (0x0)
>      chip-id = 0 (0x0)
>      x-config-reg-migration-enabled = true
>      bypass-iommu = false
>      bus: phb3-root.0
>        type phb3-root
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   include/hw/pci-host/pnv_phb3.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/hw/pci-host/pnv_phb3.h b/include/hw/pci-host/pnv_phb3.h
> index 2e423c3890..658ee40e13 100644
> --- a/include/hw/pci-host/pnv_phb3.h
> +++ b/include/hw/pci-host/pnv_phb3.h
> @@ -105,7 +105,7 @@ struct PnvPBCQState {
>   /*
>    * PHB3 PCIe Root port
>    */
> -#define TYPE_PNV_PHB3_ROOT_BUS "pnv-phb3-root-bus"
> +#define TYPE_PNV_PHB3_ROOT_BUS "phb3-root"

hmm, what about "pnv-phb3-root" ? I like the 'pnv' prefix to identify
the machine.


Thanks,

C.



>   
>   #define TYPE_PNV_PHB3_ROOT_PORT "pnv-phb3-root-port"
>   
> 

Re: [PATCH 05/17] pnv.c: simplify pnv_phb_attach_root_port()

[Cédric Le Goater]

On 12/28/21 20:37, Daniel Henrique Barboza wrote:
> The root port 'chassis' and 'slot' attributes are being set in the
> realize() callback of phb3_root_port and phb4_root_port.
> 
> Remove the unneeded 'chassis' and 'slot' setting from
> pnv_phb_attach_root_port().

You should simply resend a modified version of the patch introducing
pnv_phb_attach_root_port() I think.

Thanks,

C.


> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/ppc/pnv.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 1bd84d20c1..605296fab5 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1222,12 +1222,10 @@ static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
>   }
>   
>   /* Attach a root port */
> -static void pnv_phb_attach_root_port(PCIHostState *pci, int id, const char *name)
> +static void pnv_phb_attach_root_port(PCIHostState *pci, const char *name)
>   {
>       PCIDevice *root = pci_new(PCI_DEVFN(0, 0), name);
>   
> -    qdev_prop_set_uint8(&root->qdev, "chassis", id);
> -    qdev_prop_set_uint16(&root->qdev, "slot", id);
>       pci_realize_and_unref(root, pci->bus, &error_fatal);
>   }
>   
> @@ -1326,7 +1324,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>               return;
>           }
>   
> -        pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), phb->phb_id,
> +        pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
>                                    TYPE_PNV_PHB3_ROOT_PORT);
>       }
>   }
> @@ -1506,7 +1504,7 @@ static void pnv_chip_power9_pec_realize(PnvChip *chip, Error **errp)
>           for (j = 0; j < pec->num_stacks; j++) {
>               PnvPHB4 *phb = &pec->stacks[j].phb;
>   
> -            pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), phb->phb_id,
> +            pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
>                                        TYPE_PNV_PHB4_ROOT_PORT);
>           }
>       }
> @@ -1754,7 +1752,7 @@ static void pnv_chip_power10_phb_realize(PnvChip *chip, Error **errp)
>           for (j = 0; j < pec->num_stacks; j++) {
>               PnvPHB4 *phb = &pec->stacks[j].phb;
>   
> -            pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb), phb->phb_id,
> +            pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
>                                        TYPE_PNV_PHB5_ROOT_PORT);
>           }
>       }
> 

Re: [PATCH 06/17] pnv_phb4.c: attach default root port in phb4 realize()

[Cédric Le Goater]

On 12/28/21 20:37, Daniel Henrique Barboza wrote:
> We're adding the default pnv_phb4_root_port in
> pnv_chip_power9_pec_realize() by going into each stack, from eack pec,

each

> accessing the stack PHB and adding the port.
> 
> This will be an annoyance when trying to implement user creatable PHB4
> devices because, when that happens, stack->phb is not guaranteed to be
> valid at that time (we'll assign a PHB to its stack in phb4_realize(),
> after stk_realize()).
> 
> Let's move the attachment of the default root port to pnv_phb4_realize()
> instead. This will create all the default root ports we already create
> today, and it'll be one less thing to worry about when implementing user
> creatable PHB4s.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

This makes sense. I would simply modify and resend the initial patch.

Thanks,

C.

> ---
>   hw/pci-host/pnv_phb4.c |  7 +++++++
>   hw/ppc/pnv.c           | 20 +++-----------------
>   include/hw/ppc/pnv.h   |  1 +
>   3 files changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 4554490e51..daa468b812 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -22,6 +22,7 @@
>   #include "hw/irq.h"
>   #include "hw/qdev-properties.h"
>   #include "qom/object.h"
> +#include "sysemu/sysemu.h"
>   #include "trace.h"
>   
>   #define phb_error(phb, fmt, ...)                                        \
> @@ -1224,6 +1225,12 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>       pnv_phb4_update_xsrc(phb);
>   
>       phb->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc, xsrc->nr_irqs);
> +
> +    /* Add the default pnv-phb4-root-port */
> +    if (defaults_enabled()) {
> +        pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
> +                                 TYPE_PNV_PHB4_ROOT_PORT);
> +    }
>   }
>   
>   static const char *pnv_phb4_root_bus_path(PCIHostState *host_bridge,
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 605296fab5..c88fef26cf 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1222,7 +1222,7 @@ static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
>   }
>   
>   /* Attach a root port */
> -static void pnv_phb_attach_root_port(PCIHostState *pci, const char *name)
> +void pnv_phb_attach_root_port(PCIHostState *pci, const char *name)
>   {
>       PCIDevice *root = pci_new(PCI_DEVFN(0, 0), name);
>   
> @@ -1478,7 +1478,7 @@ static void pnv_chip_quad_realize(Pnv9Chip *chip9, Error **errp)
>   static void pnv_chip_power9_pec_realize(PnvChip *chip, Error **errp)
>   {
>       Pnv9Chip *chip9 = PNV9_CHIP(chip);
> -    int i, j;
> +    int i;
>   
>       for (i = 0; i < chip->num_pecs; i++) {
>           PnvPhb4PecState *pec = &chip9->pecs[i];
> @@ -1500,13 +1500,6 @@ static void pnv_chip_power9_pec_realize(PnvChip *chip, Error **errp)
>   
>           pnv_xscom_add_subregion(chip, pec_nest_base, &pec->nest_regs_mr);
>           pnv_xscom_add_subregion(chip, pec_pci_base, &pec->pci_regs_mr);
> -
> -        for (j = 0; j < pec->num_stacks; j++) {
> -            PnvPHB4 *phb = &pec->stacks[j].phb;
> -
> -            pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
> -                                     TYPE_PNV_PHB4_ROOT_PORT);
> -        }
>       }
>   }
>   
> @@ -1726,7 +1719,7 @@ static void pnv_chip_power10_quad_realize(Pnv10Chip *chip10, Error **errp)
>   static void pnv_chip_power10_phb_realize(PnvChip *chip, Error **errp)
>   {
>       Pnv10Chip *chip10 = PNV10_CHIP(chip);
> -    int i, j;
> +    int i;
>   
>       for (i = 0; i < chip->num_pecs; i++) {
>           PnvPhb4PecState *pec = &chip10->pecs[i];
> @@ -1748,13 +1741,6 @@ static void pnv_chip_power10_phb_realize(PnvChip *chip, Error **errp)
>   
>           pnv_xscom_add_subregion(chip, pec_nest_base, &pec->nest_regs_mr);
>           pnv_xscom_add_subregion(chip, pec_pci_base, &pec->pci_regs_mr);
> -
> -        for (j = 0; j < pec->num_stacks; j++) {
> -            PnvPHB4 *phb = &pec->stacks[j].phb;
> -
> -            pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
> -                                     TYPE_PNV_PHB5_ROOT_PORT);
> -        }
>       }
>   }
>   
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 6c48fc62ff..7f2197dcc0 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -196,6 +196,7 @@ DECLARE_INSTANCE_CHECKER(PnvChip, PNV_CHIP_POWER10,
>   
>   PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
>   void pnv_chip_parent_fixup(PnvChip *chip, Object *obj, int index);
> +void pnv_phb_attach_root_port(PCIHostState *pci, const char *name);
>   
>   #define TYPE_PNV_MACHINE       MACHINE_TYPE_NAME("powernv")
>   typedef struct PnvMachineClass PnvMachineClass;
> 

Re: [PATCH 16/17] pnv_phb4.c: do not set 'root-bus' as bus name

[Cédric Le Goater]

On 12/28/21 20:38, Daniel Henrique Barboza wrote:
> This change has the same motivation as the one done for pnv-phb3-root-bus
> buses previously. Defaulting every bus to 'root-bus' makes it impossible to attach
> root ports to specific buses and it doesn't allow for custom bus
> naming because we're ignoring the 'id' value when registering the root
> bus.
> 
> After this patch, creating pnv-phb4 devices with 'id' being set will
> result in the following qtree:
> 
> qemu-system-ppc64 -m 4G -machine powernv9,accel=tcg \
>     -device pnv-phb4,chip-id=0,index=0,id=pcie.0 \
>     -device pnv-phb4,chip-id=1,index=4,id=pcie.1
> 
> bus: main-system-bus
>    type System
>    dev: pnv-phb4, id "pcie.1"
>      index = 4 (0x4)
>      chip-id = 1 (0x1)
>      version = 704374636546 (0xa400000002)
>      device-id = 1217 (0x4c1)
>      x-config-reg-migration-enabled = true
>      bypass-iommu = false
>      bus: pcie.1
>        type pnv-phb4-root-bus
>    dev: pnv-phb4, id "pcie.0"
>      index = 0 (0x0)
>      chip-id = 0 (0x0)
>      version = 704374636546 (0xa400000002)
>      device-id = 1217 (0x4c1)
>      x-config-reg-migration-enabled = true
>      bypass-iommu = false
>      bus: pcie.0
>        type pnv-phb4-root-bus
> 
> And without setting any ids:
> 
> qemu-system-ppc64 -m 4G -machine powernv9,accel=tcg \
>     -device pnv-phb4,chip-id=0,index=0,id=pcie.0 \
>     -device pnv-phb4,chip-id=1,index=4,id=pcie.1
> 
> bus: main-system-bus
>    type System
>    dev: pnv-phb4, id ""
>      index = 4 (0x4)
>      chip-id = 1 (0x1)
>      version = 704374636546 (0xa400000002)
>      device-id = 1217 (0x4c1)
>      x-config-reg-migration-enabled = true
>      bypass-iommu = false
>      bus: pnv-phb4-root-bus.1
>        type pnv-phb4-root-bus
>    dev: pnv-phb4, id ""
>      index = 0 (0x0)
>      chip-id = 0 (0x0)
>      version = 704374636546 (0xa400000002)
>      device-id = 1217 (0x4c1)
>      x-config-reg-migration-enabled = true
>      bypass-iommu = false
>      bus: pnv-phb4-root-bus.0
>        type pnv-phb4-root-bus
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> ---
>   hw/pci-host/pnv_phb4.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 7b53c12b7c..982a61ebc0 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1354,7 +1354,7 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>       memory_region_init(&phb->pci_mmio, OBJECT(phb), name,
>                          PCI_MMIO_TOTAL_SIZE);
>   
> -    pci->bus = pci_register_root_bus(dev, "root-bus",
> +    pci->bus = pci_register_root_bus(dev, dev->id,
>                                        pnv_phb4_set_irq, pnv_phb4_map_irq, phb,
>                                        &phb->pci_mmio, &phb->pci_io,
>                                        0, 4, TYPE_PNV_PHB4_ROOT_BUS);
> 

Re: [PATCH 07/17] pnv_phb4.c: check if root port exists in rc_config functions

[Cédric Le Goater]

On 12/28/21 20:37, Daniel Henrique Barboza wrote:
> pnv_phb4_rc_config_read() and pnv_phb4_rc_config_write() are asserting
> the existence of the root port. The root port is now optional, and there
> will be cases where a pnv-phb4 device won't have a root port attached.

May be we should enforce a stronger link between the two objects to avoid
creating an empty PHB device.

> Instead of asserting, check if the root port exists before read/writing
> into it> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb4.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index daa468b812..6bd907f91a 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -152,7 +152,9 @@ static void pnv_phb4_rc_config_write(PnvPHB4 *phb, unsigned off,
>       }
>   
>       pdev = pci_find_device(pci->bus, 0, 0);
> -    assert(pdev);
> +    if (!pdev) {

We should log an error at least.

> +        return;
> +    }
>   
>       pci_host_config_write_common(pdev, off, PHB_RC_CONFIG_SIZE,
>                                    bswap32(val), 4);
> @@ -171,7 +173,9 @@ static uint64_t pnv_phb4_rc_config_read(PnvPHB4 *phb, unsigned off,
>       }
>   
>       pdev = pci_find_device(pci->bus, 0, 0);
> -    assert(pdev);
> +    if (!pdev) {
> +        return 0x0;

         return ~0ull;

> +    }
>   
>       val = pci_host_config_read_common(pdev, off, PHB_RC_CONFIG_SIZE, 4);
>       return bswap32(val);
> 

Re: [PATCH 09/17] pnv_phb4_pec.c: move pnv_pec_phb_offset() to pnv_phb4.c

[Cédric Le Goater]

On 12/28/21 20:37, Daniel Henrique Barboza wrote:
> The logic inside pnv_pec_phb_offset() wiil be useful in the next patch

will

> to determine the stack that should contain a PHB4 device.
> 
> Move the function to pnv_phb4.c and make it public since there's no
> pnv_phb4_pec.h header. While we're at it, add 'stack_index' as a
> parameter and make the function return 'phb-id' directly. And rename it
> to pnv_pec_get_phb_id() to be even clearer about the function intent.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Looks good,

> ---
>   hw/pci-host/pnv_phb4.c         | 17 +++++++++++++++++
>   hw/pci-host/pnv_phb4_pec.c     | 15 +--------------
>   include/hw/pci-host/pnv_phb4.h |  2 ++
>   3 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 0ea505cc94..36c56007ba 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1164,6 +1164,23 @@ static AddressSpace *pnv_phb4_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>       return &ds->dma_as;
>   }
>   
> +/*
> + * Return the index/phb-id of a PHB4 that belongs to a
> + * pec->stacks[stack_index] stack.
> + */
> +int pnv_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index)

pnv_phb4_pec_get_phb_id() would be cleaner.

Thanks,

C.



> +{
> +    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
> +    int index = pec->index;
> +    int offset = 0;
> +
> +    while (index--) {
> +        offset += pecc->num_stacks[index];
> +    }
> +
> +    return offset + stack_index;
> +}
> +
>   /*
>    * Set the object properties of a phb in relation with its stack.
>    *
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 700ee4b185..bc2f8bb8b1 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -374,19 +374,6 @@ static void pnv_pec_instance_init(Object *obj)
>       }
>   }
>   
> -static int pnv_pec_phb_offset(PnvPhb4PecState *pec)
> -{
> -    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
> -    int index = pec->index;
> -    int offset = 0;
> -
> -    while (index--) {
> -        offset += pecc->num_stacks[index];
> -    }
> -
> -    return offset;
> -}
> -
>   static void pnv_pec_realize(DeviceState *dev, Error **errp)
>   {
>       PnvPhb4PecState *pec = PNV_PHB4_PEC(dev);
> @@ -422,7 +409,7 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>       for (i = 0; i < pec->num_stacks; i++) {
>           PnvPhb4PecStack *stack = &pec->stacks[i];
>           Object *stk_obj = OBJECT(stack);
> -        int phb_id = pnv_pec_phb_offset(pec) + i;
> +        int phb_id =  pnv_pec_get_phb_id(pec, i);
>   
>           object_property_set_int(stk_obj, "stack-no", i, &error_abort);
>           object_property_set_int(stk_obj, "phb-id", phb_id, &error_abort);
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index d7838513f1..0fa88ca3fa 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -15,6 +15,7 @@
>   #include "hw/ppc/xive.h"
>   #include "qom/object.h"
>   
> +typedef struct PnvPhb4PecState PnvPhb4PecState;
>   typedef struct PnvPhb4PecStack PnvPhb4PecStack;
>   typedef struct PnvPHB4 PnvPHB4;
>   typedef struct PnvChip PnvChip;
> @@ -134,6 +135,7 @@ struct PnvPHB4 {
>   void pnv_phb4_pic_print_info(PnvPHB4 *phb, Monitor *mon);
>   void pnv_phb4_update_regions(PnvPhb4PecStack *stack);
>   void pnv_phb4_set_stack_phb_props(PnvPhb4PecStack *stack, PnvPHB4 *phb);
> +int pnv_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index);
>   extern const MemoryRegionOps pnv_phb4_xscom_ops;
>   
>   /*
> 

Re: [PATCH 11/17] pnv_phb4_pec.c: use pnv_pec_get_phb_id() in pnv_pec_dt_xscom()

[Cédric Le Goater]

On 12/28/21 20:38, Daniel Henrique Barboza wrote:
> Relying on stack->phb to write the xscom DT of the PEC is something that
> we won't be able to do with user creatable pnv-phb4 devices.
> 
> Hopefully, this can be done by using pnv_pec_get_phb_id(), which is
> already used by pnv_pec_realize() to set the phb-id of the stack. Use
> the same idea in pnv_pec_dt_xscom() to write ibm,phb-index without the
> need to accessing stack->phb, since stack->phb is not granted to be !=
> NULL when user creatable phbs are introduced.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Couldn't we do that already without the previous change ?

Thanks,

C.


> ---
>   hw/pci-host/pnv_phb4_pec.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 4f6db26633..56ffd446ab 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -466,8 +466,7 @@ static int pnv_pec_dt_xscom(PnvXScomInterface *dev, void *fdt,
>                         pecc->compat_size)));
>   
>       for (i = 0; i < pec->num_stacks; i++) {
> -        PnvPhb4PecStack *stack = &pec->stacks[i];
> -        PnvPHB4 *phb = &stack->phb;
> +        int phb_id =  pnv_pec_get_phb_id(pec, i);
>           int stk_offset;
>   
>           name = g_strdup_printf("stack@%x", i);
> @@ -477,7 +476,7 @@ static int pnv_pec_dt_xscom(PnvXScomInterface *dev, void *fdt,
>           _FDT((fdt_setprop(fdt, stk_offset, "compatible", pecc->stk_compat,
>                             pecc->stk_compat_size)));
>           _FDT((fdt_setprop_cell(fdt, stk_offset, "reg", i)));
> -        _FDT((fdt_setprop_cell(fdt, stk_offset, "ibm,phb-index", phb->phb_id)));
> +        _FDT((fdt_setprop_cell(fdt, stk_offset, "ibm,phb-index", phb_id)));
>       }
>   
>       return 0;
> 

Re: [PATCH 00/17] ppc/pnv: enable pnv-phb4 user devices

[Daniel Henrique Barboza]

On 1/3/22 05:21, Cédric Le Goater wrote:
> Hello Daniel,
> 
> On 12/28/21 20:37, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> This series implements pnv-phb4 user devices for the powernv9 machine.
>> It also includes a couple of pnv-phb3 and pnv-phb3-root-port fixes that
>> were also applied for the pnv4 equivalents.
>>
>> During the enablement I had to rollback from the previously added
>> support for user creatable pnv-phb4-pec devices. The most important
>> reason is user experience. PEC devices that doesn't spawn the PHB
>> devices will be just a placeholder to add PHBs, having no use of their
>> own as far as the user sees it. From this standpoint it makes more sense
>> to just create all PECs and attach the PHBs the user wants on them.
>> Patch 14 also describes technical reasons to rollback this support.
>>
>> The series is rebased using Cedric's 'powernv-6.2' branch [1]i, which
>> includes the '[PATCH 0/5] ppc/pnv: Preliminary cleanups before user
>> created PHBs' patches [2].
> 
> It would be easier if you based the patchset on mainline. It's not
> a problem to resend patches of another person or/and even rework
> them to fit your needs.


Sure, I'll send the v2 based on the mainline + the required patches.


Thanks,


Daniel

> 
> Thanks,
> 
> C.

Re: [PATCH 03/17] pnv_phb3.h: change TYPE_PNV_PHB3_ROOT_BUS name

[Daniel Henrique Barboza]

On 1/3/22 05:28, Cédric Le Goater wrote:
> On 12/28/21 20:37, Daniel Henrique Barboza wrote:
>> The TYPE_PNV_PHB3_ROOT_BUS name is used as the default bus name when
>> the dev has no 'id'. However, pnv-phb3-root-bus is a bit too long to be
>> used as a bus name.
>>
>> Most common QEMU buses and PCI controllers are named based on their bus
>> type (e.g. pSeries spapr-pci-host-bridge is called 'pci'). The most
>> common name for a PCIE bus controller in QEMU is 'pcie'. Naming it
>> 'pcie' would break the documented use of the pnv-phb3 device, since
>> 'pcie.0' would now refer to the root bus instead of the first root port.
>>
>> There's nothing particularly wrong with the 'root-bus' name used before,
>> aside from the fact that 'root-bus' is being used for pnv-phb3 and
>> pnv-phb4 created buses, which is not quite correct since these buses
>> aren't implemented the same way in QEMU - you can't plug a
>> pnv-phb4-root-port into a pnv-phb3 root bus, for example.
>>
>> This patch renames it as 'phb3-root', which is a compromise between the
>> existing and the previously used name. Creating 3 phbs without ID will
>> result in an "info qtree" output similar to this:
>>
>> bus: main-system-bus
>>    type System
>>    dev: pnv-phb3, id ""
>>      index = 2 (0x2)
>>      chip-id = 0 (0x0)
>>      x-config-reg-migration-enabled = true
>>      bypass-iommu = false
>>      bus: phb3-root.2
>>        type phb3-root
>>    dev: pnv-phb3, id ""
>>      index = 1 (0x1)
>>      chip-id = 0 (0x0)
>>      x-config-reg-migration-enabled = true
>>      bypass-iommu = false
>>      bus: phb3-root.1
>>        type phb3-root
>>    dev: pnv-phb3, id ""
>>      index = 0 (0x0)
>>      chip-id = 0 (0x0)
>>      x-config-reg-migration-enabled = true
>>      bypass-iommu = false
>>      bus: phb3-root.0
>>        type phb3-root
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   include/hw/pci-host/pnv_phb3.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/hw/pci-host/pnv_phb3.h b/include/hw/pci-host/pnv_phb3.h
>> index 2e423c3890..658ee40e13 100644
>> --- a/include/hw/pci-host/pnv_phb3.h
>> +++ b/include/hw/pci-host/pnv_phb3.h
>> @@ -105,7 +105,7 @@ struct PnvPBCQState {
>>   /*
>>    * PHB3 PCIe Root port
>>    */
>> -#define TYPE_PNV_PHB3_ROOT_BUS "pnv-phb3-root-bus"
>> +#define TYPE_PNV_PHB3_ROOT_BUS "phb3-root"
> 
> hmm, what about "pnv-phb3-root" ? I like the 'pnv' prefix to identify
> the machine.

Works for me. I'll make the change in v2.

Thanks,


Daniel

> 
> 
> Thanks,
> 
> C.
> 
> 
> 
>>   #define TYPE_PNV_PHB3_ROOT_PORT "pnv-phb3-root-port"
>>
> 

Re: [PATCH 00/17] ppc/pnv: enable pnv-phb4 user devices

[Cédric Le Goater]

On 1/3/22 19:58, Daniel Henrique Barboza wrote:
> 
> 
> On 1/3/22 05:21, Cédric Le Goater wrote:
>> Hello Daniel,
>>
>> On 12/28/21 20:37, Daniel Henrique Barboza wrote:
>>> Hi,
>>>
>>> This series implements pnv-phb4 user devices for the powernv9 machine.
>>> It also includes a couple of pnv-phb3 and pnv-phb3-root-port fixes that
>>> were also applied for the pnv4 equivalents.
>>>
>>> During the enablement I had to rollback from the previously added
>>> support for user creatable pnv-phb4-pec devices. The most important
>>> reason is user experience. PEC devices that doesn't spawn the PHB
>>> devices will be just a placeholder to add PHBs, having no use of their
>>> own as far as the user sees it. From this standpoint it makes more sense
>>> to just create all PECs and attach the PHBs the user wants on them.
>>> Patch 14 also describes technical reasons to rollback this support.
>>>
>>> The series is rebased using Cedric's 'powernv-6.2' branch [1]i, which
>>> includes the '[PATCH 0/5] ppc/pnv: Preliminary cleanups before user
>>> created PHBs' patches [2].
>>
>> It would be easier if you based the patchset on mainline. It's not
>> a problem to resend patches of another person or/and even rework
>> them to fit your needs.
> 
> Sure, I'll send the v2 based on the mainline + the required patches.

Let me merge a couple first. It should reduce the overhead. I will drop
these :

   ppc/pnv: Attach PHB3 root port device when defaults are enabled
   ppc/pnv: Attach PHB4 root port device when defaults are enabled

They are in the way for your changes.

Thanks,

C.

Re: [PATCH 00/17] ppc/pnv: enable pnv-phb4 user devices

[Daniel Henrique Barboza]

On 1/3/22 18:20, Cédric Le Goater wrote:
> On 1/3/22 19:58, Daniel Henrique Barboza wrote:
>>
>>
>> On 1/3/22 05:21, Cédric Le Goater wrote:
>>> Hello Daniel,
>>>
>>> On 12/28/21 20:37, Daniel Henrique Barboza wrote:
>>>> Hi,
>>>>
>>>> This series implements pnv-phb4 user devices for the powernv9 machine.
>>>> It also includes a couple of pnv-phb3 and pnv-phb3-root-port fixes that
>>>> were also applied for the pnv4 equivalents.
>>>>
>>>> During the enablement I had to rollback from the previously added
>>>> support for user creatable pnv-phb4-pec devices. The most important
>>>> reason is user experience. PEC devices that doesn't spawn the PHB
>>>> devices will be just a placeholder to add PHBs, having no use of their
>>>> own as far as the user sees it. From this standpoint it makes more sense
>>>> to just create all PECs and attach the PHBs the user wants on them.
>>>> Patch 14 also describes technical reasons to rollback this support.
>>>>
>>>> The series is rebased using Cedric's 'powernv-6.2' branch [1]i, which
>>>> includes the '[PATCH 0/5] ppc/pnv: Preliminary cleanups before user
>>>> created PHBs' patches [2].
>>>
>>> It would be easier if you based the patchset on mainline. It's not
>>> a problem to resend patches of another person or/and even rework
>>> them to fit your needs.
>>
>> Sure, I'll send the v2 based on the mainline + the required patches.
> 
> Let me merge a couple first. It should reduce the overhead. I will drop
> these :

No problem. I'll re-send the v2 after the merge.

> 
>    ppc/pnv: Attach PHB3 root port device when defaults are enabled
>    ppc/pnv: Attach PHB4 root port device when defaults are enabled
> 
> They are in the way for your changes.


I`ll drop these in my side as well.


Thanks,


Daniel







> 
> Thanks,
> 
> C.

Re: [PATCH 00/17] ppc/pnv: enable pnv-phb4 user devices

[Cédric Le Goater]

On 12/28/21 20:37, Daniel Henrique Barboza wrote:
> Hi,
> 
> This series implements pnv-phb4 user devices for the powernv9 machine.
> It also includes a couple of pnv-phb3 and pnv-phb3-root-port fixes that
> were also applied for the pnv4 equivalents.
> 
> During the enablement I had to rollback from the previously added
> support for user creatable pnv-phb4-pec devices. The most important
> reason is user experience. PEC devices that doesn't spawn the PHB
> devices will be just a placeholder to add PHBs, having no use of their
> own as far as the user sees it. From this standpoint it makes more sense
> to just create all PECs and attach the PHBs the user wants on them.
> Patch 14 also describes technical reasons to rollback this support.
> 
> The series is rebased using Cedric's 'powernv-6.2' branch [1]i, which
> includes the '[PATCH 0/5] ppc/pnv: Preliminary cleanups before user
> created PHBs' patches [2].
> 
> [1] https://github.com/legoater/qemu/tree/powernv-6.2
> [2] https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg03810.html


Applied patches 2-16 in ppc-next.

Thanks,

C.

Re: [PATCH 01/17] pnv_phb3.c: add unique chassis and slot for pnv_phb3_root_port

[Daniel Henrique Barboza]

On 1/3/22 05:24, Cédric Le Goater wrote:
> On 12/28/21 20:37, Daniel Henrique Barboza wrote:
>> When creating a pnv_phb3_root_port using the command line, the first
>> root port is created successfully, but the second fails with the
>> following error:
>>
>> qemu-system-ppc64: -device pnv-phb3-root-port,bus=phb3-root.0,id=pcie.3:
>> Can't add chassis slot, error -16
>>
>> This error comes from the realize() function of its parent type,
>> rp_realize() from TYPE_PCIE_ROOT_PORT. pcie_chassis_add_slot() fails
>> with -EBUSY if there's an existing PCIESlot that has the same
>> chassis/slot value, regardless of being in a different bus.
>>
>> One way to prevent this error is simply set chassis and slot values in
>> the command line. However, since phb3 root buses only supports a single
>> root port, we can just get an unique chassis/slot value by checking
>> which root bus the pnv_phb3_root_port is going to be attached, get the
>> equivalent phb3 device and use its chip-id and index values, which are
>> guaranteed to be unique.
> 
> I guess parent_realize() will fail if we add 2 root port devices under
> the same phb ?

If we change chassis/slot for each new pci root port the QEMU emulation will
allow it. The problem is with skiboot which, at least according to the commit
that introduced powernv9 support [1], does not support multiple PCIE devices in the
same PHB:

----
No default device layout is provided and PCI devices can be added on
any of the available PCIe Root Port (pcie.0 .. 2 of a Power9 chip)
with address 0x0 as the firwware (skiboot) only accepts a single
device per root port.
----

That said, I'm taking this information at face value. Perhaps this is a test
worth doing to at least document this restriction more explicitly in the
docs.


[1] https://github.com/qemu/qemu/commit/4f9924c4d4cf9c039e247c5cdbbf71bce4e573c3


Thanks,

Daniel

> 
> Thanks,
> 
> C.
> 
> 
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/pci-host/pnv_phb3.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
>> index 4e2d680d44..130d392b3e 100644
>> --- a/hw/pci-host/pnv_phb3.c
>> +++ b/hw/pci-host/pnv_phb3.c
>> @@ -1156,8 +1156,24 @@ static const TypeInfo pnv_phb3_root_bus_info = {
>>   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);
>>
> 

Re: [PATCH 01/17] pnv_phb3.c: add unique chassis and slot for pnv_phb3_root_port

[Daniel Henrique Barboza]

On 1/3/22 05:24, Cédric Le Goater wrote:
> On 12/28/21 20:37, Daniel Henrique Barboza wrote:
>> When creating a pnv_phb3_root_port using the command line, the first
>> root port is created successfully, but the second fails with the
>> following error:
>>
>> qemu-system-ppc64: -device pnv-phb3-root-port,bus=phb3-root.0,id=pcie.3:
>> Can't add chassis slot, error -16
>>
>> This error comes from the realize() function of its parent type,
>> rp_realize() from TYPE_PCIE_ROOT_PORT. pcie_chassis_add_slot() fails
>> with -EBUSY if there's an existing PCIESlot that has the same
>> chassis/slot value, regardless of being in a different bus.
>>
>> One way to prevent this error is simply set chassis and slot values in
>> the command line. However, since phb3 root buses only supports a single
>> root port, we can just get an unique chassis/slot value by checking
>> which root bus the pnv_phb3_root_port is going to be attached, get the
>> equivalent phb3 device and use its chip-id and index values, which are
>> guaranteed to be unique.
> 
> I guess parent_realize() will fail if we add 2 root port devices under
> the same phb ?

Yes. It will fail with an error similar to the one that happens with phb4 root
ports:


qemu-system-ppc64: -device pnv-phb4-root-port,bus=pnv-phb4-root-bus.0,slot=8: Bus 'pnv-phb4-root-bus.0' is full

This happens because we're setting max_dev = 1 in the root bus class, in both cases:


static void pnv_phb4_root_bus_class_init(ObjectClass *klass, void *data)
{
     BusClass *k = BUS_CLASS(klass);

     /*
      * PHB4 has only a single root complex. Enforce the limit on the
      * parent bus
      */
     k->max_dev = 1;
}


I also did a quick experiment with both. I set their root bus class max_dev to 2, then changed
the code to allow adding a second root port in the PHB. The machine boots, which is good, but the
OS doesn't see the extra root port in the output of 'lspci'. Same result for both.


I can't see if this has to do with skiboot lacking support for it or something that we need
to do in QEMU. For PHB4s, seeing how we've tied together the stack->phb->root port relationship
throughout the code I can't say I'm surprised. Even if skiboot starts to allow multiple pci devices
attached directly in the root complex we'll have to do some rework in the QEMU side to allow it.


Thanks,


Daniel

> 
> Thanks,
> 
> C.
> 
> 
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/pci-host/pnv_phb3.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
>> index 4e2d680d44..130d392b3e 100644
>> --- a/hw/pci-host/pnv_phb3.c
>> +++ b/hw/pci-host/pnv_phb3.c
>> @@ -1156,8 +1156,24 @@ static const TypeInfo pnv_phb3_root_bus_info = {
>>   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);
>>
> 

Re: [PATCH 11/17] pnv_phb4_pec.c: use pnv_pec_get_phb_id() in pnv_pec_dt_xscom()

[Daniel Henrique Barboza]

On 1/3/22 06:08, Cédric Le Goater wrote:
> On 12/28/21 20:38, Daniel Henrique Barboza wrote:
>> Relying on stack->phb to write the xscom DT of the PEC is something that
>> we won't be able to do with user creatable pnv-phb4 devices.
>>
>> Hopefully, this can be done by using pnv_pec_get_phb_id(), which is
>> already used by pnv_pec_realize() to set the phb-id of the stack. Use
>> the same idea in pnv_pec_dt_xscom() to write ibm,phb-index without the
>> need to accessing stack->phb, since stack->phb is not granted to be !=
>> NULL when user creatable phbs are introduced.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Couldn't we do that already without the previous change ?

Yes. In fact I'll postpone the previous patch until we actually need it (in this
series it would be patch 15).


Thanks,

Daniel

> 
> Thanks,
> 
> C.
> 
> 
>> ---
>>   hw/pci-host/pnv_phb4_pec.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>> index 4f6db26633..56ffd446ab 100644
>> --- a/hw/pci-host/pnv_phb4_pec.c
>> +++ b/hw/pci-host/pnv_phb4_pec.c
>> @@ -466,8 +466,7 @@ static int pnv_pec_dt_xscom(PnvXScomInterface *dev, void *fdt,
>>                         pecc->compat_size)));
>>       for (i = 0; i < pec->num_stacks; i++) {
>> -        PnvPhb4PecStack *stack = &pec->stacks[i];
>> -        PnvPHB4 *phb = &stack->phb;
>> +        int phb_id =  pnv_pec_get_phb_id(pec, i);
>>           int stk_offset;
>>           name = g_strdup_printf("stack@%x", i);
>> @@ -477,7 +476,7 @@ static int pnv_pec_dt_xscom(PnvXScomInterface *dev, void *fdt,
>>           _FDT((fdt_setprop(fdt, stk_offset, "compatible", pecc->stk_compat,
>>                             pecc->stk_compat_size)));
>>           _FDT((fdt_setprop_cell(fdt, stk_offset, "reg", i)));
>> -        _FDT((fdt_setprop_cell(fdt, stk_offset, "ibm,phb-index", phb->phb_id)));
>> +        _FDT((fdt_setprop_cell(fdt, stk_offset, "ibm,phb-index", phb_id)));
>>       }
>>       return 0;
>>
> 

Re: [PATCH 09/17] pnv_phb4_pec.c: move pnv_pec_phb_offset() to pnv_phb4.c

[Daniel Henrique Barboza]

On 1/3/22 06:00, Cédric Le Goater wrote:
> On 12/28/21 20:37, Daniel Henrique Barboza wrote:
>> The logic inside pnv_pec_phb_offset() wiil be useful in the next patch
> 
> will
> 
>> to determine the stack that should contain a PHB4 device.
>>
>> Move the function to pnv_phb4.c and make it public since there's no
>> pnv_phb4_pec.h header. While we're at it, add 'stack_index' as a
>> parameter and make the function return 'phb-id' directly. And rename it
>> to pnv_pec_get_phb_id() to be even clearer about the function intent.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
> Looks good,
> 
>> ---
>>   hw/pci-host/pnv_phb4.c         | 17 +++++++++++++++++
>>   hw/pci-host/pnv_phb4_pec.c     | 15 +--------------
>>   include/hw/pci-host/pnv_phb4.h |  2 ++
>>   3 files changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
>> index 0ea505cc94..36c56007ba 100644
>> --- a/hw/pci-host/pnv_phb4.c
>> +++ b/hw/pci-host/pnv_phb4.c
>> @@ -1164,6 +1164,23 @@ static AddressSpace *pnv_phb4_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>>       return &ds->dma_as;
>>   }
>> +/*
>> + * Return the index/phb-id of a PHB4 that belongs to a
>> + * pec->stacks[stack_index] stack.
>> + */
>> +int pnv_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index)
> 
> pnv_phb4_pec_get_phb_id() would be cleaner.

Changed in the upcoming v2.


Daniel

> 
> Thanks,
> 
> C.
> 
> 
> 
>> +{
>> +    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
>> +    int index = pec->index;
>> +    int offset = 0;
>> +
>> +    while (index--) {
>> +        offset += pecc->num_stacks[index];
>> +    }
>> +
>> +    return offset + stack_index;
>> +}
>> +
>>   /*
>>    * Set the object properties of a phb in relation with its stack.
>>    *
>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>> index 700ee4b185..bc2f8bb8b1 100644
>> --- a/hw/pci-host/pnv_phb4_pec.c
>> +++ b/hw/pci-host/pnv_phb4_pec.c
>> @@ -374,19 +374,6 @@ static void pnv_pec_instance_init(Object *obj)
>>       }
>>   }
>> -static int pnv_pec_phb_offset(PnvPhb4PecState *pec)
>> -{
>> -    PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
>> -    int index = pec->index;
>> -    int offset = 0;
>> -
>> -    while (index--) {
>> -        offset += pecc->num_stacks[index];
>> -    }
>> -
>> -    return offset;
>> -}
>> -
>>   static void pnv_pec_realize(DeviceState *dev, Error **errp)
>>   {
>>       PnvPhb4PecState *pec = PNV_PHB4_PEC(dev);
>> @@ -422,7 +409,7 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>>       for (i = 0; i < pec->num_stacks; i++) {
>>           PnvPhb4PecStack *stack = &pec->stacks[i];
>>           Object *stk_obj = OBJECT(stack);
>> -        int phb_id = pnv_pec_phb_offset(pec) + i;
>> +        int phb_id =  pnv_pec_get_phb_id(pec, i);
>>           object_property_set_int(stk_obj, "stack-no", i, &error_abort);
>>           object_property_set_int(stk_obj, "phb-id", phb_id, &error_abort);
>> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
>> index d7838513f1..0fa88ca3fa 100644
>> --- a/include/hw/pci-host/pnv_phb4.h
>> +++ b/include/hw/pci-host/pnv_phb4.h
>> @@ -15,6 +15,7 @@
>>   #include "hw/ppc/xive.h"
>>   #include "qom/object.h"
>> +typedef struct PnvPhb4PecState PnvPhb4PecState;
>>   typedef struct PnvPhb4PecStack PnvPhb4PecStack;
>>   typedef struct PnvPHB4 PnvPHB4;
>>   typedef struct PnvChip PnvChip;
>> @@ -134,6 +135,7 @@ struct PnvPHB4 {
>>   void pnv_phb4_pic_print_info(PnvPHB4 *phb, Monitor *mon);
>>   void pnv_phb4_update_regions(PnvPhb4PecStack *stack);
>>   void pnv_phb4_set_stack_phb_props(PnvPhb4PecStack *stack, PnvPHB4 *phb);
>> +int pnv_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index);
>>   extern const MemoryRegionOps pnv_phb4_xscom_ops;
>>   /*
>>
>