Re: [PATCH v5 0/5] user creatable pnv-phb4 devices
On 3/10/22 19:49, Thomas Huth wrote:
On 11/01/2022 14.10, Daniel Henrique Barboza wrote:
Hi,
This version implements Cedric's review suggestions from v4. No
drastic design changes were made.
Changes from v4:
- patches 1,3,5: unchanged
- patch 2:
* renamed function to pnv_phb4_xscom_realize()
* pnv4_phb4_xscom_realize() is now called at the end of phb4_realize()
- patch 4:
* changed pnv_phb4_get_stack signature to use chip and phb
* added a new helper called pnv_pec_stk_default_phb_realize() to
realize the default phb when running with defaults
- v4 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02148.html
Daniel Henrique Barboza (5):
ppc/pnv: set phb4 properties in stk_realize()
ppc/pnv: move PHB4 XSCOM init to phb4_realize()
ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack
ppc/pnv: Introduce user creatable pnv-phb4 devices
ppc/pnv: turn pnv_phb4_update_regions() into static
It's now possible to crash QEMU with the pnv-phb4 device:
$ ./qemu-system-ppc64 -nographic -M powernv9 -device pnv-phb4
Unexpected error in object_property_try_add() at
../../devel/qemu/qom/object.c:1229:
qemu-system-ppc64: -device pnv-phb4: attempt to add duplicate property
'pnv-phb4[0]' to object (type 'power9_v2.0-pnv-chip')
Aborted (core dumped)
Any ideas how to fix this?
This was introduced by :
commit 6e7b96750359 ("ppc/pnv: fix default PHB4 QOM hierarchy")
It could be fixed with :
@@ -1598,15 +1598,15 @@ static void pnv_phb4_realize(DeviceState
error_propagate(errp, local_err);
return;
}
-}
-/* Reparent the PHB to the chip to build the device tree */
-pnv_chip_parent_fixup(chip, OBJECT(phb), phb->phb_id);
+/* Reparent the PHB to the chip to build 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, _err)) {
-error_propagate(errp, local_err);
-return;
+s = qdev_get_parent_bus(DEVICE(chip));
+if (!qdev_set_parent_bus(DEVICE(phb), s, _err)) {
+error_propagate(errp, local_err);
+return;
+}
}
/* Set the "big_phb" flag */
but I am not sure we want to keep user-created PHB* devices.
Thanks,
C.
Re: [PATCH v5 0/5] user creatable pnv-phb4 devices
On 3/11/22 09:45, Cédric Le Goater wrote:
Hello,
In 3/11/22 03:18, Daniel Henrique Barboza wrote:
On 3/10/22 15:49, Thomas Huth wrote:
On 11/01/2022 14.10, Daniel Henrique Barboza wrote:
Hi,
This version implements Cedric's review suggestions from v4. No
drastic design changes were made.
Changes from v4:
- patches 1,3,5: unchanged
- patch 2:
* renamed function to pnv_phb4_xscom_realize()
* pnv4_phb4_xscom_realize() is now called at the end of phb4_realize()
- patch 4:
* changed pnv_phb4_get_stack signature to use chip and phb
* added a new helper called pnv_pec_stk_default_phb_realize() to
realize the default phb when running with defaults
- v4 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02148.html
Daniel Henrique Barboza (5):
ppc/pnv: set phb4 properties in stk_realize()
ppc/pnv: move PHB4 XSCOM init to phb4_realize()
ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack
ppc/pnv: Introduce user creatable pnv-phb4 devices
ppc/pnv: turn pnv_phb4_update_regions() into static
It's now possible to crash QEMU with the pnv-phb4 device:
$ ./qemu-system-ppc64 -nographic -M powernv9 -device pnv-phb4
Unexpected error in object_property_try_add() at
../../devel/qemu/qom/object.c:1229:
qemu-system-ppc64: -device pnv-phb4: attempt to add duplicate property
'pnv-phb4[0]' to object (type 'power9_v2.0-pnv-chip')
Aborted (core dumped)
Any ideas how to fix this?
Thanks for catching this up.
The issue here is that we're not handling the case where an user adds a
pnv-phb4 device
when running default settings (no -nodefaults). With default settings we are
adding all
pnv-phb4 devices that are available to the machine, having no room for any
additional
user creatable pnv-phb4 devices.
A similar situation happens with the powernv8 machine which errors out with a
different
error message:
$ ./qemu-system-ppc64 -nographic -M powernv8 -device pnv-phb3
qemu-system-ppc64: -device pnv-phb3: Can't add chassis slot, error -16
Adding all possible phbs by default is a behavior these machines had since they
were introduced,
and I don't think we want to change it. Thus, a fix would be to forbid user
created pnv-phb devices
when running with defaults.
On a real system with POWER{8,9,10} processors, PHBs are sub-units of
the processor, they can be deactivated by firmware but not plugged in
or out like a PCI adapter on a slot. Nevertheless, it seemed to be
good idea to have user-created PHBs for testing purposes.
Let's come back to the PowerNV requirements.
1. having a limited set of PHBs speedups boot time.
2. it is useful to be able to mimic a partially broken topology you
some time have to deal with during bring-up.
PowerNV is also used for distro install tests and having libvirt
support eases these tasks. libvirt prefers to run the machine with
-nodefaults to be sure not to drag unexpected devices which would need
to be defined in the domain file without being specified on the QEMU
command line. For this reason :
3. -nodefaults should not include default PHBs
The solution we came with was to introduce user-created PHB{3,4,5}
devices on the powernv{8,9,10} machines. Reality proves to be a bit
more complex, internally when modeling such devices, and externally
when dealing with the user interface.
So, to make sure that we don't ship a crappy feature in QEMU 7.0,
I think we should step back a little.
Req 1. and 2. can be simply addressed differently with a machine option:
"phb-mask=", which QEMU would use to enable/disable PHB device
nodes when creating the device tree.
Would this property only impact the device-tree?
Let's say that we're running a 2 socket pnv4 machine, with default settings.
That
would give us 12 PHBs. So phb-mask= is kind of a no-op because you're adding
all PHBs, phb-mask=0001 would add just the first PHB (index=0 chip-id=0) and so
on. Is that correct?
For Req 3., we need to make sure we are taking the right approach. It
seems that we should expose a new type of user-created PHB device,
a generic virtualized one, that libvirt would use and not one depending
on the processor revision. This needs more thinking.
libvirt support isn't upstream yet. We have room to make changes.
I agree that creating generic, un-versioned pnv-phb and pnv-phb-root-port
devices
that can be used by all pnv machines would be good for libvirt support.
Hence, I am proposing we drop user-created PHB{3,4,5} for QEMU-7.0.
All the cleanups we did are not lost and they will be useful for the
next steps in QEMU 7.1.
Seems like a good idea for now. Even if we decide to expose them in the end, if
we
keep them user visible for the 7.0 release we won't be able to change our minds
in 7.1.
Thanks,
Daniel
Thanks,
C.
Re: [PATCH v5 0/5] user creatable pnv-phb4 devices
Hello,
In 3/11/22 03:18, Daniel Henrique Barboza wrote:
On 3/10/22 15:49, Thomas Huth wrote:
On 11/01/2022 14.10, Daniel Henrique Barboza wrote:
Hi,
This version implements Cedric's review suggestions from v4. No
drastic design changes were made.
Changes from v4:
- patches 1,3,5: unchanged
- patch 2:
* renamed function to pnv_phb4_xscom_realize()
* pnv4_phb4_xscom_realize() is now called at the end of phb4_realize()
- patch 4:
* changed pnv_phb4_get_stack signature to use chip and phb
* added a new helper called pnv_pec_stk_default_phb_realize() to
realize the default phb when running with defaults
- v4 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02148.html
Daniel Henrique Barboza (5):
ppc/pnv: set phb4 properties in stk_realize()
ppc/pnv: move PHB4 XSCOM init to phb4_realize()
ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack
ppc/pnv: Introduce user creatable pnv-phb4 devices
ppc/pnv: turn pnv_phb4_update_regions() into static
It's now possible to crash QEMU with the pnv-phb4 device:
$ ./qemu-system-ppc64 -nographic -M powernv9 -device pnv-phb4
Unexpected error in object_property_try_add() at
../../devel/qemu/qom/object.c:1229:
qemu-system-ppc64: -device pnv-phb4: attempt to add duplicate property
'pnv-phb4[0]' to object (type 'power9_v2.0-pnv-chip')
Aborted (core dumped)
Any ideas how to fix this?
Thanks for catching this up.
The issue here is that we're not handling the case where an user adds a
pnv-phb4 device
when running default settings (no -nodefaults). With default settings we are
adding all
pnv-phb4 devices that are available to the machine, having no room for any
additional
user creatable pnv-phb4 devices.
A similar situation happens with the powernv8 machine which errors out with a
different
error message:
$ ./qemu-system-ppc64 -nographic -M powernv8 -device pnv-phb3
qemu-system-ppc64: -device pnv-phb3: Can't add chassis slot, error -16
Adding all possible phbs by default is a behavior these machines had since they
were introduced,
and I don't think we want to change it. Thus, a fix would be to forbid user
created pnv-phb devices
when running with defaults.
On a real system with POWER{8,9,10} processors, PHBs are sub-units of
the processor, they can be deactivated by firmware but not plugged in
or out like a PCI adapter on a slot. Nevertheless, it seemed to be
good idea to have user-created PHBs for testing purposes.
Let's come back to the PowerNV requirements.
1. having a limited set of PHBs speedups boot time.
2. it is useful to be able to mimic a partially broken topology you
some time have to deal with during bring-up.
PowerNV is also used for distro install tests and having libvirt
support eases these tasks. libvirt prefers to run the machine with
-nodefaults to be sure not to drag unexpected devices which would need
to be defined in the domain file without being specified on the QEMU
command line. For this reason :
3. -nodefaults should not include default PHBs
The solution we came with was to introduce user-created PHB{3,4,5}
devices on the powernv{8,9,10} machines. Reality proves to be a bit
more complex, internally when modeling such devices, and externally
when dealing with the user interface.
So, to make sure that we don't ship a crappy feature in QEMU 7.0,
I think we should step back a little.
Req 1. and 2. can be simply addressed differently with a machine option:
"phb-mask=", which QEMU would use to enable/disable PHB device
nodes when creating the device tree.
For Req 3., we need to make sure we are taking the right approach. It
seems that we should expose a new type of user-created PHB device,
a generic virtualized one, that libvirt would use and not one depending
on the processor revision. This needs more thinking.
Hence, I am proposing we drop user-created PHB{3,4,5} for QEMU-7.0.
All the cleanups we did are not lost and they will be useful for the
next steps in QEMU 7.1.
Thanks,
C.
Re: [PATCH v5 0/5] user creatable pnv-phb4 devices
On 3/10/22 15:49, Thomas Huth wrote: On 11/01/2022 14.10, Daniel Henrique Barboza wrote: Hi, This version implements Cedric's review suggestions from v4. No drastic design changes were made. Changes from v4: - patches 1,3,5: unchanged - patch 2: * renamed function to pnv_phb4_xscom_realize() * pnv4_phb4_xscom_realize() is now called at the end of phb4_realize() - patch 4: * changed pnv_phb4_get_stack signature to use chip and phb * added a new helper called pnv_pec_stk_default_phb_realize() to realize the default phb when running with defaults - v4 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02148.html Daniel Henrique Barboza (5): ppc/pnv: set phb4 properties in stk_realize() ppc/pnv: move PHB4 XSCOM init to phb4_realize() ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack ppc/pnv: Introduce user creatable pnv-phb4 devices ppc/pnv: turn pnv_phb4_update_regions() into static It's now possible to crash QEMU with the pnv-phb4 device: $ ./qemu-system-ppc64 -nographic -M powernv9 -device pnv-phb4 Unexpected error in object_property_try_add() at ../../devel/qemu/qom/object.c:1229: qemu-system-ppc64: -device pnv-phb4: attempt to add duplicate property 'pnv-phb4[0]' to object (type 'power9_v2.0-pnv-chip') Aborted (core dumped) Any ideas how to fix this? Thanks for catching this up. The issue here is that we're not handling the case where an user adds a pnv-phb4 device when running default settings (no -nodefaults). With default settings we are adding all pnv-phb4 devices that are available to the machine, having no room for any additional user creatable pnv-phb4 devices. A similar situation happens with the powernv8 machine which errors out with a different error message: $ ./qemu-system-ppc64 -nographic -M powernv8 -device pnv-phb3 qemu-system-ppc64: -device pnv-phb3: Can't add chassis slot, error -16 Adding all possible phbs by default is a behavior these machines had since they were introduced, and I don't think we want to change it. Thus, a fix would be to forbid user created pnv-phb devices when running with defaults. I'll see what I can do. Thanks, Daniel Thomas
Re: [PATCH v5 0/5] user creatable pnv-phb4 devices
On 11/01/2022 14.10, Daniel Henrique Barboza wrote: Hi, This version implements Cedric's review suggestions from v4. No drastic design changes were made. Changes from v4: - patches 1,3,5: unchanged - patch 2: * renamed function to pnv_phb4_xscom_realize() * pnv4_phb4_xscom_realize() is now called at the end of phb4_realize() - patch 4: * changed pnv_phb4_get_stack signature to use chip and phb * added a new helper called pnv_pec_stk_default_phb_realize() to realize the default phb when running with defaults - v4 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02148.html Daniel Henrique Barboza (5): ppc/pnv: set phb4 properties in stk_realize() ppc/pnv: move PHB4 XSCOM init to phb4_realize() ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack ppc/pnv: Introduce user creatable pnv-phb4 devices ppc/pnv: turn pnv_phb4_update_regions() into static It's now possible to crash QEMU with the pnv-phb4 device: $ ./qemu-system-ppc64 -nographic -M powernv9 -device pnv-phb4 Unexpected error in object_property_try_add() at ../../devel/qemu/qom/object.c:1229: qemu-system-ppc64: -device pnv-phb4: attempt to add duplicate property 'pnv-phb4[0]' to object (type 'power9_v2.0-pnv-chip') Aborted (core dumped) Any ideas how to fix this? Thomas
Re: [PATCH v5 0/5] user creatable pnv-phb4 devices
On 1/11/22 14:10, Daniel Henrique Barboza wrote: Hi, This version implements Cedric's review suggestions from v4. No drastic design changes were made. Changes from v4: - patches 1,3,5: unchanged - patch 2: * renamed function to pnv_phb4_xscom_realize() * pnv4_phb4_xscom_realize() is now called at the end of phb4_realize() - patch 4: * changed pnv_phb4_get_stack signature to use chip and phb * added a new helper called pnv_pec_stk_default_phb_realize() to realize the default phb when running with defaults - v4 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02148.html Daniel Henrique Barboza (5): ppc/pnv: set phb4 properties in stk_realize() ppc/pnv: move PHB4 XSCOM init to phb4_realize() ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack ppc/pnv: Introduce user creatable pnv-phb4 devices ppc/pnv: turn pnv_phb4_update_regions() into static hw/pci-host/pnv_phb4.c | 430 ++--- hw/pci-host/pnv_phb4_pec.c | 329 ++--- hw/ppc/pnv.c | 2 + include/hw/pci-host/pnv_phb4.h | 8 +- 4 files changed, 431 insertions(+), 338 deletions(-) Applied to ppc7.0. Thanks, C.
[PATCH v5 0/5] user creatable pnv-phb4 devices
Hi, This version implements Cedric's review suggestions from v4. No drastic design changes were made. Changes from v4: - patches 1,3,5: unchanged - patch 2: * renamed function to pnv_phb4_xscom_realize() * pnv4_phb4_xscom_realize() is now called at the end of phb4_realize() - patch 4: * changed pnv_phb4_get_stack signature to use chip and phb * added a new helper called pnv_pec_stk_default_phb_realize() to realize the default phb when running with defaults - v4 link: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg02148.html Daniel Henrique Barboza (5): ppc/pnv: set phb4 properties in stk_realize() ppc/pnv: move PHB4 XSCOM init to phb4_realize() ppc/pnv: turn 'phb' into a pointer in struct PnvPhb4PecStack ppc/pnv: Introduce user creatable pnv-phb4 devices ppc/pnv: turn pnv_phb4_update_regions() into static hw/pci-host/pnv_phb4.c | 430 ++--- hw/pci-host/pnv_phb4_pec.c | 329 ++--- hw/ppc/pnv.c | 2 + include/hw/pci-host/pnv_phb4.h | 8 +- 4 files changed, 431 insertions(+), 338 deletions(-) -- 2.33.1
