| Message ID | 20250424222112.36194-9-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | single-binary: Make hw/arm/ common | expand |
On 4/24/25 15:20, Philippe Mathieu-Daudé wrote: > A machine defined with the DEFINE_MACHINE_ARM_AARCH64() macro > will be available on qemu-system-arm and qemu-system-aarch64 > binaries. > > One defined with DEFINE_MACHINE_AARCH64() will only be available > in the qemu-system-aarch64 binary. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > include/hw/arm/machines-qom.h | 13 +++++++++++++ > target/arm/machine.c | 12 ++++++++++++ > 2 files changed, 25 insertions(+) > I won't block this change as we need to move on, but I still consider we do a bad compromise between code readability/grepability, to avoid a code size increase of +0.0005%. Anyway, we can always change that later when adding a second architecture. Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
On 25/4/25 00:35, Pierrick Bouvier wrote: > On 4/24/25 15:20, Philippe Mathieu-Daudé wrote: >> A machine defined with the DEFINE_MACHINE_ARM_AARCH64() macro >> will be available on qemu-system-arm and qemu-system-aarch64 >> binaries. >> >> One defined with DEFINE_MACHINE_AARCH64() will only be available >> in the qemu-system-aarch64 binary. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> include/hw/arm/machines-qom.h | 13 +++++++++++++ >> target/arm/machine.c | 12 ++++++++++++ >> 2 files changed, 25 insertions(+) >> > > I won't block this change as we need to move on, but I still consider we > do a bad compromise between code readability/grepability, to avoid a > code size increase of +0.0005%. I know, I'm just trying to keep moving while keeping everybody happy (there was no further update on your v4 comments). > Anyway, we can always change that later when adding a second architecture. I expect this to not change for (current) homogeneous machines. Heterogeneous machines won't use these fixed arrays; there I expect compound literals to be more useful. > Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> Thanks!
On Fri, 25 Apr 2025, Philippe Mathieu-Daudé wrote: > A machine defined with the DEFINE_MACHINE_ARM_AARCH64() macro > will be available on qemu-system-arm and qemu-system-aarch64 > binaries. > > One defined with DEFINE_MACHINE_AARCH64() will only be available > in the qemu-system-aarch64 binary. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > include/hw/arm/machines-qom.h | 13 +++++++++++++ > target/arm/machine.c | 12 ++++++++++++ > 2 files changed, 25 insertions(+) > > diff --git a/include/hw/arm/machines-qom.h b/include/hw/arm/machines-qom.h > index a17225f5f92..6277ee986d9 100644 > --- a/include/hw/arm/machines-qom.h > +++ b/include/hw/arm/machines-qom.h > @@ -9,10 +9,23 @@ > #ifndef HW_ARM_MACHINES_QOM_H > #define HW_ARM_MACHINES_QOM_H > > +#include "hw/boards.h" > + > #define TYPE_TARGET_ARM_MACHINE \ > "target-info-arm-machine" > > #define TYPE_TARGET_AARCH64_MACHINE \ > "target-info-aarch64-machine" > > +extern InterfaceInfo arm_aarch64_machine_interfaces[]; > +extern InterfaceInfo aarch64_machine_interfaces[]; > + > +#define DEFINE_MACHINE_ARM_AARCH64(namestr, machine_initfn) \ > + DEFINE_MACHINE_WITH_INTERFACES(namestr, machine_initfn, \ > + arm_aarch64_machine_interfaces) > + > +#define DEFINE_MACHINE_AARCH64(namestr, machine_initfn) \ > + DEFINE_MACHINE_WITH_INTERFACES(namestr, machine_initfn, \ > + aarch64_machine_interfaces) > + > #endif > diff --git a/target/arm/machine.c b/target/arm/machine.c > index 978249fb71b..193c7a9cff0 100644 > --- a/target/arm/machine.c > +++ b/target/arm/machine.c > @@ -8,6 +8,7 @@ > #include "cpu-features.h" > #include "migration/cpu.h" > #include "target/arm/gtimer.h" > +#include "hw/arm/machines-qom.h" > > static bool vfp_needed(void *opaque) > { > @@ -1111,3 +1112,14 @@ const VMStateDescription vmstate_arm_cpu = { > NULL > } > }; > + > +InterfaceInfo arm_aarch64_machine_interfaces[] = { > + { TYPE_TARGET_ARM_MACHINE }, > + { TYPE_TARGET_AARCH64_MACHINE }, > + { } > +}; > + > +InterfaceInfo aarch64_machine_interfaces[] = { > + { TYPE_TARGET_AARCH64_MACHINE }, > + { } > +}; Why do you need these? If you define DEFINE_MACHINE_WITH_INTERFACES as OBJECT_DEFINE_TYPE_WITH_INTERFACES then you can write: DEFINE_MACHINE_WITH_INTERFACES(name, initfn, { TYPE_TARGET_ARM_MACHINE }, { TYPE_TARGET_AARCH64_MACHINE }, { }) and no more macros needed. Ideally those places that are now blown up should use DEFINE_MACHINE too. Maybe they don't yet because the parent type is hardcoded so we should really have DEFINE_MACHINE_EXTENDED(name, parent, initfn, interfaces...) and remove more bolier plate that way? Regards, BALATON Zoltan
On 4/24/25 17:16, BALATON Zoltan wrote: > On Fri, 25 Apr 2025, Philippe Mathieu-Daudé wrote: >> A machine defined with the DEFINE_MACHINE_ARM_AARCH64() macro >> will be available on qemu-system-arm and qemu-system-aarch64 >> binaries. >> >> One defined with DEFINE_MACHINE_AARCH64() will only be available >> in the qemu-system-aarch64 binary. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> include/hw/arm/machines-qom.h | 13 +++++++++++++ >> target/arm/machine.c | 12 ++++++++++++ >> 2 files changed, 25 insertions(+) >> >> diff --git a/include/hw/arm/machines-qom.h b/include/hw/arm/machines-qom.h >> index a17225f5f92..6277ee986d9 100644 >> --- a/include/hw/arm/machines-qom.h >> +++ b/include/hw/arm/machines-qom.h >> @@ -9,10 +9,23 @@ >> #ifndef HW_ARM_MACHINES_QOM_H >> #define HW_ARM_MACHINES_QOM_H >> >> +#include "hw/boards.h" >> + >> #define TYPE_TARGET_ARM_MACHINE \ >> "target-info-arm-machine" >> >> #define TYPE_TARGET_AARCH64_MACHINE \ >> "target-info-aarch64-machine" >> >> +extern InterfaceInfo arm_aarch64_machine_interfaces[]; >> +extern InterfaceInfo aarch64_machine_interfaces[]; >> + >> +#define DEFINE_MACHINE_ARM_AARCH64(namestr, machine_initfn) \ >> + DEFINE_MACHINE_WITH_INTERFACES(namestr, machine_initfn, \ >> + arm_aarch64_machine_interfaces) >> + >> +#define DEFINE_MACHINE_AARCH64(namestr, machine_initfn) \ >> + DEFINE_MACHINE_WITH_INTERFACES(namestr, machine_initfn, \ >> + aarch64_machine_interfaces) >> + >> #endif >> diff --git a/target/arm/machine.c b/target/arm/machine.c >> index 978249fb71b..193c7a9cff0 100644 >> --- a/target/arm/machine.c >> +++ b/target/arm/machine.c >> @@ -8,6 +8,7 @@ >> #include "cpu-features.h" >> #include "migration/cpu.h" >> #include "target/arm/gtimer.h" >> +#include "hw/arm/machines-qom.h" >> >> static bool vfp_needed(void *opaque) >> { >> @@ -1111,3 +1112,14 @@ const VMStateDescription vmstate_arm_cpu = { >> NULL >> } >> }; >> + >> +InterfaceInfo arm_aarch64_machine_interfaces[] = { >> + { TYPE_TARGET_ARM_MACHINE }, >> + { TYPE_TARGET_AARCH64_MACHINE }, >> + { } >> +}; >> + >> +InterfaceInfo aarch64_machine_interfaces[] = { >> + { TYPE_TARGET_AARCH64_MACHINE }, >> + { } >> +}; > > Why do you need these? If you define DEFINE_MACHINE_WITH_INTERFACES as > OBJECT_DEFINE_TYPE_WITH_INTERFACES then you can write: > This was requested in v4 by Richard to remove anonymous array duplication in .data. > DEFINE_MACHINE_WITH_INTERFACES(name, initfn, { TYPE_TARGET_ARM_MACHINE }, > { TYPE_TARGET_AARCH64_MACHINE }, { }) > > and no more macros needed. Ideally those places that are now blown up > should use DEFINE_MACHINE too. Maybe they don't yet because the parent > type is hardcoded so we should really have > Not sure what you mean by "no more macros needed". arm_aarch64_machine_interfaces or aarch64_machine_interfaces are arrays (defined only once), which are passed as a parameter to DEFINE_MACHINE_WITH_INTERFACES, or manually set with ".interfaces =". > DEFINE_MACHINE_EXTENDED(name, parent, initfn, interfaces...) > > and remove more bolier plate that way? > Could you can share a concrete example of what you expect, with the new macros to add, and how to use them for a given board? > Regards, > BALATON Zoltan
On Thu, 24 Apr 2025, Pierrick Bouvier wrote: > On 4/24/25 17:16, BALATON Zoltan wrote: >> On Fri, 25 Apr 2025, Philippe Mathieu-Daudé wrote: >>> A machine defined with the DEFINE_MACHINE_ARM_AARCH64() macro >>> will be available on qemu-system-arm and qemu-system-aarch64 >>> binaries. >>> >>> One defined with DEFINE_MACHINE_AARCH64() will only be available >>> in the qemu-system-aarch64 binary. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> include/hw/arm/machines-qom.h | 13 +++++++++++++ >>> target/arm/machine.c | 12 ++++++++++++ >>> 2 files changed, 25 insertions(+) >>> >>> diff --git a/include/hw/arm/machines-qom.h b/include/hw/arm/machines-qom.h >>> index a17225f5f92..6277ee986d9 100644 >>> --- a/include/hw/arm/machines-qom.h >>> +++ b/include/hw/arm/machines-qom.h >>> @@ -9,10 +9,23 @@ >>> #ifndef HW_ARM_MACHINES_QOM_H >>> #define HW_ARM_MACHINES_QOM_H >>> >>> +#include "hw/boards.h" >>> + >>> #define TYPE_TARGET_ARM_MACHINE \ >>> "target-info-arm-machine" >>> >>> #define TYPE_TARGET_AARCH64_MACHINE \ >>> "target-info-aarch64-machine" >>> >>> +extern InterfaceInfo arm_aarch64_machine_interfaces[]; >>> +extern InterfaceInfo aarch64_machine_interfaces[]; >>> + >>> +#define DEFINE_MACHINE_ARM_AARCH64(namestr, machine_initfn) \ >>> + DEFINE_MACHINE_WITH_INTERFACES(namestr, machine_initfn, \ >>> + arm_aarch64_machine_interfaces) >>> + >>> +#define DEFINE_MACHINE_AARCH64(namestr, machine_initfn) \ >>> + DEFINE_MACHINE_WITH_INTERFACES(namestr, machine_initfn, \ >>> + aarch64_machine_interfaces) >>> + >>> #endif >>> diff --git a/target/arm/machine.c b/target/arm/machine.c >>> index 978249fb71b..193c7a9cff0 100644 >>> --- a/target/arm/machine.c >>> +++ b/target/arm/machine.c >>> @@ -8,6 +8,7 @@ >>> #include "cpu-features.h" >>> #include "migration/cpu.h" >>> #include "target/arm/gtimer.h" >>> +#include "hw/arm/machines-qom.h" >>> >>> static bool vfp_needed(void *opaque) >>> { >>> @@ -1111,3 +1112,14 @@ const VMStateDescription vmstate_arm_cpu = { >>> NULL >>> } >>> }; >>> + >>> +InterfaceInfo arm_aarch64_machine_interfaces[] = { >>> + { TYPE_TARGET_ARM_MACHINE }, >>> + { TYPE_TARGET_AARCH64_MACHINE }, >>> + { } >>> +}; >>> + >>> +InterfaceInfo aarch64_machine_interfaces[] = { >>> + { TYPE_TARGET_AARCH64_MACHINE }, >>> + { } >>> +}; >> >> Why do you need these? If you define DEFINE_MACHINE_WITH_INTERFACES as >> OBJECT_DEFINE_TYPE_WITH_INTERFACES then you can write: >> > > This was requested in v4 by Richard to remove anonymous array duplication in > .data. > >> DEFINE_MACHINE_WITH_INTERFACES(name, initfn, { TYPE_TARGET_ARM_MACHINE }, >> { TYPE_TARGET_AARCH64_MACHINE }, { }) >> >> and no more macros needed. Ideally those places that are now blown up >> should use DEFINE_MACHINE too. Maybe they don't yet because the parent >> type is hardcoded so we should really have >> > > Not sure what you mean by "no more macros needed". No other specialised macros needed for each machine type other than DEFINE_MACHINE_WITH_INTERFACES or DEFINE_MACHINE_EXTENDED. So I suggested to keep DEFINE_MACHINE by making it more general so it can cover the new uses instead of bringing back the boiler plate and losing the clarity hinding these behind the macros. > arm_aarch64_machine_interfaces or aarch64_machine_interfaces are arrays > (defined only once), which are passed as a parameter to > DEFINE_MACHINE_WITH_INTERFACES, or manually set with ".interfaces =". Look at how OBJECT_DEFINE_TYPE_WITH_INTERFACES is defined. >> DEFINE_MACHINE_EXTENDED(name, parent, initfn, interfaces...) >> >> and remove more bolier plate that way? >> > > Could you can share a concrete example of what you expect, with the new > macros to add, and how to use them for a given board? I tried to do that in this message you replied to. Regards, BALATON Zoltan
diff --git a/include/hw/arm/machines-qom.h b/include/hw/arm/machines-qom.h index a17225f5f92..6277ee986d9 100644 --- a/include/hw/arm/machines-qom.h +++ b/include/hw/arm/machines-qom.h @@ -9,10 +9,23 @@ #ifndef HW_ARM_MACHINES_QOM_H #define HW_ARM_MACHINES_QOM_H +#include "hw/boards.h" + #define TYPE_TARGET_ARM_MACHINE \ "target-info-arm-machine" #define TYPE_TARGET_AARCH64_MACHINE \ "target-info-aarch64-machine" +extern InterfaceInfo arm_aarch64_machine_interfaces[]; +extern InterfaceInfo aarch64_machine_interfaces[]; + +#define DEFINE_MACHINE_ARM_AARCH64(namestr, machine_initfn) \ + DEFINE_MACHINE_WITH_INTERFACES(namestr, machine_initfn, \ + arm_aarch64_machine_interfaces) + +#define DEFINE_MACHINE_AARCH64(namestr, machine_initfn) \ + DEFINE_MACHINE_WITH_INTERFACES(namestr, machine_initfn, \ + aarch64_machine_interfaces) + #endif diff --git a/target/arm/machine.c b/target/arm/machine.c index 978249fb71b..193c7a9cff0 100644 --- a/target/arm/machine.c +++ b/target/arm/machine.c @@ -8,6 +8,7 @@ #include "cpu-features.h" #include "migration/cpu.h" #include "target/arm/gtimer.h" +#include "hw/arm/machines-qom.h" static bool vfp_needed(void *opaque) { @@ -1111,3 +1112,14 @@ const VMStateDescription vmstate_arm_cpu = { NULL } }; + +InterfaceInfo arm_aarch64_machine_interfaces[] = { + { TYPE_TARGET_ARM_MACHINE }, + { TYPE_TARGET_AARCH64_MACHINE }, + { } +}; + +InterfaceInfo aarch64_machine_interfaces[] = { + { TYPE_TARGET_AARCH64_MACHINE }, + { } +};
A machine defined with the DEFINE_MACHINE_ARM_AARCH64() macro will be available on qemu-system-arm and qemu-system-aarch64 binaries. One defined with DEFINE_MACHINE_AARCH64() will only be available in the qemu-system-aarch64 binary. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/hw/arm/machines-qom.h | 13 +++++++++++++ target/arm/machine.c | 12 ++++++++++++ 2 files changed, 25 insertions(+)