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")
Signed-off-by: Bertrand Marquis <bertrand.marquis@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
---
Changes in v1:
- was "libacpi: Fix cross building x86 on arm"
- move XEN_ACPI_ definitions in a new header guest-acpi.h
- adapt mk_dsdt.c
- remove todo in public header
---
 tools/libacpi/mk_dsdt.c                  |  2 +-
 xen/include/public/arch-x86/guest-acpi.h | 50 ++++++++++++++++++++++++
 xen/include/public/arch-x86/xen.h        |  6 ---
 3 files changed, 51 insertions(+), 7 deletions(-)
 create mode 100644 xen/include/public/arch-x86/guest-acpi.h

diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
index c5ba4c0b2fd3..1176da80ef44 100644
--- a/tools/libacpi/mk_dsdt.c
+++ b/tools/libacpi/mk_dsdt.c
@@ -18,7 +18,7 @@
 #include <stdlib.h>
 #include <stdbool.h>
 #if defined(CONFIG_X86)
-#include <xen/arch-x86/xen.h>
+#include <xen/arch-x86/guest-acpi.h>
 #include <xen/hvm/hvm_info_table.h>
 #elif defined(CONFIG_ARM_64)
 #include <xen/arch-arm.h>
diff --git a/xen/include/public/arch-x86/guest-acpi.h b/xen/include/public/arch-x86/guest-acpi.h
new file mode 100644
index 000000000000..eb288faf7bba
--- /dev/null
+++ b/xen/include/public/arch-x86/guest-acpi.h
@@ -0,0 +1,50 @@
+/******************************************************************************
+ * arch-x86/xen-acpi.h
+ *
+ * XEN ACPI interface to x86 Xen.
+ *
+ * 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__
+
+#if defined(__XEN__) || defined(__XEN_TOOLS__)
+
+/* 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__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index 58a1e87ee971..546dd4496ac6 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -325,12 +325,6 @@ struct xen_arch_domainconfig {
 /* Max  XEN_X86_* constant. Used for ABI checking. */
 #define XEN_X86_MISC_FLAGS_MAX XEN_X86_ASSISTED_X2APIC
 
-/* 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
 
 /*
-- 
2.25.1

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.

> Signed-off-by: Bertrand Marquis <bertrand.marquis@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).

> --- /dev/null
> +++ b/xen/include/public/arch-x86/guest-acpi.h
> @@ -0,0 +1,50 @@
> +/******************************************************************************
> + * arch-x86/xen-acpi.h

Stale file name.

> + * XEN ACPI interface to x86 Xen.

Perhaps also here s/XEN/Guest/.

> + * 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.

> +#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.)

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__ */

Hi Jan,

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