Re: [PATCH v1] x86/public: move XEN_ACPI_ in a new header

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan,

> On 25 Aug 2022, at 08:47, Jan Beulich <jbeulich@xxxxxxxx> 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.marquis@xxxxxxx>
>> ---
>> 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__ */