[PATCH v2 00/18] user creatable pnv-phb3/pnv-phb4 devices

[PATCH v2 00/18] user creatable pnv-phb3/pnv-phb4 devices

[Daniel Henrique Barboza]

Hi,

This second version was rebased with upstream and includes fixed/amended
versions of relevant patches that were sent to the mailing list and aren't
upstream yet. In this process 4 patches from v1 were discarded, becoming
either irrelevant or squashed into others.

The patches are organized as follows:

- patches 1-4: enable user creatable phb3/phb4 root ports
- patches 5-10: enable user creatable pnv-phb3 devices
- patches 11-18: enable user creatable pnv-phb4 devices

Here are some examples of what we're able to do with this series:

* powernv8 machine with -nodefaults,2 pnv-phb3s with 'pcie.N' name,
one of them with a root port and a netcard:

$ qemu-system-ppc64 -m 4G -machine powernv8,accel=tcg -smp 2,cores=2,threads=1 \
-bios skiboot.lid  -kernel vmlinux -initrd buildroot.rootfs.cpio \
-append 'console=hvc0 ro xmon=on' \
-nodefaults \
-serial mon:stdio -nographic \
-device pnv-phb3,chip-id=0,index=0,id=pcie.0 \
-device pnv-phb3,chip-id=0,index=2,id=pcie.2 \
-device pnv-phb3-root-port,bus=pcie.2,id=pcie.5 \
-netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \
-device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=pcie.5,addr=0x0

* powernv9 machine with -nodefaults, 3 of the available 12 pnv-phb4 devices
created, 2 root ports, one of the port with a pcie-pci-bridge and
devices connected in the bridge:

$ qemu-system-ppc64 -m 4G -machine powernv9 \
-smp 2,sockets=2,cores=1,threads=1 \
-accel tcg,thread=multi -bios skiboot.lid \
-kernel vmlinux -initrd buildroot.rootfs.cpio \
-append 'console=hvc0 ro xmon=on' \
-nodefaults \
-serial mon:stdio -nographic \
-device pnv-phb4,chip-id=0,index=0,id=pcie.0 \
-device pnv-phb4,chip-id=0,index=4,id=pcie.1 \
-device pnv-phb4,chip-id=1,index=3,id=pcie.2 \
-device pnv-phb4-root-port,id=root0,bus=pcie.2 \
-device pnv-phb4-root-port,id=root1,bus=pcie.1 \
-device pcie-pci-bridge,id=bridge1,bus=root0,addr=0x0 \
-device nvme,bus=bridge1,addr=0x1,drive=drive0,serial=1234 \
-drive file=./simics-disk.raw,if=none,id=drive0,format=raw,cache=none \
-device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=bridge1,addr=0x3 \
-netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \
-device nec-usb-xhci,bus=bridge1,addr=0x2 


* powernv8/9 with default settings can be used as usual. The work done
in this series didn't change the name of the buses created by the
default root ports (named pcie.0...N):

$ qemu-system-ppc64 -m 4G \
-machine powernv9 -smp 2,sockets=2,cores=1,threads=1  \
-accel tcg,thread=multi -bios skiboot.lid  \
-kernel vmlinux -initrd buildroot.rootfs.cpio \
-append 'console=hvc0 ro xmon=on' \
-serial mon:stdio -nographic \
-device pcie-pci-bridge,id=bridge1,bus=pcie.0,addr=0x0 \
-device nvme,bus=bridge1,addr=0x1,drive=drive0,serial=1234  \
-drive file=./simics-disk.raw,if=none,id=drive0,format=raw,cache=none \
-device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=bridge1,addr=0x3 \
-netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \
-device nec-usb-xhci,bus=bridge1,addr=0x2 


Changes from v1:
- rebased with upstream at 7d4ae4d497807
- added relevant patches that aren't upstream yet from "ppc/pnv:
Preliminary cleanups before user created PHBs" [1] and "ppc/pnv: Add
support for user created PHB3/PHB4 devices" [2] series
- renamed phb3/phb4 default buses name to 'pnv-phb3-root' and
'pnv-phb4-root'
- renamed pnv_pec_get_phb_id() to pnv_phb4_pec_get_phb_id()
- patch 'introduce pnv_pec_init_stack_xscom()' moved to patch 16 to
be closer with patch 17 that uses it
- v1 link: https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg04427.html

[1] https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg03810.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg01548.html


Cédric Le Goater (5):
  ppc/pnv: Attach PHB3 root port device when defaults are enabled
  ppc/pnv: Introduce support for user created PHB3 devices
  ppc/pnv: Reparent user created PHB3 devices to the PnvChip
  ppc/pnv: Complete user created PHB3 devices
  ppc/pnv: Move num_phbs under Pnv8Chip

Daniel Henrique Barboza (13):
  pnv_phb3.c: add unique chassis and slot for pnv_phb3_root_port
  pnv_phb4.c: add unique chassis and slot for pnv_phb4_root_port
  pnv_phb4.c: make pnv-phb4-root-port user creatable
  pnv_phb4.c: check if root port exists in rc_config functions
  pnv_phb3.h: change TYPE_PNV_PHB3_ROOT_BUS name
  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_pec: use pnv_phb4_pec_get_phb_id() in pnv_pec_dt_xscom()
  pnv_phb4.h: turn phb into a pointer in struct PnvPhb4PecStack
  pnv_phb4_pec.c: use 'default_enabled()' to init stack->phb
  pnv_phb4.c: introduce pnv_pec_init_stack_xscom()
  ppc/pnv: Introduce user creatable pnv-phb4 devices
  pnv_phb4.c: change TYPE_PNV_PHB4_ROOT_BUS name

 hw/pci-host/pnv_phb3.c         |  57 ++++++++--
 hw/pci-host/pnv_phb4.c         | 193 ++++++++++++++++++++++++++++++---
 hw/pci-host/pnv_phb4_pec.c     |  86 ++++++---------
 hw/ppc/pnv.c                   |  55 ++++++++--
 include/hw/pci-host/pnv_phb3.h |   4 +-
 include/hw/pci-host/pnv_phb4.h |  15 ++-
 include/hw/ppc/pnv.h           |   8 +-
 7 files changed, 322 insertions(+), 96 deletions(-)

-- 
2.33.1

[PATCH v2 01/18] 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 c78084cce7..3467bbb5d9 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -1142,8 +1142,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 v2 02/18] 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 5ba26e250a..836b0c156c 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1338,8 +1338,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 v2 03/18] ppc/pnv: Attach PHB3 root port device when defaults are enabled

[Daniel Henrique Barboza]

From: Cédric Le Goater <clg@kaod.org>

This cleanups the PHB3 model a bit more since the root port is an
independent device and it will ease our task when adding user created
PHB3s.

pnv_phb_attach_root_port() is made public in pnv.c so it can be reused
with the pnv_phb4 root port later.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb3.c         | 15 ++++++---------
 hw/ppc/pnv.c                   |  8 ++++++++
 include/hw/pci-host/pnv_phb3.h |  2 --
 include/hw/ppc/pnv.h           |  1 +
 4 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index 3467bbb5d9..fdc8d0b437 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -19,6 +19,7 @@
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
 #include "qom/object.h"
+#include "sysemu/sysemu.h"
 
 #define phb3_error(phb, fmt, ...)                                       \
     qemu_log_mask(LOG_GUEST_ERROR, "phb3[%d:%d]: " fmt "\n",            \
@@ -981,10 +982,6 @@ static void pnv_phb3_instance_init(Object *obj)
     /* Power Bus Common Queue */
     object_initialize_child(obj, "pbcq", &phb->pbcq, TYPE_PNV_PBCQ);
 
-    /* Root Port */
-    object_initialize_child(obj, "root", &phb->root, TYPE_PNV_PHB3_ROOT_PORT);
-    qdev_prop_set_int32(DEVICE(&phb->root), "addr", PCI_DEVFN(0, 0));
-    qdev_prop_set_bit(DEVICE(&phb->root), "multifunction", false);
 }
 
 static void pnv_phb3_realize(DeviceState *dev, Error **errp)
@@ -1053,10 +1050,10 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
 
     pci_setup_iommu(pci->bus, pnv_phb3_dma_iommu, phb);
 
-    /* Add a single Root port */
-    qdev_prop_set_uint8(DEVICE(&phb->root), "chassis", phb->chip_id);
-    qdev_prop_set_uint16(DEVICE(&phb->root), "slot", phb->phb_id);
-    qdev_realize(DEVICE(&phb->root), BUS(pci->bus), &error_fatal);
+    if (defaults_enabled()) {
+        pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
+                                 TYPE_PNV_PHB3_ROOT_PORT);
+    }
 }
 
 void pnv_phb3_update_regions(PnvPHB3 *phb)
@@ -1177,7 +1174,7 @@ static void pnv_phb3_root_port_class_init(ObjectClass *klass, void *data)
 
     device_class_set_parent_realize(dc, pnv_phb3_root_port_realize,
                                     &rpc->parent_realize);
-    dc->user_creatable = false;
+    dc->user_creatable = true;
 
     k->vendor_id = PCI_VENDOR_ID_IBM;
     k->device_id = 0x03dc;
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 9de8b83530..3a263f631a 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1156,6 +1156,14 @@ static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
     }
 }
 
