Hi Jan,
> On 25 Aug 2022, at 08:47, Jan Beulich <jbeul...@suse.com> wrote:
>
> On 24.08.2022 17:29, Bertrand Marquis wrote:
>> When Xen is compiled for x86 on an arm machine, libacpi build is failing
>> due to a wrong include path:
>> - arch-x86/xen.h includes xen.h
>> - xen.h includes arch-arm.h (as __i386__ and __x86_64__ are not defined
>> but arm ones are).
>>
>> To solve this issue move XEN_ACPI_ definitions in a new header
>> guest-acpi.h that can be included cleanly by mk_dsdt.c
>>
>> Previous users needing any of the XEN_ACPI_ definitions will now need to
>> include arch-x86/guest-acpi.h instead of arch-x86/xen.h
>>
>> Fixes: d6ac8e22c7c5 ("acpi/x86: define ACPI IO registers for PVH
>> guests")
>
> Nit: Please don't wrap this line.
Ok
>
>> Signed-off-by: Bertrand Marquis <bertrand.marq...@arm.com>
>> ---
>> The x86 header is including ../xen.h before the ifndef/define so that it
>> gets included back by xen.h. This is wrongly making the assumption that
>> we are using an x86 compiler which is not the case when building the
>> tools for x86 on an arm host.
>> Moving the definitions to an independent header is making things cleaner
>> but some might need to include a new header but the risk is low.
>>
>> For the release manager:
>> - risk: very low, the definitions moved are only used in mk_dsdt and
>> external users would just have to include the new header.
>> - advantage: we can now compile xen for x86 on arm build machines
>
> You will want to actually Cc him on v2, so he can ack the change (or
> not).
Ack
>
>> --- /dev/null
>> +++ b/xen/include/public/arch-x86/guest-acpi.h
>> @@ -0,0 +1,50 @@
>> +/******************************************************************************
>> + * arch-x86/xen-acpi.h
>
> Stale file name.
Right, forgot to change the content after renaming, will fix.
>
>> + * XEN ACPI interface to x86 Xen.
>
> Perhaps also here s/XEN/Guest/.
Ok.
>
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> copy
>> + * of this software and associated documentation files (the "Software"), to
>> + * deal in the Software without restriction, including without limitation
>> the
>> + * rights to use, copy, modify, merge, publish, distribute, sublicense,
>> and/or
>> + * sell copies of the Software, and to permit persons to whom the Software
>> is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> THE
>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> + * DEALINGS IN THE SOFTWARE.
>> + *
>> + */
>> +
>> +#ifndef __XEN_PUBLIC_ARCH_X86_XEN_ACPI_H__
>> +#define __XEN_PUBLIC_ARCH_X86_XEN_ACPI_H__
>
> Please make the guard match the file name.
Yes
>
>> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
>
> While separating it out, may I suggest to limit this to just the tool
> stack? There's no use of these #define-s in the hypervisor, and none
> is to be expected. (Of course this will want justifying this way in
> the description.)
Ok
Thanks for the review
Cheers
Bertrand
>
> Jan
>
>> +/* Location of online VCPU bitmap. */
>> +#define XEN_ACPI_CPU_MAP 0xaf00
>> +#define XEN_ACPI_CPU_MAP_LEN ((HVM_MAX_VCPUS + 7) / 8)
>> +
>> +/* GPE0 bit set during CPU hotplug */
>> +#define XEN_ACPI_GPE0_CPUHP_BIT 2
>> +
>> +#endif
>> +
>> +#endif /* __XEN_PUBLIC_ARCH_X86_XEN_ACPI_H__ */
