Re: [PATCH v5 11/16] hw/microblaze: Support various endianness for s3adsp1800 machines

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Daniel,

On 6/2/25 14:20, Daniel P. Berrangé wrote:
> On Thu, Feb 06, 2025 at 02:10:47PM +0100, Philippe Mathieu-Daudé wrote:
>> Introduce an abstract machine parent class which defines
>> the 'little_endian' property. Duplicate the current machine,
>> which endian is tied to the binary endianness, to one big
>> endian and a little endian machine; updating the machine
>> description. Keep the current default machine for each binary.
>>
>> 'petalogix-s3adsp1800' machine is aliased as:
>> - 'petalogix-s3adsp1800-be' on big-endian binary,
>> - 'petalogix-s3adsp1800-le' on little-endian one.
> 
> Does it makes sense to expose these as different machine types ?
> 
> If all the HW is identical in both cases, it feels like the
> endianness could just be a bool property of the machine type,
> rather than a new machine type.

Our test suites expect "qemu-system-foo -M bar" to work out of
the box, we can not have non-default properties.

(This is related to the raspberry pi discussion in
https://lore.kernel.org/qemu-devel/20250204002240.97830-1-philmd@linaro.org/).

My plan is to deprecate 'petalogix-s3adsp1800', so once we
remove it we can merge both qemu-system-microblaze and
qemu-system-microblazeel into a single binary.

If you don't want to add more machines, what should be the
endianness of the 'petalogix-s3adsp1800' machine in a binary
with no particular endianness? Either we add for explicit
endianness (fixing test suites) or we add one machine for
each endianness; I fail to see other options not too
confusing for our users.

This approach is the same I took to merge MIPS*, SH4* and
Xtensa* machines in endianness-agnostic binaries.

Also the same I'm using to merge 32/64-bit targets into the
same binaries.
Assuming we have a qemu-system-x86 binary able to run i386 and
x86_64 machines, what do you expect when starting '-M pc'? How
to not confuse users wanting to run FreeDOS in 32-bit mode?

Again, IMO having '-M pc,mode=32' is simpler, but that breaks
the test suites assumptions than machines can start with no
default values (see QOM introspection tests for example).

> 
>>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/microblaze/petalogix_s3adsp1800_mmu.c | 62 +++++++++++++++++++-----
>>   1 file changed, 51 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c
>> index 96aed4ed1a3..aea727eb7ee 100644
>> --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c
>> +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c
>> @@ -55,8 +55,17 @@
>>   #define ETHLITE_IRQ         1
>>   #define UARTLITE_IRQ        3
>>   
>> +typedef struct PetalogixS3adsp1800MachineClass {
>> +    MachineClass parent_obj;
>> +
>> +    bool little_endian;
>> +} PetalogixS3adsp1800MachineClass;
>> +
>>   #define TYPE_PETALOGIX_S3ADSP1800_MACHINE \
>> -            MACHINE_TYPE_NAME("petalogix-s3adsp1800")
>> +            MACHINE_TYPE_NAME("petalogix-s3adsp1800-common")
>> +DECLARE_CLASS_CHECKERS(PetalogixS3adsp1800MachineClass,
>> +                       PETALOGIX_S3ADSP1800_MACHINE,
>> +                       TYPE_PETALOGIX_S3ADSP1800_MACHINE)
>>   
>>   static void
>>   petalogix_s3adsp1800_init(MachineState *machine)
>> @@ -71,11 +80,13 @@ petalogix_s3adsp1800_init(MachineState *machine)
>>       MemoryRegion *phys_ram = g_new(MemoryRegion, 1);
>>       qemu_irq irq[32];
>>       MemoryRegion *sysmem = get_system_memory();
>> +    PetalogixS3adsp1800MachineClass *pmc;
>>   
>> +    pmc = PETALOGIX_S3ADSP1800_MACHINE_GET_CLASS(machine);
>>       cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU));
>>       object_property_set_str(OBJECT(cpu), "version", "7.10.d", &error_abort);
>>       object_property_set_bool(OBJECT(cpu), "little-endian",
>> -                             !TARGET_BIG_ENDIAN, &error_abort);
>> +                             pmc->little_endian, &error_abort);
>>       qdev_realize(DEVICE(cpu), NULL, &error_abort);
>>   
>>       /* Attach emulated BRAM through the LMB.  */
>> @@ -95,7 +106,7 @@ petalogix_s3adsp1800_init(MachineState *machine)
>>                             64 * KiB, 1, 0x89, 0x18, 0x0000, 0x0, 1);
>>   
>>       dev = qdev_new("xlnx.xps-intc");
>> -    qdev_prop_set_bit(dev, "little-endian", !TARGET_BIG_ENDIAN);
>> +    qdev_prop_set_bit(dev, "little-endian", pmc->little_endian);
>>       qdev_prop_set_uint32(dev, "kind-of-intr",
>>                            1 << ETHLITE_IRQ | 1 << UARTLITE_IRQ);
>>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> @@ -107,7 +118,7 @@ petalogix_s3adsp1800_init(MachineState *machine)
>>       }
>>   
>>       dev = qdev_new(TYPE_XILINX_UARTLITE);
>> -    qdev_prop_set_bit(dev, "little-endian", !TARGET_BIG_ENDIAN);
>> +    qdev_prop_set_bit(dev, "little-endian", pmc->little_endian);
>>       qdev_prop_set_chr(dev, "chardev", serial_hd(0));
>>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>       sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, UARTLITE_BASEADDR);
>> @@ -115,7 +126,7 @@ petalogix_s3adsp1800_init(MachineState *machine)
>>   
>>       /* 2 timers at irq 2 @ 62 Mhz.  */
>>       dev = qdev_new("xlnx.xps-timer");
>> -    qdev_prop_set_bit(dev, "little-endian", !TARGET_BIG_ENDIAN);
>> +    qdev_prop_set_bit(dev, "little-endian", pmc->little_endian);
>>       qdev_prop_set_uint32(dev, "one-timer-only", 0);
>>       qdev_prop_set_uint32(dev, "clock-frequency", 62 * 1000000);
>>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> @@ -123,7 +134,7 @@ petalogix_s3adsp1800_init(MachineState *machine)
>>       sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq[TIMER_IRQ]);
>>   
>>       dev = qdev_new("xlnx.xps-ethernetlite");
>> -    qdev_prop_set_bit(dev, "little-endian", !TARGET_BIG_ENDIAN);
>> +    qdev_prop_set_bit(dev, "little-endian", pmc->little_endian);
>>       qemu_configure_nic_device(dev, true, NULL);
>>       qdev_prop_set_uint32(dev, "tx-ping-pong", 0);
>>       qdev_prop_set_uint32(dev, "rx-ping-pong", 0);
>> @@ -133,26 +144,55 @@ petalogix_s3adsp1800_init(MachineState *machine)
>>   
>>       create_unimplemented_device("xps_gpio", GPIO_BASEADDR, 0x10000);
>>   
>> -    microblaze_load_kernel(cpu, !TARGET_BIG_ENDIAN, ddr_base, ram_size,
>> +    microblaze_load_kernel(cpu, pmc->little_endian, ddr_base, ram_size,
>>                              machine->initrd_filename,
>>                              BINARY_DEVICE_TREE_FILE,
>>                              NULL);
>>   }
>>   
>> -static void petalogix_s3adsp1800_machine_class_init(ObjectClass *oc, void *data)
>> +static void petalogix_s3adsp1800_machine_class_init(ObjectClass *oc,
>> +                                                    bool little_endian)
>>   {
>>       MachineClass *mc = MACHINE_CLASS(oc);
>> +    PetalogixS3adsp1800MachineClass *pmc = PETALOGIX_S3ADSP1800_MACHINE_CLASS(oc);
>>   
>> -    mc->desc = "PetaLogix linux refdesign for xilinx Spartan 3ADSP1800";
>>       mc->init = petalogix_s3adsp1800_init;
>> -    mc->is_default = true;
>> +    pmc->little_endian = little_endian;
>> +    mc->desc = little_endian
>> +        ? "PetaLogix linux refdesign for xilinx Spartan 3ADSP1800 (little endian)"
>> +        : "PetaLogix linux refdesign for xilinx Spartan 3ADSP1800 (big endian)";
>> +    if (little_endian == !TARGET_BIG_ENDIAN) {
>> +        mc->is_default = true;
>> +        mc->alias = "petalogix-s3adsp1800";
>> +    }
>> +}
>> +
>> +static void petalogix_s3adsp1800_machine_class_init_be(ObjectClass *oc, void *data)
>> +{
>> +    petalogix_s3adsp1800_machine_class_init(oc, false);
>> +}
>> +
>> +static void petalogix_s3adsp1800_machine_class_init_le(ObjectClass *oc, void *data)
>> +{
>> +    petalogix_s3adsp1800_machine_class_init(oc, true);
>>   }
>>   
>>   static const TypeInfo petalogix_s3adsp1800_machine_types[] = {
>>       {
>>           .name           = TYPE_PETALOGIX_S3ADSP1800_MACHINE,
>>           .parent         = TYPE_MACHINE,
>> -        .class_init     = petalogix_s3adsp1800_machine_class_init,
>> +        .abstract       = true,
>> +        .class_size     = sizeof(PetalogixS3adsp1800MachineClass),
>> +    },
>> +    {
>> +        .name           = MACHINE_TYPE_NAME("petalogix-s3adsp1800-be"),
>> +        .parent         = TYPE_PETALOGIX_S3ADSP1800_MACHINE,
>> +        .class_init     = petalogix_s3adsp1800_machine_class_init_be,
>> +    },
>> +    {
>> +        .name           = MACHINE_TYPE_NAME("petalogix-s3adsp1800-le"),
>> +        .parent         = TYPE_PETALOGIX_S3ADSP1800_MACHINE,
>> +        .class_init     = petalogix_s3adsp1800_machine_class_init_le,
>>       },
>>   };
>>   
>> -- 
>> 2.47.1
>>
>>
> 
> With regards,
> Daniel