[PATCH 1/4] hw/microblaze: Add endianness property to the petalogix_s3adsp1800 machine

Thomas Huth posted 4 patches 7 months ago
Maintainers: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Alistair Francis <alistair@alistair23.me>, Peter Maydell <peter.maydell@linaro.org>
[PATCH 1/4] hw/microblaze: Add endianness property to the petalogix_s3adsp1800 machine
Posted by Thomas Huth 7 months ago
From: Thomas Huth <thuth@redhat.com>

Since the microblaze target can now handle both endianness, big and
little, we should provide a config knob for the user to select the
desired endianness.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/microblaze/petalogix_s3adsp1800_mmu.c | 41 +++++++++++++++++++++---
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c
index 032f6f70eac..edc5d0dcfd0 100644
--- a/hw/microblaze/petalogix_s3adsp1800_mmu.c
+++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c
@@ -58,9 +58,20 @@
 #define TYPE_PETALOGIX_S3ADSP1800_MACHINE \
             MACHINE_TYPE_NAME("petalogix-s3adsp1800")
 
+struct S3Adsp1800MachineState {
+    MachineState parent_class;
+
+    EndianMode endianness;
+};
+
+OBJECT_DECLARE_TYPE(S3Adsp1800MachineState, MachineClass,
+                    PETALOGIX_S3ADSP1800_MACHINE)
+
+
 static void
 petalogix_s3adsp1800_init(MachineState *machine)
 {
+    S3Adsp1800MachineState *psms = PETALOGIX_S3ADSP1800_MACHINE(machine);
     ram_addr_t ram_size = machine->ram_size;
     DeviceState *dev;
     MicroBlazeCPU *cpu;
@@ -71,13 +82,12 @@ petalogix_s3adsp1800_init(MachineState *machine)
     MemoryRegion *phys_ram = g_new(MemoryRegion, 1);
     qemu_irq irq[32];
     MemoryRegion *sysmem = get_system_memory();
-    EndianMode endianness = TARGET_BIG_ENDIAN ? ENDIAN_MODE_BIG
-                                              : ENDIAN_MODE_LITTLE;
+    EndianMode endianness = psms->endianness;
 
     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);
+                             endianness == ENDIAN_MODE_LITTLE, &error_abort);
     qdev_realize(DEVICE(cpu), NULL, &error_abort);
 
     /* Attach emulated BRAM through the LMB.  */
@@ -135,20 +145,41 @@ petalogix_s3adsp1800_init(MachineState *machine)
 
     create_unimplemented_device("xps_gpio", GPIO_BASEADDR, 0x10000);
 
-    microblaze_load_kernel(cpu, !TARGET_BIG_ENDIAN, ddr_base, ram_size,
-                           machine->initrd_filename,
+    microblaze_load_kernel(cpu, endianness == ENDIAN_MODE_LITTLE, ddr_base,
+                           ram_size, machine->initrd_filename,
                            BINARY_DEVICE_TREE_FILE,
                            NULL);
 }
 