+/* Attach a root port device */
+void pnv_phb_attach_root_port(PCIHostState *pci, const char *name)
+{
+    PCIDevice *root = pci_new(PCI_DEVFN(0, 0), name);
+
+    pci_realize_and_unref(root, pci->bus, &error_fatal);
+}
+
 static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
 {
     PnvChipClass *pcc = PNV_CHIP_GET_CLASS(dev);
diff --git a/include/hw/pci-host/pnv_phb3.h b/include/hw/pci-host/pnv_phb3.h
index e9c13e6bd8..2e423c3890 100644
--- a/include/hw/pci-host/pnv_phb3.h
+++ b/include/hw/pci-host/pnv_phb3.h
@@ -155,8 +155,6 @@ struct PnvPHB3 {
 
     PnvPBCQState pbcq;
 
-    PnvPHB3RootPort root;
-
     QLIST_HEAD(, PnvPhb3DMASpace) dma_spaces;
 
     PnvChip *chip;
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index c781525277..c726288e5e 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -177,6 +177,7 @@ DECLARE_INSTANCE_CHECKER(PnvChip, PNV_CHIP_POWER10,
                          TYPE_PNV_CHIP_POWER10)
 
 PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
+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 v2 04/18] pnv_phb4.c: make pnv-phb4-root-port user creatable

[Daniel Henrique Barboza]

We want to create only the absolutely minimal amount of devices when
running with -nodefaults. The root port is something that the machine
can boot up without. But, to do that, we need to provide a way for the
user to add them by hand.

This patch makes pnv-phb4-root-port user creatable and then uses the
pnv_phb_attach_root_port() helper to add a pnv_phb4_root_port only when
running with default settings.

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

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 836b0c156c..14827f8464 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, ...)                                        \
@@ -1159,12 +1160,6 @@ static void pnv_phb4_instance_init(Object *obj)
 
     /* XIVE interrupt source object */
     object_initialize_child(obj, "source", &phb->xsrc, TYPE_XIVE_SOURCE);
-
-    /* Root Port */
-    object_initialize_child(obj, "root", &phb->root, TYPE_PNV_PHB4_ROOT_PORT);
-
-    qdev_prop_set_int32(DEVICE(&phb->root), "addr", PCI_DEVFN(0, 0));
-    qdev_prop_set_bit(DEVICE(&phb->root), "multifunction", false);
 }
 
 static void pnv_phb4_realize(DeviceState *dev, Error **errp)
@@ -1208,10 +1203,11 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
     pci_setup_iommu(pci->bus, pnv_phb4_dma_iommu, phb);
     pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
 
-    /* Add a single Root port */
-    qdev_prop_set_uint8(DEVICE(&phb->root), "chassis", phb->chip_id);
-    qdev_prop_set_uint16(DEVICE(&phb->root), "slot", phb->phb_id);
-    qdev_realize(DEVICE(&phb->root), BUS(pci->bus), &error_fatal);
+    /* Add a single Root port if running with defaults */
+    if (defaults_enabled()) {
+        pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
+                                 TYPE_PNV_PHB4_ROOT_PORT);
+    }
 
     /* Setup XIVE Source */
     if (phb->big_phb) {
@@ -1369,7 +1365,7 @@ static void pnv_phb4_root_port_class_init(ObjectClass *klass, void *data)
     PCIERootPortClass *rpc = PCIE_ROOT_PORT_CLASS(klass);
 
     dc->desc     = "IBM PHB4 PCIE Root Port";
-    dc->user_creatable = false;
+    dc->user_creatable = true;
 
     device_class_set_parent_realize(dc, pnv_phb4_root_port_realize,
                                     &rpc->parent_realize);
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index 4a19338db3..ea63df9676 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -78,8 +78,6 @@ OBJECT_DECLARE_SIMPLE_TYPE(PnvPHB4, PNV_PHB4)
 struct PnvPHB4 {
     PCIExpressHost parent_obj;
 
-    PnvPHB4RootPort root;
-
     uint32_t chip_id;
     uint32_t phb_id;
 
-- 
2.33.1

[PATCH v2 05/18] 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 | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 14827f8464..83dedc878a 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -152,7 +152,10 @@ static void pnv_phb4_rc_config_write(PnvPHB4 *phb, unsigned off,
     }
 
     pdev = pci_find_device(pci->bus, 0, 0);
-    assert(pdev);
+    if (!pdev) {
+        phb_error(phb, "rc_config_write device not found\n");
+        return;
+    }
 
     pci_host_config_write_common(pdev, off, PHB_RC_CONFIG_SIZE,
                                  bswap32(val), 4);
@@ -171,7 +174,10 @@ static uint64_t pnv_phb4_rc_config_read(PnvPHB4 *phb, unsigned off,
     }
 
     pdev = pci_find_device(pci->bus, 0, 0);
-    assert(pdev);
+    if (!pdev) {
+        phb_error(phb, "rc_config_read device not found\n");
+        return ~0ull;
+    }
 
     val = pci_host_config_read_common(pdev, off, PHB_RC_CONFIG_SIZE, 4);
     return bswap32(val);
-- 
2.33.1

[PATCH v2 06/18] ppc/pnv: Introduce support for user created PHB3 devices

[Daniel Henrique Barboza]

From: Cédric Le Goater <clg@kaod.org>

PHB3 devices and PCI devices can now be added to the powernv8 machine
using :

  -device pnv-phb3,chip-id=0,index=1 \
  -device nec-usb-xhci,bus=pci.1,addr=0x0

The 'index' property identifies the PHB3 in the chip. In case of user
created devices, a lookup on 'chip-id' is required to assign the
owning chip.

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/pci-host/pnv_phb3.c | 11 ++++++++++-
 hw/ppc/pnv.c           | 23 ++++++++++++++++++-----
 include/hw/ppc/pnv.h   |  2 ++
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index fdc8d0b437..1ebe43df5d 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -991,6 +991,15 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
     PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
     int i;
 
+    /* User created devices */
+    if (!phb->chip) {
+        phb->chip = pnv_get_chip(pnv, phb->chip_id);
+        if (!phb->chip) {
+            error_setg(errp, "invalid chip id: %d", phb->chip_id);
+            return;
+        }
+    }
+
     if (phb->phb_id >= PNV_CHIP_GET_CLASS(phb->chip)->num_phbs) {
         error_setg(errp, "invalid PHB index: %d", phb->phb_id);
         return;
@@ -1104,7 +1113,7 @@ static void pnv_phb3_class_init(ObjectClass *klass, void *data)
     dc->realize = pnv_phb3_realize;
     device_class_set_props(dc, pnv_phb3_properties);
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
-    dc->user_creatable = false;
+    dc->user_creatable = true;
 }
 
 static const TypeInfo pnv_phb3_type_info = {
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 3a263f631a..ad02d56aa7 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1117,14 +1117,14 @@ static void pnv_chip_power8_instance_init(Object *obj)
 
     object_initialize_child(obj, "homer", &chip8->homer, TYPE_PNV8_HOMER);
 
-    for (i = 0; i < pcc->num_phbs; i++) {
+    if (defaults_enabled()) {
+        chip->num_phbs = pcc->num_phbs;
+    }
+
+    for (i = 0; i < chip->num_phbs; i++) {
         object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3);
     }
 
-    /*
-     * Number of PHBs is the chip default
-     */
-    chip->num_phbs = pcc->num_phbs;
 }
 
 static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
@@ -1814,6 +1814,19 @@ static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
     return NULL;
 }
 
+PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id)
+{
+    int i;
+
+    for (i = 0; i < pnv->num_chips; i++) {
+        PnvChip *chip = pnv->chips[i];
+        if (chip->chip_id == chip_id) {
+            return chip;
+        }
+    }
+    return NULL;
+}
+
 static int pnv_ics_resend_child(Object *child, void *opaque)
 {
     PnvPHB3 *phb3 = (PnvPHB3 *) object_dynamic_cast(child, TYPE_PNV_PHB3);
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index c726288e5e..64bab7112b 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -218,6 +218,8 @@ struct PnvMachineState {
     hwaddr       fw_load_addr;
 };
 
+PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id);
+
 #define PNV_FDT_ADDR          0x01000000
 #define PNV_TIMEBASE_FREQ     512000000ULL
 
-- 
2.33.1

[PATCH v2 07/18] ppc/pnv: Reparent user created PHB3 devices to the PnvChip

[Daniel Henrique Barboza]

From: Cédric Le Goater <clg@kaod.org>

The powernv machine uses the object hierarchy to populate the device
tree and each device should be parented to the chip it belongs to.
This is not the case for user created devices which are parented to
the container "/unattached".

Make sure a PHB3 device is parented to its chip by reparenting the
object if necessary.

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/pci-host/pnv_phb3.c |  6 ++++++
 hw/ppc/pnv.c           | 17 +++++++++++++++++
 include/hw/ppc/pnv.h   |  1 +
 3 files changed, 24 insertions(+)

diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index 1ebe43df5d..a52aedcad3 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -998,6 +998,12 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
             error_setg(errp, "invalid chip id: %d", phb->chip_id);
             return;
         }
