[PATCH v3 00/10] user creatable pnv-phb4 devices

[PATCH v3 00/10] user creatable pnv-phb4 devices

[Daniel Henrique Barboza]

Hi,

This v3 contains new versions of pnv-phb4 exclusive patches from
version 2. Patches 1-10 are already accepted.

I changed how patch 9 (v2 patch 17) works by doing everything possible
in extra patches/cleanups beforehand, and then using patch 9 to flip the
switch in a single step. This means that handling the default initialization
of pnv-phb4s is done at the same time we enable user creatable pnv-phb4s.

There's also a change in how XSCOM initializion is being handled. We're not
using a flag to do a partial XSCOM initialization during phb4_realize() anymore.
Intead, we moved XSCOM initialization code, as less intrusive as we could, to
phb4_realize(). 

This time I also took the precaution of testing the default case
(i.e. running without -nodefaults) in every patch. v2 was breaking
this default run between some patches.

changes from v2:
- former patch 16: removed
- patch 10 (v2 18): unchanged
- patches 4,5,7,8: new
- patch 9 (former 17):
  * added g_assert() if stack == NULL
  * added a comment explaining why we shouldn't assert on user error
with wrong chip-id/index values
- minor changes across the patches due to the design changes 
- v2 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg00671.html 

Daniel Henrique Barboza (10):
  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_pec.c: remove stack 'phb-id' alias
  pnv_phb4_pec.c: move phb4 properties setup to pec_realize()
  ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack
  ppc/pnv: move PHB4 related XSCOM init to phb4_realize()
  pnv_phb4.c: check stack->phb not NULL in phb4_update_regions()
  ppc/pnv: Introduce user creatable pnv-phb4 devices
  pnv_phb4.c: change TYPE_PNV_PHB4_ROOT_BUS name

 hw/pci-host/pnv_phb4.c         | 162 ++++++++++++++++++++++++++++++++-
 hw/pci-host/pnv_phb4_pec.c     |  58 +++++-------
 hw/ppc/pnv.c                   |   2 +
 include/hw/pci-host/pnv_phb4.h |  12 ++-
 4 files changed, 191 insertions(+), 43 deletions(-)

-- 
2.33.1

[PATCH v3 01/10] 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.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/pnv_phb4.c         | 19 +++++++++++++++++++
 hw/pci-host/pnv_phb4_pec.c     |  7 +------
 include/hw/pci-host/pnv_phb4.h |  1 +
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 83dedc878a..4c045fd8cd 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1158,6 +1158,25 @@ 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);
+
+    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..97b9d4cb0e 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -582,12 +582,7 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
     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 v3 02/10] 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.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
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 4c045fd8cd..fb6c4f993a 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 97b9d4cb0e..513a698e17 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 v3 03/10] 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 513a698e17..1f264d0a9c 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 v3 04/10] pnv_phb4_pec.c: remove stack 'phb-id' alias

[Daniel Henrique Barboza]

This alias is a indirect way of setting stack->phb->index. Since we have
access to a valid stack->phb (for default_enabled() at least - next
patch will deal with it accordingly) we can directly set the phb 'index'
attribute.

Let's also take the opportunity to explain why we're having to deal with
stack->phb attributes during pec_realize().

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

diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 1f264d0a9c..417fac4cef 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -395,8 +395,17 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
         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);
+
+        /*
+         * stack->phb->index is dependent on the position the
+         * stack occupies in pec->stacks[]. We have this information
+         * available here via the 'i' iterator so it's convenient to
+         * do it now.
+         */
+        object_property_set_int(OBJECT(&stack->phb), "index", phb_id,
+                                &error_abort);
+
         if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
             return;
         }
@@ -534,7 +543,6 @@ 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)
-- 
2.33.1

Re: [PATCH v3 04/10] pnv_phb4_pec.c: remove stack 'phb-id' alias

[Cédric Le Goater]

On 1/10/22 15:33, Daniel Henrique Barboza wrote:
> This alias is a indirect way of setting stack->phb->index. Since we have
> access to a valid stack->phb (for default_enabled() at least - next
> patch will deal with it accordingly) we can directly set the phb 'index'
> attribute.
> 
> Let's also take the opportunity to explain why we're having to deal with
> stack->phb attributes during pec_realize().
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb4_pec.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 1f264d0a9c..417fac4cef 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -395,8 +395,17 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>           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);
> +
> +        /*
> +         * stack->phb->index is dependent on the position the
> +         * stack occupies in pec->stacks[]. We have this information
> +         * available here via the 'i' iterator so it's convenient to
> +         * do it now.
> +         */
> +        object_property_set_int(OBJECT(&stack->phb), "index", phb_id,
> +                                &error_abort);

I don't like the fact that we are exposing ->phb under the PEC model.
It looks like this is going to be a problem afterwards when defaults
are disabled.

We should move the setting of the PHB ID under pnv_pec_stk_realize()
before the PHB is realized and compute the id with :

        int phb_id = pnv_phb4_pec_get_phb_id(pec, stack->stack_no);

Thanks,

C.

