[RFC,PATCH-for-10.1,19/19] system/vl: Filter machine list for binary using machine_binary_filter()

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 system/vl.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

On 4/3/25 16:49, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   system/vl.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/system/vl.c b/system/vl.c
> index d8a0fe713c9..554f5f2a467 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -27,6 +27,8 @@
>   #include "qemu/datadir.h"
>   #include "qemu/units.h"
>   #include "qemu/module.h"
> +#include "qemu/target_info.h"
> +#include "qemu/target_info-qom.h"
>   #include "exec/cpu-common.h"
>   #include "exec/page-vary.h"
>   #include "hw/qdev-properties.h"
> @@ -833,14 +835,29 @@ static bool usb_parse(const char *cmdline, Error **errp)
>   /***********************************************************/
>   /* machine registration */
>   
> +static char *machine_binary_filter(void)
> +{
> +    if (target_info_is_stub()) {
> +        return NULL;
> +    }
> +    return g_strconcat(TYPE_LEGACY_BINARY_PREFIX,
> +                       "qemu-system-", target_name(), NULL);

No, we should not have such things.
We can make it work with proper QOM types, defined by target, instead of 
relying on string construction/compare like this.

> +}
> +
>   static MachineClass *find_machine(const char *name, GSList *machines)
>   {
>       GSList *el;
> +    g_autofree char *binary_filter = machine_binary_filter();
>   
>       for (el = machines; el; el = el->next) {
>           MachineClass *mc = el->data;
>   
>           if (!strcmp(mc->name, name) || !g_strcmp0(mc->alias, name)) {
> +            if (binary_filter && !object_class_dynamic_cast(el->data,
> +                                                            binary_filter)) {
> +                /* Machine is not for this binary: fail */
> +                return NULL;
> +            }
>               return mc;
>           }
>       }
> @@ -1563,6 +1580,7 @@ static void machine_help_func(const QDict *qdict)
>       g_autoptr(GSList) machines = NULL;
>       GSList *el;
>       const char *type = qdict_get_try_str(qdict, "type");
> +    g_autofree char *binary_filter = machine_binary_filter();
>   
>       machines = object_class_get_list(TYPE_MACHINE, false);

If we define a proper TYPE_TARGET_MACHINE per target, and we add this to 
TargetInfo, this can become:

machines = object_class_get_list(target_machine_type(), false);

And we don't need any other string hack to detect what is the correct type.

>       if (type) {
> @@ -1577,6 +1595,12 @@ static void machine_help_func(const QDict *qdict)
>       machines = g_slist_sort(machines, machine_class_cmp);
>       for (el = machines; el; el = el->next) {
>           MachineClass *mc = el->data;
> +
> +        if (binary_filter && !object_class_dynamic_cast(el->data,
> +                                                        binary_filter)) {
> +            /* Machine is not for this binary: skip */
> +            continue;
> +        }

With the approach above, this is not needed anymore.

>           if (mc->alias) {
>               printf("%-20s %s (alias of %s)\n", mc->alias, mc->desc, mc->name);
>           }

I think we are missing a commit here, defining a proper 
TYPE_TARGET_MACHINE_PREFIX, that is target dependent, instead of the 
TYPE_LEGACY_BINARY_PREFIX.

And we should include in this type in TargetInfo, the same way it was 
done for cpus.

Hi Pierrick,

On 4/4/25 19:10, Pierrick Bouvier wrote:
> On 4/3/25 16:49, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   system/vl.c | 24 ++++++++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/system/vl.c b/system/vl.c
>> index d8a0fe713c9..554f5f2a467 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -27,6 +27,8 @@
>>   #include "qemu/datadir.h"
>>   #include "qemu/units.h"
>>   #include "qemu/module.h"
>> +#include "qemu/target_info.h"
>> +#include "qemu/target_info-qom.h"
>>   #include "exec/cpu-common.h"
>>   #include "exec/page-vary.h"
>>   #include "hw/qdev-properties.h"
>> @@ -833,14 +835,29 @@ static bool usb_parse(const char *cmdline, Error 
>> **errp)
>>   /***********************************************************/
>>   /* machine registration */
>> +static char *machine_binary_filter(void)
>> +{
>> +    if (target_info_is_stub()) {
>> +        return NULL;
>> +    }
>> +    return g_strconcat(TYPE_LEGACY_BINARY_PREFIX,
>> +                       "qemu-system-", target_name(), NULL);
> 
> No, we should not have such things.
> We can make it work with proper QOM types, defined by target, instead of 
> relying on string construction/compare like this.

I am not understanding you, do you mind sharing code snippets of what
you have in mind?

> 
>> +}
>> +
>>   static MachineClass *find_machine(const char *name, GSList *machines)
>>   {
>>       GSList *el;
>> +    g_autofree char *binary_filter = machine_binary_filter();
>>       for (el = machines; el; el = el->next) {
>>           MachineClass *mc = el->data;
>>           if (!strcmp(mc->name, name) || !g_strcmp0(mc->alias, name)) {
>> +            if (binary_filter && !object_class_dynamic_cast(el->data,
>> +                                                            
>> binary_filter)) {
>> +                /* Machine is not for this binary: fail */
>> +                return NULL;
>> +            }
>>               return mc;
>>           }
>>       }
>> @@ -1563,6 +1580,7 @@ static void machine_help_func(const QDict *qdict)
>>       g_autoptr(GSList) machines = NULL;
>>       GSList *el;
>>       const char *type = qdict_get_try_str(qdict, "type");
>> +    g_autofree char *binary_filter = machine_binary_filter();
>>       machines = object_class_get_list(TYPE_MACHINE, false);
> 
> If we define a proper TYPE_TARGET_MACHINE per target, and we add this to 
> TargetInfo, this can become:
> 
> machines = object_class_get_list(target_machine_type(), false);
> 
> And we don't need any other string hack to detect what is the correct type.
> 
>>       if (type) {
>> @@ -1577,6 +1595,12 @@ static void machine_help_func(const QDict *qdict)
>>       machines = g_slist_sort(machines, machine_class_cmp);
>>       for (el = machines; el; el = el->next) {
>>           MachineClass *mc = el->data;
>> +
>> +        if (binary_filter && !object_class_dynamic_cast(el->data,
>> +                                                        
>> binary_filter)) {
>> +            /* Machine is not for this binary: skip */
>> +            continue;
>> +        }
> 
> With the approach above, this is not needed anymore.
> 
>>           if (mc->alias) {
>>               printf("%-20s %s (alias of %s)\n", mc->alias, mc->desc, 
>> mc->name);
>>           }
> 
> I think we are missing a commit here, defining a proper 
> TYPE_TARGET_MACHINE_PREFIX, that is target dependent, instead of the 
> TYPE_LEGACY_BINARY_PREFIX.
> 
> And we should include in this type in TargetInfo, the same way it was 
> done for cpus.

On 4/4/25 11:01, Philippe Mathieu-Daudé wrote:
> Hi Pierrick,
> 
> On 4/4/25 19:10, Pierrick Bouvier wrote:
>> On 4/3/25 16:49, Philippe Mathieu-Daudé wrote:
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>    system/vl.c | 24 ++++++++++++++++++++++++
>>>    1 file changed, 24 insertions(+)
>>>
>>> diff --git a/system/vl.c b/system/vl.c
>>> index d8a0fe713c9..554f5f2a467 100644
>>> --- a/system/vl.c
>>> +++ b/system/vl.c
>>> @@ -27,6 +27,8 @@
>>>    #include "qemu/datadir.h"
>>>    #include "qemu/units.h"
>>>    #include "qemu/module.h"
>>> +#include "qemu/target_info.h"
>>> +#include "qemu/target_info-qom.h"
>>>    #include "exec/cpu-common.h"
>>>    #include "exec/page-vary.h"
>>>    #include "hw/qdev-properties.h"
>>> @@ -833,14 +835,29 @@ static bool usb_parse(const char *cmdline, Error
>>> **errp)
>>>    /***********************************************************/
>>>    /* machine registration */
>>> +static char *machine_binary_filter(void)
>>> +{
>>> +    if (target_info_is_stub()) {
>>> +        return NULL;
>>> +    }
>>> +    return g_strconcat(TYPE_LEGACY_BINARY_PREFIX,
>>> +                       "qemu-system-", target_name(), NULL);
>>
>> No, we should not have such things.
>> We can make it work with proper QOM types, defined by target, instead of
>> relying on string construction/compare like this.
> 
> I am not understanding you, do you mind sharing code snippets of what
> you have in mind?
> 

Instead of the current and previous patch,

we define TYPE_TARGET_MACHINE_PREFIX.

For each target, we define a specific TYPE_TARGET_MACHINE variant, like:
- TYPE_TARGET_MACHINE_ARM
- TYPE_TARGET_MACHINE_AARCH64
...

In TargetInfo, we add a new function target_machine_type(), that returns 
this type, specialized for each architecture.
As a first step, the stub implementation can return TYPE_MACHINE, and we 
can enable this architecture per architecture later.

For the first architecture implementation, arm, we will define 
TYPE_TARGET_MACHINE_ARM, and TYPE_TARGET_MACHINE_AARCH64, which will 
allow concerned files to be common, while still maintaining a specific 
set of machines per target.

Is that more clear?

>>
>>> +}
>>> +
>>>    static MachineClass *find_machine(const char *name, GSList *machines)
>>>    {
>>>        GSList *el;
>>> +    g_autofree char *binary_filter = machine_binary_filter();
>>>        for (el = machines; el; el = el->next) {
>>>            MachineClass *mc = el->data;
>>>            if (!strcmp(mc->name, name) || !g_strcmp0(mc->alias, name)) {
>>> +            if (binary_filter && !object_class_dynamic_cast(el->data,
>>> +
>>> binary_filter)) {
>>> +                /* Machine is not for this binary: fail */
>>> +                return NULL;
>>> +            }
>>>                return mc;
>>>            }
>>>        }
>>> @@ -1563,6 +1580,7 @@ static void machine_help_func(const QDict *qdict)
>>>        g_autoptr(GSList) machines = NULL;
>>>        GSList *el;
>>>        const char *type = qdict_get_try_str(qdict, "type");
>>> +    g_autofree char *binary_filter = machine_binary_filter();
>>>        machines = object_class_get_list(TYPE_MACHINE, false);
>>
>> If we define a proper TYPE_TARGET_MACHINE per target, and we add this to
>> TargetInfo, this can become:
>>
>> machines = object_class_get_list(target_machine_type(), false);
>>
>> And we don't need any other string hack to detect what is the correct type.
>>
>>>        if (type) {
>>> @@ -1577,6 +1595,12 @@ static void machine_help_func(const QDict *qdict)
>>>        machines = g_slist_sort(machines, machine_class_cmp);
>>>        for (el = machines; el; el = el->next) {
>>>            MachineClass *mc = el->data;
>>> +
>>> +        if (binary_filter && !object_class_dynamic_cast(el->data,
>>> +
>>> binary_filter)) {
>>> +            /* Machine is not for this binary: skip */
>>> +            continue;
>>> +        }
>>
>> With the approach above, this is not needed anymore.
>>
>>>            if (mc->alias) {
>>>                printf("%-20s %s (alias of %s)\n", mc->alias, mc->desc,
>>> mc->name);
>>>            }
>>
>> I think we are missing a commit here, defining a proper
>> TYPE_TARGET_MACHINE_PREFIX, that is target dependent, instead of the
>> TYPE_LEGACY_BINARY_PREFIX.
>>
>> And we should include in this type in TargetInfo, the same way it was
>> done for cpus.
>

On 4/4/25 11:08, Pierrick Bouvier wrote:
> On 4/4/25 11:01, Philippe Mathieu-Daudé wrote:
>> Hi Pierrick,
>>
>> On 4/4/25 19:10, Pierrick Bouvier wrote:
>>> On 4/3/25 16:49, Philippe Mathieu-Daudé wrote:
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>     system/vl.c | 24 ++++++++++++++++++++++++
>>>>     1 file changed, 24 insertions(+)
>>>>
>>>> diff --git a/system/vl.c b/system/vl.c
>>>> index d8a0fe713c9..554f5f2a467 100644
>>>> --- a/system/vl.c
>>>> +++ b/system/vl.c
>>>> @@ -27,6 +27,8 @@
>>>>     #include "qemu/datadir.h"
>>>>     #include "qemu/units.h"
>>>>     #include "qemu/module.h"
>>>> +#include "qemu/target_info.h"
>>>> +#include "qemu/target_info-qom.h"
>>>>     #include "exec/cpu-common.h"
>>>>     #include "exec/page-vary.h"
>>>>     #include "hw/qdev-properties.h"
>>>> @@ -833,14 +835,29 @@ static bool usb_parse(const char *cmdline, Error
>>>> **errp)
>>>>     /***********************************************************/
>>>>     /* machine registration */
>>>> +static char *machine_binary_filter(void)
>>>> +{
>>>> +    if (target_info_is_stub()) {
>>>> +        return NULL;
>>>> +    }
>>>> +    return g_strconcat(TYPE_LEGACY_BINARY_PREFIX,
>>>> +                       "qemu-system-", target_name(), NULL);
>>>
>>> No, we should not have such things.
>>> We can make it work with proper QOM types, defined by target, instead of
>>> relying on string construction/compare like this.
>>
>> I am not understanding you, do you mind sharing code snippets of what
>> you have in mind?
>>
> 
> Instead of the current and previous patch,
> 
> we define TYPE_TARGET_MACHINE_PREFIX.
> 
> For each target, we define a specific TYPE_TARGET_MACHINE variant, like:
> - TYPE_TARGET_MACHINE_ARM
> - TYPE_TARGET_MACHINE_AARCH64
> ...
> 
> In TargetInfo, we add a new function target_machine_type(), that returns
> this type, specialized for each architecture.
> As a first step, the stub implementation can return TYPE_MACHINE, and we
> can enable this architecture per architecture later.
> 
> For the first architecture implementation, arm, we will define
> TYPE_TARGET_MACHINE_ARM, and TYPE_TARGET_MACHINE_AARCH64, which will
> allow concerned files to be common, while still maintaining a specific
> set of machines per target.
> 

Note: Those TYPE_TARGET_MACHINE_* types are QOM interfaces, that every 
concerned machine implements.

Once things are done this way, the only required change is:
- machines = object_class_get_list(TYPE_MACHINE, false);
+ machines = object_class_get_list(target_machine_type(), false);

As a further step, it will be very easy to support having multiple 
targets enabled at the same time (build a list of machine types instead 
of a single one), but we can do this *later* when tackling heterogeneous 
emulation.

> Is that more clear?
> 
>>>
>>>> +}
>>>> +
>>>>     static MachineClass *find_machine(const char *name, GSList *machines)
>>>>     {
>>>>         GSList *el;
>>>> +    g_autofree char *binary_filter = machine_binary_filter();
>>>>         for (el = machines; el; el = el->next) {
>>>>             MachineClass *mc = el->data;
>>>>             if (!strcmp(mc->name, name) || !g_strcmp0(mc->alias, name)) {
>>>> +            if (binary_filter && !object_class_dynamic_cast(el->data,
>>>> +
>>>> binary_filter)) {
>>>> +                /* Machine is not for this binary: fail */
>>>> +                return NULL;
>>>> +            }
>>>>                 return mc;
>>>>             }
>>>>         }
>>>> @@ -1563,6 +1580,7 @@ static void machine_help_func(const QDict *qdict)
>>>>         g_autoptr(GSList) machines = NULL;
>>>>         GSList *el;
>>>>         const char *type = qdict_get_try_str(qdict, "type");
>>>> +    g_autofree char *binary_filter = machine_binary_filter();
>>>>         machines = object_class_get_list(TYPE_MACHINE, false);
>>>
>>> If we define a proper TYPE_TARGET_MACHINE per target, and we add this to
>>> TargetInfo, this can become:
>>>
>>> machines = object_class_get_list(target_machine_type(), false);
>>>
>>> And we don't need any other string hack to detect what is the correct type.
>>>
>>>>         if (type) {
>>>> @@ -1577,6 +1595,12 @@ static void machine_help_func(const QDict *qdict)
>>>>         machines = g_slist_sort(machines, machine_class_cmp);
>>>>         for (el = machines; el; el = el->next) {
>>>>             MachineClass *mc = el->data;
>>>> +
>>>> +        if (binary_filter && !object_class_dynamic_cast(el->data,
>>>> +
>>>> binary_filter)) {
>>>> +            /* Machine is not for this binary: skip */
>>>> +            continue;
>>>> +        }
>>>
>>> With the approach above, this is not needed anymore.
>>>
>>>>             if (mc->alias) {
>>>>                 printf("%-20s %s (alias of %s)\n", mc->alias, mc->desc,
>>>> mc->name);
>>>>             }
>>>
>>> I think we are missing a commit here, defining a proper
>>> TYPE_TARGET_MACHINE_PREFIX, that is target dependent, instead of the
>>> TYPE_LEGACY_BINARY_PREFIX.
>>>
>>> And we should include in this type in TargetInfo, the same way it was
>>> done for cpus.
>>
>