[PATCH v3 0/7] xen: fix dom0 PV boot on some AMD machines

[PATCH v3 0/7] xen: fix dom0 PV boot on some AMD machines

[Juergen Gross]

There have been reports of failed boots with Xen due to an overlap of
the kernel's memory with ACPI NVS reported in the E820 map of the host.

This series fixes this issue by moving the NVS area in dom0 to some
higher address.

Changes in V2:
- split of v1 patch 5
- new patch 6

Changes in V3:
- addressed comments

Juergen Gross (7):
  xen: use correct end address of kernel for conflict checking
  xen: introduce generic helper checking for memory map conflicts
  xen: move checks for e820 conflicts further up
  xen: move max_pfn in xen_memory_setup() out of function scope
  xen: add capability to remap non-RAM pages to different PFNs
  xen: allow mapping ACPI data using a different physical address
  xen: tolerate ACPI NVS memory overlapping with Xen allocated memory

 arch/x86/include/asm/acpi.h        |   8 ++
 arch/x86/kernel/acpi/boot.c        |  10 ++
 arch/x86/kernel/mmconf-fam10h_64.c |   2 +-
 arch/x86/kernel/x86_init.c         |   2 +-
 arch/x86/xen/mmu_pv.c              |   5 +-
 arch/x86/xen/p2m.c                 |  99 ++++++++++++++
 arch/x86/xen/setup.c               | 202 ++++++++++++++++++++++-------
 arch/x86/xen/xen-ops.h             |   6 +-
 8 files changed, 282 insertions(+), 52 deletions(-)

-- 
2.43.0

[PATCH v3 6/7] xen: allow mapping ACPI data using a different physical address

[Juergen Gross]

When running as a Xen PV dom0 the system needs to map ACPI data of the
host using host physical addresses, while those addresses can conflict
with the guest physical addresses of the loaded linux kernel. The same
problem might apply in case a PV guest is configured to use the host
memory map.

This conflict can be solved by mapping the ACPI data to a different
guest physical address, but mapping the data via acpi_os_ioremap()
must still be possible using the host physical address, as this
address might be generated by AML when referencing some of the ACPI
data.

When configured to support running as a Xen PV domain, have an
implementation of acpi_os_ioremap() being aware of the possibility to
need above mentioned translation of a host physical address to the
guest physical address.

This modification requires to fix some #include of asm/acpi.h in x86
code to use linux/acpi.h instead.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch (Jan Beulich)
V3:
- add const attribute (Jan Beulich)
- guard ACPI related code with CONFIG_ACPI (Jan Beulich)
- use CONFIG_XEN_PV instead of CONFIG_XEN_PV_DOM0
---
 arch/x86/include/asm/acpi.h        |  8 +++++++
 arch/x86/kernel/acpi/boot.c        | 10 +++++++++
 arch/x86/kernel/mmconf-fam10h_64.c |  2 +-
 arch/x86/kernel/x86_init.c         |  2 +-
 arch/x86/xen/p2m.c                 | 34 ++++++++++++++++++++++++++++++
 arch/x86/xen/setup.c               |  2 +-
 6 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
index 21bc53f5ed0c..5ab1a4598d00 100644
--- a/arch/x86/include/asm/acpi.h
+++ b/arch/x86/include/asm/acpi.h
@@ -174,6 +174,14 @@ void acpi_generic_reduced_hw_init(void);
 void x86_default_set_root_pointer(u64 addr);
 u64 x86_default_get_root_pointer(void);
 
+#ifdef CONFIG_XEN_PV
+/* A Xen PV domain needs a special acpi_os_ioremap() handling. */
+extern void __iomem * (*acpi_os_ioremap)(acpi_physical_address phys,
+					 acpi_size size);
+void __iomem *x86_acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
+#define acpi_os_ioremap acpi_os_ioremap
+#endif
+
 #else /* !CONFIG_ACPI */
 
 #define acpi_lapic 0
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 9f4618dcd704..2de8510c56dd 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -1778,3 +1778,13 @@ u64 x86_default_get_root_pointer(void)
 {
 	return boot_params.acpi_rsdp_addr;
 }
