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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.08.2022 16:41, Bertrand Marquis 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.

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