| Message ID | 20250206131052.30207-12-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/microblaze: Allow running cross-endian vCPUs | expand |
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. > > 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
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
On Thu, Feb 06, 2025 at 02:53:58PM +0100, Philippe Mathieu-Daudé wrote: > 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. We would pick an arbitrary endianness of our choosing I guess. How does this work in physical machines ? Is the choice of endianess a firmware setting, or a choice by the vendor when manufacturing in some way ? Picking an arbitrary endianess is compatible with our test suite, it just has the implication that we would only end up testing the machine in a single endianness configuration. If we wanted to test both endianness options, the test would need amending to know to try both values of the endian property on the machine. > This approach is the same I took to merge MIPS*, SH4* and > Xtensa* machines in endianness-agnostic binaries. If we have prior art like this, then remaining consistentv is desirable and thus my comments are too late. > 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). With x86 there's no need for mode=32. Whether the machine supports 64-bit or not is a property of the CPU model chosen. eg "qemu -M pc -cpu Nehalem" would be 64-bit and "qemu -M pc -cpu pentium" would be 32-bit. The qemu-system-i386 binary is pretty much pointless as a separate thing. Libvirt will happily use qemu-system-x86_64 to run 32-bit guests, by just specifying a 32-bit CPU model With regards, Daniel
On 6/2/25 15:31, Daniel P. Berrangé wrote: > On Thu, Feb 06, 2025 at 02:53:58PM +0100, Philippe Mathieu-Daudé wrote: >> 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. > > We would pick an arbitrary endianness of our choosing > I guess. How does this work in physical machines ? Is > the choice of endianess a firmware setting, or a choice > by the vendor when manufacturing in some way ? Like MIPS*, SH4* and Xtensa*, it is a jumper on the board (wired to a CPU pin which is sampled once at cold reset). > Picking an arbitrary endianess is compatible with our > test suite, it just has the implication that we would > only end up testing the machine in a single endianness > configuration. > > If we wanted to test both endianness options, the test > would need amending to know to try both values of the > endian property on the machine. > >> This approach is the same I took to merge MIPS*, SH4* and >> Xtensa* machines in endianness-agnostic binaries. > > If we have prior art like this, then remaining consistentv is > desirable and thus my comments are too late. My worry is about how to not break what distros currently ship. > >> 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). > > With x86 there's no need for mode=32. Whether the machine > supports 64-bit or not is a property of the CPU model > chosen. eg "qemu -M pc -cpu Nehalem" would be 64-bit and > "qemu -M pc -cpu pentium" would be 32-bit. > > The qemu-system-i386 binary is pretty much pointless as a > separate thing. Libvirt will happily use qemu-system-x86_64 > to run 32-bit guests, by just specifying a 32-bit CPU > model IIRC Thomas tried to convert TARGET_X86_64 to a runtime check but it wasn't that easy: https://lore.kernel.org/qemu-devel/20230425133851.489283-1-thuth@redhat.com/
On 06/02/2025 16.04, Philippe Mathieu-Daudé wrote: > On 6/2/25 15:31, Daniel P. Berrangé wrote: >> On Thu, Feb 06, 2025 at 02:53:58PM +0100, Philippe Mathieu-Daudé wrote: >>> 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. >> >> We would pick an arbitrary endianness of our choosing >> I guess. How does this work in physical machines ? Is >> the choice of endianess a firmware setting, or a choice >> by the vendor when manufacturing in some way ? > > Like MIPS*, SH4* and Xtensa*, it is a jumper on the board > (wired to a CPU pin which is sampled once at cold reset). If it's a jumper on the board instead of two separate boards, then I think this is a good indication that this should only be a machine property, and not two separate boards? > My worry is about how to not break what distros currently ship. So once the two binaries got unified, maybe we could still add a hook somewhere that checks argv[0] and sets the endianess property according to the name of the binary, so users can still use a symlink to force the opposite behavior? Anyway, you likely don't have to solve this problem right in this series here already, we can think of this later when one of the binaries gets marked as deprecated. So for this series here, I'd suggest to go ahead without adding the -le and -be machine types and only use a property first, then tackle the remaining questions later...? Thomas
On Thu, Feb 06, 2025 at 04:04:20PM +0100, Philippe Mathieu-Daudé wrote: > On 6/2/25 15:31, Daniel P. Berrangé wrote: > > On Thu, Feb 06, 2025 at 02:53:58PM +0100, Philippe Mathieu-Daudé wrote: > > > 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. > > > > We would pick an arbitrary endianness of our choosing > > I guess. How does this work in physical machines ? Is > > the choice of endianess a firmware setting, or a choice > > by the vendor when manufacturing in some way ? > > Like MIPS*, SH4* and Xtensa*, it is a jumper on the board > (wired to a CPU pin which is sampled once at cold reset). That makes me thing even more it is just a machine type property. None the less, since you've already taken this pattern of dual machine types for BE & LE on MIPS/SH4/XTensa, I think we should stick with your precedent. Consistent modelling of endian handling across all machine types is most important IMHO With regards, Daniel
On Thu, Feb 6, 2025 at 7:04 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > On 6/2/25 15:31, Daniel P. Berrangé wrote: > > We would pick an arbitrary endianness of our choosing > > I guess. How does this work in physical machines ? Is > > the choice of endianess a firmware setting, or a choice > > by the vendor when manufacturing in some way ? > > Like MIPS*, SH4* and Xtensa*, it is a jumper on the board > (wired to a CPU pin which is sampled once at cold reset). This is not exactly the case for xtensa. Each xtensa CPU is either big- or little-endian and it's a static property of the CPU configuration. On physical machines it's either fixed (e.g. if the CPU is a part of an ASIC), or it's a part of a bitstream that gets loaded into an FPGA and there may be a selector for one of the bitstreams in the onboard FLASH. In either case there's normally no board-level switch for the CPU endianness. Also big- and little-endian instruction encodings are different on otherwise identical xtensa CPUs.
On 6/2/25 18:34, Max Filippov wrote: > On Thu, Feb 6, 2025 at 7:04 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> On 6/2/25 15:31, Daniel P. Berrangé wrote: >>> We would pick an arbitrary endianness of our choosing >>> I guess. How does this work in physical machines ? Is >>> the choice of endianess a firmware setting, or a choice >>> by the vendor when manufacturing in some way ? >> >> Like MIPS*, SH4* and Xtensa*, it is a jumper on the board >> (wired to a CPU pin which is sampled once at cold reset). > > This is not exactly the case for xtensa. Each xtensa CPU is either > big- or little-endian and it's a static property of the CPU > configuration. On physical machines it's either fixed (e.g. if > the CPU is a part of an ASIC), or it's a part of a bitstream that > gets loaded into an FPGA and there may be a selector for one > of the bitstreams in the onboard FLASH. In either case there's > normally no board-level switch for the CPU endianness. Yes, this is what we meant in QEMU terms, "static property". > Also big- and little-endian instruction encodings are different on > otherwise identical xtensa CPUs. This is handled within the CPU decode logic, and doesn't have to be exposed. IOW, we don't need 2 distinct TCG frontends to decode the Xtensa ISA.
On 6/2/25 18:12, Daniel P. Berrangé wrote: > On Thu, Feb 06, 2025 at 04:04:20PM +0100, Philippe Mathieu-Daudé wrote: >> On 6/2/25 15:31, Daniel P. Berrangé wrote: >>> On Thu, Feb 06, 2025 at 02:53:58PM +0100, Philippe Mathieu-Daudé wrote: >>>> 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. >>> >>> We would pick an arbitrary endianness of our choosing >>> I guess. How does this work in physical machines ? Is >>> the choice of endianess a firmware setting, or a choice >>> by the vendor when manufacturing in some way ? >> >> Like MIPS*, SH4* and Xtensa*, it is a jumper on the board >> (wired to a CPU pin which is sampled once at cold reset). > > That makes me thing even more it is just a machine type property. I'm happy with a machine property, this was even my first approach using OnOffAuto until I ran the test-suite and have qom-introspection failing. What should be the default? Per the SH4 datasheet: Bit 31—Endian Flag (ENDIAN): Samples the value of the endian specification external pin (MD5) in a power-on reset by the RESET pin. The endian mode of all spaces is determined by this bit. ENDIAN is a read-only bit. There is no default per the spec, and I believe using one is a mistake. > None the less, since you've already taken this pattern of > dual machine types for BE & LE on MIPS/SH4/XTensa, I think > we should stick with your precedent. Consistent modelling > of endian handling across all machine types is most important > IMHO > > > With regards, > Daniel
On Thu, Feb 06, 2025 at 06:49:38PM +0100, Philippe Mathieu-Daudé wrote: > On 6/2/25 18:12, Daniel P. Berrangé wrote: > > On Thu, Feb 06, 2025 at 04:04:20PM +0100, Philippe Mathieu-Daudé wrote: > > > On 6/2/25 15:31, Daniel P. Berrangé wrote: > > > > On Thu, Feb 06, 2025 at 02:53:58PM +0100, Philippe Mathieu-Daudé wrote: > > > > > 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. > > > > > > > > We would pick an arbitrary endianness of our choosing > > > > I guess. How does this work in physical machines ? Is > > > > the choice of endianess a firmware setting, or a choice > > > > by the vendor when manufacturing in some way ? > > > > > > Like MIPS*, SH4* and Xtensa*, it is a jumper on the board > > > (wired to a CPU pin which is sampled once at cold reset). > > > > That makes me thing even more it is just a machine type property. > > I'm happy with a machine property, this was even my first approach > using OnOffAuto until I ran the test-suite and have qom-introspection > failing. > > What should be the default? > > Per the SH4 datasheet: > > Bit 31—Endian Flag (ENDIAN): Samples the value of the endian > specification external pin (MD5) in a power-on reset by the > RESET pin. The endian mode of all spaces is determined by this > bit. ENDIAN is a read-only bit. > > There is no default per the spec, and I believe using one is > a mistake. If it is left as an unspecified choice in the spec, then I would presume HW vendors are picking an option based on what they expect "most" common usage to be amongst their customers. IOW, if we know of typically used guest OS prefer big or little, that could influence our choice. With regards, Daniel
+Michal On 6/2/25 19:06, Daniel P. Berrangé wrote: > On Thu, Feb 06, 2025 at 06:49:38PM +0100, Philippe Mathieu-Daudé wrote: >> On 6/2/25 18:12, Daniel P. Berrangé wrote: >>> On Thu, Feb 06, 2025 at 04:04:20PM +0100, Philippe Mathieu-Daudé wrote: >>>> On 6/2/25 15:31, Daniel P. Berrangé wrote: >>>>> On Thu, Feb 06, 2025 at 02:53:58PM +0100, Philippe Mathieu-Daudé wrote: >>>>>> 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. >>>>> >>>>> We would pick an arbitrary endianness of our choosing >>>>> I guess. How does this work in physical machines ? Is >>>>> the choice of endianess a firmware setting, or a choice >>>>> by the vendor when manufacturing in some way ? >>>> >>>> Like MIPS*, SH4* and Xtensa*, it is a jumper on the board >>>> (wired to a CPU pin which is sampled once at cold reset). >>> >>> That makes me thing even more it is just a machine type property. >> >> I'm happy with a machine property, this was even my first approach >> using OnOffAuto until I ran the test-suite and have qom-introspection >> failing. >> >> What should be the default? >> >> Per the SH4 datasheet: >> >> Bit 31—Endian Flag (ENDIAN): Samples the value of the endian >> specification external pin (MD5) in a power-on reset by the >> RESET pin. The endian mode of all spaces is determined by this >> bit. ENDIAN is a read-only bit. >> >> There is no default per the spec, and I believe using one is >> a mistake. > > If it is left as an unspecified choice in the spec, then I would > presume HW vendors are picking an option based on what they > expect "most" common usage to be amongst their customers. IOW, > if we know of typically used guest OS prefer big or little, that > could influence our choice. Please have a look at this thread: https://lore.kernel.org/qemu-devel/d3346a55-584b-427b-8c87-32f7411a9290@amd.com/
On Thu, Feb 06, 2025 at 07:24:55PM +0100, Philippe Mathieu-Daudé wrote: > +Michal > > On 6/2/25 19:06, Daniel P. Berrangé wrote: > > On Thu, Feb 06, 2025 at 06:49:38PM +0100, Philippe Mathieu-Daudé wrote: > > > On 6/2/25 18:12, Daniel P. Berrangé wrote: > > > > On Thu, Feb 06, 2025 at 04:04:20PM +0100, Philippe Mathieu-Daudé wrote: > > > > > On 6/2/25 15:31, Daniel P. Berrangé wrote: > > > > > > On Thu, Feb 06, 2025 at 02:53:58PM +0100, Philippe Mathieu-Daudé wrote: > > > > > > > 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. > > > > > > > > > > > > We would pick an arbitrary endianness of our choosing > > > > > > I guess. How does this work in physical machines ? Is > > > > > > the choice of endianess a firmware setting, or a choice > > > > > > by the vendor when manufacturing in some way ? > > > > > > > > > > Like MIPS*, SH4* and Xtensa*, it is a jumper on the board > > > > > (wired to a CPU pin which is sampled once at cold reset). > > > > > > > > That makes me thing even more it is just a machine type property. > > > > > > I'm happy with a machine property, this was even my first approach > > > using OnOffAuto until I ran the test-suite and have qom-introspection > > > failing. > > > > > > What should be the default? > > > > > > Per the SH4 datasheet: > > > > > > Bit 31—Endian Flag (ENDIAN): Samples the value of the endian > > > specification external pin (MD5) in a power-on reset by the > > > RESET pin. The endian mode of all spaces is determined by this > > > bit. ENDIAN is a read-only bit. > > > > > > There is no default per the spec, and I believe using one is > > > a mistake. > > > > If it is left as an unspecified choice in the spec, then I would > > presume HW vendors are picking an option based on what they > > expect "most" common usage to be amongst their customers. IOW, > > if we know of typically used guest OS prefer big or little, that > > could influence our choice. > > Please have a look at this thread: > https://lore.kernel.org/qemu-devel/d3346a55-584b-427b-8c87-32f7411a9290@amd.com/ That seems to give a pretty clear choice for qemu defaults "I am not aware about anybody configuring MB to big endian" so in that particular case, defaulting to LE would be most sensible. With regards, Daniel
(sorry, posted too quick) On 6/2/25 19:24, Philippe Mathieu-Daudé wrote: > +Michal > > On 6/2/25 19:06, Daniel P. Berrangé wrote: >> On Thu, Feb 06, 2025 at 06:49:38PM +0100, Philippe Mathieu-Daudé wrote: >>> On 6/2/25 18:12, Daniel P. Berrangé wrote: >>>> On Thu, Feb 06, 2025 at 04:04:20PM +0100, Philippe Mathieu-Daudé wrote: >>>>> On 6/2/25 15:31, Daniel P. Berrangé wrote: >>>>>> On Thu, Feb 06, 2025 at 02:53:58PM +0100, Philippe Mathieu-Daudé >>>>>> wrote: >>>>>>> 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. >>>>>> >>>>>> We would pick an arbitrary endianness of our choosing >>>>>> I guess. How does this work in physical machines ? Is >>>>>> the choice of endianess a firmware setting, or a choice >>>>>> by the vendor when manufacturing in some way ? >>>>> >>>>> Like MIPS*, SH4* and Xtensa*, it is a jumper on the board >>>>> (wired to a CPU pin which is sampled once at cold reset). >>>> >>>> That makes me thing even more it is just a machine type property. >>> >>> I'm happy with a machine property, this was even my first approach >>> using OnOffAuto until I ran the test-suite and have qom-introspection >>> failing. >>> >>> What should be the default? >>> >>> Per the SH4 datasheet: >>> >>> Bit 31—Endian Flag (ENDIAN): Samples the value of the endian >>> specification external pin (MD5) in a power-on reset by the >>> RESET pin. The endian mode of all spaces is determined by this >>> bit. ENDIAN is a read-only bit. >>> >>> There is no default per the spec, and I believe using one is >>> a mistake. >> >> If it is left as an unspecified choice in the spec, then I would >> presume HW vendors are picking an option based on what they >> expect "most" common usage to be amongst their customers. IOW, >> if we know of typically used guest OS prefer big or little, that >> could influence our choice. > > Please have a look at this thread: > https://lore.kernel.org/qemu-devel/ > d3346a55-584b-427b-8c87-32f7411a9290@amd.com/ Per Michal: Truth is that I am not aware about anybody configuring MB to big endian and we are on AXI for quite a long time. There is still code in Linux kernel for it but I can't see any reason to keep it around. I don't think that make sense to keep big endian configurations alive at all. So with that in mind, using a default of little-endian=true for MicroBlaze seems safe as of 2025. For MIPS, big-endian was the default but as the industry shifted, it stayed big-endian on low-end 32-bit cores but mostly became little-endian on high-end 64-bit cores. For Renesas RX, the "Endian" section in the "CPU" chapter of the hardware manual mentions: Both big and little endian are supported for the treatment of data, and the endian is switched by changing the setting on a mode pin (MDE) at the time of a power-on reset. SH-4 "is bi-endian, running in either big-endian or little-endian byte ordering."
On 6/2/25 19:29, Daniel P. Berrangé wrote: > On Thu, Feb 06, 2025 at 07:24:55PM +0100, Philippe Mathieu-Daudé wrote: >> +Michal >> >> On 6/2/25 19:06, Daniel P. Berrangé wrote: >>> On Thu, Feb 06, 2025 at 06:49:38PM +0100, Philippe Mathieu-Daudé wrote: >>>> On 6/2/25 18:12, Daniel P. Berrangé wrote: >>>>> On Thu, Feb 06, 2025 at 04:04:20PM +0100, Philippe Mathieu-Daudé wrote: >>>>>> On 6/2/25 15:31, Daniel P. Berrangé wrote: >>>>>>> On Thu, Feb 06, 2025 at 02:53:58PM +0100, Philippe Mathieu-Daudé wrote: >>>>>>>> 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. >>>>>>> >>>>>>> We would pick an arbitrary endianness of our choosing >>>>>>> I guess. How does this work in physical machines ? Is >>>>>>> the choice of endianess a firmware setting, or a choice >>>>>>> by the vendor when manufacturing in some way ? >>>>>> >>>>>> Like MIPS*, SH4* and Xtensa*, it is a jumper on the board >>>>>> (wired to a CPU pin which is sampled once at cold reset). >>>>> >>>>> That makes me thing even more it is just a machine type property. >>>> >>>> I'm happy with a machine property, this was even my first approach >>>> using OnOffAuto until I ran the test-suite and have qom-introspection >>>> failing. >>>> >>>> What should be the default? >>>> >>>> Per the SH4 datasheet: >>>> >>>> Bit 31—Endian Flag (ENDIAN): Samples the value of the endian >>>> specification external pin (MD5) in a power-on reset by the >>>> RESET pin. The endian mode of all spaces is determined by this >>>> bit. ENDIAN is a read-only bit. >>>> >>>> There is no default per the spec, and I believe using one is >>>> a mistake. >>> >>> If it is left as an unspecified choice in the spec, then I would >>> presume HW vendors are picking an option based on what they >>> expect "most" common usage to be amongst their customers. IOW, >>> if we know of typically used guest OS prefer big or little, that >>> could influence our choice. >> >> Please have a look at this thread: >> https://lore.kernel.org/qemu-devel/d3346a55-584b-427b-8c87-32f7411a9290@amd.com/ > > That seems to give a pretty clear choice for qemu defaults > > "I am not aware about anybody configuring MB to big endian" > > so in that particular case, defaulting to LE would be most sensible. Or maybe I should stop trying to unify the current binaries, and add the new data-drive machines in a new binary, as you already suggested: https://lore.kernel.org/qemu-devel/ZEoyNt0UtSYRt9Go@redhat.com/ But then I'm worried about our users, on how to deprecate the current microblaze{,el} binaries to have them use the new one seamlessly. And more importantly, one of the goal is to maintain LESS binaries, no more.
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. To clarify what we are trying to achieve here: 1/ Current situation: $ qemu-system-microblaze -M help Supported machines are: none empty machine petalogix-ml605 PetaLogix linux refdesign for xilinx ml605 (big endian) (deprecated) petalogix-s3adsp1800 PetaLogix linux refdesign for xilinx Spartan 3ADSP1800 (default) xlnx-zynqmp-pmu Xilinx ZynqMP PMU machine (big endian) (deprecated) $ qemu-system-microblazeel -M help Supported machines are: none empty machine petalogix-ml605 PetaLogix linux refdesign for xilinx ml605 (little endian) petalogix-s3adsp1800 PetaLogix linux refdesign for xilinx Spartan 3ADSP1800 (default) xlnx-zynqmp-pmu Xilinx ZynqMP PMU machine (little endian) 1 architecture declined in 2 binaries, total of 6 machines 2/ With this series: qemu-system-microblaze -M help Supported machines are: none empty machine petalogix-ml605 PetaLogix linux refdesign for xilinx ml605 (big endian) (deprecated) petalogix-s3adsp1800 PetaLogix linux refdesign for xilinx Spartan 3ADSP1800 (big endian) (alias of petalogix-s3adsp1800-be) petalogix-s3adsp1800-be PetaLogix linux refdesign for xilinx Spartan 3ADSP1800 (big endian) (default) petalogix-s3adsp1800-le PetaLogix linux refdesign for xilinx Spartan 3ADSP1800 (little endian) xlnx-zynqmp-pmu Xilinx ZynqMP PMU machine (big endian) (deprecated) qemu-system-microblazeel -M help Supported machines are: none empty machine petalogix-ml605 PetaLogix linux refdesign for xilinx ml605 (little endian) petalogix-s3adsp1800-be PetaLogix linux refdesign for xilinx Spartan 3ADSP1800 (big endian) petalogix-s3adsp1800 PetaLogix linux refdesign for xilinx Spartan 3ADSP1800 (little endian) (alias of petalogix-s3adsp1800-le) petalogix-s3adsp1800-le PetaLogix linux refdesign for xilinx Spartan 3ADSP1800 (little endian) (default) xlnx-zynqmp-pmu Xilinx ZynqMP PMU machine (little endian) 1 architecture declined in 2 binaries, total of 10 machines 3/ Once the deprecation period passed and we can merge the 2 binaries: $ qemu-system-microblaze -M help Supported machines are: none empty machine petalogix-ml605 PetaLogix linux refdesign for xilinx ml605 (little endian) petalogix-s3adsp1800-be PetaLogix linux refdesign for xilinx Spartan 3ADSP1800 (big endian) petalogix-s3adsp1800-le PetaLogix linux refdesign for xilinx Spartan 3ADSP1800 (little endian) xlnx-zynqmp-pmu Xilinx ZynqMP PMU machine (little endian) 1 architecture, 1 binary, 4 machines (which could be reduced to 3 if we want a default endianness for the s3adsp1800). Due to the deprecation policy, step 2/ is necessary to reach 3/.
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, }, };