Re: [PATCH v2 05/23] q800: move CPU object into Q800MachineState

[Mark Cave-Ayland]

On 13/06/2023 12:50, Markus Armbruster wrote:

> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:
>
> > On 01/06/2023 10:00, Markus Armbruster wrote:
> >
> > > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:
> > >
> > > > On 31/05/2023 16:00, Markus Armbruster wrote:
> > > >
> > > > > Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> > > > >
> > > > > > On 31/5/23 14:53, Mark Cave-Ayland wrote:
> > > > > > > Also change the instantiation of the CPU to use object_initialize_child()
> > > > > > > followed by a separate realisation.
> > > > > > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > > > > > > ---
> > > > > > >      hw/m68k/q800.c         | 13 ++++++++-----
> > > > > > >      include/hw/m68k/q800.h |  2 ++
> > > > > > >      2 files changed, 10 insertions(+), 5 deletions(-)
> > > > > > > diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> > > > > > > index 3730b30dd1..c34b2548ca 100644
> > > > > > > --- a/hw/m68k/q800.c
> > > > > > > +++ b/hw/m68k/q800.c
> > > > > > > @@ -364,7 +364,7 @@ static uint8_t fake_mac_rom[] = {
> > > > > > >        static void q800_machine_init(MachineState *machine)
> > > > > > >      {
> > > > > > > -    M68kCPU *cpu = NULL;
> > > > > > > +    Q800MachineState *m = Q800_MACHINE(machine);
> > > > > > >          int linux_boot;
> > > > > > >          int32_t kernel_size;
> > > > > > >          uint64_t elf_entry;
> > > > > > > @@ -407,8 +407,10 @@ static void q800_machine_init(MachineState *machine)
> > > > > > >          }
> > > > > > >            /* init CPUs */
> > > > > > > -    cpu = M68K_CPU(cpu_create(machine->cpu_type));
> > > > > > > -    qemu_register_reset(main_cpu_reset, cpu);
> > > > > > > +    object_initialize_child(OBJECT(machine), "cpu", &m->cpu,
> > > > > > > +                            M68K_CPU_TYPE_NAME("m68040"));
> > > > > > > +    object_property_set_bool(OBJECT(&m->cpu), "realized", true, &error_fatal);
> > > > > >
> > > > > > CPUs are QDev-based, shouldn't we use qdev_realize()?
> > > > >
> > > > > Yes, we should.
> > > > > [...]
> > > >
> > > > Interesting. I remember thinking that CPUs were different, so I'm fairly sure I 
> > > > borrowed this from some similar code in hw/arm :)
> > > >
> > > > Shouldn't the above be directly equivalent to qdev_realize(dev, NULL, 
> > > > &error_fatal) given that the CPU doesn't connect to a bus?
> > >
> > > It's been a while since I worked on this...
> > >
> > > Commit ce189ab230b (qdev: Convert bus-less devices to qdev_realize()
> > > with Coccinelle) looks like you're right.
> >
> > Thanks for the confirmation! Given that this matches existing code that doesn't 
> > use cpu_create(), I'm inclined to keep this as-is to avoid creating another 
> > pattern for instantiating CPUs.
>
> Wherever you *can* use qdev_realize(), you should.  The less we access
> property "realized" outside qdev core, the better.

No worries, in that case I will switch it to use qdev_realize() in v4.

> I few accesses have crept in since I converted the tree to
> qdev_realize() & friends.  Another conversion pass would be in order.


ATB,

Mark.