Re: [PATCH v7 4/7] mac_newworld: Add machine types for different mac99 configs

[Mark Cave-Ayland]

On 04/01/2023 21:59, BALATON Zoltan wrote:

> The mac99 machine emulates different machines depending on machine
> properties or even if it is run as qemu-system-ppc64 or
> qemu-system-ppc. This is very confusing for users and many hours were
> lost trying to explain it or finding out why commands users came up
> with are not working as expected. (E.g. Windows users might think
> qemu-system-ppc64 is just the 64 bit version of qemu-system-ppc and
> then fail to boot a 32 bit OS with -M mac99 trying to follow an
> example that had qemu-system-ppc.) To avoid such confusion, add
> explicit machine types for the different configs which will work the
> same with both qemu-system-ppc and qemu-system-ppc64 and also make the
> command line clearer for new users.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Some thoughts on this: the first is that not everyone agrees that for qemu-system-X 
that X represents the target. There were previous discussion where some KVM people 
assumed X represented the host, i.e. ppc64 was the binary that ran all PPC guests but 
with hardware acceleration for ppc64 guests on ppc64 hosts. This was a while ago, so 
it may be worth starting a thread on qemu-devel to see what the current consensus is.

Secondly it's not clear to me why you've chosen names like "powermac_3_1" instead of 
"g4agp"? Does powermac_3_1 uniquely identify the G4 AGP Sawtooth model? For QEMU it 
is always best to emulate real machines, and whilst I understand you want to separate 
out the two versions of the mac99 machine, having "powermac_X_Y" seems less clear to me.

Finally can you post links to the device trees that you are using for each of the new 
machine types so that we have a clear reference point for future changes to the QEMU 
Mac machines? Even better include the links in the comments for each machine so that 
the information is easily visible for developers.

> ---
>   hw/ppc/mac_newworld.c | 94 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 94 insertions(+)
>
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 60c9c27986..3f5d1ec097 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -642,9 +642,103 @@ static const TypeInfo core99_machine_info = {
>       },
>   };
>   
> +static void powermac3_1_machine_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
> +    core99_machine_class_init(oc, data);
> +    mc->desc = "Apple Power Mac G4 AGP (Sawtooth)";
> +    mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("7400_v2.9");
> +}
> +
> +static void powermac3_1_instance_init(Object *obj)
> +{
> +    Core99MachineState *cms = CORE99_MACHINE(obj);
> +
> +    cms->via_config = CORE99_VIA_CONFIG_PMU;
> +    return;
> +}
> +
> +static const TypeInfo powermac3_1_machine_info = {
> +    .name          = MACHINE_TYPE_NAME("powermac3_1"),
> +    .parent        = TYPE_MACHINE,
> +    .class_init    = powermac3_1_machine_class_init,
> +    .instance_init = powermac3_1_instance_init,
> +    .instance_size = sizeof(Core99MachineState),
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_FW_PATH_PROVIDER },
> +        { }
> +    },
> +};
> +
> +static void powerbook3_2_machine_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
> +    core99_machine_class_init(oc, data);
> +    mc->desc = "Apple PowerBook G4 Titanium (Mercury)";
> +    mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("7400_v2.9");
> +}
> +
> +static void powerbook3_2_instance_init(Object *obj)
> +{
> +    Core99MachineState *cms = CORE99_MACHINE(obj);
> +
> +    cms->via_config = CORE99_VIA_CONFIG_PMU_ADB;
> +    return;
> +}
> +
> +static const TypeInfo powerbook3_2_machine_info = {
> +    .name          = MACHINE_TYPE_NAME("powerbook3_2"),
> +    .parent        = TYPE_MACHINE,
> +    .class_init    = powerbook3_2_machine_class_init,
> +    .instance_init = powerbook3_2_instance_init,
> +    .instance_size = sizeof(Core99MachineState),
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_FW_PATH_PROVIDER },
> +        { }
> +    },
> +};
> +
> +#ifdef TARGET_PPC64
> +static void powermac7_3_machine_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
> +    core99_machine_class_init(oc, data);
> +    mc->desc = "Apple Power Mac G5 (Niagara)";
> +    mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("970fx_v3.1");
> +}
> +
> +static void powermac7_3_instance_init(Object *obj)
> +{
> +    Core99MachineState *cms = CORE99_MACHINE(obj);
> +
> +    cms->via_config = CORE99_VIA_CONFIG_PMU;
> +    return;
> +}
> +
> +static const TypeInfo powermac7_3_machine_info = {
> +    .name          = MACHINE_TYPE_NAME("powermac7_3"),
> +    .parent        = TYPE_MACHINE,
> +    .class_init    = powermac7_3_machine_class_init,
> +    .instance_init = powermac7_3_instance_init,
> +    .instance_size = sizeof(Core99MachineState),
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_FW_PATH_PROVIDER },
> +        { }
> +    },
> +};
> +#endif
> +
>   static void mac_machine_register_types(void)
>   {
>       type_register_static(&core99_machine_info);
> +    type_register_static(&powermac3_1_machine_info);
> +    type_register_static(&powerbook3_2_machine_info);
> +#ifdef TARGET_PPC64
> +    type_register_static(&powermac7_3_machine_info);
> +#endif
>   }
>   
>   type_init(mac_machine_register_types)


ATB,

Mark.