+static int machine_get_endianness(Object *obj, Error **errp G_GNUC_UNUSED)
+{
+    S3Adsp1800MachineState *ms = PETALOGIX_S3ADSP1800_MACHINE(obj);
+    return ms->endianness;
+}
+
+static void machine_set_endianness(Object *obj, int endianness, Error **errp)
+{
+    S3Adsp1800MachineState *ms = PETALOGIX_S3ADSP1800_MACHINE(obj);
+    ms->endianness = endianness;
+}
+
 static void petalogix_s3adsp1800_machine_class_init(ObjectClass *oc,
                                                     const void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    ObjectProperty *prop;
 
     mc->desc = "PetaLogix linux refdesign for xilinx Spartan 3ADSP1800";
     mc->init = petalogix_s3adsp1800_init;
     mc->is_default = true;
+
+    prop = object_class_property_add_enum(oc, "endianness", "EndianMode",
+                                          &EndianMode_lookup,
+                                          machine_get_endianness,
+                                          machine_set_endianness);
+    object_property_set_default_str(prop, TARGET_BIG_ENDIAN ? "big" : "little");
+    object_class_property_set_description(oc, "endianness",
+            "Defines whether the machine runs in big or little endian mode");
 }
 
 static const TypeInfo petalogix_s3adsp1800_machine_types[] = {
-- 
2.49.0
Re: [PATCH 1/4] hw/microblaze: Add endianness property to the petalogix_s3adsp1800 machine
Posted by Philippe Mathieu-Daudé 6 months, 3 weeks ago
On 15/5/25 15:20, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> Since the microblaze target can now handle both endianness, big and
> little, we should provide a config knob for the user to select the
> desired endianness.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/microblaze/petalogix_s3adsp1800_mmu.c | 41 +++++++++++++++++++++---
>   1 file changed, 36 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Re: [PATCH 1/4] hw/microblaze: Add endianness property to the petalogix_s3adsp1800 machine
Posted by Richard Henderson 6 months, 3 weeks ago
On 5/15/25 14:20, Thomas Huth wrote:
> +static int machine_get_endianness(Object *obj, Error **errp G_GNUC_UNUSED)
> +{
> +    S3Adsp1800MachineState *ms = PETALOGIX_S3ADSP1800_MACHINE(obj);
> +    return ms->endianness;
> +}
> +
> +static void machine_set_endianness(Object *obj, int endianness, Error **errp)
> +{
> +    S3Adsp1800MachineState *ms = PETALOGIX_S3ADSP1800_MACHINE(obj);
> +    ms->endianness = endianness;
> +}
> +
>   static void petalogix_s3adsp1800_machine_class_init(ObjectClass *oc,
>                                                       const void *data)
>   {
>       MachineClass *mc = MACHINE_CLASS(oc);
> +    ObjectProperty *prop;
>   
>       mc->desc = "PetaLogix linux refdesign for xilinx Spartan 3ADSP1800";
>       mc->init = petalogix_s3adsp1800_init;
>       mc->is_default = true;
> +
> +    prop = object_class_property_add_enum(oc, "endianness", "EndianMode",
> +                                          &EndianMode_lookup,
> +                                          machine_get_endianness,
> +                                          machine_set_endianness);
> +    object_property_set_default_str(prop, TARGET_BIG_ENDIAN ? "big" : "little");
> +    object_class_property_set_description(oc, "endianness",
> +            "Defines whether the machine runs in big or little endian mode");


Better with Property?  You don't have to write get/set...

   static const Property props[] = {
     DEFINE_PROP_ENDIAN("endianness", S3Adsp1800MachineState, endianness,
                        TARGET_BIG_ENDIAN ? ENDIAN_MODE_BIG : ENDIAN_MODE_LITTLE),
   };

   device_class_set_props(dc, props);


r~
Re: [PATCH 1/4] hw/microblaze: Add endianness property to the petalogix_s3adsp1800 machine
Posted by Philippe Mathieu-Daudé 6 months, 3 weeks ago
+Markus

On 24/5/25 13:55, Richard Henderson wrote:
> On 5/15/25 14:20, Thomas Huth wrote:
>> +static int machine_get_endianness(Object *obj, Error **errp 
>> G_GNUC_UNUSED)
>> +{
>> +    S3Adsp1800MachineState *ms = PETALOGIX_S3ADSP1800_MACHINE(obj);
>> +    return ms->endianness;
>> +}
>> +
>> +static void machine_set_endianness(Object *obj, int endianness, Error 
>> **errp)
>> +{
>> +    S3Adsp1800MachineState *ms = PETALOGIX_S3ADSP1800_MACHINE(obj);
>> +    ms->endianness = endianness;
>> +}
>> +
>>   static void petalogix_s3adsp1800_machine_class_init(ObjectClass *oc,
>>                                                       const void *data)
>>   {
>>       MachineClass *mc = MACHINE_CLASS(oc);
>> +    ObjectProperty *prop;
>>       mc->desc = "PetaLogix linux refdesign for xilinx Spartan 
>> 3ADSP1800";
>>       mc->init = petalogix_s3adsp1800_init;
>>       mc->is_default = true;
>> +
>> +    prop = object_class_property_add_enum(oc, "endianness", 
>> "EndianMode",
>> +                                          &EndianMode_lookup,
>> +                                          machine_get_endianness,
>> +                                          machine_set_endianness);
>> +    object_property_set_default_str(prop, TARGET_BIG_ENDIAN ? "big" : 
>> "little");
>> +    object_class_property_set_description(oc, "endianness",
>> +            "Defines whether the machine runs in big or little endian 
>> mode");
> 
> 
> Better with Property?  You don't have to write get/set...
> 
>    static const Property props[] = {
>      DEFINE_PROP_ENDIAN("endianness", S3Adsp1800MachineState, endianness,
>                         TARGET_BIG_ENDIAN ? ENDIAN_MODE_BIG : 
> ENDIAN_MODE_LITTLE),
>    };
> 
>    device_class_set_props(dc, props);

DEFINE_PROP_FOO() are restricted to QDev (DeviceClass). Here we have
a MachineClass, which only inherits ObjectClass, not DeviceClass.

Markus once explained me the difference between QDev properties
and bare object ones; I asked why we couldn't make qdev properties
generic to objects, but I don't remember the historical rationale.
QDev predates QOM, QDev used static properties, QOM introduced
dynamic ones? We definitively should document that...

Re: [PATCH 1/4] hw/microblaze: Add endianness property to the petalogix_s3adsp1800 machine
Posted by Markus Armbruster 6 months, 3 weeks ago
Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> +Markus
>
> On 24/5/25 13:55, Richard Henderson wrote:
>> On 5/15/25 14:20, Thomas Huth wrote:
>>> +static int machine_get_endianness(Object *obj, Error **errp G_GNUC_UNUSED)
>>> +{
>>> +    S3Adsp1800MachineState *ms = PETALOGIX_S3ADSP1800_MACHINE(obj);
>>> +    return ms->endianness;
>>> +}
>>> +
>>> +static void machine_set_endianness(Object *obj, int endianness, Error **errp)
>>> +{
>>> +    S3Adsp1800MachineState *ms = PETALOGIX_S3ADSP1800_MACHINE(obj);
>>> +    ms->endianness = endianness;
>>> +}
>>> +
>>>   static void petalogix_s3adsp1800_machine_class_init(ObjectClass *oc,
>>>                                                       const void *data)
>>>   {
>>>       MachineClass *mc = MACHINE_CLASS(oc);
>>> +    ObjectProperty *prop;
>>>       mc->desc = "PetaLogix linux refdesign for xilinx Spartan 3ADSP1800";
>>>       mc->init = petalogix_s3adsp1800_init;
>>>       mc->is_default = true;
>>> +
>>> +    prop = object_class_property_add_enum(oc, "endianness", "EndianMode",
>>> +                                          &EndianMode_lookup,
>>> +                                          machine_get_endianness,
>>> +                                          machine_set_endianness);
>>> +    object_property_set_default_str(prop, TARGET_BIG_ENDIAN ? "big" : "little");
>>> +    object_class_property_set_description(oc, "endianness",
>>> +            "Defines whether the machine runs in big or little endian mode");
>> Better with Property?  You don't have to write get/set...
>>    static const Property props[] = {
>>      DEFINE_PROP_ENDIAN("endianness", S3Adsp1800MachineState, endianness,
>>                         TARGET_BIG_ENDIAN ? ENDIAN_MODE_BIG : ENDIAN_MODE_LITTLE),
>>    };
>>    device_class_set_props(dc, props);
>
> DEFINE_PROP_FOO() are restricted to QDev (DeviceClass). Here we have
> a MachineClass, which only inherits ObjectClass, not DeviceClass.
>
> Markus once explained me the difference between QDev properties
> and bare object ones; I asked why we couldn't make qdev properties
> generic to objects, but I don't remember the historical rationale.
> QDev predates QOM, QDev used static properties, QOM introduced
> dynamic ones? We definitively should document that...

Yes.

Qdev properties are defined in data at compile time, and are connected
to the device class.  Specifically, a DeviceClass has an array of
Property, where each element specifies a property of its DeviceState
instances.  The array never changes.

The DEFINE_PROP_FOO() macros expand into Property literals suitable for
the array.  Boilerplate is relatively light: no need to write setters or
getters unless you define a new FOO.

QOM properties are defined in code at runtime, and are connected to the
Object, i.e. the instance, not the class.  They should be created in
.instance_init(), but nothing prevents creation elsewhere.  Most of the
time, we create the exact same properties for all instances of an
ObjectClass, but not always.

Defining properties at runtime is more flexible than defining them in
data at compile time.  However:

* Static data is much easier to reason about than behavior of code.
  Qdev properties are statically known.  device_add FOO,help can
  reliably show them: it dumps the unchanging array.  QOM properties can
  be different each time you create an object.  device_add FOO,help
  needs to create a temporary instance, and dumps whatever properties
  this instance got.  The next instance may get different ones.

* The property descriptions are duplicated in every instance, which is a
  waste of space.  See "QOM class properties" below.

* The functions to create properties take getter and setter functions as
  arguments.  Functions you get to write basically for each property.
  Much more boilerplate than with qdev.

I challenged the need for this much flexibility at the time, without
success.  We actually use the flexibility only rarely.  I believe this
aspect of QOM's design was a mistake.

QOM actually has a second kind of property: QOM class properties are
also defined in code at runtime, but connected to the ObjectClass.  They
should be created in .class_init(), but nothing prevents creation
elsewhere.  Despite their name, they are properties of the instance,
just like ordinary QOM properties.  The difference is only the sharing.

Most properties should be class properties, but aren't.  This is because
class properties were added late, and are underdocumented.

Qdev is actually a leaky layer above QOM.  I became one when we rebased
it on top of QOM.

A qdev's .instance_init() iterates over the property array and adds a
QOM class property for each element[*].

This Property part of qdev isn't actually device-specific.  We could
lift it into Object.  But would that be an improvement?  I'm not sure;
QOM is confusing enough as it is.

>               We definitively should document that...

I know just enough about QOM to be dangerous :)



[*] It also adds a "legacy property", but I forgot what that is, and why
it exists.