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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

> 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

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