> +
>           if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
>               return;
>           }
> @@ -534,7 +543,6 @@ 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)
> 

Re: [PATCH v3 04/10] pnv_phb4_pec.c: remove stack 'phb-id' alias

[Daniel Henrique Barboza]

On 1/10/22 12:49, Cédric Le Goater wrote:
> On 1/10/22 15:33, Daniel Henrique Barboza wrote:
>> This alias is a indirect way of setting stack->phb->index. Since we have
>> access to a valid stack->phb (for default_enabled() at least - next
>> patch will deal with it accordingly) we can directly set the phb 'index'
>> attribute.
>>
>> Let's also take the opportunity to explain why we're having to deal with
>> stack->phb attributes during pec_realize().
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/pci-host/pnv_phb4_pec.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>> index 1f264d0a9c..417fac4cef 100644
>> --- a/hw/pci-host/pnv_phb4_pec.c
>> +++ b/hw/pci-host/pnv_phb4_pec.c
>> @@ -395,8 +395,17 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>>           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);
>> +
>> +        /*
>> +         * stack->phb->index is dependent on the position the
>> +         * stack occupies in pec->stacks[]. We have this information
>> +         * available here via the 'i' iterator so it's convenient to
>> +         * do it now.
>> +         */
>> +        object_property_set_int(OBJECT(&stack->phb), "index", phb_id,
>> +                                &error_abort);
> 
> I don't like the fact that we are exposing ->phb under the PEC model.
> It looks like this is going to be a problem afterwards when defaults
> are disabled.
> 
> We should move the setting of the PHB ID under pnv_pec_stk_realize()
> before the PHB is realized and compute the id with :
> 
>         int phb_id = pnv_phb4_pec_get_phb_id(pec, stack->stack_no);


Oh, if stack->stack_no is the stack index of the pec->stacks[] array then we should
instead move all this stuff into phb4_realize().


Daniel


> 
> Thanks,
> 
> C.
> 
>> +
>>           if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
>>               return;
>>           }
>> @@ -534,7 +543,6 @@ 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)
>>
> 

Re: [PATCH v3 04/10] pnv_phb4_pec.c: remove stack 'phb-id' alias

[Cédric Le Goater]

On 1/10/22 17:27, Daniel Henrique Barboza wrote:
> 
> 
> On 1/10/22 12:49, Cédric Le Goater wrote:
>> On 1/10/22 15:33, Daniel Henrique Barboza wrote:
>>> This alias is a indirect way of setting stack->phb->index. Since we have
>>> access to a valid stack->phb (for default_enabled() at least - next
>>> patch will deal with it accordingly) we can directly set the phb 'index'
>>> attribute.
>>>
>>> Let's also take the opportunity to explain why we're having to deal with
>>> stack->phb attributes during pec_realize().
>>>
>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> ---
>>>   hw/pci-host/pnv_phb4_pec.c | 12 ++++++++++--
>>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>>> index 1f264d0a9c..417fac4cef 100644
>>> --- a/hw/pci-host/pnv_phb4_pec.c
>>> +++ b/hw/pci-host/pnv_phb4_pec.c
>>> @@ -395,8 +395,17 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>>>           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);
>>> +
>>> +        /*
>>> +         * stack->phb->index is dependent on the position the
>>> +         * stack occupies in pec->stacks[]. We have this information
>>> +         * available here via the 'i' iterator so it's convenient to
>>> +         * do it now.
>>> +         */
>>> +        object_property_set_int(OBJECT(&stack->phb), "index", phb_id,
>>> +                                &error_abort);
>>
>> I don't like the fact that we are exposing ->phb under the PEC model.
>> It looks like this is going to be a problem afterwards when defaults
>> are disabled.
>>
>> We should move the setting of the PHB ID under pnv_pec_stk_realize()
>> before the PHB is realized and compute the id with :
>>
>>         int phb_id = pnv_phb4_pec_get_phb_id(pec, stack->stack_no);
> 
> 
> Oh, if stack->stack_no is the stack index of the pec->stacks[] array then we should
> instead move all this stuff into phb4_realize().

The properties are generally set by the parent object and for
user created PHB4 devices, the user will set the PHB4 "index"
property which we will use to do a lookup of the owning stack.
It's similar to chip-id.

Thanks,

C.

[PATCH v3 05/10] pnv_phb4_pec.c: move phb4 properties setup to pec_realize()

[Daniel Henrique Barboza]

Previous patch added stack->phb->index handling in pec_realize() for
specific reasons (phb->index is reliant on the stack index in
pec->stacks[]).

Move pnv_phb4_set_stack_phb_props() from stk_realize() to pec_realize()
to have a single spot in which we set PHB4 properties for the default
created stack->phb. This will give us one less spot to worry about when
introducing user creatable pnv-phb4s and having to deal with stack->phb
being NULL depending on -nodefaults being set.

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

diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 417fac4cef..d2851e8040 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -405,6 +405,7 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
          */
         object_property_set_int(OBJECT(&stack->phb), "index", phb_id,
                                 &error_abort);
