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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan,

> On 23 Aug 2022, at 17:15, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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"?

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