+
+        /*
+         * Reparent user created devices to the chip to build
+         * correctly the device tree.
+         */
+        pnv_chip_parent_fixup(phb->chip, OBJECT(phb), phb->phb_id);
     }
 
     if (phb->phb_id >= PNV_CHIP_GET_CLASS(phb->chip)->num_phbs) {
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index ad02d56aa7..fa5e7bc751 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1814,6 +1814,23 @@ static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
     return NULL;
 }
 
+void pnv_chip_parent_fixup(PnvChip *chip, Object *obj, int index)
+{
+    Object *parent = OBJECT(chip);
+    g_autofree char *default_id =
+        g_strdup_printf("%s[%d]", object_get_typename(obj), index);
+
+    if (obj->parent == parent) {
+        return;
+    }
+
+    object_ref(obj);
+    object_unparent(obj);
+    object_property_add_child(
+        parent, DEVICE(obj)->id ? DEVICE(obj)->id : default_id, obj);
+    object_unref(obj);
+}
+
 PnvChip *pnv_get_chip(PnvMachineState *pnv, uint32_t chip_id)
 {
     int i;
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 64bab7112b..f4219da7c5 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -178,6 +178,7 @@ DECLARE_INSTANCE_CHECKER(PnvChip, PNV_CHIP_POWER10,
 
 PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
 void pnv_phb_attach_root_port(PCIHostState *pci, const char *name);
+void pnv_chip_parent_fixup(PnvChip *chip, Object *obj, int index);
 
 #define TYPE_PNV_MACHINE       MACHINE_TYPE_NAME("powernv")
 typedef struct PnvMachineClass PnvMachineClass;
-- 
2.33.1

[PATCH v2 08/18] ppc/pnv: Complete user created PHB3 devices

[Daniel Henrique Barboza]

From: Cédric Le Goater <clg@kaod.org>

PHB3s ared SysBus devices and should be allowed to be dynamically
created.

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/pci-host/pnv_phb3.c | 9 +++++++++
 hw/ppc/pnv.c           | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
index a52aedcad3..7fb35dc031 100644
--- a/hw/pci-host/pnv_phb3.c
+++ b/hw/pci-host/pnv_phb3.c
@@ -993,6 +993,9 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
 
     /* User created devices */
     if (!phb->chip) {
+        Error *local_err = NULL;
+        BusState *s;
+
         phb->chip = pnv_get_chip(pnv, phb->chip_id);
         if (!phb->chip) {
             error_setg(errp, "invalid chip id: %d", phb->chip_id);
@@ -1004,6 +1007,12 @@ static void pnv_phb3_realize(DeviceState *dev, Error **errp)
          * correctly the device tree.
          */
         pnv_chip_parent_fixup(phb->chip, OBJECT(phb), phb->phb_id);
+
+        s = qdev_get_parent_bus(DEVICE(phb->chip));
+        if (!qdev_set_parent_bus(DEVICE(phb), s, &local_err)) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
     if (phb->phb_id >= PNV_CHIP_GET_CLASS(phb->chip)->num_phbs) {
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index fa5e7bc751..8dc6382357 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1941,6 +1941,8 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data)
 
     pmc->compat = compat;
     pmc->compat_size = sizeof(compat);
+
+    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB3);
 }
 
 static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
-- 
2.33.1

[PATCH v2 09/18] ppc/pnv: Move num_phbs under Pnv8Chip

[Daniel Henrique Barboza]

From: Cédric Le Goater <clg@kaod.org>

It is not used elsewhere so that's where it belongs.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/pnv.c         | 7 +++----
 include/hw/ppc/pnv.h | 4 ++--
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 8dc6382357..fe7e67e73a 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1099,7 +1099,6 @@ static void pnv_chip_power10_intc_print_info(PnvChip *chip, PowerPCCPU *cpu,
 
 static void pnv_chip_power8_instance_init(Object *obj)
 {
-    PnvChip *chip = PNV_CHIP(obj);
     Pnv8Chip *chip8 = PNV8_CHIP(obj);
     PnvChipClass *pcc = PNV_CHIP_GET_CLASS(obj);
     int i;
@@ -1118,10 +1117,10 @@ static void pnv_chip_power8_instance_init(Object *obj)
     object_initialize_child(obj, "homer", &chip8->homer, TYPE_PNV8_HOMER);
 
     if (defaults_enabled()) {
-        chip->num_phbs = pcc->num_phbs;
+        chip8->num_phbs = pcc->num_phbs;
     }
 
-    for (i = 0; i < chip->num_phbs; i++) {
+    for (i = 0; i < chip8->num_phbs; i++) {
         object_initialize_child(obj, "phb[*]", &chip8->phbs[i], TYPE_PNV_PHB3);
     }
 
@@ -1247,7 +1246,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
                                 &chip8->homer.regs);
 
     /* PHB3 controllers */
-    for (i = 0; i < chip->num_phbs; i++) {
+    for (i = 0; i < chip8->num_phbs; i++) {
         PnvPHB3 *phb = &chip8->phbs[i];
 
         object_property_set_int(OBJECT(phb), "index", i, &error_fatal);
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index f4219da7c5..0e9e16544f 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -52,7 +52,6 @@ struct PnvChip {
     uint64_t     cores_mask;
     PnvCore      **cores;
 
-    uint32_t     num_phbs;
     uint32_t     num_pecs;
 
     MemoryRegion xscom_mmio;
@@ -82,6 +81,7 @@ struct Pnv8Chip {
 
 #define PNV8_CHIP_PHB3_MAX 4
     PnvPHB3      phbs[PNV8_CHIP_PHB3_MAX];
+    uint32_t     num_phbs;
 
     XICSFabric    *xics;
 };
@@ -136,8 +136,8 @@ struct PnvChipClass {
     /*< public >*/
     uint64_t     chip_cfam_id;
     uint64_t     cores_mask;
-    uint32_t     num_phbs;
     uint32_t     num_pecs;
+    uint32_t     num_phbs;
 
     DeviceRealize parent_realize;
 
-- 
2.33.1

[PATCH v2 10/18] 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 'pnv-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: pnv-phb3-root.2
      type pnv-phb3-root
(...)
  dev: pnv-phb3, id ""
    index = 1 (0x1)
    chip-id = 0 (0x0)
    x-config-reg-migration-enabled = true
    bypass-iommu = false
    bus: pnv-phb3-root.1
      type pnv-phb3-root
(...)
  dev: pnv-phb3, id ""
    index = 0 (0x0)
    chip-id = 0 (0x0)
    x-config-reg-migration-enabled = true
    bypass-iommu = false
    bus: pnv-phb3-root.0
      type pnv-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..af6ec83cf6 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 "pnv-phb3-root"
 
 #define TYPE_PNV_PHB3_ROOT_PORT "pnv-phb3-root-port"
 
-- 
2.33.1

[PATCH v2 11/18] 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         | 25 +++++++++++++++++++++++++
 hw/pci-host/pnv_phb4_pec.c     | 12 +-----------
 include/hw/pci-host/pnv_phb4.h |  1 +
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 83dedc878a..6c1a33bc66 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1158,6 +1158,31 @@ 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_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 f3e4fa0c82..057d4b07fb 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -577,17 +577,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_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 ea63df9676..7f5b9cc0ac 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -131,6 +131,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 v2 12/18] pnv_phb4_pec.c: move pnv_pec_phb_offset() to pnv_phb4.c

[Daniel Henrique Barboza]

The logic inside pnv_pec_phb_offset() will 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_phb4_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 6c1a33bc66..4c785bbe4c 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1158,6 +1158,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_phb4_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 057d4b07fb..e47d19dfff 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);
@@ -405,7 +392,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_phb4_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 7f5b9cc0ac..b2c4a6b263 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;
@@ -132,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);
+int pnv_phb4_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index);
 extern const MemoryRegionOps pnv_phb4_xscom_ops;
 
 /*
-- 
2.33.1

[PATCH v2 13/18] pnv_phb4_pec: use pnv_phb4_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_phb4_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.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
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 e47d19dfff..0675fc55bc 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -449,8 +449,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_phb4_pec_get_phb_id(pec, i);
         int stk_offset;
 
         name = g_strdup_printf("stack@%x", i);
@@ -460,7 +459,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 v2 14/18] 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 4c785bbe4c..9e670e41d2 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1449,7 +1449,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 0675fc55bc..be6eefdbb1 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -563,7 +563,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 b2c4a6b263..2fb5e119c4 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -178,8 +178,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 v2 15/18] 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_phb4_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_phb4_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 be6eefdbb1..638691783b 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>
 
@@ -392,11 +393,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_phb4_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_phb4_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;
         }
@@ -531,10 +550,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)
@@ -588,6 +603,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 v2 16/18] 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 9e670e41d2..430a5c10f4 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1158,6 +1158,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 638691783b..41c79d24c4 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -556,10 +556,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);
@@ -583,20 +579,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 2fb5e119c4..610580a88f 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -132,6 +132,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_phb4_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index);
 extern const MemoryRegionOps pnv_phb4_xscom_ops;
-- 
2.33.1

[PATCH v2 17/18] 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 430a5c10f4..1c2334d491 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1236,6 +1236,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_phb4_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);
@@ -1243,8 +1278,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;
@@ -1298,6 +1374,10 @@ 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);
+
+    if (stack_init_xscom) {
+        pnv_pec_init_stack_xscom(stack);
+    }
 }
 
 static const char *pnv_phb4_root_bus_path(PCIHostState *host_bridge,
@@ -1347,7 +1427,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 41c79d24c4..4417beb97d 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -573,13 +573,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 fe7e67e73a..837146a2fb 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1960,6 +1960,8 @@ static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
     pmc->compat = compat;
     pmc->compat_size = sizeof(compat);
     pmc->dt_power_mgt = pnv_dt_power_mgt;
+
+    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 v2 18/18] 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 'pnv-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: pnv-phb4-root.11
      type pnv-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: pnv-phb4-root.6
      type pnv-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: pnv-phb4-root.5
      type pnv-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: pnv-phb4-root.0
      type pnv-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 610580a88f..0aec495cbf 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 "pnv-phb4-root"
 #define TYPE_PNV_PHB4_ROOT_PORT "pnv-phb4-root-port"
 
 typedef struct PnvPHB4RootPort {
-- 
2.33.1

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

[Cédric Le Goater]

On 1/5/22 22:23, 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.
> 
> 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 | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index c78084cce7..3467bbb5d9 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1142,8 +1142,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 v2 02/18] pnv_phb4.c: add unique chassis and slot for pnv_phb4_root_port

[Cédric Le Goater]

On 1/5/22 22:23, Daniel Henrique Barboza wrote:
> 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>


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

Thanks,

C.


> ---
>   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 5ba26e250a..836b0c156c 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1338,8 +1338,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);
> 

Re: [PATCH v2 04/18] pnv_phb4.c: make pnv-phb4-root-port user creatable

[Cédric Le Goater]

On 1/5/22 22:23, Daniel Henrique Barboza wrote:
> We want to create only the absolutely minimal amount of devices when
> running with -nodefaults. The root port is something that the machine
> can boot up without. But, to do that, we need to provide a way for the
> user to add them by hand.
> 
> This patch makes pnv-phb4-root-port user creatable and then uses the
> pnv_phb_attach_root_port() helper to add a pnv_phb4_root_port only when
> running with default settings.
> 
> 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         | 18 +++++++-----------
>   include/hw/pci-host/pnv_phb4.h |  2 --
>   2 files changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 836b0c156c..14827f8464 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, ...)                                        \
> @@ -1159,12 +1160,6 @@ static void pnv_phb4_instance_init(Object *obj)
>   
>       /* XIVE interrupt source object */
>       object_initialize_child(obj, "source", &phb->xsrc, TYPE_XIVE_SOURCE);
> -
> -    /* Root Port */
> -    object_initialize_child(obj, "root", &phb->root, TYPE_PNV_PHB4_ROOT_PORT);
> -
> -    qdev_prop_set_int32(DEVICE(&phb->root), "addr", PCI_DEVFN(0, 0));
> -    qdev_prop_set_bit(DEVICE(&phb->root), "multifunction", false);
>   }
>   
>   static void pnv_phb4_realize(DeviceState *dev, Error **errp)
> @@ -1208,10 +1203,11 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>       pci_setup_iommu(pci->bus, pnv_phb4_dma_iommu, phb);
>       pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>   
> -    /* Add a single Root port */
> -    qdev_prop_set_uint8(DEVICE(&phb->root), "chassis", phb->chip_id);
> -    qdev_prop_set_uint16(DEVICE(&phb->root), "slot", phb->phb_id);
> -    qdev_realize(DEVICE(&phb->root), BUS(pci->bus), &error_fatal);
> +    /* Add a single Root port if running with defaults */
> +    if (defaults_enabled()) {
> +        pnv_phb_attach_root_port(PCI_HOST_BRIDGE(phb),
> +                                 TYPE_PNV_PHB4_ROOT_PORT);
> +    }
>   
>       /* Setup XIVE Source */
>       if (phb->big_phb) {
> @@ -1369,7 +1365,7 @@ static void pnv_phb4_root_port_class_init(ObjectClass *klass, void *data)
>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_CLASS(klass);
>   
>       dc->desc     = "IBM PHB4 PCIE Root Port";
> -    dc->user_creatable = false;
> +    dc->user_creatable = true;
>   
>       device_class_set_parent_realize(dc, pnv_phb4_root_port_realize,
>                                       &rpc->parent_realize);
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index 4a19338db3..ea63df9676 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -78,8 +78,6 @@ OBJECT_DECLARE_SIMPLE_TYPE(PnvPHB4, PNV_PHB4)
>   struct PnvPHB4 {
>       PCIExpressHost parent_obj;
>   
> -    PnvPHB4RootPort root;
> -
>       uint32_t chip_id;
>       uint32_t phb_id;
>   
> 

Re: [PATCH v2 05/18] pnv_phb4.c: check if root port exists in rc_config functions

[Cédric Le Goater]

On 1/5/22 22:23, 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.
> 
> Instead of asserting, check if the root port exists before read/writing
> into it.
> 
> 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 | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 14827f8464..83dedc878a 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -152,7 +152,10 @@ static void pnv_phb4_rc_config_write(PnvPHB4 *phb, unsigned off,
>       }
>   
>       pdev = pci_find_device(pci->bus, 0, 0);
> -    assert(pdev);
> +    if (!pdev) {
> +        phb_error(phb, "rc_config_write device not found\n");
> +        return;
> +    }
>   
>       pci_host_config_write_common(pdev, off, PHB_RC_CONFIG_SIZE,
>                                    bswap32(val), 4);
> @@ -171,7 +174,10 @@ static uint64_t pnv_phb4_rc_config_read(PnvPHB4 *phb, unsigned off,
>       }
>   
>       pdev = pci_find_device(pci->bus, 0, 0);
> -    assert(pdev);
> +    if (!pdev) {
> +        phb_error(phb, "rc_config_read device not found\n");
> +        return ~0ull;
> +    }
>   
>       val = pci_host_config_read_common(pdev, off, PHB_RC_CONFIG_SIZE, 4);
>       return bswap32(val);
> 

Re: [PATCH v2 10/18] pnv_phb3.h: change TYPE_PNV_PHB3_ROOT_BUS name

[Cédric Le Goater]

On 1/5/22 22:23, 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 'pnv-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: pnv-phb3-root.2
>        type pnv-phb3-root
> (...)
>    dev: pnv-phb3, id ""
>      index = 1 (0x1)
>      chip-id = 0 (0x0)
>      x-config-reg-migration-enabled = true
>      bypass-iommu = false
>      bus: pnv-phb3-root.1
>        type pnv-phb3-root
> (...)
>    dev: pnv-phb3, id ""
>      index = 0 (0x0)
>      chip-id = 0 (0x0)
>      x-config-reg-migration-enabled = true
>      bypass-iommu = false
>      bus: pnv-phb3-root.0
>        type pnv-phb3-root
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>


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

Thanks,

C.


> ---
>   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..af6ec83cf6 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 "pnv-phb3-root"
>   
>   #define TYPE_PNV_PHB3_ROOT_PORT "pnv-phb3-root-port"
>   
> 

Re: [PATCH v2 00/18] user creatable pnv-phb3/pnv-phb4 devices

[Cédric Le Goater]

On 1/5/22 22:23, Daniel Henrique Barboza wrote:
> Hi,
> 
> This second version was rebased with upstream and includes fixed/amended
> versions of relevant patches that were sent to the mailing list and aren't
> upstream yet. In this process 4 patches from v1 were discarded, becoming
> either irrelevant or squashed into others.
> 
> The patches are organized as follows:
> 
> - patches 1-4: enable user creatable phb3/phb4 root ports

Looking closer at models and domain files in libvirt, aren't user
creatable phb3/phb4 root ports enough ? Do we really need the
pnv-phb3/pnv-phb4 devices to be user created also ?


That said, I am no expert in libvirt,

Thanks,

C.


> - patches 5-10: enable user creatable pnv-phb3 devices
> - patches 11-18: enable user creatable pnv-phb4 devices
> 
> Here are some examples of what we're able to do with this series:
> 
> * powernv8 machine with -nodefaults,2 pnv-phb3s with 'pcie.N' name,
> one of them with a root port and a netcard:
> 
> $ qemu-system-ppc64 -m 4G -machine powernv8,accel=tcg -smp 2,cores=2,threads=1 \
> -bios skiboot.lid  -kernel vmlinux -initrd buildroot.rootfs.cpio \
> -append 'console=hvc0 ro xmon=on' \
> -nodefaults \
> -serial mon:stdio -nographic \
> -device pnv-phb3,chip-id=0,index=0,id=pcie.0 \
> -device pnv-phb3,chip-id=0,index=2,id=pcie.2 \
> -device pnv-phb3-root-port,bus=pcie.2,id=pcie.5 \
> -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \
> -device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=pcie.5,addr=0x0
> 
> * powernv9 machine with -nodefaults, 3 of the available 12 pnv-phb4 devices
> created, 2 root ports, one of the port with a pcie-pci-bridge and
> devices connected in the bridge:
> 
> $ qemu-system-ppc64 -m 4G -machine powernv9 \
> -smp 2,sockets=2,cores=1,threads=1 \
> -accel tcg,thread=multi -bios skiboot.lid \
> -kernel vmlinux -initrd buildroot.rootfs.cpio \
> -append 'console=hvc0 ro xmon=on' \
> -nodefaults \
> -serial mon:stdio -nographic \
> -device pnv-phb4,chip-id=0,index=0,id=pcie.0 \
> -device pnv-phb4,chip-id=0,index=4,id=pcie.1 \
> -device pnv-phb4,chip-id=1,index=3,id=pcie.2 \
> -device pnv-phb4-root-port,id=root0,bus=pcie.2 \
> -device pnv-phb4-root-port,id=root1,bus=pcie.1 \
> -device pcie-pci-bridge,id=bridge1,bus=root0,addr=0x0 \
> -device nvme,bus=bridge1,addr=0x1,drive=drive0,serial=1234 \
> -drive file=./simics-disk.raw,if=none,id=drive0,format=raw,cache=none \
> -device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=bridge1,addr=0x3 \
> -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \
> -device nec-usb-xhci,bus=bridge1,addr=0x2
> 
> 
> * powernv8/9 with default settings can be used as usual. The work done
> in this series didn't change the name of the buses created by the
> default root ports (named pcie.0...N):
> 
> $ qemu-system-ppc64 -m 4G \
> -machine powernv9 -smp 2,sockets=2,cores=1,threads=1  \
> -accel tcg,thread=multi -bios skiboot.lid  \
> -kernel vmlinux -initrd buildroot.rootfs.cpio \
> -append 'console=hvc0 ro xmon=on' \
> -serial mon:stdio -nographic \
> -device pcie-pci-bridge,id=bridge1,bus=pcie.0,addr=0x0 \
> -device nvme,bus=bridge1,addr=0x1,drive=drive0,serial=1234  \
> -drive file=./simics-disk.raw,if=none,id=drive0,format=raw,cache=none \
> -device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=bridge1,addr=0x3 \
> -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \
> -device nec-usb-xhci,bus=bridge1,addr=0x2
> 
> 
> Changes from v1:
> - rebased with upstream at 7d4ae4d497807
> - added relevant patches that aren't upstream yet from "ppc/pnv:
> Preliminary cleanups before user created PHBs" [1] and "ppc/pnv: Add
> support for user created PHB3/PHB4 devices" [2] series
> - renamed phb3/phb4 default buses name to 'pnv-phb3-root' and
> 'pnv-phb4-root'
> - renamed pnv_pec_get_phb_id() to pnv_phb4_pec_get_phb_id()
> - patch 'introduce pnv_pec_init_stack_xscom()' moved to patch 16 to
> be closer with patch 17 that uses it
> - v1 link: https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg04427.html
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg03810.html
> [2] https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg01548.html
> 
> 
> Cédric Le Goater (5):
>    ppc/pnv: Attach PHB3 root port device when defaults are enabled
>    ppc/pnv: Introduce support for user created PHB3 devices
>    ppc/pnv: Reparent user created PHB3 devices to the PnvChip
>    ppc/pnv: Complete user created PHB3 devices
>    ppc/pnv: Move num_phbs under Pnv8Chip
> 
> Daniel Henrique Barboza (13):
>    pnv_phb3.c: add unique chassis and slot for pnv_phb3_root_port
>    pnv_phb4.c: add unique chassis and slot for pnv_phb4_root_port
>    pnv_phb4.c: make pnv-phb4-root-port user creatable
>    pnv_phb4.c: check if root port exists in rc_config functions
>    pnv_phb3.h: change TYPE_PNV_PHB3_ROOT_BUS name
>    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_pec: use pnv_phb4_pec_get_phb_id() in pnv_pec_dt_xscom()
>    pnv_phb4.h: turn phb into a pointer in struct PnvPhb4PecStack
>    pnv_phb4_pec.c: use 'default_enabled()' to init stack->phb
>    pnv_phb4.c: introduce pnv_pec_init_stack_xscom()
>    ppc/pnv: Introduce user creatable pnv-phb4 devices
>    pnv_phb4.c: change TYPE_PNV_PHB4_ROOT_BUS name
> 
>   hw/pci-host/pnv_phb3.c         |  57 ++++++++--
>   hw/pci-host/pnv_phb4.c         | 193 ++++++++++++++++++++++++++++++---
>   hw/pci-host/pnv_phb4_pec.c     |  86 ++++++---------
>   hw/ppc/pnv.c                   |  55 ++++++++--
>   include/hw/pci-host/pnv_phb3.h |   4 +-
>   include/hw/pci-host/pnv_phb4.h |  15 ++-
>   include/hw/ppc/pnv.h           |   8 +-
>   7 files changed, 322 insertions(+), 96 deletions(-)
> 

Re: [PATCH v2 00/18] user creatable pnv-phb3/pnv-phb4 devices

[Daniel Henrique Barboza]

On 1/6/22 05:18, Cédric Le Goater wrote:
> On 1/5/22 22:23, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> This second version was rebased with upstream and includes fixed/amended
>> versions of relevant patches that were sent to the mailing list and aren't
>> upstream yet. In this process 4 patches from v1 were discarded, becoming
>> either irrelevant or squashed into others.
>>
>> The patches are organized as follows:
>>
>> - patches 1-4: enable user creatable phb3/phb4 root ports
> 
> Looking closer at models and domain files in libvirt, aren't user
> creatable phb3/phb4 root ports enough ? Do we really need the
> pnv-phb3/pnv-phb4 devices to be user created also ?

We need user creatable phbs for a handful of reasons.

Let's suppose we go this route and all PHBs are available all the time and user just
adds root-ports. Using spapr-phb as the closest example of a default PHB that's always
available, we would need to express the PHBs in the XML. For a 4 socket powernv9 domain
we would have 24 PHBs in the XML.

No, hiding them is not an option because it would break assumptions Libvirt makes where all
PCI controllers are expressed in the XML, and the controller relationship via controller
indexes are awlays explicit. Having "ghost PHBs" that exists but aren't visible in the
XML was something that I was having to deal with with user creatable PEC controllers and
it's both complex to do and and has a good chance of getting NACKed by the community
because it's too hard to use. So having these default PHBs expressed in the XML would be a
must.

So right off the bat, for a 1 socket powernv9 machine, we'll have 6 phbs that will need
to be in the XML regardless of being used or not. And for each socket added/remove we'll
have to add/remove default PHBs available in the domain XML. So if the user starts with
1 sockets, and then adds a root port, the XML can look similar to this:

     <controller type='pci' index='0' model='pcie-root'/>
     <controller type='pci' index='1' model='pcie-root'/>
     <controller type='pci' index='2' model='pcie-root'/>
     <controller type='pci' index='3' model='pcie-root'/>
     <controller type='pci' index='4' model='pcie-root'/>
     <controller type='pci' index='5' model='pcie-root'/>
     <controller type='pci' index='6' model='pcie-root-port'/>
     (... user adds more controllers with index=7,8 ...)

Now if the user adds another socket we have a problem. The root ports and other devices will
be using controller indexes that the default PHBs would use. The user did a CPU topology
change and now will have to adjust PCI topology as a result. Similar complications will
happen if the user then removes a socket from the domain.

Now, with user creatable PHBs, the situation above will be expressed like:


     <controller type='pci' index='0' model='pcie-root'/>
       <model name='pnv-phb4'/>
       <target index='4' chip-id='0'/>
     </controller>
     <controller type='pci' index='1' model='pcie-root-port'/>
     (... user adds more controllers with index=2,3 ...)

If the user adds 4 sockets this doesn't change because Libvirt is creating a single PHB and
renaming it to 'pcie.0' to be consistent. If the user decides, in a 2 socket pnv9 domain, to use
the PHB that belongs to the second chip, the only change is the chip-id element:


     <controller type='pci' index='0' model='pcie-root'/>
       <model name='pnv-phb4'/>
       <target index='4' chip-id='1'/>
     </controller>
     <controller type='pci' index='1' model='pcie-root-port'/>
     (... user adds more controllers with index=2,3 ...)

If the user then removes the socket the domain will error out when starting because you're creating a
PHB with a wrong chip-id. A simple matter of changing the chip-id value while retaining the PCI topology
as is, without needing to reassign controller indexes all over again.

Another big deal is to able to rename buses. Libvirt uses 'pcie.N' with these controllers,
we're using 'pnv-phb3/4-root-bus'. Without user creatable PHBs we would be signing a contract
with Libvirt that we will never rename these buses in QEMU side again, once Libvirt starts
support it, because now Libvirt is counting on this info to correctly assign the root ports to
the specific PHBs. Being able to rename the buses is also crucial for the PCI topology
consistency I commented above.

There's also a good argument about long term extensibility. We're doing a lot of work in the
QEMU side but we'll be able to later on the road, for instance, support multiple root-ports in
the same PHB, or even devices other than root-ports in PHBs, device hotplug and so on with
minimal - and most important, backward friendly - Libvirt changes. Having default PHBs appearing
all the time will force us to make several assumptions that we wouldn't be able to break later on,
and probably will break older domains that were created before the changes.


I am probably forgetting more problems that this would cause in Libvirt. But at last, but definitely
not the least for the implementation of the Libvirt side, we (in this case, I) would need to make lots
and lots of code to support default PHBs that can be added/removed via SMP changes, while trying to
keep existing PCI topologies minimally consistent, with lots of documentation explaining why are we
adding/removing default PHBs due to SMP changes, and in the end the user experience would still
be awkward.


Enabling user creatable pnv-phb3/phb4 is the way for Libvirt support, and I daresay it also makes
for an improved QEMU experience as well. If I want a -nodefaults machine with 4 sockets but only 2 PHBs
I can do that after this series. Decopling SMP from PCI topology has advantages outside of Libvirt
support.


Thanks,


Daniel

> 
> 
> That said, I am no expert in libvirt,
> 
> Thanks,
> 
> C.
> 
> 
>> - patches 5-10: enable user creatable pnv-phb3 devices
>> - patches 11-18: enable user creatable pnv-phb4 devices
>>
>> Here are some examples of what we're able to do with this series:
>>
>> * powernv8 machine with -nodefaults,2 pnv-phb3s with 'pcie.N' name,
>> one of them with a root port and a netcard:
>>
>> $ qemu-system-ppc64 -m 4G -machine powernv8,accel=tcg -smp 2,cores=2,threads=1 \
>> -bios skiboot.lid  -kernel vmlinux -initrd buildroot.rootfs.cpio \
>> -append 'console=hvc0 ro xmon=on' \
>> -nodefaults \
>> -serial mon:stdio -nographic \
>> -device pnv-phb3,chip-id=0,index=0,id=pcie.0 \
>> -device pnv-phb3,chip-id=0,index=2,id=pcie.2 \
>> -device pnv-phb3-root-port,bus=pcie.2,id=pcie.5 \
>> -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \
>> -device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=pcie.5,addr=0x0
>>
>> * powernv9 machine with -nodefaults, 3 of the available 12 pnv-phb4 devices
>> created, 2 root ports, one of the port with a pcie-pci-bridge and
>> devices connected in the bridge:
>>
>> $ qemu-system-ppc64 -m 4G -machine powernv9 \
>> -smp 2,sockets=2,cores=1,threads=1 \
>> -accel tcg,thread=multi -bios skiboot.lid \
>> -kernel vmlinux -initrd buildroot.rootfs.cpio \
>> -append 'console=hvc0 ro xmon=on' \
>> -nodefaults \
>> -serial mon:stdio -nographic \
>> -device pnv-phb4,chip-id=0,index=0,id=pcie.0 \
>> -device pnv-phb4,chip-id=0,index=4,id=pcie.1 \
>> -device pnv-phb4,chip-id=1,index=3,id=pcie.2 \
>> -device pnv-phb4-root-port,id=root0,bus=pcie.2 \
>> -device pnv-phb4-root-port,id=root1,bus=pcie.1 \
>> -device pcie-pci-bridge,id=bridge1,bus=root0,addr=0x0 \
>> -device nvme,bus=bridge1,addr=0x1,drive=drive0,serial=1234 \
>> -drive file=./simics-disk.raw,if=none,id=drive0,format=raw,cache=none \
>> -device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=bridge1,addr=0x3 \
>> -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \
>> -device nec-usb-xhci,bus=bridge1,addr=0x2
>>
>>
>> * powernv8/9 with default settings can be used as usual. The work done
>> in this series didn't change the name of the buses created by the
>> default root ports (named pcie.0...N):
>>
>> $ qemu-system-ppc64 -m 4G \
>> -machine powernv9 -smp 2,sockets=2,cores=1,threads=1  \
>> -accel tcg,thread=multi -bios skiboot.lid  \
>> -kernel vmlinux -initrd buildroot.rootfs.cpio \
>> -append 'console=hvc0 ro xmon=on' \
>> -serial mon:stdio -nographic \
>> -device pcie-pci-bridge,id=bridge1,bus=pcie.0,addr=0x0 \
>> -device nvme,bus=bridge1,addr=0x1,drive=drive0,serial=1234  \
>> -drive file=./simics-disk.raw,if=none,id=drive0,format=raw,cache=none \
>> -device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=bridge1,addr=0x3 \
>> -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \
>> -device nec-usb-xhci,bus=bridge1,addr=0x2
>>
>>
>> Changes from v1:
>> - rebased with upstream at 7d4ae4d497807
>> - added relevant patches that aren't upstream yet from "ppc/pnv:
>> Preliminary cleanups before user created PHBs" [1] and "ppc/pnv: Add
>> support for user created PHB3/PHB4 devices" [2] series
>> - renamed phb3/phb4 default buses name to 'pnv-phb3-root' and
>> 'pnv-phb4-root'
>> - renamed pnv_pec_get_phb_id() to pnv_phb4_pec_get_phb_id()
>> - patch 'introduce pnv_pec_init_stack_xscom()' moved to patch 16 to
>> be closer with patch 17 that uses it
>> - v1 link: https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg04427.html
>>
>> [1] https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg03810.html
>> [2] https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg01548.html
>>
>>
>> Cédric Le Goater (5):
>>    ppc/pnv: Attach PHB3 root port device when defaults are enabled
>>    ppc/pnv: Introduce support for user created PHB3 devices
>>    ppc/pnv: Reparent user created PHB3 devices to the PnvChip
>>    ppc/pnv: Complete user created PHB3 devices
>>    ppc/pnv: Move num_phbs under Pnv8Chip
>>
>> Daniel Henrique Barboza (13):
>>    pnv_phb3.c: add unique chassis and slot for pnv_phb3_root_port
>>    pnv_phb4.c: add unique chassis and slot for pnv_phb4_root_port
>>    pnv_phb4.c: make pnv-phb4-root-port user creatable
>>    pnv_phb4.c: check if root port exists in rc_config functions
>>    pnv_phb3.h: change TYPE_PNV_PHB3_ROOT_BUS name
>>    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_pec: use pnv_phb4_pec_get_phb_id() in pnv_pec_dt_xscom()
>>    pnv_phb4.h: turn phb into a pointer in struct PnvPhb4PecStack
>>    pnv_phb4_pec.c: use 'default_enabled()' to init stack->phb
>>    pnv_phb4.c: introduce pnv_pec_init_stack_xscom()
>>    ppc/pnv: Introduce user creatable pnv-phb4 devices
>>    pnv_phb4.c: change TYPE_PNV_PHB4_ROOT_BUS name
>>
>>   hw/pci-host/pnv_phb3.c         |  57 ++++++++--
>>   hw/pci-host/pnv_phb4.c         | 193 ++++++++++++++++++++++++++++++---
>>   hw/pci-host/pnv_phb4_pec.c     |  86 ++++++---------
>>   hw/ppc/pnv.c                   |  55 ++++++++--
>>   include/hw/pci-host/pnv_phb3.h |   4 +-
>>   include/hw/pci-host/pnv_phb4.h |  15 ++-
>>   include/hw/ppc/pnv.h           |   8 +-
>>   7 files changed, 322 insertions(+), 96 deletions(-)
>>
> 

Re: [PATCH v2 11/18] pnv_phb4.c: introduce pnv_phb4_set_stack_phb_props()

[Cédric Le Goater]

On 1/5/22 22:23, Daniel Henrique Barboza wrote:
> 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>

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

Thanks,

C.

> ---
>   hw/pci-host/pnv_phb4.c         | 25 +++++++++++++++++++++++++
>   hw/pci-host/pnv_phb4_pec.c     | 12 +-----------
>   include/hw/pci-host/pnv_phb4.h |  1 +
>   3 files changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 83dedc878a..6c1a33bc66 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1158,6 +1158,31 @@ 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_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 f3e4fa0c82..057d4b07fb 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -577,17 +577,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_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 ea63df9676..7f5b9cc0ac 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -131,6 +131,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;
>   
>   /*
> 

Re: [PATCH v2 12/18] pnv_phb4_pec.c: move pnv_pec_phb_offset() to pnv_phb4.c

[Cédric Le Goater]

On 1/5/22 22:23, Daniel Henrique Barboza wrote:
> The logic inside pnv_pec_phb_offset() will 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_phb4_pec_get_phb_id() to be even clearer about the function
> intent.
> 
> 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         | 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 6c1a33bc66..4c785bbe4c 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1158,6 +1158,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_phb4_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 057d4b07fb..e47d19dfff 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);
> @@ -405,7 +392,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_phb4_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 7f5b9cc0ac..b2c4a6b263 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;
> @@ -132,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);
> +int pnv_phb4_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index);
>   extern const MemoryRegionOps pnv_phb4_xscom_ops;
>   
>   /*
> 

Re: [PATCH v2 13/18] pnv_phb4_pec: use pnv_phb4_pec_get_phb_id() in pnv_pec_dt_xscom()

[Cédric Le Goater]

On 1/5/22 22:23, 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_phb4_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.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 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_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 e47d19dfff..0675fc55bc 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -449,8 +449,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_phb4_pec_get_phb_id(pec, i);
>           int stk_offset;
>   
>           name = g_strdup_printf("stack@%x", i);
> @@ -460,7 +459,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 v2 18/18] pnv_phb4.c: change TYPE_PNV_PHB4_ROOT_BUS name

[Cédric Le Goater]

On 1/5/22 22:23, Daniel Henrique Barboza wrote:
> 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 'pnv-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: pnv-phb4-root.11
>        type pnv-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: pnv-phb4-root.6
>        type pnv-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: pnv-phb4-root.5
>        type pnv-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: pnv-phb4-root.0
>        type pnv-phb4-root
>        dev: pnv-phb4-root-port, id ""
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>


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

Thanks,

C.



> ---
>   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 610580a88f..0aec495cbf 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 "pnv-phb4-root"
>   #define TYPE_PNV_PHB4_ROOT_PORT "pnv-phb4-root-port"
>   
>   typedef struct PnvPHB4RootPort {
> 

Re: [PATCH v2 14/18] pnv_phb4.h: turn phb into a pointer in struct PnvPhb4PecStack

[Cédric Le Goater]

On 1/5/22 22:23, Daniel Henrique Barboza wrote:
> 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 4c785bbe4c..9e670e41d2 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1449,7 +1449,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 0675fc55bc..be6eefdbb1 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -563,7 +563,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)) {

Shouldn't that be stack->phb ?

What about the object_initialize_child() ?


C.

>           return;
>       }
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index b2c4a6b263..2fb5e119c4 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -178,8 +178,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 {
> 

Re: [PATCH v2 15/18] pnv_phb4_pec.c: use 'default_enabled()' to init stack->phb

[Cédric Le Goater]

On 1/5/22 22:23, Daniel Henrique Barboza wrote:
> 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_phb4_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_phb4_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 be6eefdbb1..638691783b 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>
>   
> @@ -392,11 +393,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_phb4_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);

This setting of "phb-id" is indeed a problem if the phb object becomes
dynamic. This is because the property is an alias.

>           object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort);
> +
> +        /* Create and realize the default stack->phb */
> +        if (defaults_enabled()) {

This should be under pnv_pec_stk_realize() and not pnv_pec_realize()
  
> +            PnvPHB4 *phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
> +            int phb_id = pnv_phb4_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)) {

AFAICT, pnv_pec_stk_realize() still realizes the PHB object but when
defaults_enabled(), pnv_pec_realize() also realizes the PHB object.

How does that work ?


>               return;
>           }
> @@ -531,10 +550,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");

I think this belongs to the previous patch.

>   }
>   
>   static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
> @@ -588,6 +603,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 *),

That's weird. I think I understand why. See next email.

Thanks,

C.


>           DEFINE_PROP_END_OF_LIST(),
>   };
>   
> 

Re: [PATCH v2 14/18] pnv_phb4.h: turn phb into a pointer in struct PnvPhb4PecStack

[Cédric Le Goater]

On 1/5/22 22:23, Daniel Henrique Barboza wrote:
> 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 4c785bbe4c..9e670e41d2 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1449,7 +1449,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 0675fc55bc..be6eefdbb1 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -563,7 +563,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 b2c4a6b263..2fb5e119c4 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -178,8 +178,11 @@ struct PnvPhb4PecStack {
>       /* The owner PEC */
>       PnvPhb4PecState *pec;
>   
> -    /* The actual PHB */
> -    PnvPHB4 phb;

I would keep this object for default_enabled() and rename it to
avoid conflicts with the next one :

> +    /*
> +     * PHB4 pointer. pnv_phb4_update_regions() needs to access
> +     * the PHB4 via a PnvPhb4PecStack pointer.
> +     */
> +    PnvPHB4 *phb;

and introduce this pointer for dynamic phbs. if default_enabled(),
then just make it point to the above instance.

Thanks,

C.


>   };
>   
>   struct PnvPhb4PecState {
> 

Re: [PATCH v2 16/18] pnv_phb4.c: introduce pnv_pec_init_stack_xscom()

[Cédric Le Goater]

On 1/5/22 22:23, Daniel Henrique Barboza wrote:
> 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.

I think this patch prepares ground for user-created devices and should come
earlier in the series.

> 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         | 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 9e670e41d2..430a5c10f4 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1158,6 +1158,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 638691783b..41c79d24c4 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -556,10 +556,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);
> @@ -583,20 +579,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 2fb5e119c4..610580a88f 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -132,6 +132,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_phb4_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index);
>   extern const MemoryRegionOps pnv_phb4_xscom_ops;
> 

