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

[Philippe Mathieu-Daudé]

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
20250204002240.97830-1-philmd@linaro.org/">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