+        pnv_phb4_set_stack_phb_props(stack, &stack->phb);
 
         if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
             return;
@@ -576,7 +577,6 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
     pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(&stack->phb),
                           &pnv_phb4_xscom_ops, &stack->phb, name, 0x40);
 
-    pnv_phb4_set_stack_phb_props(stack, &stack->phb);
     if (!sysbus_realize(SYS_BUS_DEVICE(&stack->phb), errp)) {
         return;
     }
-- 
2.33.1

Re: [PATCH v3 05/10] pnv_phb4_pec.c: move phb4 properties setup to pec_realize()

[Cédric Le Goater]

On 1/10/22 15:33, Daniel Henrique Barboza wrote:
> Previous patch added stack->phb->index handling in pec_realize() for
> specific reasons (phb->index is reliant on the stack index in
> pec->stacks[]).
> 
> Move pnv_phb4_set_stack_phb_props() from stk_realize() to pec_realize()
> to have a single spot in which we set PHB4 properties for the default
> created stack->phb. This will give us one less spot to worry about when
> introducing user creatable pnv-phb4s and having to deal with stack->phb
> being NULL depending on -nodefaults being set.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb4_pec.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 417fac4cef..d2851e8040 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -405,6 +405,7 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>            */
>           object_property_set_int(OBJECT(&stack->phb), "index", phb_id,
>                                   &error_abort);
> +        pnv_phb4_set_stack_phb_props(stack, &stack->phb);

hmm, this is exposing the PHB under the PEC. I am not sure this is useful
anymore. I will think about it.

C.


>   
>           if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
>               return;
> @@ -576,7 +577,6 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>       pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(&stack->phb),
>                             &pnv_phb4_xscom_ops, &stack->phb, name, 0x40);
>   
> -    pnv_phb4_set_stack_phb_props(stack, &stack->phb);
>       if (!sysbus_realize(SYS_BUS_DEVICE(&stack->phb), errp)) {
>           return;
>       }
> 

[PATCH v3 06/10] ppc/pnv: 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     | 12 ++++++------
 include/hw/pci-host/pnv_phb4.h |  7 +++++--
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index fb6c4f993a..1a7395772f 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1443,7 +1443,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 d2851e8040..042dc0b775 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -403,9 +403,9 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
          * available here via the 'i' iterator so it's convenient to
          * do it now.
          */
-        object_property_set_int(OBJECT(&stack->phb), "index", phb_id,
+        object_property_set_int(OBJECT(stack->phb), "index", phb_id,
                                 &error_abort);
-        pnv_phb4_set_stack_phb_props(stack, &stack->phb);
+        pnv_phb4_set_stack_phb_props(stack, stack->phb);
 
         if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
             return;
@@ -543,7 +543,7 @@ 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);
+    stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
 }
 
 static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
@@ -574,10 +574,10 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
     /* 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);
+    pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(stack->phb),
+                          &pnv_phb4_xscom_ops, stack->phb, name, 0x40);
 
-    if (!sysbus_realize(SYS_BUS_DEVICE(&stack->phb), errp)) {
+    if (stack->phb && !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

Re: [PATCH v3 06/10] ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack

[Cédric Le Goater]

On 1/10/22 15:33, 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     | 12 ++++++------
>   include/hw/pci-host/pnv_phb4.h |  7 +++++--
>   3 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index fb6c4f993a..1a7395772f 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1443,7 +1443,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 d2851e8040..042dc0b775 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -403,9 +403,9 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>            * available here via the 'i' iterator so it's convenient to
>            * do it now.
>            */
> -        object_property_set_int(OBJECT(&stack->phb), "index", phb_id,
> +        object_property_set_int(OBJECT(stack->phb), "index", phb_id,
>                                   &error_abort);
> -        pnv_phb4_set_stack_phb_props(stack, &stack->phb);
> +        pnv_phb4_set_stack_phb_props(stack, stack->phb);
>   
>           if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
>               return;
> @@ -543,7 +543,7 @@ 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);
> +    stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));

Move qdev_new under pnv_pec_stk_realize() please.

Thanks,

C.