Re: [PATCH v2 17/18] ppc/pnv: Introduce user creatable pnv-phb4 devices

[Cédric Le Goater]

On 1/5/22 22:23, Daniel Henrique Barboza wrote:
> 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 430a5c10f4..1c2334d491 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1236,6 +1236,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) {

assert() maybe ?


> +        /* 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_phb4_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);
> @@ -1243,8 +1278,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;

The stack object should check the validity of the stack->phb pointer always.

> +
> +        /*
> +         * 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;

This looks wrong.

> +    }
>   
>       /* Set the "big_phb" flag */
>       phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3;
> @@ -1298,6 +1374,10 @@ 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);
> +
> +    if (stack_init_xscom) {
> +        pnv_pec_init_stack_xscom(stack);
> +    }

Isn't it done under the pnv_pec_stk_realize() routine already ?

>   }
>   
>   static const char *pnv_phb4_root_bus_path(PCIHostState *host_bridge,
> @@ -1347,7 +1427,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 41c79d24c4..4417beb97d 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -573,13 +573,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);


This looks wrong also. I don't understand why.

Thanks,

C.


>   }
>   
>   static Property pnv_pec_stk_properties[] = {
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index fe7e67e73a..837146a2fb 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1960,6 +1960,8 @@ static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
>       pmc->compat = compat;
>       pmc->compat_size = sizeof(compat);
>       pmc->dt_power_mgt = pnv_dt_power_mgt;
> +
> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB4);
>   }
>   
>   static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
> 

