| Message ID | c3d431217010f669de809a76c2f1c15a0313ae53.1661246753.git.bertrand.marquis@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] libacpi: Fix cross building x86 on arm | expand |
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. 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. > diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h > index 58a1e87ee971..ea33a56eb6a0 100644 > --- a/xen/include/public/arch-x86/xen.h > +++ b/xen/include/public/arch-x86/xen.h > @@ -24,6 +24,7 @@ > * Copyright (c) 2004-2006, K A Fraser > */ > > +/* TODO: when cross building, this will include the wrong arch header */ > #include "../xen.h" I'm firmly against adding such a comment in a public header, the more that it's misleading: Cross-building of Xen, for example, works quite fine. The issue is limited to HOSTCC != CC (or yet more precisely the target architecture of each), afaict. Jan
Hi Jan, > On 23 Aug 2022, at 13:33, Jan Beulich <jbeulich@suse.com> 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. There is not such an issue on arm. 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 > >> diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h >> index 58a1e87ee971..ea33a56eb6a0 100644 >> --- a/xen/include/public/arch-x86/xen.h >> +++ b/xen/include/public/arch-x86/xen.h >> @@ -24,6 +24,7 @@ >> * Copyright (c) 2004-2006, K A Fraser >> */ >> >> +/* TODO: when cross building, this will include the wrong arch header */ >> #include "../xen.h" > > I'm firmly against adding such a comment in a public header, the more > that it's misleading: Cross-building of Xen, for example, works quite > fine. The issue is limited to HOSTCC != CC (or yet more precisely the > target architecture of each), afaict. Point was the todo was more to show where the issue is coming from. I am really ok to remove this. Cheers Bertrand > > Jan
On 23.08.2022 15:34, Bertrand Marquis wrote: >> On 23 Aug 2022, at 13:33, Jan Beulich <jbeulich@suse.com> 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. > There is not such an issue on arm. Then why does the tool include xen/arch-arm.h for Arm64? > 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. 2) Seddery on the headers, producing a local one to be used by the tool. Jan
> On 23 Aug 2022, at 15:31, Jan Beulich <jbeulich@suse.com> wrote: > > On 23.08.2022 15:34, Bertrand Marquis wrote: >>> On 23 Aug 2022, at 13:33, Jan Beulich <jbeulich@suse.com> 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 I will try to reduce the problem a bit more to find what we would need to pull out in a standalone header. > > 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. Bertrand > > Jan
Hi, > On 23 Aug 2022, at 15:41, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: > > > >> On 23 Aug 2022, at 15:31, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 23.08.2022 15:34, Bertrand Marquis wrote: >>>> On 23 Aug 2022, at 13:33, Jan Beulich <jbeulich@suse.com> 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 > > 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 and those are in fact only used by mk_dsdt.c. How about moving those to a xen-acpi.h header and include that one in xen.h ? Other solution as those are only used in mk_dsdt, I could just define them there … This is the commit which created the issue: https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=d6ac8e22c7c5525db1da79fd1d1f03ee6b557f0d Any other idea how to properly fix this ? Cheers Bertrand > >> >> 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. > > Bertrand > >> >> Jan
On 23.08.2022 17:09, Bertrand Marquis wrote: >> On 23 Aug 2022, at 15:41, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: >>> On 23 Aug 2022, at 15:31, Jan Beulich <jbeulich@suse.com> wrote: >>> On 23.08.2022 15:34, Bertrand Marquis wrote: >>>>> On 23 Aug 2022, at 13:33, Jan Beulich <jbeulich@suse.com> 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. > 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. >>> 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? Jan
> On 23 Aug 2022, at 16:45, Jan Beulich <jbeulich@suse.com> wrote: > > On 23.08.2022 17:09, Bertrand Marquis wrote: >>> On 23 Aug 2022, at 15:41, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: >>>> On 23 Aug 2022, at 15:31, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 23.08.2022 15:34, Bertrand Marquis wrote: >>>>>> On 23 Aug 2022, at 13:33, Jan Beulich <jbeulich@suse.com> 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
On 23.08.2022 16:41, Bertrand Marquis wrote: > > >> On 23 Aug 2022, at 15:31, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 23.08.2022 15:34, Bertrand Marquis wrote: >>>> On 23 Aug 2022, at 13:33, Jan Beulich <jbeulich@suse.com> 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. At first I was surprised you get away there without including xen.h - this may change at any time, as soon as you grow a dependency. But then the inclusion by arch-x86/xen.h looks suspicious, since xen.h itself includes arch-x86/xen.h (first thing), so unless I'm missing something arch-x86/xen.h can't really have a dependency on xen.h. So maybe in the short term you could get away with removing that include as a "fix"? Jan
Hi Jan, > On 23 Aug 2022, at 17:15, Jan Beulich <jbeulich@suse.com> wrote: > > On 23.08.2022 16:41, Bertrand Marquis wrote: >> >> >>> On 23 Aug 2022, at 15:31, Jan Beulich <jbeulich@suse.com> wrote: >>> >>> On 23.08.2022 15:34, Bertrand Marquis wrote: >>>>> On 23 Aug 2022, at 13:33, Jan Beulich <jbeulich@suse.com> 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. > > At first I was surprised you get away there without including xen.h - > this may change at any time, as soon as you grow a dependency. > > But then the inclusion by arch-x86/xen.h looks suspicious, since xen.h > itself includes arch-x86/xen.h (first thing), so unless I'm missing > something arch-x86/xen.h can't really have a dependency on xen.h. So > maybe in the short term you could get away with removing that include > as a "fix"? Just removing the include is ending up in errors (I tried that first). I will dig deeper to check if those are possible to solve but some files including arch-x86/xen.h should actually including xen.h instead and I think the amount of changes might get a bit bigger. I will give it a try. Bertrand > > Jan
On 23.08.2022 17:56, Bertrand Marquis wrote: >> On 23 Aug 2022, at 16:45, Jan Beulich <jbeulich@suse.com> wrote: >> On 23.08.2022 17:09, Bertrand Marquis wrote: >>> 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 You're referring to it being necessary to explicitly include both headers. That's not what I'm referring to, though: The tool imo shouldn't include hvm_info_table.h, and hence the HVM_MAX_VCPUS would need to move as well. > Including hvm_info_table.h from xen-acpi.h could create include path issues. Include path issues? Both are / would be public headers. But as said, I don't think any new header introduced for the purpose at hand should include _any_ other public header. > But as those are used nowhere apart from mk_dsdt, I would probably skip the > include of xen-acpi.h from xen.h. Hmm, yes, that's reasonable I guess as far as XEN_ACPI_* go. Of course HVM_MAX_VCPUS is a different matter. > Any chance that those XEN_ACPI_ are needed by some external tools that > could get broken by this modification ? Requiring them to include another header is, I think, a tolerable form of breakage, the more that such breakage isn't very likely anyway. Jan
> On 24 Aug 2022, at 08:37, Jan Beulich <jbeulich@suse.com> wrote: > > On 23.08.2022 17:56, Bertrand Marquis wrote: >>> On 23 Aug 2022, at 16:45, Jan Beulich <jbeulich@suse.com> wrote: >>> On 23.08.2022 17:09, Bertrand Marquis wrote: >>>> 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 > > You're referring to it being necessary to explicitly include both headers. > That's not what I'm referring to, though: The tool imo shouldn't include > hvm_info_table.h, and hence the HVM_MAX_VCPUS would need to move as well. Any suggestion where ? The more I dig, the more I find that everything is including xen.h and going round. Arch-x86_*.h headers are including arch-x86/xen.h including xen.h > >> Including hvm_info_table.h from xen-acpi.h could create include path issues. > > Include path issues? Both are / would be public headers. But as said, I > don't think any new header introduced for the purpose at hand should > include _any_ other public header. For now I can create a arch-x86/xen-acpi.h and move there the XEN_ACPI_* definitions and include that one instead in mk_dsdt.h. The change will be small and should not have much impact. Modifying HVM_MAX_VCPUS is not per say needed and I think would not be enough to make the situation cleaner. > >> But as those are used nowhere apart from mk_dsdt, I would probably skip the >> include of xen-acpi.h from xen.h. > > Hmm, yes, that's reasonable I guess as far as XEN_ACPI_* go. Of course > HVM_MAX_VCPUS is a different matter. > >> Any chance that those XEN_ACPI_ are needed by some external tools that >> could get broken by this modification ? > > Requiring them to include another header is, I think, a tolerable form > of breakage, the more that such breakage isn't very likely anyway. Then if you are ok with it, I will just submit the xen-acpi.h creation patch and fix mk_dsdt compilation for x86 on arm. The rest would require more thinking and I do not think it should be done now. Can you confirm you agree ? Cheers Bertrand > > Jan
On 24.08.2022 14:43, Bertrand Marquis wrote: > > >> On 24 Aug 2022, at 08:37, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 23.08.2022 17:56, Bertrand Marquis wrote: >>>> On 23 Aug 2022, at 16:45, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 23.08.2022 17:09, Bertrand Marquis wrote: >>>>> 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 >> >> You're referring to it being necessary to explicitly include both headers. >> That's not what I'm referring to, though: The tool imo shouldn't include >> hvm_info_table.h, and hence the HVM_MAX_VCPUS would need to move as well. > > Any suggestion where ? Not really, no. That's why I said this is the one part where improvement is more difficult. Otoh hvm_info_table.h is self-contained right now and doesn't even produce potentially misleading struct layout for the one struct it declares. So perhaps not too bad if left alone. > The more I dig, the more I find that everything is including xen.h and going round. > Arch-x86_*.h headers are including arch-x86/xen.h including xen.h Indeed, all quite odd. >>> Including hvm_info_table.h from xen-acpi.h could create include path issues. >> >> Include path issues? Both are / would be public headers. But as said, I >> don't think any new header introduced for the purpose at hand should >> include _any_ other public header. > > For now I can create a arch-x86/xen-acpi.h and move there the XEN_ACPI_* > definitions and include that one instead in mk_dsdt.h. > The change will be small and should not have much impact. > > Modifying HVM_MAX_VCPUS is not per say needed and I think would not be > enough to make the situation cleaner. > >> >>> But as those are used nowhere apart from mk_dsdt, I would probably skip the >>> include of xen-acpi.h from xen.h. >> >> Hmm, yes, that's reasonable I guess as far as XEN_ACPI_* go. Of course >> HVM_MAX_VCPUS is a different matter. >> >>> Any chance that those XEN_ACPI_ are needed by some external tools that >>> could get broken by this modification ? >> >> Requiring them to include another header is, I think, a tolerable form >> of breakage, the more that such breakage isn't very likely anyway. > > Then if you are ok with it, I will just submit the xen-acpi.h creation patch and fix > mk_dsdt compilation for x86 on arm. > > The rest would require more thinking and I do not think it should be done now. > > Can you confirm you agree ? Almost - I don't like xen-acpi.h as the name of the new header. Just arch-x86/acpi.h would likely be too generic, so I'd like to suggest arch-x86/hvm-acpi.h or arch-x86/guest-acpi.h. I have a slight preference to the latter, because "hvm" also covers "pvh", yet PVH Dom0 is dealt with entirely differently ACPI-wise. Plus "guest" isn't misleading as to PV, because PV guests don't have ACPI anyway. Jan
Hi Jan, > On 24 Aug 2022, at 14:05, Jan Beulich <jbeulich@suse.com> wrote: > > On 24.08.2022 14:43, Bertrand Marquis wrote: >> >> >>> On 24 Aug 2022, at 08:37, Jan Beulich <jbeulich@suse.com> wrote: >>> >>> On 23.08.2022 17:56, Bertrand Marquis wrote: >>>>> On 23 Aug 2022, at 16:45, Jan Beulich <jbeulich@suse.com> wrote: >>>>> On 23.08.2022 17:09, Bertrand Marquis wrote: >>>>>> 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 >>> >>> You're referring to it being necessary to explicitly include both headers. >>> That's not what I'm referring to, though: The tool imo shouldn't include >>> hvm_info_table.h, and hence the HVM_MAX_VCPUS would need to move as well. >> >> Any suggestion where ? > > Not really, no. That's why I said this is the one part where improvement > is more difficult. Otoh hvm_info_table.h is self-contained right now and > doesn't even produce potentially misleading struct layout for the one > struct it declares. So perhaps not too bad if left alone. > >> The more I dig, the more I find that everything is including xen.h and going round. >> Arch-x86_*.h headers are including arch-x86/xen.h including xen.h > > Indeed, all quite odd. > >>>> Including hvm_info_table.h from xen-acpi.h could create include path issues. >>> >>> Include path issues? Both are / would be public headers. But as said, I >>> don't think any new header introduced for the purpose at hand should >>> include _any_ other public header. >> >> For now I can create a arch-x86/xen-acpi.h and move there the XEN_ACPI_* >> definitions and include that one instead in mk_dsdt.h. >> The change will be small and should not have much impact. >> >> Modifying HVM_MAX_VCPUS is not per say needed and I think would not be >> enough to make the situation cleaner. >> >>> >>>> But as those are used nowhere apart from mk_dsdt, I would probably skip the >>>> include of xen-acpi.h from xen.h. >>> >>> Hmm, yes, that's reasonable I guess as far as XEN_ACPI_* go. Of course >>> HVM_MAX_VCPUS is a different matter. >>> >>>> Any chance that those XEN_ACPI_ are needed by some external tools that >>>> could get broken by this modification ? >>> >>> Requiring them to include another header is, I think, a tolerable form >>> of breakage, the more that such breakage isn't very likely anyway. >> >> Then if you are ok with it, I will just submit the xen-acpi.h creation patch and fix >> mk_dsdt compilation for x86 on arm. >> >> The rest would require more thinking and I do not think it should be done now. >> >> Can you confirm you agree ? > > Almost - I don't like xen-acpi.h as the name of the new header. Just > arch-x86/acpi.h would likely be too generic, so I'd like to suggest > arch-x86/hvm-acpi.h or arch-x86/guest-acpi.h. I have a slight > preference to the latter, because "hvm" also covers "pvh", yet PVH > Dom0 is dealt with entirely differently ACPI-wise. Plus "guest" > isn't misleading as to PV, because PV guests don't have ACPI anyway. Guest-acpi.h it will be. Bertrand > > Jan
diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c index c5ba4c0b2fd3..ba5468f43c13 100644 --- 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 #include <xen/arch-x86/xen.h> #include <xen/hvm/hvm_info_table.h> #elif defined(CONFIG_ARM_64) diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h index 58a1e87ee971..ea33a56eb6a0 100644 --- a/xen/include/public/arch-x86/xen.h +++ b/xen/include/public/arch-x86/xen.h @@ -24,6 +24,7 @@ * Copyright (c) 2004-2006, K A Fraser */ +/* TODO: when cross building, this will include the wrong arch header */ #include "../xen.h" #ifndef __XEN_PUBLIC_ARCH_X86_XEN_H__
When Xen is compiled for x86 on an arm machine, libacpi build is failing due to a wrong include path: - arch-x86/xen.h includes xen.h - xen.h includes arch-arm.h (as __i386__ and __x86_64__ are not defined but arm ones are). To workaround this for now, enforce defining __x86_64__ in mk_dsdt.c when compiled for x86 to follow the right include path. Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> --- The x86 header is including ../xen.h before the ifndef/define so that it gets included back by xen.h. This is wrongly making the assumption that we are using an x86 compiler which is not the case when building the tools for x86 on an arm host. This patch is not a good solution but the headers are doing some weird stuff which are going back to 2008 in the git history and the commit message do not include any valid reason. --- --- tools/libacpi/mk_dsdt.c | 10 ++++++++++ xen/include/public/arch-x86/xen.h | 1 + 2 files changed, 11 insertions(+)