>   }
>   
>   static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
> @@ -574,10 +574,10 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>       /* 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);
> +    pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(stack->phb),
> +                          &pnv_phb4_xscom_ops, stack->phb, name, 0x40);
>   
> -    if (!sysbus_realize(SYS_BUS_DEVICE(&stack->phb), errp)) {
> +    if (stack->phb && !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 {
> 

[PATCH v3 07/10] ppc/pnv: move PHB4 related XSCOM init to phb4_realize()

[Daniel Henrique Barboza]

Before enabling pnv-phb4 user creatable devices we need to handle PHB4
specific code in pnv_pec_stk_realize().

The 'stack->phb_regs_mr' PHB4 passthrough XSCOM initialization relies on
'stack->phb' being not NULL. Moving 'stack->phb_regs_mr' region_init()
and add_subregion() to phb4_realize() time is a natural thing to do and
it'll spare us from checking 'phb->stack != NULL' in stk_realize() when
user creatable pnv-phb4s are implemented.

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

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 1a7395772f..152911a285 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1194,6 +1194,31 @@ void pnv_phb4_set_stack_phb_props(PnvPhb4PecStack *stack,
                              &error_abort);
 }
 
+static void pnv_phb4_init_xscom_passthrough(PnvPHB4 *phb)
+{
+    PnvPhb4PecState *pec;
+    PnvPhb4PecClass *pecc;
+    uint32_t pec_pci_base;
+    char name[64];
+
+    assert(phb->stack);
+
+    pec = phb->stack->pec;
+    pecc = PNV_PHB4_PEC_GET_CLASS(pec);
+    pec_pci_base = pecc->xscom_pci_base(pec);
+
+    /* PHB pass-through */
+    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb",
+             pec->chip_id, pec->index, phb->stack->stack_no);
+    pnv_xscom_region_init(&phb->stack->phb_regs_mr, OBJECT(phb),
+                          &pnv_phb4_xscom_ops, phb, name, 0x40);
+
+    pnv_xscom_add_subregion(pec->chip,
+                            pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
+                            0x40 * phb->stack->stack_no,
+                            &phb->stack->phb_regs_mr);
+}
+
 static void pnv_phb4_instance_init(Object *obj)
 {
     PnvPHB4 *phb = PNV_PHB4(obj);
@@ -1223,6 +1248,8 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
     memory_region_init_io(&phb->mr_regs, OBJECT(phb), &pnv_phb4_reg_ops, phb,
                           name, 0x2000);
 
+    pnv_phb4_init_xscom_passthrough(phb);
+
     /*
      * PHB4 doesn't support IO space. However, qemu gets very upset if
      * we don't have an IO region to anchor IO BARs onto so we just
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
index 042dc0b775..5e02a51f04 100644
--- a/hw/pci-host/pnv_phb4_pec.c
+++ b/hw/pci-host/pnv_phb4_pec.c
@@ -571,12 +571,6 @@ 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 */
-    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);
-
     if (stack->phb && !sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) {
         return;
     }
@@ -591,10 +585,6 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
     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);
 }
 
 static Property pnv_pec_stk_properties[] = {
-- 
2.33.1

Re: [PATCH v3 07/10] ppc/pnv: move PHB4 related XSCOM init to phb4_realize()

[Cédric Le Goater]

On 1/10/22 15:33, Daniel Henrique Barboza wrote:
> Before enabling pnv-phb4 user creatable devices we need to handle PHB4
> specific code in pnv_pec_stk_realize().
> 
> The 'stack->phb_regs_mr' PHB4 passthrough XSCOM initialization relies on
> 'stack->phb' being not NULL. Moving 'stack->phb_regs_mr' region_init()
> and add_subregion() to phb4_realize() time is a natural thing to do and
> it'll spare us from checking 'phb->stack != NULL' in stk_realize() when
> user creatable pnv-phb4s are implemented.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb4.c     | 27 +++++++++++++++++++++++++++
>   hw/pci-host/pnv_phb4_pec.c | 10 ----------
>   2 files changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 1a7395772f..152911a285 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1194,6 +1194,31 @@ void pnv_phb4_set_stack_phb_props(PnvPhb4PecStack *stack,
>                                &error_abort);
>   }
>   
> +static void pnv_phb4_init_xscom_passthrough(PnvPHB4 *phb)
> +{
> +    PnvPhb4PecState *pec;
> +    PnvPhb4PecClass *pecc;
> +    uint32_t pec_pci_base;
> +    char name[64];
> +
> +    assert(phb->stack);
> +
> +    pec = phb->stack->pec;
> +    pecc = PNV_PHB4_PEC_GET_CLASS(pec);
> +    pec_pci_base = pecc->xscom_pci_base(pec);
> +
> +    /* PHB pass-through */
> +    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb",
> +             pec->chip_id, pec->index, phb->stack->stack_no);
> +    pnv_xscom_region_init(&phb->stack->phb_regs_mr, OBJECT(phb),
> +                          &pnv_phb4_xscom_ops, phb, name, 0x40);
> +
> +    pnv_xscom_add_subregion(pec->chip,
> +                            pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
> +                            0x40 * phb->stack->stack_no,
> +                            &phb->stack->phb_regs_mr);
> +}
> +
>   static void pnv_phb4_instance_init(Object *obj)
>   {
>       PnvPHB4 *phb = PNV_PHB4(obj);
> @@ -1223,6 +1248,8 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>       memory_region_init_io(&phb->mr_regs, OBJECT(phb), &pnv_phb4_reg_ops, phb,
>                             name, 0x2000);
>   
> +    pnv_phb4_init_xscom_passthrough(phb);
> +
>       /*
>        * PHB4 doesn't support IO space. However, qemu gets very upset if
>        * we don't have an IO region to anchor IO BARs onto so we just
> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> index 042dc0b775..5e02a51f04 100644
> --- a/hw/pci-host/pnv_phb4_pec.c
> +++ b/hw/pci-host/pnv_phb4_pec.c
> @@ -571,12 +571,6 @@ 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 */
> -    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);
> -
>       if (stack->phb && !sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) {
>           return;
>       }
> @@ -591,10 +585,6 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>       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);
>   }


