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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan