Re: [PATCH 3/5] hw/ppc/e500: QOM-attach CPUs to the machine container

[Philippe Mathieu-Daudé]

On 3/11/23 08:40, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>
> > Instead of having CPUs dangling in the /unattached/device
> > bucket, attach them to the machine container.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   hw/ppc/e500.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> > index e04114fb3c..f8177c0280 100644
> > --- a/hw/ppc/e500.c
> > +++ b/hw/ppc/e500.c
> > @@ -946,6 +946,7 @@ void ppce500_init(MachineState *machine)
> >               exit(1);
> >           }
> >   
> > +        object_property_add_child(OBJECT(machine), "cpu[*]", OBJECT(cs));
> >           /*
> >            * Secondary CPU starts in halted state for now. Needs to change
> >            * when implementing non-kernel boot.
>
> A peek at "info qom-tree" confirms the CPU is in /machine/unattached/.
> Along with most onboard devices.  Details below.
>
> Quick test...  I count 563 machines.  394 seem to have CPU(s) in or
> below /machine/unattached/, 129 elsewhere, and 40 I can't easily
> examine, because they don't start to monitor without additional CLI
> arguments.
>
> Where should CPUs be?

It is machine specific.

- For System-on-Chip, it would be in /soc

- For systems that fully model CPU topology, I'd expect a consistent
  topology path. (If it is part of a cluster, in that /cluster).

- For mainframes, it should be part of the CPU cards that can be
  inserted?

- For a single Pentium CPU, maybe /machine is sufficient.

> Is /machine/unattached/ basically where we dump products of lazy
> modelling?

Unfortunately, yes. Also where CLI created devices are I guess.

> If yes, should we try to empty it out?

If it is useful. For components expected to be referenced externally
by humans, probably. If only used by scripts, maybe not, except if
human have to debug.

> If we shouldn't, then why move this one out?

When looking for a component in the tree, I start to look at /machine,
having to fish for it elsewhere is not very natural. I'd change your
question by:
- Why do we need /unattached?
or
- Why do we have 2 different folders, /machine and /unattached?
If it is a headache, why not just simply merge them both?

> $ qemu-system-ppc -nodefaults -S -display none -M ppce500 -monitor stdio
> QEMU 8.1.50 monitor - type 'help' for more information
> (qemu) info qom-tree
> /machine (ppce500-machine)
>    /e500-ccsr (e500-ccsr)
>      /e500-ccsr[0] (memory-region)
>      /e500-pci-bar0[0] (memory-region)
>    /pci-host (e500-pcihost)
>      /bm-e500[0] (memory-region)
>      /pci bus memory[0] (memory-region)
>      /pci-conf-data[0] (memory-region)
>      /pci-conf-idx[0] (memory-region)
>      /pci-container[0] (memory-region)
>      /pci-pio[0] (memory-region)
>      /pci.0 (PCI)
>      /pci.reg[0] (memory-region)
>    /peripheral (container)
>    /peripheral-anon (container)
[...]

>    /unattached (container)
>      /device[0] (e500v2_v30-powerpc-cpu)
>        /unnamed-gpio-in[0] (irq)
>        /unnamed-gpio-in[1] (irq)
>        /unnamed-gpio-in[2] (irq)
>        /unnamed-gpio-in[3] (irq)
>        /unnamed-gpio-in[4] (irq)
>        /unnamed-gpio-in[5] (irq)
>        /unnamed-gpio-in[6] (irq)
>      /device[1] (mpc-i2c)
>        /i2c (i2c-bus)
>        /mpc-i2c[0] (memory-region)
>      /device[2] (ds1338)
>      /device[3] (unimplemented-device)
>        /esdhc[0] (memory-region)
>      /device[4] (generic-sdhci)
>        /sd-bus (sdhci-bus)
>        /sdhci[0] (memory-region)
>      /device[5] (mpc8544-guts)
>        /mpc8544.guts[0] (memory-region)
>      /device[6] (e500-host-bridge)
>        /bus master container[0] (memory-region)
>        /bus master[0] (memory-region)
>      /device[7] (e500-spin)
>        /e500 spin pv device[0] (memory-region)
>      /device[8] (mpc8xxx_gpio)
>        /mpc8xxx_gpio[0] (memory-region)
>        /unnamed-gpio-in[0] (irq)
>        /unnamed-gpio-in[10] (irq)
>        /unnamed-gpio-in[11] (irq)
>        /unnamed-gpio-in[12] (irq)
>        /unnamed-gpio-in[13] (irq)
>        /unnamed-gpio-in[14] (irq)
>        /unnamed-gpio-in[15] (irq)
>        /unnamed-gpio-in[16] (irq)
>        /unnamed-gpio-in[17] (irq)
>        /unnamed-gpio-in[18] (irq)
>        /unnamed-gpio-in[19] (irq)
>        /unnamed-gpio-in[1] (irq)
>        /unnamed-gpio-in[20] (irq)
>        /unnamed-gpio-in[21] (irq)
>        /unnamed-gpio-in[22] (irq)
>        /unnamed-gpio-in[23] (irq)
>        /unnamed-gpio-in[24] (irq)
>        /unnamed-gpio-in[25] (irq)
>        /unnamed-gpio-in[26] (irq)
>        /unnamed-gpio-in[27] (irq)
>        /unnamed-gpio-in[28] (irq)
>        /unnamed-gpio-in[29] (irq)
>        /unnamed-gpio-in[2] (irq)
>        /unnamed-gpio-in[30] (irq)
>        /unnamed-gpio-in[31] (irq)
>        /unnamed-gpio-in[3] (irq)
>        /unnamed-gpio-in[4] (irq)
>        /unnamed-gpio-in[5] (irq)
>        /unnamed-gpio-in[6] (irq)
>        /unnamed-gpio-in[7] (irq)
>        /unnamed-gpio-in[8] (irq)
>        /unnamed-gpio-in[9] (irq)
>      /device[9] (platform-bus-device)
>        /platform bus[0] (memory-region)

Actually most of these do have a QOM parent.

Correctly placing them in the tree should help when trying to
resolve a component and avoiding an ambiguous match.

>      /io[0] (memory-region)
>      /non-qdev-gpio[0] (irq)
>      /sysbus (System)
>      /system[0] (memory-region)