Re: [RFC PATCH] libacpi: Fix cross building x86 on arm

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

> On 23 Aug 2022, at 16:45, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 23.08.2022 17:09, Bertrand Marquis wrote:
>>> On 23 Aug 2022, at 15:41, Bertrand Marquis <Bertrand.Marquis@xxxxxxx> wrote:
>>>> On 23 Aug 2022, at 15:31, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> On 23.08.2022 15:34, Bertrand Marquis wrote:
>>>>>> On 23 Aug 2022, at 13:33, Jan Beulich <jbeulich@xxxxxxxx> 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