Re: [PATCH v2 00/18] user creatable pnv-phb3/pnv-phb4 devices

[Cédric Le Goater]

On 1/6/22 13:36, Daniel Henrique Barboza wrote:
> 
> 
> On 1/6/22 05:18, Cédric Le Goater wrote:
>> On 1/5/22 22:23, Daniel Henrique Barboza wrote:
>>> Hi,
>>>
>>> This second version was rebased with upstream and includes fixed/amended
>>> versions of relevant patches that were sent to the mailing list and aren't
>>> upstream yet. In this process 4 patches from v1 were discarded, becoming
>>> either irrelevant or squashed into others.
>>>
>>> The patches are organized as follows:
>>>
>>> - patches 1-4: enable user creatable phb3/phb4 root ports
>>
>> Looking closer at models and domain files in libvirt, aren't user
>> creatable phb3/phb4 root ports enough ? Do we really need the
>> pnv-phb3/pnv-phb4 devices to be user created also ?
> 
> We need user creatable phbs for a handful of reasons.
> 
> Let's suppose we go this route and all PHBs are available all the time and user just
> adds root-ports. Using spapr-phb as the closest example of a default PHB that's always
> available, we would need to express the PHBs in the XML. For a 4 socket powernv9 domain
> we would have 24 PHBs in the XML.
> 
> No, hiding them is not an option because it would break assumptions Libvirt makes where all
> PCI controllers are expressed in the XML, and the controller relationship via controller
> indexes are awlays explicit. Having "ghost PHBs" that exists but aren't visible in the
> XML was something that I was having to deal with with user creatable PEC controllers and
> it's both complex to do and and has a good chance of getting NACKed by the community
> because it's too hard to use. So having these default PHBs expressed in the XML would be a
> must.
> 
> So right off the bat, for a 1 socket powernv9 machine, we'll have 6 phbs that will need
> to be in the XML regardless of being used or not. And for each socket added/remove we'll
> have to add/remove default PHBs available in the domain XML. So if the user starts with
> 1 sockets, and then adds a root port, the XML can look similar to this:
> 
>      <controller type='pci' index='0' model='pcie-root'/>
>      <controller type='pci' index='1' model='pcie-root'/>
>      <controller type='pci' index='2' model='pcie-root'/>
>      <controller type='pci' index='3' model='pcie-root'/>
>      <controller type='pci' index='4' model='pcie-root'/>
>      <controller type='pci' index='5' model='pcie-root'/>
>      <controller type='pci' index='6' model='pcie-root-port'/>
>      (... user adds more controllers with index=7,8 ...)
> 
> Now if the user adds another socket we have a problem. The root ports and other devices will
> be using controller indexes that the default PHBs would use. The user did a CPU topology
> change and now will have to adjust PCI topology as a result. Similar complications will
> happen if the user then removes a socket from the domain.
> 
> Now, with user creatable PHBs, the situation above will be expressed like:
> 
> 
>      <controller type='pci' index='0' model='pcie-root'/>
>        <model name='pnv-phb4'/>
>        <target index='4' chip-id='0'/>
>      </controller>
>      <controller type='pci' index='1' model='pcie-root-port'/>
>      (... user adds more controllers with index=2,3 ...)
> 
> If the user adds 4 sockets this doesn't change because Libvirt is creating a single PHB and
> renaming it to 'pcie.0' to be consistent. If the user decides, in a 2 socket pnv9 domain, to use
> the PHB that belongs to the second chip, the only change is the chip-id element:
> 
> 
>      <controller type='pci' index='0' model='pcie-root'/>
>        <model name='pnv-phb4'/>
>        <target index='4' chip-id='1'/>
>      </controller>
>      <controller type='pci' index='1' model='pcie-root-port'/>
>      (... user adds more controllers with index=2,3 ...)
> 
> If the user then removes the socket the domain will error out when starting because you're creating a
> PHB with a wrong chip-id. A simple matter of changing the chip-id value while retaining the PCI topology
> as is, without needing to reassign controller indexes all over again.
> 
> Another big deal is to able to rename buses. Libvirt uses 'pcie.N' with these controllers,
> we're using 'pnv-phb3/4-root-bus'. Without user creatable PHBs we would be signing a contract
> with Libvirt that we will never rename these buses in QEMU side again, once Libvirt starts
> support it, because now Libvirt is counting on this info to correctly assign the root ports to
> the specific PHBs. Being able to rename the buses is also crucial for the PCI topology
> consistency I commented above.
> 
> There's also a good argument about long term extensibility. We're doing a lot of work in the
> QEMU side but we'll be able to later on the road, for instance, support multiple root-ports in
> the same PHB, or even devices other than root-ports in PHBs, device hotplug and so on with
> minimal - and most important, backward friendly - Libvirt changes. Having default PHBs appearing
> all the time will force us to make several assumptions that we wouldn't be able to break later on,
> and probably will break older domains that were created before the changes.
> 
> 
> I am probably forgetting more problems that this would cause in Libvirt. But at last, but definitely
> not the least for the implementation of the Libvirt side, we (in this case, I) would need to make lots
> and lots of code to support default PHBs that can be added/removed via SMP changes, while trying to
> keep existing PCI topologies minimally consistent, with lots of documentation explaining why are we
> adding/removing default PHBs due to SMP changes, and in the end the user experience would still
> be awkward.
> 
> 
> Enabling user creatable pnv-phb3/phb4 is the way for Libvirt support, and I daresay it also makes
> for an improved QEMU experience as well. If I want a -nodefaults machine with 4 sockets but only 2 PHBs
> I can do that after this series. Decopling SMP from PCI topology has advantages outside of Libvirt
> support.
Thanks for this detailed explanation. I am convinced !
> - patches 5-10: enable user creatable pnv-phb3 devices

