| Message ID | 20230531125400.288917-6-mark.cave-ayland@ilande.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | q800: add support for booting MacOS Classic - part 1 | expand |
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()? > + qemu_register_reset(main_cpu_reset, &m->cpu); > > /* RAM */ > memory_region_add_subregion(get_system_memory(), 0, machine->ram); > @@ -430,7 +432,8 @@ static void q800_machine_init(MachineState *machine) > > /* IRQ Glue */ > glue = qdev_new(TYPE_GLUE); > - object_property_set_link(OBJECT(glue), "cpu", OBJECT(cpu), &error_abort); > + object_property_set_link(OBJECT(glue), "cpu", OBJECT(&m->cpu), > + &error_abort); > sysbus_realize_and_unref(SYS_BUS_DEVICE(glue), &error_fatal); > > /* VIA 1 */ > @@ -605,7 +608,7 @@ static void q800_machine_init(MachineState *machine) > > macfb_mode = (NUBUS_MACFB(dev)->macfb).mode; > > - cs = CPU(cpu); > + cs = CPU(&m->cpu); > if (linux_boot) { > uint64_t high; > void *param_blob, *param_ptr, *param_rng_seed; > diff --git a/include/hw/m68k/q800.h b/include/hw/m68k/q800.h > index 76ea6560b2..0f54f1c2cf 100644 > --- a/include/hw/m68k/q800.h > +++ b/include/hw/m68k/q800.h > @@ -29,6 +29,8 @@ > > struct Q800MachineState { > MachineState parent_obj; > + > + M68kCPU cpu; Declared in "target/m68k/cpu-qom.h" (missing #include). > }; > > #define TYPE_Q800_MACHINE MACHINE_TYPE_NAME("q800")
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. [...]
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? ATB, Mark.
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.
On 31/05/2023 18:43, Philippe Mathieu-Daudé wrote: > 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(-) > > >> @@ -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")); > > Shouldn't we keep using machine->cpu_type? > > If the m68040 is the single CPU usable, we should set > MachineClass::valid_cpu_types[] in q800_machine_class_init(). > >> + object_property_set_bool(OBJECT(&m->cpu), "realized", true, &error_fatal); >> + qemu_register_reset(main_cpu_reset, &m->cpu); Yes I can do that: I don't think it makes any difference to the q800 machine here because the MacOS toolbox ROM doesn't appear to boot with anything other than a 68040 CPU, but it could be useful to make this explicit. ATB, Mark.
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. ATB, Mark.
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. I few accesses have crept in since I converted the tree to qdev_realize() & friends. Another conversion pass would be in order.
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.
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); + qemu_register_reset(main_cpu_reset, &m->cpu); /* RAM */ memory_region_add_subregion(get_system_memory(), 0, machine->ram); @@ -430,7 +432,8 @@ static void q800_machine_init(MachineState *machine) /* IRQ Glue */ glue = qdev_new(TYPE_GLUE); - object_property_set_link(OBJECT(glue), "cpu", OBJECT(cpu), &error_abort); + object_property_set_link(OBJECT(glue), "cpu", OBJECT(&m->cpu), + &error_abort); sysbus_realize_and_unref(SYS_BUS_DEVICE(glue), &error_fatal); /* VIA 1 */ @@ -605,7 +608,7 @@ static void q800_machine_init(MachineState *machine) macfb_mode = (NUBUS_MACFB(dev)->macfb).mode; - cs = CPU(cpu); + cs = CPU(&m->cpu); if (linux_boot) { uint64_t high; void *param_blob, *param_ptr, *param_rng_seed; diff --git a/include/hw/m68k/q800.h b/include/hw/m68k/q800.h index 76ea6560b2..0f54f1c2cf 100644 --- a/include/hw/m68k/q800.h +++ b/include/hw/m68k/q800.h @@ -29,6 +29,8 @@ struct Q800MachineState { MachineState parent_obj; + + M68kCPU cpu; }; #define TYPE_Q800_MACHINE MACHINE_TYPE_NAME("q800")
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(-)