Re: [RFC PATCH 2/4] hw/arm/raspi: Replace TARGET_AARCH64 by legacy_binary_is_64bit()

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Mar 05, 2025 at 05:12:46PM +0100, Philippe Mathieu-Daudé wrote:
> For legacy ARM binaries, legacy_binary_is_64bit() is
> equivalent of the compile time TARGET_AARCH64 definition.
> 
> Use it as TypeInfo::registerable() callback to dynamically
> add Aarch64 specific types in qemu-system-aarch64 binary,
> removing the need of TARGET_AARCH64 #ifdef'ry.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/bcm2836.c | 6 ++----
>  hw/arm/raspi.c   | 7 +++----
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
> index 95e16806fa1..88a32e5fc20 100644
> --- a/hw/arm/bcm2836.c
> +++ b/hw/arm/bcm2836.c


> @@ -219,12 +218,11 @@ static const TypeInfo bcm283x_types[] = {
>          .name           = TYPE_BCM2836,
>          .parent         = TYPE_BCM283X,
>          .class_init     = bcm2836_class_init,
> -#ifdef TARGET_AARCH64
>      }, {
>          .name           = TYPE_BCM2837,
>          .parent         = TYPE_BCM283X,
> +        .registerable   = legacy_binary_is_64bit,
>          .class_init     = bcm2837_class_init,
> -#endif
>      }, {
>          .name           = TYPE_BCM283X,
>          .parent         = TYPE_BCM283X_BASE,

So historically we have a subset of machines that are only exposed in
the qemu-system-aarch64 binary, and not qemu-system-arm.

You're attempting to build a single binary to cover both 32 & 64 bit
arm, so need to be able to filter what machines are available to
create when the symlink indicates invokation of the 32-bit binary.

If we extend your approach into the future, we may eventually have
a qemu-system-any with all targets, and symlinks from the legacy
binary names, so need to filter based on target as well as on
32/64-bit.


The reason you want the .registerable callback is so that we can
have a single static list of all machine types passed into
DEFINE_TYPES in bulk, instead of implementing a manual constructor
to do type registration and conditionally calling type_register()
for each type.

So I can understand why you took this approach, but conceptually I'm not
comfortable with the semantics of 'type_register' being a method that
registers a type, except when it doesn't register a type. That feels
like dubious semantic design.

If I step back a level, I would ask why do we need to skip the registration
of machine types at all ?

It is because existing code blindly assumes that if a machine class is
registered, then it is valid to instantiate that machine class. IMHO this
is where the current design limitation is.

No matter what we do the code will exist in  memory in the binary. If we
register a type that's a tiny bit of memory to hold the "Class" instance
and ought to be logically harmless if we never instantiate a type.

IMHO we should always unconditionally register all types for which we
have code in memory. If there are types like machine types, where some
Classes should be blocked from instantiation based on certain criteria,
we should instead represent that with a check at that class level in
some manner.

i.e, we could have a helper

    bool machine_is_available(MachineClass *mc)

which we use as a filter when querying machine types that are available to
instantiate (-M help, query-machines), and as a pre-condition prior to
instiating a machine type (-M <name>).

That may in turn imply a callback

   bool (*is_available)(void)

on the MachineClass, for which your legacy_binary_is_64bit() method would
serve as an impl.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|