These are fine. No need to resend.

> - patches 11-18: enable user creatable pnv-phb4 devices

patches 11-17 need some care. 18 is fine.

>>> Here are some examples of what we're able to do with this series:
>>>
>>> * powernv8 machine with -nodefaults,2 pnv-phb3s with 'pcie.N' name,
>>> one of them with a root port and a netcard:
>>>
>>> $ qemu-system-ppc64 -m 4G -machine powernv8,accel=tcg -smp 2,cores=2,threads=1 \
>>> -bios skiboot.lid  -kernel vmlinux -initrd buildroot.rootfs.cpio \
>>> -append 'console=hvc0 ro xmon=on' \
>>> -nodefaults \
>>> -serial mon:stdio -nographic \
>>> -device pnv-phb3,chip-id=0,index=0,id=pcie.0 \
>>> -device pnv-phb3,chip-id=0,index=2,id=pcie.2 \
>>> -device pnv-phb3-root-port,bus=pcie.2,id=pcie.5 \
>>> -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \
>>> -device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=pcie.5,addr=0x0
>>>
>>> * powernv9 machine with -nodefaults, 3 of the available 12 pnv-phb4 devices
>>> created, 2 root ports, one of the port with a pcie-pci-bridge and
>>> devices connected in the bridge:
>>>
>>> $ qemu-system-ppc64 -m 4G -machine powernv9 \
>>> -smp 2,sockets=2,cores=1,threads=1 \
>>> -accel tcg,thread=multi -bios skiboot.lid \
>>> -kernel vmlinux -initrd buildroot.rootfs.cpio \
>>> -append 'console=hvc0 ro xmon=on' \
>>> -nodefaults \
>>> -serial mon:stdio -nographic \
>>> -device pnv-phb4,chip-id=0,index=0,id=pcie.0 \
>>> -device pnv-phb4,chip-id=0,index=4,id=pcie.1 \
>>> -device pnv-phb4,chip-id=1,index=3,id=pcie.2 \
>>> -device pnv-phb4-root-port,id=root0,bus=pcie.2 \
>>> -device pnv-phb4-root-port,id=root1,bus=pcie.1 \
>>> -device pcie-pci-bridge,id=bridge1,bus=root0,addr=0x0 \
>>> -device nvme,bus=bridge1,addr=0x1,drive=drive0,serial=1234 \
>>> -drive file=./simics-disk.raw,if=none,id=drive0,format=raw,cache=none \
>>> -device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=bridge1,addr=0x3 \
>>> -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \
>>> -device nec-usb-xhci,bus=bridge1,addr=0x2
>>>
>>>
>>> * powernv8/9 with default settings can be used as usual. The work done
>>> in this series didn't change the name of the buses created by the
>>> default root ports (named pcie.0...N):
>>>
>>> $ qemu-system-ppc64 -m 4G \
>>> -machine powernv9 -smp 2,sockets=2,cores=1,threads=1  \
>>> -accel tcg,thread=multi -bios skiboot.lid  \
>>> -kernel vmlinux -initrd buildroot.rootfs.cpio \
>>> -append 'console=hvc0 ro xmon=on' \
>>> -serial mon:stdio -nographic \
>>> -device pcie-pci-bridge,id=bridge1,bus=pcie.0,addr=0x0 \
>>> -device nvme,bus=bridge1,addr=0x1,drive=drive0,serial=1234  \
>>> -drive file=./simics-disk.raw,if=none,id=drive0,format=raw,cache=none \
>>> -device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=bridge1,addr=0x3 \
>>> -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \
>>> -device nec-usb-xhci,bus=bridge1,addr=0x2

