| 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
On 4/25/25 02:43, BALATON Zoltan wrote: > 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. > This is exactly what we have in this series. Patch 7 introduces DEFINE_MACHINE_WITH_INTERFACES. I guess Philippe chose a new name to avoid modifying all existing DEFINE_MACHINE, and I think it's understandable, as we want those changes to impact hw/arm only first. That said, it would be very easy to refactor/modify later, so it's not a big deal. This patch introduces DEFINE_MACHINE_ARM_AARCH64 and DEFINE_MACHINE_AARCH64. Is the problem with those specialized DEFINE_MACHINE_{ARM, AARCH64} definition? If yes, and if you prefer an explicit DEFINE_MACHINE_WITH_INTERFACES(..., arm_aarch64_machine_interfaces), I'm sure Philippe would be open to make such a change to satisfy reviews. Let's just try to decide something, and move on. >> 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. > This macro is not used for any machine definition so far, and DEFINE_MACHINE is the "standard" macro used, at least the one most commonly used in the codebase. So it makes sense to simply expand the latter. >>> 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. > If you refer to "DEFINE_MACHINE_EXTENDED(name, parent, initfn, interfaces...)", this is almost exactly what patch 7 is introducing with DEFINE_MACHINE_WITH_INTERFACES(namestr, machine_initfn, ifaces). > Regards, > BALATON Zoltan
On Fri, 25 Apr 2025, Pierrick Bouvier wrote: > On 4/25/25 02:43, BALATON Zoltan wrote: >> 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. >> > > This is exactly what we have in this series. > Patch 7 introduces DEFINE_MACHINE_WITH_INTERFACES. > I guess Philippe chose a new name to avoid modifying all existing > DEFINE_MACHINE, and I think it's understandable, as we want those changes to > impact hw/arm only first. That said, it would be very easy to refactor/modify > later, so it's not a big deal. > > This patch introduces DEFINE_MACHINE_ARM_AARCH64 and DEFINE_MACHINE_AARCH64. > > Is the problem with those specialized DEFINE_MACHINE_{ARM, AARCH64} > definition? > If yes, and if you prefer an explicit DEFINE_MACHINE_WITH_INTERFACES(..., > arm_aarch64_machine_interfaces), I'm sure Philippe would be open to make such > a change to satisfy reviews. > > Let's just try to decide something, and move on. > >>> 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. >> > > This macro is not used for any machine definition so far, and DEFINE_MACHINE > is the "standard" macro used, at least the one most commonly used in the > codebase. So it makes sense to simply expand the latter. I was referring to that as an example how a DEFINE_MACHINE_WITH_INTERFACES should work not suggesting to use OBJECT_DEFINE_TYPE_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? >> >> I tried to do that in this message you replied to. >> > > If you refer to "DEFINE_MACHINE_EXTENDED(name, parent, initfn, > interfaces...)", this is almost exactly what patch 7 is introducing with > DEFINE_MACHINE_WITH_INTERFACES(namestr, machine_initfn, ifaces). The difference is that OBJECT_DEFINE_TYPE_WITH_INTERFACES takes a list of interfaces and defines the array itself and you pass the array which is limiting as you then need to define a lot of arrays to pass to your macro instead of only passing the elements and let it define tha array. I just want to see instead of static const TypeInfo machine_types[] = { ...lots of boiler plate code here }; something like DEFINE_MACHINE_EXTENDED(machine1, TYPE_WHATEVER_MACHINE, {INTERFACE1}, {INTERFACE2}, {}) DEFINE_MACHINE_EXTENDED(machine2, TYPE_OTHER_MACHINE, {INTERFACE1}, {INTERFACE3}, {}) DEFINE_MACHINE_EXTENDED(machine3, TYPE_THIRD_MACHINE, {INTERFACE1}, {}) Regards, BALATON Zoltan
On 4/25/25 13:29, BALATON Zoltan wrote: > On Fri, 25 Apr 2025, Pierrick Bouvier wrote: >> On 4/25/25 02:43, BALATON Zoltan wrote: >>> 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. >>> >> >> This is exactly what we have in this series. >> Patch 7 introduces DEFINE_MACHINE_WITH_INTERFACES. >> I guess Philippe chose a new name to avoid modifying all existing >> DEFINE_MACHINE, and I think it's understandable, as we want those changes to >> impact hw/arm only first. That said, it would be very easy to refactor/modify >> later, so it's not a big deal. >> >> This patch introduces DEFINE_MACHINE_ARM_AARCH64 and DEFINE_MACHINE_AARCH64. >> >> Is the problem with those specialized DEFINE_MACHINE_{ARM, AARCH64} >> definition? >> If yes, and if you prefer an explicit DEFINE_MACHINE_WITH_INTERFACES(..., >> arm_aarch64_machine_interfaces), I'm sure Philippe would be open to make such >> a change to satisfy reviews. >> >> Let's just try to decide something, and move on. >> >>>> 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. >>> >> >> This macro is not used for any machine definition so far, and DEFINE_MACHINE >> is the "standard" macro used, at least the one most commonly used in the >> codebase. So it makes sense to simply expand the latter. > > I was referring to that as an example how a DEFINE_MACHINE_WITH_INTERFACES > should work not suggesting to use OBJECT_DEFINE_TYPE_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? >>> >>> I tried to do that in this message you replied to. >>> >> >> If you refer to "DEFINE_MACHINE_EXTENDED(name, parent, initfn, >> interfaces...)", this is almost exactly what patch 7 is introducing with >> DEFINE_MACHINE_WITH_INTERFACES(namestr, machine_initfn, ifaces). > > The difference is that OBJECT_DEFINE_TYPE_WITH_INTERFACES takes a list of > interfaces and defines the array itself and you pass the array which is > limiting as you then need to define a lot of arrays to pass to your macro > instead of only passing the elements and let it define tha array. > > I just want to see instead of > > static const TypeInfo machine_types[] = { > ...lots of boiler plate code here > }; > > something like > > DEFINE_MACHINE_EXTENDED(machine1, TYPE_WHATEVER_MACHINE, {INTERFACE1}, {INTERFACE2}, {}) > DEFINE_MACHINE_EXTENDED(machine2, TYPE_OTHER_MACHINE, {INTERFACE1}, {INTERFACE3}, {}) > DEFINE_MACHINE_EXTENDED(machine3, TYPE_THIRD_MACHINE, {INTERFACE1}, {}) > Ok, I understand better. It was my point as well on v4, that introducing those symbols is less readable and less scalable, for a negligible benefit in terms of code size, which was the primary concern. We can always reconsider this later, especially when adding another architecture to single binary, it's not a problem and something set in stone. Would you be ok if we proceed with the current version, knowing those limitations, for now? > Regards, > BALATON Zoltan
On 25/4/25 22:36, Pierrick Bouvier wrote: > On 4/25/25 13:29, BALATON Zoltan wrote: >> On Fri, 25 Apr 2025, Pierrick Bouvier wrote: >>> On 4/25/25 02:43, BALATON Zoltan wrote: >>>> 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. >>>> >>> >>> This is exactly what we have in this series. >>> Patch 7 introduces DEFINE_MACHINE_WITH_INTERFACES. >>> I guess Philippe chose a new name to avoid modifying all existing >>> DEFINE_MACHINE, and I think it's understandable, as we want those >>> changes to >>> impact hw/arm only first. That said, it would be very easy to >>> refactor/modify >>> later, so it's not a big deal. >>> >>> This patch introduces DEFINE_MACHINE_ARM_AARCH64 and >>> DEFINE_MACHINE_AARCH64. >>> >>> Is the problem with those specialized DEFINE_MACHINE_{ARM, AARCH64} >>> definition? >>> If yes, and if you prefer an explicit >>> DEFINE_MACHINE_WITH_INTERFACES(..., >>> arm_aarch64_machine_interfaces), I'm sure Philippe would be open to >>> make such >>> a change to satisfy reviews. >>> >>> Let's just try to decide something, and move on. >>> >>>>> 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. >>>> >>> >>> This macro is not used for any machine definition so far, and >>> DEFINE_MACHINE >>> is the "standard" macro used, at least the one most commonly used in the >>> codebase. So it makes sense to simply expand the latter. >> >> I was referring to that as an example how a >> DEFINE_MACHINE_WITH_INTERFACES >> should work not suggesting to use OBJECT_DEFINE_TYPE_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? >>>> >>>> I tried to do that in this message you replied to. >>>> >>> >>> If you refer to "DEFINE_MACHINE_EXTENDED(name, parent, initfn, >>> interfaces...)", this is almost exactly what patch 7 is introducing with >>> DEFINE_MACHINE_WITH_INTERFACES(namestr, machine_initfn, ifaces). >> >> The difference is that OBJECT_DEFINE_TYPE_WITH_INTERFACES takes a list of >> interfaces and defines the array itself and you pass the array which is >> limiting as you then need to define a lot of arrays to pass to your macro >> instead of only passing the elements and let it define tha array. >> >> I just want to see instead of >> >> static const TypeInfo machine_types[] = { >> ...lots of boiler plate code here >> }; >> >> something like >> >> DEFINE_MACHINE_EXTENDED(machine1, TYPE_WHATEVER_MACHINE, {INTERFACE1}, >> {INTERFACE2}, {}) >> DEFINE_MACHINE_EXTENDED(machine2, TYPE_OTHER_MACHINE, {INTERFACE1}, >> {INTERFACE3}, {}) >> DEFINE_MACHINE_EXTENDED(machine3, TYPE_THIRD_MACHINE, {INTERFACE1}, {}) >> > > Ok, I understand better. > > It was my point as well on v4, that introducing those symbols is less > readable and less scalable, for a negligible benefit in terms of code > size, which was the primary concern. > We can always reconsider this later, especially when adding another > architecture to single binary, it's not a problem and something set in > stone. > > Would you be ok if we proceed with the current version, knowing those > limitations, for now? If Zoltan disagrees, we need Richard to agree to go back on v4. Keep in mind that what we are trying to achieve is quite more complex than code style or .rodata savings, besides we eventually want to have dynamic machines & DSL.
On Mon, 28 Apr 2025, Philippe Mathieu-Daudé wrote: > On 25/4/25 22:36, Pierrick Bouvier wrote: >> On 4/25/25 13:29, BALATON Zoltan wrote: >>> On Fri, 25 Apr 2025, Pierrick Bouvier wrote: >>>> On 4/25/25 02:43, BALATON Zoltan wrote: >>>>> 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. >>>>> >>>> >>>> This is exactly what we have in this series. >>>> Patch 7 introduces DEFINE_MACHINE_WITH_INTERFACES. >>>> I guess Philippe chose a new name to avoid modifying all existing >>>> DEFINE_MACHINE, and I think it's understandable, as we want those changes >>>> to >>>> impact hw/arm only first. That said, it would be very easy to >>>> refactor/modify >>>> later, so it's not a big deal. >>>> >>>> This patch introduces DEFINE_MACHINE_ARM_AARCH64 and >>>> DEFINE_MACHINE_AARCH64. >>>> >>>> Is the problem with those specialized DEFINE_MACHINE_{ARM, AARCH64} >>>> definition? >>>> If yes, and if you prefer an explicit DEFINE_MACHINE_WITH_INTERFACES(..., >>>> arm_aarch64_machine_interfaces), I'm sure Philippe would be open to make >>>> such >>>> a change to satisfy reviews. >>>> >>>> Let's just try to decide something, and move on. >>>> >>>>>> 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. >>>>> >>>> >>>> This macro is not used for any machine definition so far, and >>>> DEFINE_MACHINE >>>> is the "standard" macro used, at least the one most commonly used in the >>>> codebase. So it makes sense to simply expand the latter. >>> >>> I was referring to that as an example how a DEFINE_MACHINE_WITH_INTERFACES >>> should work not suggesting to use OBJECT_DEFINE_TYPE_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? >>>>> >>>>> I tried to do that in this message you replied to. >>>>> >>>> >>>> If you refer to "DEFINE_MACHINE_EXTENDED(name, parent, initfn, >>>> interfaces...)", this is almost exactly what patch 7 is introducing with >>>> DEFINE_MACHINE_WITH_INTERFACES(namestr, machine_initfn, ifaces). >>> >>> The difference is that OBJECT_DEFINE_TYPE_WITH_INTERFACES takes a list of >>> interfaces and defines the array itself and you pass the array which is >>> limiting as you then need to define a lot of arrays to pass to your macro >>> instead of only passing the elements and let it define tha array. >>> >>> I just want to see instead of >>> >>> static const TypeInfo machine_types[] = { >>> ...lots of boiler plate code here >>> }; >>> >>> something like >>> >>> DEFINE_MACHINE_EXTENDED(machine1, TYPE_WHATEVER_MACHINE, {INTERFACE1}, >>> {INTERFACE2}, {}) >>> DEFINE_MACHINE_EXTENDED(machine2, TYPE_OTHER_MACHINE, {INTERFACE1}, >>> {INTERFACE3}, {}) >>> DEFINE_MACHINE_EXTENDED(machine3, TYPE_THIRD_MACHINE, {INTERFACE1}, {}) >>> >> >> Ok, I understand better. >> >> It was my point as well on v4, that introducing those symbols is less >> readable and less scalable, for a negligible benefit in terms of code size, >> which was the primary concern. >> We can always reconsider this later, especially when adding another >> architecture to single binary, it's not a problem and something set in >> stone. >> >> Would you be ok if we proceed with the current version, knowing those >> limitations, for now? > > If Zoltan disagrees, we need Richard to agree to go back on v4. > > Keep in mind that what we are trying to achieve is quite more complex > than code style or .rodata savings, besides we eventually want to have > dynamic machines & DSL. Since you are touching the lines using DEFINE_MACHINE it's a good opportunity to change the macro to be more general to be able to keep using it instead of replacing it with the boiler plate it's supposed to hide. Adding one or two more parameters to the macro is not a big change so I don't see why you don't want to do it. This could be addressed later to revert to use the macro again but in practice it will not be addressed because everybody will be busy doing other things and doing that now would prevent some churn. I too, don't like doing unrelated clean up which is not the main goal, but if it's not much more work then it's not unreasonable to do it. I only oppose to that if it's a lot of work so I would not ask such change but what I asked is not unrelated and quite simple change. That said, I can't stop you so if you still don't want to do it now then you can move on. I don't care that much as long as you stay within hw/arm, but will raise my concern again when you submit a similar patch that touches parts I care more about. If others don't think it's a problem and not bothered by the boiler plate code then it's not so important but otherwise I think I have a valid point. I remember when I started to get to know QEMU it was quite difficult to wade through all the QOM boiler plate just to see what is related to the actual functionality. These macros help to make code more readable and accessible for new people. Regards, BALATON Zoltan
On 4/28/25 3:31 AM, BALATON Zoltan wrote: > Since you are touching the lines using DEFINE_MACHINE it's a good > opportunity to change the macro to be more general to be able to keep > using it instead of replacing it with the boiler plate it's supposed to > hide. Adding one or two more parameters to the macro is not a big change > so I don't see why you don't want to do it. This could be addressed later > to revert to use the macro again but in practice it will not be addressed > because everybody will be busy doing other things and doing that now would > prevent some churn. I too, don't like doing unrelated clean up which is > not the main goal, but if it's not much more work then it's not > unreasonable to do it. I only oppose to that if it's a lot of work so I > would not ask such change but what I asked is not unrelated and quite > simple change. > > That said, I can't stop you so if you still don't want to do it now then > you can move on. I don't care that much as long as you stay within hw/arm, > but will raise my concern again when you submit a similar patch that > touches parts I care more about. If others don't think it's a problem and > not bothered by the boiler plate code then it's not so important but > otherwise I think I have a valid point. I remember when I started to get > to know QEMU it was quite difficult to wade through all the QOM boiler > plate just to see what is related to the actual functionality. These > macros help to make code more readable and accessible for new people. > Having been through that recently, I agree with you that it can be hard to follow at first. Luckily, we have perfect compiler based completion for all editors those days (I sincerely hope everyone spent 2 hours configuring this on their own favorite one), and it's easy to see where things are defined and used, even when code is cryptic. That said, pushing to someone adding a new field the responsibility of cleaning up the whole thing is not a fair request. You can't expect your friends to clean your shared house because they brought a cake for dinner. > Regards, > BALATON Zoltan Regards, Pierrick
On Mon, 28 Apr 2025, Pierrick Bouvier wrote: > On 4/28/25 3:31 AM, BALATON Zoltan wrote: >> Since you are touching the lines using DEFINE_MACHINE it's a good >> opportunity to change the macro to be more general to be able to keep >> using it instead of replacing it with the boiler plate it's supposed to >> hide. Adding one or two more parameters to the macro is not a big change >> so I don't see why you don't want to do it. This could be addressed later >> to revert to use the macro again but in practice it will not be addressed >> because everybody will be busy doing other things and doing that now would >> prevent some churn. I too, don't like doing unrelated clean up which is >> not the main goal, but if it's not much more work then it's not >> unreasonable to do it. I only oppose to that if it's a lot of work so I >> would not ask such change but what I asked is not unrelated and quite >> simple change. >> >> That said, I can't stop you so if you still don't want to do it now then >> you can move on. I don't care that much as long as you stay within hw/arm, >> but will raise my concern again when you submit a similar patch that >> touches parts I care more about. If others don't think it's a problem and >> not bothered by the boiler plate code then it's not so important but >> otherwise I think I have a valid point. I remember when I started to get >> to know QEMU it was quite difficult to wade through all the QOM boiler >> plate just to see what is related to the actual functionality. These >> macros help to make code more readable and accessible for new people. > > Having been through that recently, I agree with you that it can be hard to > follow at first. Luckily, we have perfect compiler based completion for all > editors those days (I sincerely hope everyone spent 2 hours configuring this > on their own favorite one), and it's easy to see where things are defined and > used, even when code is cryptic. It's not about typing but reading it. The verbose struct definitions are hard to follow and makes board code look more complex than it should be. > That said, pushing to someone adding a new field the responsibility of > cleaning up the whole thing is not a fair request. You can't expect your > friends to clean your shared house because they brought a cake for dinner. I tend to get such requests to clean up unrelated things whenever I try to change anything in PPC Mac emulation which I also complain about and think is not reasonable to ask. But I did not ask for unrelated cleanup here and changing the patch so you don't do this: -DEFINE_MACHINE("none", machine_none_machine_init) +static const TypeInfo null_machine_types[] = { + { + .name = MACHINE_TYPE_NAME("none"), + .parent = TYPE_MACHINE, + .class_init = null_machine_class_init, + }, +}; + +DEFINE_TYPES(null_machine_types) but instead add the .interfaces field to a variant of DEFINE_MACHINE once and keep the one line definition is not something unreasonable to ask. I think you can ask your friends to not make a mess in the shared house while having a party or at least clean up after that. Adding one more parameter to the macro is also simple to do so I don't get why you're so opposed to this. Regards, BALATON Zoltan
On 4/28/25 11:44 AM, BALATON Zoltan wrote: > On Mon, 28 Apr 2025, Pierrick Bouvier wrote: >> On 4/28/25 3:31 AM, BALATON Zoltan wrote: >>> Since you are touching the lines using DEFINE_MACHINE it's a good >>> opportunity to change the macro to be more general to be able to keep >>> using it instead of replacing it with the boiler plate it's supposed to >>> hide. Adding one or two more parameters to the macro is not a big change >>> so I don't see why you don't want to do it. This could be addressed later >>> to revert to use the macro again but in practice it will not be addressed >>> because everybody will be busy doing other things and doing that now would >>> prevent some churn. I too, don't like doing unrelated clean up which is >>> not the main goal, but if it's not much more work then it's not >>> unreasonable to do it. I only oppose to that if it's a lot of work so I >>> would not ask such change but what I asked is not unrelated and quite >>> simple change. >>> >>> That said, I can't stop you so if you still don't want to do it now then >>> you can move on. I don't care that much as long as you stay within hw/arm, >>> but will raise my concern again when you submit a similar patch that >>> touches parts I care more about. If others don't think it's a problem and >>> not bothered by the boiler plate code then it's not so important but >>> otherwise I think I have a valid point. I remember when I started to get >>> to know QEMU it was quite difficult to wade through all the QOM boiler >>> plate just to see what is related to the actual functionality. These >>> macros help to make code more readable and accessible for new people. >> >> Having been through that recently, I agree with you that it can be hard to >> follow at first. Luckily, we have perfect compiler based completion for all >> editors those days (I sincerely hope everyone spent 2 hours configuring this >> on their own favorite one), and it's easy to see where things are defined and >> used, even when code is cryptic. > > It's not about typing but reading it. The verbose struct definitions are > hard to follow and makes board code look more complex than it should be. > >> That said, pushing to someone adding a new field the responsibility of >> cleaning up the whole thing is not a fair request. You can't expect your >> friends to clean your shared house because they brought a cake for dinner. > > I tend to get such requests to clean up unrelated things whenever I try to > change anything in PPC Mac emulation which I also complain about and think > is not reasonable to ask. But I did not ask for unrelated cleanup here and > changing the patch so you don't do this: > > -DEFINE_MACHINE("none", machine_none_machine_init) > +static const TypeInfo null_machine_types[] = { > + { > + .name = MACHINE_TYPE_NAME("none"), > + .parent = TYPE_MACHINE, > + .class_init = null_machine_class_init, > + }, > +}; > + > +DEFINE_TYPES(null_machine_types) > > but instead add the .interfaces field to a variant of DEFINE_MACHINE once > and keep the one line definition is not something unreasonable to ask. I > think you can ask your friends to not make a mess in the shared house > while having a party or at least clean up after that. Adding one more > parameter to the macro is also simple to do so I don't get why you're so > opposed to this. > Maybe there is a misunderstanding on my side, but it seems that what you asked is exactly patch 7, which introduce DEFINE_MACHINE_WITH_INTERFACES. That said, patch 4 ("hw/core/null-machine: Define machine as generic QOM type") could use it to define the null machine. Philippe, could you change patch 4 to use DEFINE_MACHINE_WITH_INTERFACES instead? Thanks, Pierrick > Regards, > BALATON Zoltan
On Mon, 28 Apr 2025, Pierrick Bouvier wrote: > On 4/28/25 11:44 AM, BALATON Zoltan wrote: >> On Mon, 28 Apr 2025, Pierrick Bouvier wrote: >>> On 4/28/25 3:31 AM, BALATON Zoltan wrote: >>>> Since you are touching the lines using DEFINE_MACHINE it's a good >>>> opportunity to change the macro to be more general to be able to keep >>>> using it instead of replacing it with the boiler plate it's supposed to >>>> hide. Adding one or two more parameters to the macro is not a big change >>>> so I don't see why you don't want to do it. This could be addressed later >>>> to revert to use the macro again but in practice it will not be addressed >>>> because everybody will be busy doing other things and doing that now >>>> would >>>> prevent some churn. I too, don't like doing unrelated clean up which is >>>> not the main goal, but if it's not much more work then it's not >>>> unreasonable to do it. I only oppose to that if it's a lot of work so I >>>> would not ask such change but what I asked is not unrelated and quite >>>> simple change. >>>> >>>> That said, I can't stop you so if you still don't want to do it now then >>>> you can move on. I don't care that much as long as you stay within >>>> hw/arm, >>>> but will raise my concern again when you submit a similar patch that >>>> touches parts I care more about. If others don't think it's a problem and >>>> not bothered by the boiler plate code then it's not so important but >>>> otherwise I think I have a valid point. I remember when I started to get >>>> to know QEMU it was quite difficult to wade through all the QOM boiler >>>> plate just to see what is related to the actual functionality. These >>>> macros help to make code more readable and accessible for new people. >>> >>> Having been through that recently, I agree with you that it can be hard to >>> follow at first. Luckily, we have perfect compiler based completion for >>> all >>> editors those days (I sincerely hope everyone spent 2 hours configuring >>> this >>> on their own favorite one), and it's easy to see where things are defined >>> and >>> used, even when code is cryptic. >> >> It's not about typing but reading it. The verbose struct definitions are >> hard to follow and makes board code look more complex than it should be. >> >>> That said, pushing to someone adding a new field the responsibility of >>> cleaning up the whole thing is not a fair request. You can't expect your >>> friends to clean your shared house because they brought a cake for dinner. >> >> I tend to get such requests to clean up unrelated things whenever I try to >> change anything in PPC Mac emulation which I also complain about and think >> is not reasonable to ask. But I did not ask for unrelated cleanup here and >> changing the patch so you don't do this: >> >> -DEFINE_MACHINE("none", machine_none_machine_init) >> +static const TypeInfo null_machine_types[] = { >> + { >> + .name = MACHINE_TYPE_NAME("none"), >> + .parent = TYPE_MACHINE, >> + .class_init = null_machine_class_init, >> + }, >> +}; >> + >> +DEFINE_TYPES(null_machine_types) >> >> but instead add the .interfaces field to a variant of DEFINE_MACHINE once >> and keep the one line definition is not something unreasonable to ask. I >> think you can ask your friends to not make a mess in the shared house >> while having a party or at least clean up after that. Adding one more >> parameter to the macro is also simple to do so I don't get why you're so >> opposed to this. >> > > Maybe there is a misunderstanding on my side, but it seems that what you > asked is exactly patch 7, which introduce DEFINE_MACHINE_WITH_INTERFACES. Almost but not quite. I don't know why I can't get this through to you. If you compare patch 7 to how DO_OBJECT_DEFINE_TYPE_EXTENDED is defined do you notice the difference in how .interfaces is set? With the same way as in DO_OBJECT_DEFINE_TYPE_EXTENDED you don't need separate InterfaceInfo arm_aarch64_machine_interfaces[] definitions or different macros in the next patch just list the needed interfaces in the machine definitions. Regards, BALATON Zoltan
On 4/28/25 6:10 PM, BALATON Zoltan wrote: > On Mon, 28 Apr 2025, Pierrick Bouvier wrote: >> On 4/28/25 11:44 AM, BALATON Zoltan wrote: >>> On Mon, 28 Apr 2025, Pierrick Bouvier wrote: >>>> On 4/28/25 3:31 AM, BALATON Zoltan wrote: >>>>> Since you are touching the lines using DEFINE_MACHINE it's a good >>>>> opportunity to change the macro to be more general to be able to keep >>>>> using it instead of replacing it with the boiler plate it's supposed to >>>>> hide. Adding one or two more parameters to the macro is not a big change >>>>> so I don't see why you don't want to do it. This could be addressed later >>>>> to revert to use the macro again but in practice it will not be addressed >>>>> because everybody will be busy doing other things and doing that now >>>>> would >>>>> prevent some churn. I too, don't like doing unrelated clean up which is >>>>> not the main goal, but if it's not much more work then it's not >>>>> unreasonable to do it. I only oppose to that if it's a lot of work so I >>>>> would not ask such change but what I asked is not unrelated and quite >>>>> simple change. >>>>> >>>>> That said, I can't stop you so if you still don't want to do it now then >>>>> you can move on. I don't care that much as long as you stay within >>>>> hw/arm, >>>>> but will raise my concern again when you submit a similar patch that >>>>> touches parts I care more about. If others don't think it's a problem and >>>>> not bothered by the boiler plate code then it's not so important but >>>>> otherwise I think I have a valid point. I remember when I started to get >>>>> to know QEMU it was quite difficult to wade through all the QOM boiler >>>>> plate just to see what is related to the actual functionality. These >>>>> macros help to make code more readable and accessible for new people. >>>> >>>> Having been through that recently, I agree with you that it can be hard to >>>> follow at first. Luckily, we have perfect compiler based completion for >>>> all >>>> editors those days (I sincerely hope everyone spent 2 hours configuring >>>> this >>>> on their own favorite one), and it's easy to see where things are defined >>>> and >>>> used, even when code is cryptic. >>> >>> It's not about typing but reading it. The verbose struct definitions are >>> hard to follow and makes board code look more complex than it should be. >>> >>>> That said, pushing to someone adding a new field the responsibility of >>>> cleaning up the whole thing is not a fair request. You can't expect your >>>> friends to clean your shared house because they brought a cake for dinner. >>> >>> I tend to get such requests to clean up unrelated things whenever I try to >>> change anything in PPC Mac emulation which I also complain about and think >>> is not reasonable to ask. But I did not ask for unrelated cleanup here and >>> changing the patch so you don't do this: >>> >>> -DEFINE_MACHINE("none", machine_none_machine_init) >>> +static const TypeInfo null_machine_types[] = { >>> + { >>> + .name = MACHINE_TYPE_NAME("none"), >>> + .parent = TYPE_MACHINE, >>> + .class_init = null_machine_class_init, >>> + }, >>> +}; >>> + >>> +DEFINE_TYPES(null_machine_types) >>> >>> but instead add the .interfaces field to a variant of DEFINE_MACHINE once >>> and keep the one line definition is not something unreasonable to ask. I >>> think you can ask your friends to not make a mess in the shared house >>> while having a party or at least clean up after that. Adding one more >>> parameter to the macro is also simple to do so I don't get why you're so >>> opposed to this. >>> >> >> Maybe there is a misunderstanding on my side, but it seems that what you >> asked is exactly patch 7, which introduce DEFINE_MACHINE_WITH_INTERFACES. > > Almost but not quite. I don't know why I can't get this through to you. If > you compare patch 7 to how DO_OBJECT_DEFINE_TYPE_EXTENDED is defined do > you notice the difference in how .interfaces is set? With the same way as > in DO_OBJECT_DEFINE_TYPE_EXTENDED you don't need separate InterfaceInfo > arm_aarch64_machine_interfaces[] definitions or different macros in the > next patch just list the needed interfaces in the machine definitions. > I'm sorry, I don't understand what you want exactly, despite asking several times. I think it would be more clear if you could apply this series on your side, write a small patch showing *exactly* what you expect, and applying this to one of the board concerned. Then, we can do the change you request. > Regards, > BALATON Zoltan
On Mon, 28 Apr 2025, Pierrick Bouvier wrote: > On 4/28/25 6:10 PM, BALATON Zoltan wrote: >> On Mon, 28 Apr 2025, Pierrick Bouvier wrote: >>> On 4/28/25 11:44 AM, BALATON Zoltan wrote: >>>> On Mon, 28 Apr 2025, Pierrick Bouvier wrote: >>>>> On 4/28/25 3:31 AM, BALATON Zoltan wrote: >>>>>> Since you are touching the lines using DEFINE_MACHINE it's a good >>>>>> opportunity to change the macro to be more general to be able to keep >>>>>> using it instead of replacing it with the boiler plate it's supposed to >>>>>> hide. Adding one or two more parameters to the macro is not a big >>>>>> change >>>>>> so I don't see why you don't want to do it. This could be addressed >>>>>> later >>>>>> to revert to use the macro again but in practice it will not be >>>>>> addressed >>>>>> because everybody will be busy doing other things and doing that now >>>>>> would >>>>>> prevent some churn. I too, don't like doing unrelated clean up which is >>>>>> not the main goal, but if it's not much more work then it's not >>>>>> unreasonable to do it. I only oppose to that if it's a lot of work so I >>>>>> would not ask such change but what I asked is not unrelated and quite >>>>>> simple change. >>>>>> >>>>>> That said, I can't stop you so if you still don't want to do it now >>>>>> then >>>>>> you can move on. I don't care that much as long as you stay within >>>>>> hw/arm, >>>>>> but will raise my concern again when you submit a similar patch that >>>>>> touches parts I care more about. If others don't think it's a problem >>>>>> and >>>>>> not bothered by the boiler plate code then it's not so important but >>>>>> otherwise I think I have a valid point. I remember when I started to >>>>>> get >>>>>> to know QEMU it was quite difficult to wade through all the QOM boiler >>>>>> plate just to see what is related to the actual functionality. These >>>>>> macros help to make code more readable and accessible for new people. >>>>> >>>>> Having been through that recently, I agree with you that it can be hard >>>>> to >>>>> follow at first. Luckily, we have perfect compiler based completion for >>>>> all >>>>> editors those days (I sincerely hope everyone spent 2 hours configuring >>>>> this >>>>> on their own favorite one), and it's easy to see where things are >>>>> defined >>>>> and >>>>> used, even when code is cryptic. >>>> >>>> It's not about typing but reading it. The verbose struct definitions are >>>> hard to follow and makes board code look more complex than it should be. >>>> >>>>> That said, pushing to someone adding a new field the responsibility of >>>>> cleaning up the whole thing is not a fair request. You can't expect your >>>>> friends to clean your shared house because they brought a cake for >>>>> dinner. >>>> >>>> I tend to get such requests to clean up unrelated things whenever I try >>>> to >>>> change anything in PPC Mac emulation which I also complain about and >>>> think >>>> is not reasonable to ask. But I did not ask for unrelated cleanup here >>>> and >>>> changing the patch so you don't do this: >>>> >>>> -DEFINE_MACHINE("none", machine_none_machine_init) >>>> +static const TypeInfo null_machine_types[] = { >>>> + { >>>> + .name = MACHINE_TYPE_NAME("none"), >>>> + .parent = TYPE_MACHINE, >>>> + .class_init = null_machine_class_init, >>>> + }, >>>> +}; >>>> + >>>> +DEFINE_TYPES(null_machine_types) >>>> >>>> but instead add the .interfaces field to a variant of DEFINE_MACHINE once >>>> and keep the one line definition is not something unreasonable to ask. I >>>> think you can ask your friends to not make a mess in the shared house >>>> while having a party or at least clean up after that. Adding one more >>>> parameter to the macro is also simple to do so I don't get why you're so >>>> opposed to this. >>>> >>> >>> Maybe there is a misunderstanding on my side, but it seems that what you >>> asked is exactly patch 7, which introduce DEFINE_MACHINE_WITH_INTERFACES. >> >> Almost but not quite. I don't know why I can't get this through to you. If >> you compare patch 7 to how DO_OBJECT_DEFINE_TYPE_EXTENDED is defined do >> you notice the difference in how .interfaces is set? With the same way as >> in DO_OBJECT_DEFINE_TYPE_EXTENDED you don't need separate InterfaceInfo >> arm_aarch64_machine_interfaces[] definitions or different macros in the >> next patch just list the needed interfaces in the machine definitions. >> > > I'm sorry, I don't understand what you want exactly, despite asking several > times. > I think it would be more clear if you could apply this series on your side, > write a small patch showing *exactly* what you expect, and applying this to > one of the board concerned. Then, we can do the change you request. You can pick the patch from this series I've just posted: https://patchew.org/QEMU/cover.1746139668.git.balaton@eik.bme.hu/ad355178b2a3fe285854ed2e25b288baf0fd6e05.1746139668.git.balaton@eik.bme.hu/ it is used in patch 12 of that series for an example. Regards, BALATON Zoltan
On 5/1/25 4:35 PM, BALATON Zoltan wrote: > On Mon, 28 Apr 2025, Pierrick Bouvier wrote: >> On 4/28/25 6:10 PM, BALATON Zoltan wrote: >>> On Mon, 28 Apr 2025, Pierrick Bouvier wrote: >>>> On 4/28/25 11:44 AM, BALATON Zoltan wrote: >>>>> On Mon, 28 Apr 2025, Pierrick Bouvier wrote: >>>>>> On 4/28/25 3:31 AM, BALATON Zoltan wrote: >>>>>>> Since you are touching the lines using DEFINE_MACHINE it's a good >>>>>>> opportunity to change the macro to be more general to be able to keep >>>>>>> using it instead of replacing it with the boiler plate it's supposed to >>>>>>> hide. Adding one or two more parameters to the macro is not a big >>>>>>> change >>>>>>> so I don't see why you don't want to do it. This could be addressed >>>>>>> later >>>>>>> to revert to use the macro again but in practice it will not be >>>>>>> addressed >>>>>>> because everybody will be busy doing other things and doing that now >>>>>>> would >>>>>>> prevent some churn. I too, don't like doing unrelated clean up which is >>>>>>> not the main goal, but if it's not much more work then it's not >>>>>>> unreasonable to do it. I only oppose to that if it's a lot of work so I >>>>>>> would not ask such change but what I asked is not unrelated and quite >>>>>>> simple change. >>>>>>> >>>>>>> That said, I can't stop you so if you still don't want to do it now >>>>>>> then >>>>>>> you can move on. I don't care that much as long as you stay within >>>>>>> hw/arm, >>>>>>> but will raise my concern again when you submit a similar patch that >>>>>>> touches parts I care more about. If others don't think it's a problem >>>>>>> and >>>>>>> not bothered by the boiler plate code then it's not so important but >>>>>>> otherwise I think I have a valid point. I remember when I started to >>>>>>> get >>>>>>> to know QEMU it was quite difficult to wade through all the QOM boiler >>>>>>> plate just to see what is related to the actual functionality. These >>>>>>> macros help to make code more readable and accessible for new people. >>>>>> >>>>>> Having been through that recently, I agree with you that it can be hard >>>>>> to >>>>>> follow at first. Luckily, we have perfect compiler based completion for >>>>>> all >>>>>> editors those days (I sincerely hope everyone spent 2 hours configuring >>>>>> this >>>>>> on their own favorite one), and it's easy to see where things are >>>>>> defined >>>>>> and >>>>>> used, even when code is cryptic. >>>>> >>>>> It's not about typing but reading it. The verbose struct definitions are >>>>> hard to follow and makes board code look more complex than it should be. >>>>> >>>>>> That said, pushing to someone adding a new field the responsibility of >>>>>> cleaning up the whole thing is not a fair request. You can't expect your >>>>>> friends to clean your shared house because they brought a cake for >>>>>> dinner. >>>>> >>>>> I tend to get such requests to clean up unrelated things whenever I try >>>>> to >>>>> change anything in PPC Mac emulation which I also complain about and >>>>> think >>>>> is not reasonable to ask. But I did not ask for unrelated cleanup here >>>>> and >>>>> changing the patch so you don't do this: >>>>> >>>>> -DEFINE_MACHINE("none", machine_none_machine_init) >>>>> +static const TypeInfo null_machine_types[] = { >>>>> + { >>>>> + .name = MACHINE_TYPE_NAME("none"), >>>>> + .parent = TYPE_MACHINE, >>>>> + .class_init = null_machine_class_init, >>>>> + }, >>>>> +}; >>>>> + >>>>> +DEFINE_TYPES(null_machine_types) >>>>> >>>>> but instead add the .interfaces field to a variant of DEFINE_MACHINE once >>>>> and keep the one line definition is not something unreasonable to ask. I >>>>> think you can ask your friends to not make a mess in the shared house >>>>> while having a party or at least clean up after that. Adding one more >>>>> parameter to the macro is also simple to do so I don't get why you're so >>>>> opposed to this. >>>>> >>>> >>>> Maybe there is a misunderstanding on my side, but it seems that what you >>>> asked is exactly patch 7, which introduce DEFINE_MACHINE_WITH_INTERFACES. >>> >>> Almost but not quite. I don't know why I can't get this through to you. If >>> you compare patch 7 to how DO_OBJECT_DEFINE_TYPE_EXTENDED is defined do >>> you notice the difference in how .interfaces is set? With the same way as >>> in DO_OBJECT_DEFINE_TYPE_EXTENDED you don't need separate InterfaceInfo >>> arm_aarch64_machine_interfaces[] definitions or different macros in the >>> next patch just list the needed interfaces in the machine definitions. >>> >> >> I'm sorry, I don't understand what you want exactly, despite asking several >> times. >> I think it would be more clear if you could apply this series on your side, >> write a small patch showing *exactly* what you expect, and applying this to >> one of the board concerned. Then, we can do the change you request. > > You can pick the patch from this series I've just posted: > https://patchew.org/QEMU/cover.1746139668.git.balaton@eik.bme.hu/ad355178b2a3fe285854ed2e25b288baf0fd6e05.1746139668.git.balaton@eik.bme.hu/ > it is used in patch 12 of that series for an example. > Sounds good, thanks. @Philippe, could you cherry-pick this patch as part of next iteration? > 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(+)