Can't we simply move the XSCOM init and mapping under phb4 realize routine ?

Thanks,

C.


>   static Property pnv_pec_stk_properties[] = {
> 

Re: [PATCH v3 07/10] ppc/pnv: move PHB4 related XSCOM init to phb4_realize()

[Daniel Henrique Barboza]

On 1/10/22 12:57, Cédric Le Goater wrote:
> On 1/10/22 15:33, Daniel Henrique Barboza wrote:
>> Before enabling pnv-phb4 user creatable devices we need to handle PHB4
>> specific code in pnv_pec_stk_realize().
>>
>> The 'stack->phb_regs_mr' PHB4 passthrough XSCOM initialization relies on
>> 'stack->phb' being not NULL. Moving 'stack->phb_regs_mr' region_init()
>> and add_subregion() to phb4_realize() time is a natural thing to do and
>> it'll spare us from checking 'phb->stack != NULL' in stk_realize() when
>> user creatable pnv-phb4s are implemented.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/pci-host/pnv_phb4.c     | 27 +++++++++++++++++++++++++++
>>   hw/pci-host/pnv_phb4_pec.c | 10 ----------
>>   2 files changed, 27 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
>> index 1a7395772f..152911a285 100644
>> --- a/hw/pci-host/pnv_phb4.c
>> +++ b/hw/pci-host/pnv_phb4.c
>> @@ -1194,6 +1194,31 @@ void pnv_phb4_set_stack_phb_props(PnvPhb4PecStack *stack,
>>                                &error_abort);
>>   }
>> +static void pnv_phb4_init_xscom_passthrough(PnvPHB4 *phb)
>> +{
>> +    PnvPhb4PecState *pec;
>> +    PnvPhb4PecClass *pecc;
>> +    uint32_t pec_pci_base;
>> +    char name[64];
>> +
>> +    assert(phb->stack);
>> +
>> +    pec = phb->stack->pec;
>> +    pecc = PNV_PHB4_PEC_GET_CLASS(pec);
>> +    pec_pci_base = pecc->xscom_pci_base(pec);
>> +
>> +    /* PHB pass-through */
>> +    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb",
>> +             pec->chip_id, pec->index, phb->stack->stack_no);
>> +    pnv_xscom_region_init(&phb->stack->phb_regs_mr, OBJECT(phb),
>> +                          &pnv_phb4_xscom_ops, phb, name, 0x40);
>> +
>> +    pnv_xscom_add_subregion(pec->chip,
>> +                            pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
>> +                            0x40 * phb->stack->stack_no,
>> +                            &phb->stack->phb_regs_mr);
>> +}
>> +
>>   static void pnv_phb4_instance_init(Object *obj)
>>   {
>>       PnvPHB4 *phb = PNV_PHB4(obj);
>> @@ -1223,6 +1248,8 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>>       memory_region_init_io(&phb->mr_regs, OBJECT(phb), &pnv_phb4_reg_ops, phb,
>>                             name, 0x2000);
>> +    pnv_phb4_init_xscom_passthrough(phb);
>> +
>>       /*
>>        * PHB4 doesn't support IO space. However, qemu gets very upset if
>>        * we don't have an IO region to anchor IO BARs onto so we just
>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
>> index 042dc0b775..5e02a51f04 100644
>> --- a/hw/pci-host/pnv_phb4_pec.c
>> +++ b/hw/pci-host/pnv_phb4_pec.c
>> @@ -571,12 +571,6 @@ 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 */
>> -    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);
>> -
>>       if (stack->phb && !sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) {
>>           return;
>>       }
>> @@ -591,10 +585,6 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
>>       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);
>>   }
> 
> 
> Can't we simply move the XSCOM init and mapping under phb4 realize routine ?

There's a lot of code to be moved when doing that but in theory we can. It would
simplify some things (e.g. we wouldn't need to check stack->phb != NULL in
phb4_update_maps() like we need to do in the next patch).

I also got cold feet about the repercussions of creating a stack without any XSCOM
mapped on it, but perhaps this is the right time to see if that's possible and, if not,
try to understand why.


Thanks,


Daniel