Could we capture some of these command lines in the documentation ? with some
of the details above ?

Thanks,

C.

Re: [PATCH v2 17/18] ppc/pnv: Introduce user creatable pnv-phb4 devices

[Daniel Henrique Barboza]

On 1/6/22 11:49, Cédric Le Goater wrote:
> On 1/5/22 22:23, Daniel Henrique Barboza wrote:
>> 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 430a5c10f4..1c2334d491 100644
>> --- a/hw/pci-host/pnv_phb4.c
>> +++ b/hw/pci-host/pnv_phb4.c
>> @@ -1236,6 +1236,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) {
> 
> assert() maybe ?
> 
> 
>> +        /* 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_phb4_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);
>> @@ -1243,8 +1278,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;
> 
> The stack object should check the validity of the stack->phb pointer always.


What do you mean by "check the validity"?



Thanks,


Daniel

> 
>> +
>> +        /*
>> +         * 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;
> 
> This looks wrong.
> 
>> +    }
>>       /* Set the "big_phb" flag */
>>       phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3;
>> @@ -1298,6 +1374,10 @@ 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);
>> +
>> +    if (stack_init_xscom) {
>> +        pnv_pec_init_stack_xscom(stack);
>> +    }
> 
> Isn't it done under the pnv_pec_stk_realize() routine already ?
> 
>>   }
>>   static const char *pnv_phb4_root_bus_path(PCIHostState *host_bridge,
>> @@ -1347,7 +1427,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 41c79d24c4..4417beb97d 100644
>> --- a/hw/pci-host/pnv_phb4_pec.c
>> +++ b/hw/pci-host/pnv_phb4_pec.c
>> @@ -573,13 +573,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);
> 
> 
> This looks wrong also. I don't understand why.
> 
> Thanks,
> 
> C.
> 
> 
>>   }
>>   static Property pnv_pec_stk_properties[] = {
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index fe7e67e73a..837146a2fb 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1960,6 +1960,8 @@ static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
>>       pmc->compat = compat;
>>       pmc->compat_size = sizeof(compat);
>>       pmc->dt_power_mgt = pnv_dt_power_mgt;
>> +
>> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PNV_PHB4);
>>   }
>>   static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
>>
> 

