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

[Markus Armbruster <armbru@redhat.com>]

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.