+
+#ifdef CONFIG_XEN_PV
+void __iomem *x86_acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
+{
+	return ioremap_cache(phys, size);
+}
+
+void __iomem * (*acpi_os_ioremap)(acpi_physical_address phys, acpi_size size) =
+	x86_acpi_os_ioremap;
+#endif
diff --git a/arch/x86/kernel/mmconf-fam10h_64.c b/arch/x86/kernel/mmconf-fam10h_64.c
index c94dec6a1834..8347a29f9db4 100644
--- a/arch/x86/kernel/mmconf-fam10h_64.c
+++ b/arch/x86/kernel/mmconf-fam10h_64.c
@@ -9,12 +9,12 @@
 #include <linux/pci.h>
 #include <linux/dmi.h>
 #include <linux/range.h>
+#include <linux/acpi.h>
 
 #include <asm/pci-direct.h>
 #include <linux/sort.h>
 #include <asm/io.h>
 #include <asm/msr.h>
-#include <asm/acpi.h>
 #include <asm/mmconfig.h>
 #include <asm/pci_x86.h>
 
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 82b128d3f309..47ef8af23101 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -8,8 +8,8 @@
 #include <linux/ioport.h>
 #include <linux/export.h>
 #include <linux/pci.h>
+#include <linux/acpi.h>
 
-#include <asm/acpi.h>
 #include <asm/bios_ebda.h>
 #include <asm/paravirt.h>
 #include <asm/pci_x86.h>
diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index 5b2aeae6f9e4..a64e9562733e 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -70,6 +70,7 @@
 #include <linux/memblock.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
+#include <linux/acpi.h>
 
 #include <asm/cache.h>
 #include <asm/setup.h>
@@ -836,6 +837,33 @@ void __init xen_do_remap_nonram(void)
 	pr_info("Remapped %u non-RAM page(s)\n", remapped);
 }
 
+#ifdef CONFIG_ACPI
+/*
+ * Xen variant of acpi_os_ioremap() taking potentially remapped non-RAM
+ * regions into acount.
+ * Any attempt to map an area crossing a remap boundary will produce a
+ * WARN() splat.
+ */
+static void __iomem *xen_acpi_os_ioremap(acpi_physical_address phys,
+					 acpi_size size)
+{
+	unsigned int i;
+	const struct nonram_remap *remap = xen_nonram_remap;
+
+	for (i = 0; i < nr_nonram_remap; i++) {
+		if (phys + size > remap->maddr &&
+		    phys < remap->maddr + remap->size) {
+			WARN_ON(phys < remap->maddr ||
+				phys + size > remap->maddr + remap->size);
+			phys = remap->paddr + phys - remap->maddr;
+			break;
+		}
+	}
+
+	return x86_acpi_os_ioremap(phys, size);
+}
+#endif /* CONFIG_ACPI */
+
 /*
  * Add a new non-RAM remap entry.
  * In case of no free entry found, just crash the system.
@@ -850,6 +878,12 @@ void __init xen_add_remap_nonram(phys_addr_t maddr, phys_addr_t paddr,
 		BUG();
 	}
 
+#ifdef CONFIG_ACPI
+	/* Switch to the Xen acpi_os_ioremap() variant. */
+	if (nr_nonram_remap == 0)
+		acpi_os_ioremap = xen_acpi_os_ioremap;
+#endif
+
 	xen_nonram_remap[nr_nonram_remap].maddr = maddr;
 	xen_nonram_remap[nr_nonram_remap].paddr = paddr;
 	xen_nonram_remap[nr_nonram_remap].size = size;
diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index 2c79bb5a9cd0..1114e49937da 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -15,12 +15,12 @@
 #include <linux/cpuidle.h>
 #include <linux/cpufreq.h>
 #include <linux/memory_hotplug.h>
+#include <linux/acpi.h>
 
 #include <asm/elf.h>
 #include <asm/vdso.h>
 #include <asm/e820/api.h>
 #include <asm/setup.h>
-#include <asm/acpi.h>
 #include <asm/numa.h>
 #include <asm/idtentry.h>
 #include <asm/xen/hypervisor.h>
-- 
2.43.0

Re: [PATCH v3 6/7] xen: allow mapping ACPI data using a different physical address

[Jan Beulich]

On 10.09.2024 12:39, Juergen Gross wrote:
> When running as a Xen PV dom0 the system needs to map ACPI data of the
> host using host physical addresses, while those addresses can conflict
> with the guest physical addresses of the loaded linux kernel. The same
> problem might apply in case a PV guest is configured to use the host
> memory map.
> 
> This conflict can be solved by mapping the ACPI data to a different
> guest physical address, but mapping the data via acpi_os_ioremap()
> must still be possible using the host physical address, as this
> address might be generated by AML when referencing some of the ACPI
> data.
> 
> When configured to support running as a Xen PV domain, have an
> implementation of acpi_os_ioremap() being aware of the possibility to
> need above mentioned translation of a host physical address to the
> guest physical address.
> 
> This modification requires to fix some #include of asm/acpi.h in x86
> code to use linux/acpi.h instead.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with a request to comment a tiny bit more:

> @@ -836,6 +837,33 @@ void __init xen_do_remap_nonram(void)
>  	pr_info("Remapped %u non-RAM page(s)\n", remapped);
>  }
>  
> +#ifdef CONFIG_ACPI
> +/*
> + * Xen variant of acpi_os_ioremap() taking potentially remapped non-RAM
> + * regions into acount.

(Nit: account)

> + * Any attempt to map an area crossing a remap boundary will produce a
> + * WARN() splat.
> + */
> +static void __iomem *xen_acpi_os_ioremap(acpi_physical_address phys,
> +					 acpi_size size)
> +{
> +	unsigned int i;
> +	const struct nonram_remap *remap = xen_nonram_remap;
> +
> +	for (i = 0; i < nr_nonram_remap; i++) {
> +		if (phys + size > remap->maddr &&
> +		    phys < remap->maddr + remap->size) {
> +			WARN_ON(phys < remap->maddr ||
> +				phys + size > remap->maddr + remap->size);
> +			phys = remap->paddr + phys - remap->maddr;

This might be slightly easier / more logical to read as

			phys += remap->paddr - remap->maddr;

Also because of "phys" not consistently expressing a physical address
(when you need convert it, the incoming value is a machine address) a
comment may help here. In fact at the first glance (and despite having
seen the code before) I thought the translation was done the wrong way
round, simply because of the name of the variable.

Jan

Re: [PATCH v3 6/7] xen: allow mapping ACPI data using a different physical address

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2637 bytes --]

On 10.09.24 14:34, Jan Beulich wrote:
> On 10.09.2024 12:39, Juergen Gross wrote:
>> When running as a Xen PV dom0 the system needs to map ACPI data of the
>> host using host physical addresses, while those addresses can conflict
>> with the guest physical addresses of the loaded linux kernel. The same
>> problem might apply in case a PV guest is configured to use the host
>> memory map.
>>
>> This conflict can be solved by mapping the ACPI data to a different
>> guest physical address, but mapping the data via acpi_os_ioremap()
>> must still be possible using the host physical address, as this
>> address might be generated by AML when referencing some of the ACPI
>> data.
>>
>> When configured to support running as a Xen PV domain, have an
>> implementation of acpi_os_ioremap() being aware of the possibility to
>> need above mentioned translation of a host physical address to the
>> guest physical address.
>>
>> This modification requires to fix some #include of asm/acpi.h in x86
>> code to use linux/acpi.h instead.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with a request to comment a tiny bit more:
> 
>> @@ -836,6 +837,33 @@ void __init xen_do_remap_nonram(void)
>>   	pr_info("Remapped %u non-RAM page(s)\n", remapped);
>>   }
>>   
>> +#ifdef CONFIG_ACPI
>> +/*
>> + * Xen variant of acpi_os_ioremap() taking potentially remapped non-RAM
>> + * regions into acount.
> 
> (Nit: account)

Indeed.

> 
>> + * Any attempt to map an area crossing a remap boundary will produce a
>> + * WARN() splat.
>> + */
>> +static void __iomem *xen_acpi_os_ioremap(acpi_physical_address phys,
>> +					 acpi_size size)
>> +{
>> +	unsigned int i;
>> +	const struct nonram_remap *remap = xen_nonram_remap;
>> +
>> +	for (i = 0; i < nr_nonram_remap; i++) {
>> +		if (phys + size > remap->maddr &&
>> +		    phys < remap->maddr + remap->size) {
>> +			WARN_ON(phys < remap->maddr ||
>> +				phys + size > remap->maddr + remap->size);
>> +			phys = remap->paddr + phys - remap->maddr;
> 
> This might be slightly easier / more logical to read as
> 
> 			phys += remap->paddr - remap->maddr;
> 
> Also because of "phys" not consistently expressing a physical address
> (when you need convert it, the incoming value is a machine address) a
> comment may help here. In fact at the first glance (and despite having
> seen the code before) I thought the translation was done the wrong way
> round, simply because of the name of the variable.

Will add a comment and change the line as you suggest.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]