| Message ID | 20250403234914.9154-20-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | qemu: Introduce TargetInfo API (for single binary) | expand |
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. >> >
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); +} + 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 (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; + } if (mc->alias) { printf("%-20s %s (alias of %s)\n", mc->alias, mc->desc, mc->name); }
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- system/vl.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)