> 
> Thanks,
> 
> C.
> 
> 
>>   static Property pnv_pec_stk_properties[] = {
>>
> 

[PATCH v3 08/10] pnv_phb4.c: check stack->phb not NULL in phb4_update_regions()

[Daniel Henrique Barboza]

The last step before enabling user creatable pnv-phb4 devices still has
to do with specific XSCOM initialization code in pnv_phb4_stk_realize().

'stack->nest_regs_mr' is being init regardless of the existence of
'stack->phb', which is verified only when trying to realize the phb.
Its MemoryRegionOps,'pnv_pec_stk_nest_xscom_ops', uses
pnv_pec_stk_nest_xscom_write() as a write callback. When trying to write
the PEC_NEST_STK_BAR_EN reg, pnv_pec_stk_update_map() is called. Inside
this function, pnv_phb4_update_regions() is called twice. This function
uses stack->phb to manipulate memory regions of the phb.

When enabling user creatable phb4s, a stack that doesn't have an
associated phb (i.e. will have stack->phb = NULL) will cause a SIGINT
during boot in pnv_phb4_update_regions(). To deal with this we have
some options, including:

- check for stack->phb being not NULL in pnv_phb4_update_regions();

- change the order of the XSCOM initialization to avoid initializing
'stack->nest_regs_mr' if 'stack->phb' is NULL. This can have unintended
side-effects: there are several other regs that are being read/written
in these memory regions, and we would forbid all read/write operations
in these regs because of writes in PEC_NEST_STK_BAR_EN being problematic;

- move the XSCOM init code to phb4_realize() like the previous patch
did with 'stack->phb_regs_mr'. Besides having the same potential side
effects than the previous alternative, a lot of code would need to be
moved from pnv_phb4_pec.c to pnv_phb4.c because all the memory region
code is static.

Being the option that is less intrusive and innocus of the alternatives,
this patch keeps the XSCOM initialization as is in
pnv_phb4_stk_realize() and check for 'stack->phb' being NULL in
pnv_phb4_update_regions().

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

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 152911a285..fc23a96b7f 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1472,6 +1472,17 @@ void pnv_phb4_update_regions(PnvPhb4PecStack *stack)
 {
     PnvPHB4 *phb = stack->phb;
 
+    /*
+     * This will happen when there's no phb associated with the stack.
+     * pnv_pec_stk_realize() will init the nested xscom address space
+     * (stack->nest_regs_mr) that uses pnv_phb4_update_regions(), via
+     * pnv_pec_stk_update_map(), which in turn is the write callback of
+     * the PEC_NEST_STK_BAR_EN reg in pnv_pec_stk_nest_xscom_write().
+     */
+    if (!stack->phb) {
+        return;
+    }
+
     /* Unmap first always */
     if (memory_region_is_mapped(&phb->mr_regs)) {
         memory_region_del_subregion(&stack->phbbar, &phb->mr_regs);
-- 
2.33.1

Re: [PATCH v3 08/10] pnv_phb4.c: check stack->phb not NULL in phb4_update_regions()

[Cédric Le Goater]

On 1/10/22 15:33, Daniel Henrique Barboza wrote:
> The last step before enabling user creatable pnv-phb4 devices still has
> to do with specific XSCOM initialization code in pnv_phb4_stk_realize().
> 
> 'stack->nest_regs_mr' is being init regardless of the existence of
> 'stack->phb', which is verified only when trying to realize the phb.
> Its MemoryRegionOps,'pnv_pec_stk_nest_xscom_ops', uses
> pnv_pec_stk_nest_xscom_write() as a write callback. When trying to write
> the PEC_NEST_STK_BAR_EN reg, pnv_pec_stk_update_map() is called. Inside
> this function, pnv_phb4_update_regions() is called twice. This function
> uses stack->phb to manipulate memory regions of the phb.
> 
> When enabling user creatable phb4s, a stack that doesn't have an
> associated phb (i.e. will have stack->phb = NULL) will cause a SIGINT
> during boot in pnv_phb4_update_regions(). To deal with this we have
> some options, including:
> 
> - check for stack->phb being not NULL in pnv_phb4_update_regions();
> 
> - change the order of the XSCOM initialization to avoid initializing
> 'stack->nest_regs_mr' if 'stack->phb' is NULL. This can have unintended
> side-effects: there are several other regs that are being read/written
> in these memory regions, and we would forbid all read/write operations
> in these regs because of writes in PEC_NEST_STK_BAR_EN being problematic;
> 
> - move the XSCOM init code to phb4_realize() like the previous patch
> did with 'stack->phb_regs_mr'. Besides having the same potential side
> effects than the previous alternative, a lot of code would need to be
> moved from pnv_phb4_pec.c to pnv_phb4.c because all the memory region
> code is static.
> 
> Being the option that is less intrusive and innocus of the alternatives,
> this patch keeps the XSCOM initialization as is in
> pnv_phb4_stk_realize() and check for 'stack->phb' being NULL in
> pnv_phb4_update_regions().
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb4.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 152911a285..fc23a96b7f 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1472,6 +1472,17 @@ void pnv_phb4_update_regions(PnvPhb4PecStack *stack)
>   {
>       PnvPHB4 *phb = stack->phb;
>   
> +    /*
> +     * This will happen when there's no phb associated with the stack.
> +     * pnv_pec_stk_realize() will init the nested xscom address space
> +     * (stack->nest_regs_mr) that uses pnv_phb4_update_regions(), via
> +     * pnv_pec_stk_update_map(), which in turn is the write callback of
> +     * the PEC_NEST_STK_BAR_EN reg in pnv_pec_stk_nest_xscom_write().
> +     */
> +    if (!stack->phb) {
> +        return;
> +    }
> +


I would assert()

Thanks,

C.


>       /* Unmap first always */
>       if (memory_region_is_mapped(&phb->mr_regs)) {
>           memory_region_del_subregion(&stack->phbbar, &phb->mr_regs);
> 

[PATCH v3 09/10] 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.

We accomplish this by doing the following:

- add a pnv_phb4_get_stack() helper to retrieve which stack an user
created phb4 would occupy;

- if a suitable pec->stack is found, setup the phb attributes in a
similar fashion as done in pnv_phb4_pec_realize() when defaults are
enabled;

- use 'defaults_enabled()' in pnv_pec_stk_instance_init() to avoid
creating a 'stack->phb' qdev that might be overwritten by an user
created pnv-phb4 device.

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

diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index fc23a96b7f..8c8f5dd0e1 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -1229,15 +1229,97 @@ 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) {
+        /*
+         * This is expected to happen since chip-id and index are
+         * being set by the user in the command line. Return an
+         * informative error instead of asserting.
+         */
+        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 chip-id %d index %d didn't match any existing PEC",
+               chip_id, index);
+
+    return NULL;
+}
+
 static void pnv_phb4_realize(DeviceState *dev, Error **errp)
 {
     PnvPHB4 *phb = PNV_PHB4(dev);
     PCIHostState *pci = PCI_HOST_BRIDGE(dev);
     XiveSource *xsrc = &phb->xsrc;
+    PnvPhb4PecStack *stack = NULL;
+    Error *local_err = NULL;
     int nr_irqs;
     char name[32];
 
-    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;
+        }
+
+        /*
+         * Assign the phb to the stack. If pnv_phb4_get_stack() returned
+         * stack = NULL without an error we're better of aborting.
+         */
+        g_assert(stack);
+        stack->phb = phb;
+
+        object_property_set_int(OBJECT(phb), "index",
+                                phb->phb_id, &error_abort);
+        pnv_phb4_set_stack_phb_props(stack, 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;
+        }
+    }
 
     /* Set the "big_phb" flag */
     phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3;
@@ -1342,7 +1424,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 5e02a51f04..1e3233e7ec 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>
 
@@ -397,15 +398,17 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
         object_property_set_int(stk_obj, "stack-no", i, &error_abort);
         object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort);
 
-        /*
-         * stack->phb->index is dependent on the position the
-         * stack occupies in pec->stacks[]. We have this information
-         * available here via the 'i' iterator so it's convenient to
-         * do it now.
-         */
-        object_property_set_int(OBJECT(stack->phb), "index", phb_id,
-                                &error_abort);
-        pnv_phb4_set_stack_phb_props(stack, stack->phb);
+        if (defaults_enabled()) {
+            /*
+             * stack->phb->index is dependent on the position the
+             * stack occupies in pec->stacks[]. We have this information
+             * available here via the 'i' iterator so it's convenient to
+             * do it now.
+             */
+            object_property_set_int(OBJECT(stack->phb), "index", phb_id,
+                                    &error_abort);
+            pnv_phb4_set_stack_phb_props(stack, stack->phb);
+        }
 
         if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
             return;
@@ -543,7 +546,9 @@ static void pnv_pec_stk_instance_init(Object *obj)
 {
     PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(obj);
 
-    stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
+    if (defaults_enabled()) {
+        stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
+    }
 }
 
 static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
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

Re: [PATCH v3 09/10] ppc/pnv: Introduce user creatable pnv-phb4 devices

[Cédric Le Goater]

On 1/10/22 15:33, 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.
> 
> We accomplish this by doing the following:
> 
> - add a pnv_phb4_get_stack() helper to retrieve which stack an user
> created phb4 would occupy;
> 
> - if a suitable pec->stack is found, setup the phb attributes in a
> similar fashion as done in pnv_phb4_pec_realize() when defaults are
> enabled;
> 
> - use 'defaults_enabled()' in pnv_pec_stk_instance_init() to avoid
> creating a 'stack->phb' qdev that might be overwritten by an user
> created pnv-phb4 device.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/pnv_phb4.c     | 86 +++++++++++++++++++++++++++++++++++++-
>   hw/pci-host/pnv_phb4_pec.c | 25 ++++++-----
>   hw/ppc/pnv.c               |  2 +
>   3 files changed, 101 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index fc23a96b7f..8c8f5dd0e1 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1229,15 +1229,97 @@ 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;


Change the pnv_phb4_get_stack() prototype to :

   static PnvPhb4PecStack *pnv_phb4_get_stack(int chip_id, int index,
                                              Error **errp);

it will simplify the pnv_phb4_realize() routine.


> +    int i, j;
> +
> +    if (chip->num_pecs == 0) {
> +        /*
> +         * This is expected to happen since chip-id and index are
> +         * being set by the user in the command line. Return an
> +         * informative error instead of asserting.
> +         */

I don't understand how this can happen since num_pecs is initialized :
  
   chip->num_pecs = pcc->num_pecs

and

   k->num_pecs = PNV9_CHIP_MAX_PEC;

> +        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 chip-id %d index %d didn't match any existing PEC",
> +               chip_id, index);
> +
> +    return NULL;
> +}
> +
>   static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>   {
>       PnvPHB4 *phb = PNV_PHB4(dev);
>       PCIHostState *pci = PCI_HOST_BRIDGE(dev);
>       XiveSource *xsrc = &phb->xsrc;
> +    PnvPhb4PecStack *stack = NULL;
> +    Error *local_err = NULL;
>       int nr_irqs;
>       char name[32];
>   
> -    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;
> +        }
> +
> +        /*
> +         * Assign the phb to the stack. If pnv_phb4_get_stack() returned
> +         * stack = NULL without an error we're better of aborting.
> +         */
> +        g_assert(stack);