Re: [PATCH v2 17/18] ppc/pnv: Introduce user creatable pnv-phb4 devices

[Cédric Le Goater]

>>> +        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;
>>
>> The stack object should check the validity of the stack->phb pointer always.
> 
> 
> What do you mean by "check the validity"?
> 

I am thinking of the usage of 'stack->phb', for instance in the routine
pnv_phb4_update_regions(). We should add an assert there.

Your changes seem to cleanup the stack <-> phb relation quite a lot. Which
is good.

Thanks,

C.

Re: [PATCH v2 17/18] ppc/pnv: Introduce user creatable pnv-phb4 devices

[Daniel Henrique Barboza]

On 1/8/22 08:11, Cédric Le Goater wrote:
>>>> +        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;
>>>
>>> The stack object should check the validity of the stack->phb pointer always.
>>
>>
>> What do you mean by "check the validity"?
>>
> 
> I am thinking of the usage of 'stack->phb', for instance in the routine
> pnv_phb4_update_regions(). We should add an assert there.

Oh ok. I'll add an assert in this point (and in the code above that where you
also pointed it out.

And about pnv_phb4_update_regions(), v3 will need to handle it but not with
an assert but with a phb != NULL verification to become a NO-OP if phb is NULL.
It's an easier and less intrusive fix than trying to change the order of the
XSCOM initialization in stk_realize().

I will (hopefully) explain it better in the v3.


Thanks,


Daniel

> 
> Your changes seem to cleanup the stack <-> phb relation quite a lot. Which
> is good.
> 
> Thanks,
> 
> C.