> On 23 Aug 2022, at 16:45, Jan Beulich <[email protected]> wrote: > > On 23.08.2022 17:09, Bertrand Marquis wrote: >>> On 23 Aug 2022, at 15:41, Bertrand Marquis <[email protected]> wrote: >>>> On 23 Aug 2022, at 15:31, Jan Beulich <[email protected]> wrote: >>>> On 23.08.2022 15:34, Bertrand Marquis wrote: >>>>>> On 23 Aug 2022, at 13:33, Jan Beulich <[email protected]> wrote: >>>>>> On 23.08.2022 12:24, Bertrand Marquis wrote: >>>>>>> --- a/tools/libacpi/mk_dsdt.c >>>>>>> +++ b/tools/libacpi/mk_dsdt.c >>>>>>> @@ -18,6 +18,16 @@ >>>>>>> #include <stdlib.h> >>>>>>> #include <stdbool.h> >>>>>>> #if defined(CONFIG_X86) >>>>>>> +/* >>>>>>> + * When building on non x86 host, arch-x86/xen.h will include xen.h >>>>>>> which will >>>>>>> + * try to include the arch xen.h (for example if built on arm, >>>>>>> x86/xen.h will >>>>>>> + * include xen.h which will include arch-arm.h). >>>>>>> + * To prevent this effect, define x86 to have the proper sub arch >>>>>>> included when >>>>>>> + * the compiler does not define it. >>>>>>> + */ >>>>>>> +#if !(defined(__i386__) || defined(__x86_64__)) >>>>>>> +#define __x86_64__ >>>>>>> +#endif >>>>>> >>>>>> Besides being confusing this depends on the order of checks in xen.h. >>>>>> >>>>>>> #include <xen/arch-x86/xen.h> >>>>>>> #include <xen/hvm/hvm_info_table.h> >>>>>>> #elif defined(CONFIG_ARM_64) >>>>>> >>>>>> At the very least you will want to #undef the auxiliary define as soon >>>>>> as practically possible. >>>>> >>>>> Ack >>>>> >>>>>> >>>>>> But I think a different solution will want finding. Did you check what >>>>>> the #include is needed for, really? I've glanced through the file >>>>>> without being able to spot anything ... After all this is a build tool, >>>>>> which generally can't correctly use many of the things declared in the >>>>>> header. >>>>> >>>>> As stated in the comment after the commit message, this is not a good >>>>> solution but an hack. >>>>> >>>>> Now I do not completely agree here, the tool is not really the problem >>>>> but the headers are. >>>> >>>> Well - the issue is the tool depending on these headers. >>> >>> Yes but the tool itself cannot solve the issue, we need to have the values >>> in properly accessible headers. >>> >>>> >>>>> There is not such an issue on arm. >>>> >>>> Then why does the tool include xen/arch-arm.h for Arm64? >>> >>> Because this header defines the values required and as no such thing as >>> include xen.h. >>> The point is on arm, the arch-arm.h header does not depend on per cpu >>> defines. >>> >>>> >>>>> The tool needs at least: >>>>> HVM_MAX_VCPUS >>>>> XEN_ACPI_CPU_MAP >>>>> XEN_ACPI_CPU_MAP_LEN >>>>> XEN_ACPI_GPE0_CPUHP_BIT >>>>> >>>>> Which are defined in arch-x86/xen.h and hvm_info_table.h. >>>>> >>>>> I am not quite sure how to get those without the current include >>>> >>>> 1) Move those #define-s to a standalone header, which the ones >>>> currently defining them would simply include. The tool would then >>>> include _only_ this one header. >>> >>> Shouldn’t we try to unify a little bit what is done on arm and x86 here ? >>> Not only for this tool but in general in the public headers > > Where possible I'm all for it. > >>> I will try to reduce the problem a bit more to find what we would need to >>> pull out in a standalone header. >> >> Only the 3 XEN_ACPI_ are needed > > Yet XEN_ACPI_CPU_MAP_LEN drives from HVM_MAX_VCPUS. > >> and those are in fact only used by mk_dsdt.c. > > Well - that's the only thing we talk about here. Building target code > is fine to use the headers. Building code to run on the build system > is where the headers should not be used. > >> How about moving those to a xen-acpi.h header and include that one in xen.h ? > > In principle okay, if there wasn't the need for HVM_MAX_VCPUS. With a > suitable comment it may be okay to live there. I'd be curious what > others think.
The problem with this already exists in the current status as this is defined in hvm_info_table.h which is never included from arch-x86/xen.h Including hvm_info_table.h from xen-acpi.h could create include path issues. But as those are used nowhere apart from mk_dsdt, I would probably skip the include of xen-acpi.h from xen.h. Any chance that those XEN_ACPI_ are needed by some external tools that could get broken by this modification ? > >> Other solution as those are only used in mk_dsdt, I could just define them >> there … > > Please let's try hard to avoid doing so. Agree > >>>> 2) Seddery on the headers, producing a local one to be used by the >>>> tool. >>> >>> You mean autogenerating something ? This would just move the problem. > > Why? You would have to handle the arch specific part there. I would rather prevent any auto-generation here and stick with better defined headers. > > Jan
