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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> 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