Re: [PATCH 07/14] ppc/pnv: Introduce a num_pecs class attribute for PHB4 PEC devices
On 12/7/21 15:03, Frederic Barrat wrote:
On 07/12/2021 11:45, Cédric Le Goater wrote:
On 12/7/21 11:00, Frederic Barrat wrote:
On 02/12/2021 15:42, Cédric Le Goater wrote:
POWER9 processor comes with 3 PHB4 PECs (PCI Express Controller) and
each PEC can have several PHBs :
* PEC0 provides 1 PHB (PHB0)
* PEC1 provides 2 PHBs (PHB1 and PHB2)
* PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)
A num_pecs class attribute represents better the logic units of the
POWER9 chip. Use that instead of num_phbs which fits POWER8 chips.
This will ease adding support for user created devices.
Signed-off-by: Cédric Le Goater
---
With this patch, chip->num_phbs is only defined and used on P8. We may want to
add a comment to make it clear.
Yes.
With the latest changes, I think we can now move num_phbs under PnvChip8
and num_pecs under PnvChip9 since they are only used in these routines :
P8:
static void pnv_chip_power8_instance_init(Object *obj)
chip->num_phbs = pcc->num_phbs;
for (i = 0; i < chip->num_phbs; i++) {
static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
for (i = 0; i < chip->num_phbs; i++) {
P9:
static void pnv_chip_power9_instance_init(Object *obj)
chip->num_pecs = pcc->num_pecs;
for (i = 0; i < chip->num_pecs; i++) {
static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
for (i = 0; i < chip->num_pecs; i++) {
As I review this series, something is bugging me though: the difference of
handling between P8 and P9.
On P9, we seem to have a more logical hiearachy:
phb <- PCI controller (PEC) <- chip
Yes. It's cleaner than P8 in terms of logic. P8 initial support was
done hastily for skiboot bringup in 2014.
With P8, we don't have an explicit PEC, but we have a PBCQ object, which is
somewhat similar. The hierarchy seems also more convoluted.
But we don't have stacks on P8. Do we ?
Stacks were introduced on P9 because all the lanes handled by a PEC could be
grouped differently, each group being called a stack. And each stack is
associated to a PHB.
On P8, there's no such split, so the doc didn't mention stacks. But each PEC
handles exactly one PHB. So we could still keep the same abstractions.
On all chips, a PEC would really be equal to a pbcq interface to the power bus.
The pbcq is servicing one (on P8) or more (on P9/P10) PHBs.
I don't see why it's treated differently. It seems both chips could be treated
the same, which would make the code easier to follow.
I agree. Daniel certainly would also :)
That's outside of the scope of this series though.
Well, this patchset enables libvirt support for the PowerNV machines.
Once this is pushed, we need to keep the API, the object model names
being part of it.
7.0 is a good time for a change, really. After that, we won't be able
to change the QOM hierarchy that much.
So maybe for a future patch? Who knows, I might volunteer...
You would introduce a phb3-pec on top of the phb3s ?
Or rename pnv_phb3_pbcq.c to pnv_phb3_pec.c and starts from there.
Conceptually, the TYPE_PNV_PBCQ and TYPE_PNV_PHB4_PEC_STACK objects seem close.
But that's easy to say in an email...
It's a start.
Here is the PHB3 QOM tree :
/pnv-phb3[0] (pnv-phb3)
/lsi (ics)
/msi (phb3-msi)
/msi32[0] (memory-region)
/msi64[0] (memory-region)
/pbcq (pnv-pbcq)
/pbcq-mmio0[0] (memory-region)
/pbcq-mmio1[0] (memory-region)
/pbcq-phb[0] (memory-region)
/xscom-pbcq-nest-0.0[0] (memory-region)
/xscom-pbcq-pci-0.0[0] (memory-region)
/xscom-pbcq-spci-0.0[0] (memory-region)
/pci-io[0] (memory-region)
/pci-mmio[0] (memory-region)
/pcie-mmcfg-mmio[0] (memory-region)
/phb3-m32[0] (memory-region)
/phb3-regs[0] (memory-region)
/phb3_iommu[0] (pnv-phb3-iommu-memory-region)
/root (pnv-phb3-root-port)
/bus master container[0] (memory-region)
/bus master[0] (memory-region)
/pci_bridge_io[0] (memory-region)
/pci_bridge_io[1] (memory-region)
/pci_bridge_mem[0] (memory-region)
/pci_bridge_pci[0] (memory-region)
/pci_bridge_pref_mem[0] (memory-region)
/pci_bridge_vga_io_hi[0] (memory-region)
/pci_bridge_vga_io_lo[0] (memory-region)
/pci_bridge_vga_mem[0] (memory-region)
/pcie.0 (PCIE)
/root-bus (pnv-phb3-root-bus)
We would swap 'pnv-phb3' and 'pnv-pbcq' and rename it to 'pnv-phb3-pec'.
Looks good to me. This should clarify the relationship between objects.
I never like the back pointer to the phb under pbcq:
(qemu) qom-list /machine/chip[0]/pnv-phb3[0]/pbcq
type (string)
parent_bus (link)
realized (bool)
hotplugged (bool)
hotpluggable (bool)
pbcq-mmio0[0] (child)
xscom-pbcq-spci-0.0[0] (child)
xscom-pbcq-nest-0.0[0] (child)
pbcq-mmio1[0] (child)
phb (link)
pbcq-phb[0] (child)
Re: [PATCH 07/14] ppc/pnv: Introduce a num_pecs class attribute for PHB4 PEC devices
On 07/12/2021 11:45, Cédric Le Goater wrote:
On 12/7/21 11:00, Frederic Barrat wrote:
On 02/12/2021 15:42, Cédric Le Goater wrote:
POWER9 processor comes with 3 PHB4 PECs (PCI Express Controller) and
each PEC can have several PHBs :
* PEC0 provides 1 PHB (PHB0)
* PEC1 provides 2 PHBs (PHB1 and PHB2)
* PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)
A num_pecs class attribute represents better the logic units of the
POWER9 chip. Use that instead of num_phbs which fits POWER8 chips.
This will ease adding support for user created devices.
Signed-off-by: Cédric Le Goater
---
With this patch, chip->num_phbs is only defined and used on P8. We may
want to add a comment to make it clear.
Yes.
With the latest changes, I think we can now move num_phbs under PnvChip8
and num_pecs under PnvChip9 since they are only used in these routines :
P8:
static void pnv_chip_power8_instance_init(Object *obj)
chip->num_phbs = pcc->num_phbs;
for (i = 0; i < chip->num_phbs; i++) {
static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
for (i = 0; i < chip->num_phbs; i++) {
P9:
static void pnv_chip_power9_instance_init(Object *obj)
chip->num_pecs = pcc->num_pecs;
for (i = 0; i < chip->num_pecs; i++) {
static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
for (i = 0; i < chip->num_pecs; i++) {
As I review this series, something is bugging me though: the
difference of handling between P8 and P9.
On P9, we seem to have a more logical hiearachy:
phb <- PCI controller (PEC) <- chip
Yes. It's cleaner than P8 in terms of logic. P8 initial support was
done hastily for skiboot bringup in 2014.
With P8, we don't have an explicit PEC, but we have a PBCQ object,
which is somewhat similar. The hierarchy seems also more convoluted.
But we don't have stacks on P8. Do we ?
Stacks were introduced on P9 because all the lanes handled by a PEC
could be grouped differently, each group being called a stack. And each
stack is associated to a PHB.
On P8, there's no such split, so the doc didn't mention stacks. But each
PEC handles exactly one PHB. So we could still keep the same abstractions.
On all chips, a PEC would really be equal to a pbcq interface to the
power bus. The pbcq is servicing one (on P8) or more (on P9/P10) PHBs.
I don't see why it's treated differently. It seems both chips could be
treated the same, which would make the code easier to follow.
I agree. Daniel certainly would also :)
That's outside of the scope of this series though.
Well, this patchset enables libvirt support for the PowerNV machines.
Once this is pushed, we need to keep the API, the object model names
being part of it.
7.0 is a good time for a change, really. After that, we won't be able
to change the QOM hierarchy that much.
So maybe for a future patch? Who knows, I might volunteer...
You would introduce a phb3-pec on top of the phb3s ?
Or rename pnv_phb3_pbcq.c to pnv_phb3_pec.c and starts from there.
Conceptually, the TYPE_PNV_PBCQ and TYPE_PNV_PHB4_PEC_STACK objects seem
close. But that's easy to say in an email...
Fred
Let me send a v2 first and may be we could rework the object hierarchy
in the 7.0 time frame. We don't have to merge this ASAP.
Thanks,
C.
Re: [PATCH 07/14] ppc/pnv: Introduce a num_pecs class attribute for PHB4 PEC devices
On 12/7/21 11:00, Frederic Barrat wrote:
On 02/12/2021 15:42, Cédric Le Goater wrote:
POWER9 processor comes with 3 PHB4 PECs (PCI Express Controller) and
each PEC can have several PHBs :
* PEC0 provides 1 PHB (PHB0)
* PEC1 provides 2 PHBs (PHB1 and PHB2)
* PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)
A num_pecs class attribute represents better the logic units of the
POWER9 chip. Use that instead of num_phbs which fits POWER8 chips.
This will ease adding support for user created devices.
Signed-off-by: Cédric Le Goater
---
With this patch, chip->num_phbs is only defined and used on P8. We may want to
add a comment to make it clear.
Yes.
With the latest changes, I think we can now move num_phbs under PnvChip8
and num_pecs under PnvChip9 since they are only used in these routines :
P8:
static void pnv_chip_power8_instance_init(Object *obj)
chip->num_phbs = pcc->num_phbs;
for (i = 0; i < chip->num_phbs; i++) {
static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
for (i = 0; i < chip->num_phbs; i++) {
P9:
static void pnv_chip_power9_instance_init(Object *obj)
chip->num_pecs = pcc->num_pecs;
for (i = 0; i < chip->num_pecs; i++) {
static void pnv_chip_power9_phb_realize(PnvChip *chip, Error **errp)
for (i = 0; i < chip->num_pecs; i++) {
As I review this series, something is bugging me though: the difference of
handling between P8 and P9.
On P9, we seem to have a more logical hiearachy:
phb <- PCI controller (PEC) <- chip
Yes. It's cleaner than P8 in terms of logic. P8 initial support was
done hastily for skiboot bringup in 2014.
With P8, we don't have an explicit PEC, but we have a PBCQ object, which is
somewhat similar. The hierarchy seems also more convoluted.
But we don't have stacks on P8. Do we ?
I don't see why it's treated differently. It seems both chips could be treated
the same, which would make the code easier to follow.
I agree. Daniel certainly would also :)
That's outside of the scope of this series though.
Well, this patchset enables libvirt support for the PowerNV machines.
Once this is pushed, we need to keep the API, the object model names
being part of it.
7.0 is a good time for a change, really. After that, we won't be able
to change the QOM hierarchy that much.
So maybe for a future patch? Who knows, I might volunteer...
You would introduce a phb3-pec on top of the phb3s ?
Let me send a v2 first and may be we could rework the object hierarchy
in the 7.0 time frame. We don't have to merge this ASAP.
Thanks,
C.
Re: [PATCH 07/14] ppc/pnv: Introduce a num_pecs class attribute for PHB4 PEC devices
On 02/12/2021 15:42, Cédric Le Goater wrote:
POWER9 processor comes with 3 PHB4 PECs (PCI Express Controller) and
each PEC can have several PHBs :
* PEC0 provides 1 PHB (PHB0)
* PEC1 provides 2 PHBs (PHB1 and PHB2)
* PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)
A num_pecs class attribute represents better the logic units of the
POWER9 chip. Use that instead of num_phbs which fits POWER8 chips.
This will ease adding support for user created devices.
Signed-off-by: Cédric Le Goater
---
With this patch, chip->num_phbs is only defined and used on P8. We may
want to add a comment to make it clear.
As I review this series, something is bugging me though: the difference
of handling between P8 and P9.
On P9, we seem to have a more logical hiearachy:
phb <- PCI controller (PEC) <- chip
With P8, we don't have an explicit PEC, but we have a PBCQ object, which
is somewhat similar. The hierarchy seems also more convoluted.
I don't see why it's treated differently. It seems both chips could be
treated the same, which would make the code easier to follow.
That's outside of the scope of this series though. So maybe for a future
patch? Who knows, I might volunteer...
Fred
include/hw/ppc/pnv.h | 2 ++
hw/ppc/pnv.c | 20 +---
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 247379ef1f88..f2c238062f4a 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -53,6 +53,7 @@ struct PnvChip {
PnvCore **cores;
uint32_t num_phbs;
+uint32_t num_pecs;
MemoryRegion xscom_mmio;
MemoryRegion xscom;
@@ -136,6 +137,7 @@ struct PnvChipClass {
uint64_t chip_cfam_id;
uint64_t cores_mask;
uint32_t num_phbs;
+uint32_t num_pecs;
DeviceRealize parent_realize;
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 45d8ecbf2bf7..185464a1d443 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -658,7 +658,7 @@ static void pnv_chip_power9_pic_print_info(PnvChip *chip,
Monitor *mon)
pnv_xive_pic_print_info(>xive, mon);
pnv_psi_pic_print_info(>psi, mon);
-for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
+for (i = 0; i < chip->num_pecs; i++) {
PnvPhb4PecState *pec = >pecs[i];
for (j = 0; j < pec->num_stacks; j++) {
pnv_phb4_pic_print_info(>stacks[j].phb, mon);
@@ -1330,15 +1330,14 @@ static void pnv_chip_power9_instance_init(Object *obj)
object_initialize_child(obj, "homer", >homer, TYPE_PNV9_HOMER);
-for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
+if (defaults_enabled()) {
+chip->num_pecs = pcc->num_pecs;
+}
+
+for (i = 0; i < chip->num_pecs; i++) {
object_initialize_child(obj, "pec[*]", >pecs[i],
TYPE_PNV_PHB4_PEC);
}
-
-/*
- * Number of PHBs is the chip default
- */
-chip->num_phbs = pcc->num_phbs;
}
static void pnv_chip_quad_realize(Pnv9Chip *chip9, Error **errp)
@@ -1374,7 +1373,7 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip,
Error **errp)
int i, j;
int phb_id = 0;
-for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
+for (i = 0; i < chip->num_pecs; i++) {
PnvPhb4PecState *pec = >pecs[i];
PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
uint32_t pec_nest_base;
@@ -1402,8 +1401,7 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip,
Error **errp)
pnv_xscom_add_subregion(chip, pec_nest_base, >nest_regs_mr);
pnv_xscom_add_subregion(chip, pec_pci_base, >pci_regs_mr);
-for (j = 0; j < pec->num_stacks && phb_id < chip->num_phbs;
- j++, phb_id++) {
+for (j = 0; j < pec->num_stacks; j++, phb_id++) {
PnvPhb4PecStack *stack = >stacks[j];
Object *obj = OBJECT(>phb);
@@ -1559,7 +1557,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
k->xscom_core_base = pnv_chip_power9_xscom_core_base;
k->xscom_pcba = pnv_chip_power9_xscom_pcba;
dc->desc = "PowerNV Chip POWER9";
-k->num_phbs = 6;
+k->num_pecs = PNV9_CHIP_MAX_PEC;
device_class_set_parent_realize(dc, pnv_chip_power9_realize,
>parent_realize);
Re: [PATCH 07/14] ppc/pnv: Introduce a num_pecs class attribute for PHB4 PEC devices
On 12/2/21 11:42, Cédric Le Goater wrote:
POWER9 processor comes with 3 PHB4 PECs (PCI Express Controller) and
each PEC can have several PHBs :
* PEC0 provides 1 PHB (PHB0)
* PEC1 provides 2 PHBs (PHB1 and PHB2)
* PEC2 provides 3 PHBs (PHB3, PHB4 and PHB5)
A num_pecs class attribute represents better the logic units of the
POWER9 chip. Use that instead of num_phbs which fits POWER8 chips.
This will ease adding support for user created devices.
Signed-off-by: Cédric Le Goater
---
Reviewed-by: Daniel Henrique Barboza
include/hw/ppc/pnv.h | 2 ++
hw/ppc/pnv.c | 20 +---
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 247379ef1f88..f2c238062f4a 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -53,6 +53,7 @@ struct PnvChip {
PnvCore **cores;
uint32_t num_phbs;
+uint32_t num_pecs;
MemoryRegion xscom_mmio;
MemoryRegion xscom;
@@ -136,6 +137,7 @@ struct PnvChipClass {
uint64_t chip_cfam_id;
uint64_t cores_mask;
uint32_t num_phbs;
+uint32_t num_pecs;
DeviceRealize parent_realize;
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 45d8ecbf2bf7..185464a1d443 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -658,7 +658,7 @@ static void pnv_chip_power9_pic_print_info(PnvChip *chip,
Monitor *mon)
pnv_xive_pic_print_info(>xive, mon);
pnv_psi_pic_print_info(>psi, mon);
-for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
+for (i = 0; i < chip->num_pecs; i++) {
PnvPhb4PecState *pec = >pecs[i];
for (j = 0; j < pec->num_stacks; j++) {
pnv_phb4_pic_print_info(>stacks[j].phb, mon);
@@ -1330,15 +1330,14 @@ static void pnv_chip_power9_instance_init(Object *obj)
object_initialize_child(obj, "homer", >homer, TYPE_PNV9_HOMER);
-for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
+if (defaults_enabled()) {
+chip->num_pecs = pcc->num_pecs;
+}
+
+for (i = 0; i < chip->num_pecs; i++) {
object_initialize_child(obj, "pec[*]", >pecs[i],
TYPE_PNV_PHB4_PEC);
}
-
-/*
- * Number of PHBs is the chip default
- */
-chip->num_phbs = pcc->num_phbs;
}
static void pnv_chip_quad_realize(Pnv9Chip *chip9, Error **errp)
@@ -1374,7 +1373,7 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip,
Error **errp)
int i, j;
int phb_id = 0;
-for (i = 0; i < PNV9_CHIP_MAX_PEC; i++) {
+for (i = 0; i < chip->num_pecs; i++) {
PnvPhb4PecState *pec = >pecs[i];
PnvPhb4PecClass *pecc = PNV_PHB4_PEC_GET_CLASS(pec);
uint32_t pec_nest_base;
@@ -1402,8 +1401,7 @@ static void pnv_chip_power9_phb_realize(PnvChip *chip,
Error **errp)
pnv_xscom_add_subregion(chip, pec_nest_base, >nest_regs_mr);
pnv_xscom_add_subregion(chip, pec_pci_base, >pci_regs_mr);
-for (j = 0; j < pec->num_stacks && phb_id < chip->num_phbs;
- j++, phb_id++) {
+for (j = 0; j < pec->num_stacks; j++, phb_id++) {
PnvPhb4PecStack *stack = >stacks[j];
Object *obj = OBJECT(>phb);
@@ -1559,7 +1557,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
k->xscom_core_base = pnv_chip_power9_xscom_core_base;
k->xscom_pcba = pnv_chip_power9_xscom_pcba;
dc->desc = "PowerNV Chip POWER9";
-k->num_phbs = 6;
+k->num_pecs = PNV9_CHIP_MAX_PEC;
device_class_set_parent_realize(dc, pnv_chip_power9_realize,
>parent_realize);
