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

[Daniel Henrique Barboza <danielhb413@gmail.com>]

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