This can not happend if pnv_phb4_get_stack() returns local_err;

> +        stack->phb = phb;
> +
> +        object_property_set_int(OBJECT(phb), "index",
> +                                phb->phb_id, &error_abort);

This is redundant. "index" has already been set by the user if the PHB
is user created or by the PnvPhb4PecStack realize routine.

> +        pnv_phb4_set_stack_phb_props(stack, phb);

I have the feeling this should be called after the 'if (!phb->stack)'
code section and not in pnv_pec_realize() like PATCH 5 does.

> +
> +        /*
> +         * 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;
> +        }
> +    }
>   
>       /* Set the "big_phb" flag */
>       phb->big_phb = phb->phb_id == 0 || phb->phb_id == 3;
> @@ -1342,7 +1424,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 5e02a51f04..1e3233e7ec 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>
>   
> @@ -397,15 +398,17 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
>           object_property_set_int(stk_obj, "stack-no", i, &error_abort);
>           object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort);
>   
> -        /*
> -         * stack->phb->index is dependent on the position the
> -         * stack occupies in pec->stacks[]. We have this information
> -         * available here via the 'i' iterator so it's convenient to
> -         * do it now.
> -         */
> -        object_property_set_int(OBJECT(stack->phb), "index", phb_id,
> -                                &error_abort);
> -        pnv_phb4_set_stack_phb_props(stack, stack->phb);
> +        if (defaults_enabled()) {
> +            /*
> +             * stack->phb->index is dependent on the position the
> +             * stack occupies in pec->stacks[]. We have this information
> +             * available here via the 'i' iterator so it's convenient to
> +             * do it now.
> +             */
> +            object_property_set_int(OBJECT(stack->phb), "index", phb_id,
> +                                    &error_abort);
> +            pnv_phb4_set_stack_phb_props(stack, stack->phb);
> +        }

