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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan,

> On 24 Aug 2022, at 14:05, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 24.08.2022 14:43, Bertrand Marquis wrote:
>> 
>> 
>>> On 24 Aug 2022, at 08:37, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> 
>>> On 23.08.2022 17:56, Bertrand Marquis wrote:
>>>>> On 23 Aug 2022, at 16:45, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>> On 23.08.2022 17:09, Bertrand Marquis wrote:
>>>>>> 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
>>> 
>>> You're referring to it being necessary to explicitly include both headers.
>>> That's not what I'm referring to, though: The tool imo shouldn't include
>>> hvm_info_table.h, and hence the HVM_MAX_VCPUS would need to move as well.
>> 
>> Any suggestion where ?
> 
> Not really, no. That's why I said this is the one part where improvement
> is more difficult. Otoh hvm_info_table.h is self-contained right now and
> doesn't even produce potentially misleading struct layout for the one
> struct it declares. So perhaps not too bad if left alone.
> 
>> The more I dig, the more I find that everything is including xen.h and going 
>> round.
>> Arch-x86_*.h headers are including arch-x86/xen.h including xen.h
> 
> Indeed, all quite odd.
> 
>>>> Including hvm_info_table.h from xen-acpi.h could create include path 
>>>> issues.
>>> 
>>> Include path issues? Both are / would be public headers. But as said, I
>>> don't think any new header introduced for the purpose at hand should
>>> include _any_ other public header.
>> 
>> For now I can create a arch-x86/xen-acpi.h and move there the XEN_ACPI_*
>> definitions and include that one instead in mk_dsdt.h.
>> The change will be small and should not have much impact.
>> 
>> Modifying HVM_MAX_VCPUS is not per say needed and I think would not be
>> enough to make the situation cleaner.
>> 
>>> 
>>>> But as those are used nowhere apart from mk_dsdt, I would probably skip the
>>>> include of xen-acpi.h from xen.h.
>>> 
>>> Hmm, yes, that's reasonable I guess as far as XEN_ACPI_* go. Of course
>>> HVM_MAX_VCPUS is a different matter.
>>> 
>>>> Any chance that those XEN_ACPI_ are needed by some external tools that
>>>> could get broken by this modification ?
>>> 
>>> Requiring them to include another header is, I think, a tolerable form
>>> of breakage, the more that such breakage isn't very likely anyway.
>> 
>> Then if you are ok with it, I will just submit the xen-acpi.h creation patch 
>> and fix
>> mk_dsdt compilation for x86 on arm.
>> 
>> The rest would require more thinking and I do not think it should be done 
>> now.
>> 
>> Can you confirm you agree ?
> 
> Almost - I don't like xen-acpi.h as the name of the new header. Just
> arch-x86/acpi.h would likely be too generic, so I'd like to suggest
> arch-x86/hvm-acpi.h or arch-x86/guest-acpi.h. I have a slight
> preference to the latter, because "hvm" also covers "pvh", yet PVH
> Dom0 is dealt with entirely differently ACPI-wise. Plus "guest"
> isn't misleading as to PV, because PV guests don't have ACPI anyway.

Guest-acpi.h it will be.

Bertrand

> 
> Jan