Re: [PATCH v5 0/5] user creatable pnv-phb4 devices

[Cédric Le Goater]

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=<uint>", 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.