Re: [PATCH v6 01/11] sysemu/tcg: Introduce tcg_builtin() helper

[Claudio Fontana]

On 1/31/21 4:23 PM, Philippe Mathieu-Daudé wrote:
> On 1/31/21 3:18 PM, Claudio Fontana wrote:
>> On 1/31/21 12:50 PM, Philippe Mathieu-Daudé wrote:
>>> Modules are registered early with type_register_static().
>>>
>>> We would like to call tcg_enabled() when registering QOM types,
>>
>>
>> Hi Philippe,
>>
>> could this not be controlled by meson at this stage?
>> On X86, I register the tcg-specific types in tcg/* in modules that are only 
>> built for TCG.
>>
>> Maybe tcg_builtin() is useful anyway, thinking long term at loadable modules,
>> but there we are interested in whether tcg code is available or not, 
>> regardless of whether it's builtin,
>> or needs to be loaded via a .so plugin..
>>
>> maybe tcg_available()?
> 
> The alternatives I found:
> 
> - reorder things in vl.c?
> 
> - use ugly #ifdef'ry, see this patch:
>   https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg08037.html

Not sure it's that ugly,
if it is followed (or replaced by) exporting those pieces to separate files, 
which are only built by meson on CONFIG_TCG.

I did not try to do it, so you know best of course.

Ciao,

Claudio

> 
> - this earlier approach I previously discarded:
> 
> -- >8 --
> diff --git a/include/qom/object.h b/include/qom/object.h
> index d378f13a116..30590c6fac3 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -403,9 +403,12 @@ struct Object
>   *   parent class initialization has occurred, but before the class itself
>   *   is initialized.  This is the function to use to undo the effects of
>   *   memcpy from the parent class to the descendants.
> - * @class_data: Data to pass to the @class_init,
> + * @class_data: Data to pass to the @class_registerable, @class_init,
>   *   @class_base_init. This can be useful when building dynamic
>   *   classes.
> + * @registerable: This function is called when modules are registered,
> + *   prior to any class initialization. When present and returning %false,
> + *   the type is not registered, the class is not present (not usable).
>   * @interfaces: The list of interfaces associated with this type.  This
>   *   should point to a static array that's terminated with a zero filled
>   *   element.
> @@ -428,6 +431,7 @@ struct TypeInfo
>      void (*class_base_init)(ObjectClass *klass, void *data);
>      void *class_data;
> 
> +    bool (*registerable)(void *data);
>      InterfaceInfo *interfaces;
>  };
> 
> diff --git a/qom/object.c b/qom/object.c
> index 2fa0119647c..0febaffa12e 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -138,6 +138,10 @@ static TypeImpl *type_new(const TypeInfo *info)
>  static TypeImpl *type_register_internal(const TypeInfo *info)
>  {
>      TypeImpl *ti;
> +
> +    if (info->registerable && !info->registerable(info->class_data)) {
> +        return NULL;
> +    }
>      ti = type_new(info);
> 
>      type_table_add(ti);
> 
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 990509d3852..1a2b1889da4 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -24,6 +24,7 @@
>  #include "hw/loader.h"
>  #include "hw/arm/boot.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/tcg.h"
>  #include "qom/object.h"
> 
>  #define SMPBOOT_ADDR    0x300 /* this should leave enough space for
> ATAGS */
> @@ -368,18 +369,26 @@ static void raspi3b_machine_class_init(ObjectClass
> *oc, void *data)
>  };
>  #endif /* TARGET_AARCH64 */
> 
> +static bool raspi_machine_requiring_tcg_accel(void *data)
> +{
> +    return tcg_builtin();
> +}
> +
>  static const TypeInfo raspi_machine_types[] = {
>      {
>          .name           = MACHINE_TYPE_NAME("raspi0"),
>          .parent         = TYPE_RASPI_MACHINE,
> +        .registerable   = raspi_machine_requiring_tcg_accel,
>          .class_init     = raspi0_machine_class_init,
>      }, {
>          .name           = MACHINE_TYPE_NAME("raspi1ap"),
>          .parent         = TYPE_RASPI_MACHINE,
> +        .registerable   = raspi_machine_requiring_tcg_accel,
>          .class_init     = raspi1ap_machine_class_init,
>      }, {
>          .name           = MACHINE_TYPE_NAME("raspi2b"),
>          .parent         = TYPE_RASPI_MACHINE,
> +        .registerable   = raspi_machine_requiring_tcg_accel,
>          .class_init     = raspi2b_machine_class_init,
>  #ifdef TARGET_AARCH64
>      }, {
> ---
> 
>>
>> Ciao,
>>
>> Claudio
>>
>>> but tcg_enabled() returns tcg_allowed which is a runtime property
>>> initialized later (See commit 2f181fbd5a9 which introduced the
>>> MachineInitPhase in "hw/qdev-core.h" representing the different
>>> phases of machine initialization and commit 0427b6257e2 which
>>> document the initialization order).
>>>
>>> As we are only interested if the TCG accelerator is builtin,
>>> regardless of being enabled, introduce the tcg_builtin() helper.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>> Cc: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  include/sysemu/tcg.h | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/include/sysemu/tcg.h b/include/sysemu/tcg.h
>>> index 00349fb18a7..6ac5c2ca89d 100644
>>> --- a/include/sysemu/tcg.h
>>> +++ b/include/sysemu/tcg.h
>>> @@ -13,8 +13,10 @@ void tcg_exec_init(unsigned long tb_size, int splitwx);
>>>  #ifdef CONFIG_TCG
>>>  extern bool tcg_allowed;
>>>  #define tcg_enabled() (tcg_allowed)
>>> +#define tcg_builtin() 1
>>>  #else
>>>  #define tcg_enabled() 0
>>> +#define tcg_builtin() 0
>>>  #endif
>>>  
>>>  #endif
>>>
>>
>