This clearly belongs to PHB4 realize handler.

Thanks,

C.

>           if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) {
>               return;
> @@ -543,7 +546,9 @@ static void pnv_pec_stk_instance_init(Object *obj)
>   {
>       PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(obj);
>   
> -    stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
> +    if (defaults_enabled()) {
> +        stack->phb = PNV_PHB4(qdev_new(TYPE_PNV_PHB4));
> +    }
>   }
>   
>   static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
> 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)
> 

[PATCH v3 10/10] 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 ""

Reviewed-by: Cédric Le Goater <clg@kaod.org>
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 2fb5e119c4..b9537b8da7 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 v3 00/10] user creatable pnv-phb4 devices

[Cédric Le Goater]

On 1/10/22 15:33, Daniel Henrique Barboza wrote:
> Hi,
> 
> This v3 contains new versions of pnv-phb4 exclusive patches from
> version 2. Patches 1-10 are already accepted.
> 
> I changed how patch 9 (v2 patch 17) works by doing everything possible
> in extra patches/cleanups beforehand, and then using patch 9 to flip the
> switch in a single step. This means that handling the default initialization
> of pnv-phb4s is done at the same time we enable user creatable pnv-phb4s.
> 
> There's also a change in how XSCOM initializion is being handled. We're not
> using a flag to do a partial XSCOM initialization during phb4_realize() anymore.
> Intead, we moved XSCOM initialization code, as less intrusive as we could, to
> phb4_realize().
> 
> This time I also took the precaution of testing the default case
> (i.e. running without -nodefaults) in every patch. v2 was breaking
> this default run between some patches.
> 
> changes from v2:
> - former patch 16: removed
> - patch 10 (v2 18): unchanged
> - patches 4,5,7,8: new
> - patch 9 (former 17):
>    * added g_assert() if stack == NULL
>    * added a comment explaining why we shouldn't assert on user error
> with wrong chip-id/index values
> - minor changes across the patches due to the design changes
> - v2 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg00671.html
> 
> Daniel Henrique Barboza (10):
>    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_pec.c: remove stack 'phb-id' alias
>    pnv_phb4_pec.c: move phb4 properties setup to pec_realize()
>    ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack
>    ppc/pnv: move PHB4 related XSCOM init to phb4_realize()
>    pnv_phb4.c: check stack->phb not NULL in phb4_update_regions()
>    ppc/pnv: Introduce user creatable pnv-phb4 devices
>    pnv_phb4.c: change TYPE_PNV_PHB4_ROOT_BUS name

I have taken Patches 10, 2, 3 in this order. There is still some PHB4
code outside the PHB4 realize routine and this is making the code too
complex. We are getting closer !

Could you use the 'ppc/pnv:' prefix ?

Thanks,

C.