[PATCH 2/2] x86/xen: fix memblock_reserve() usage on PVH

[PATCH 2/2] x86/xen: fix memblock_reserve() usage on PVH

[Roger Pau Monne]

The current usage of memblock_reserve() in init_pvh_bootparams() is done before
the .bss is zeroed, and that used to be fine when
memblock_reserved_init_regions implicitly ended up in the .meminit.data
section.  However after commit 73db3abdca58c memblock_reserved_init_regions
ends up in the .bss section, thus breaking it's usage before the .bss is
cleared.

Move and rename the call to xen_reserve_extra_memory() so it's done in the
x86_init.oem.arch_setup hook, which gets executed after the .bss has been
zeroed, but before calling e820__memory_setup().

Fixes: 38620fc4e893 ('x86/xen: attempt to inflate the memory balloon on PVH')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
While the commit that introduced the user-noticeable regression is
73db3abdca58c, I think 38620fc4e893 should have been more careful to not
initialize the memblock ranges ahead of the .bss zeroing.
---
 arch/x86/include/asm/xen/hypervisor.h |  5 -----
 arch/x86/platform/pvh/enlighten.c     |  3 ---
 arch/x86/xen/enlighten_pvh.c          | 15 ++++++++++++---
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h
index 64fbd2dbc5b7..a9088250770f 100644
--- a/arch/x86/include/asm/xen/hypervisor.h
+++ b/arch/x86/include/asm/xen/hypervisor.h
@@ -62,11 +62,6 @@ void xen_arch_unregister_cpu(int num);
 #ifdef CONFIG_PVH
 void __init xen_pvh_init(struct boot_params *boot_params);
 void __init mem_map_via_hcall(struct boot_params *boot_params_p);
-#ifdef CONFIG_XEN_PVH
-void __init xen_reserve_extra_memory(struct boot_params *bootp);
-#else
-static inline void xen_reserve_extra_memory(struct boot_params *bootp) { }
-#endif
 #endif
 
 /* Lazy mode for batching updates / context switch */
diff --git a/arch/x86/platform/pvh/enlighten.c b/arch/x86/platform/pvh/enlighten.c
index 8c2d4b8de25d..944e0290f2c0 100644
--- a/arch/x86/platform/pvh/enlighten.c
+++ b/arch/x86/platform/pvh/enlighten.c
@@ -75,9 +75,6 @@ static void __init init_pvh_bootparams(bool xen_guest)
 	} else
 		xen_raw_printk("Warning: Can fit ISA range into e820\n");
 
-	if (xen_guest)
-		xen_reserve_extra_memory(&pvh_bootparams);
-
 	pvh_bootparams.hdr.cmd_line_ptr =
 		pvh_start_info.cmdline_paddr;
 
diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
index 91c6db4ec054..728a4366ca85 100644
--- a/arch/x86/xen/enlighten_pvh.c
+++ b/arch/x86/xen/enlighten_pvh.c
@@ -9,6 +9,7 @@
 #include <asm/io_apic.h>
 #include <asm/hypervisor.h>
 #include <asm/e820/api.h>
+#include <asm/setup.h>
 
 #include <xen/xen.h>
 #include <asm/xen/interface.h>
@@ -41,8 +42,9 @@ EXPORT_SYMBOL_GPL(xen_pvh);
  * hypervisor should notify us which memory ranges are suitable for creating
  * foreign mappings, but that's not yet implemented.
  */
-void __init xen_reserve_extra_memory(struct boot_params *bootp)
+static void __init pvh_reserve_extra_memory(void)
 {
+	struct boot_params *bootp = &boot_params;
 	unsigned int i, ram_pages = 0, extra_pages;
 
 	for (i = 0; i < bootp->e820_entries; i++) {
@@ -94,6 +96,14 @@ void __init xen_reserve_extra_memory(struct boot_params *bootp)
 	}
 }
 
+static void __init pvh_arch_setup(void)
+{
+	pvh_reserve_extra_memory();
+
+	if (xen_initial_domain())
+		xen_add_preferred_consoles();
+}
+
 void __init xen_pvh_init(struct boot_params *boot_params)
 {
 	u32 msr;
@@ -107,8 +117,7 @@ void __init xen_pvh_init(struct boot_params *boot_params)
 	pfn = __pa(hypercall_page);
 	wrmsr_safe(msr, (u32)pfn, (u32)(pfn >> 32));
 
-	if (xen_initial_domain())
-		x86_init.oem.arch_setup = xen_add_preferred_consoles;
+	x86_init.oem.arch_setup = pvh_arch_setup;
 	x86_init.oem.banner = xen_banner;
 
 	xen_efi_init(boot_params);
-- 
2.45.2

Re: [PATCH 2/2] x86/xen: fix memblock_reserve() usage on PVH

[Jürgen Groß]

On 25.07.24 09:31, Roger Pau Monne wrote:
> The current usage of memblock_reserve() in init_pvh_bootparams() is done before
> the .bss is zeroed, and that used to be fine when
> memblock_reserved_init_regions implicitly ended up in the .meminit.data
> section.  However after commit 73db3abdca58c memblock_reserved_init_regions
> ends up in the .bss section, thus breaking it's usage before the .bss is
> cleared.
> 
> Move and rename the call to xen_reserve_extra_memory() so it's done in the
> x86_init.oem.arch_setup hook, which gets executed after the .bss has been
> zeroed, but before calling e820__memory_setup().
> 
> Fixes: 38620fc4e893 ('x86/xen: attempt to inflate the memory balloon on PVH')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> While the commit that introduced the user-noticeable regression is
> 73db3abdca58c, I think 38620fc4e893 should have been more careful to not
> initialize the memblock ranges ahead of the .bss zeroing.

Reviewed-by: Juergen Gross <jgross@suse.com>

I'd prefer using 73db3abdca58c for the fixes tag. Otherwise you'd need to
add this patch to the stable branches, too, which is technically not really
needed.

Additionally I'd like to drop the Fixes: tag from the prereq patch, as this
one doesn't really fix anything.

I can do both while committing.


Juergen

Re: [PATCH 2/2] x86/xen: fix memblock_reserve() usage on PVH

[Roger Pau Monné]

On Thu, Jul 25, 2024 at 10:01:17AM +0200, Jürgen Groß wrote:
> On 25.07.24 09:31, Roger Pau Monne wrote:
> > The current usage of memblock_reserve() in init_pvh_bootparams() is done before
> > the .bss is zeroed, and that used to be fine when
> > memblock_reserved_init_regions implicitly ended up in the .meminit.data
> > section.  However after commit 73db3abdca58c memblock_reserved_init_regions
> > ends up in the .bss section, thus breaking it's usage before the .bss is
> > cleared.
> > 
> > Move and rename the call to xen_reserve_extra_memory() so it's done in the
> > x86_init.oem.arch_setup hook, which gets executed after the .bss has been
> > zeroed, but before calling e820__memory_setup().
> > 
> > Fixes: 38620fc4e893 ('x86/xen: attempt to inflate the memory balloon on PVH')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > While the commit that introduced the user-noticeable regression is
> > 73db3abdca58c, I think 38620fc4e893 should have been more careful to not
> > initialize the memblock ranges ahead of the .bss zeroing.
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>
> 
> I'd prefer using 73db3abdca58c for the fixes tag. Otherwise you'd need to
> add this patch to the stable branches, too, which is technically not really
> needed.
> 
> Additionally I'd like to drop the Fixes: tag from the prereq patch, as this
> one doesn't really fix anything.
> 
> I can do both while committing.

I was unsure myself (as noted in the post-commit notes) about which
"Fixes:" tag to use.

Is there anyway that it can be noted that this commit depends on the
previous change also being present?  For backport reasons, if anyone
ends up backporting 73db3abdca58c it would also need to pick the two
patches here.

Thanks, Roger.

Re: [PATCH 2/2] x86/xen: fix memblock_reserve() usage on PVH

[Jürgen Groß]

On 25.07.24 10:10, Roger Pau Monné wrote:
> On Thu, Jul 25, 2024 at 10:01:17AM +0200, Jürgen Groß wrote:
>> On 25.07.24 09:31, Roger Pau Monne wrote:
>>> The current usage of memblock_reserve() in init_pvh_bootparams() is done before
>>> the .bss is zeroed, and that used to be fine when
>>> memblock_reserved_init_regions implicitly ended up in the .meminit.data
>>> section.  However after commit 73db3abdca58c memblock_reserved_init_regions
>>> ends up in the .bss section, thus breaking it's usage before the .bss is
>>> cleared.
>>>
>>> Move and rename the call to xen_reserve_extra_memory() so it's done in the
>>> x86_init.oem.arch_setup hook, which gets executed after the .bss has been
>>> zeroed, but before calling e820__memory_setup().
>>>
>>> Fixes: 38620fc4e893 ('x86/xen: attempt to inflate the memory balloon on PVH')
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> While the commit that introduced the user-noticeable regression is
>>> 73db3abdca58c, I think 38620fc4e893 should have been more careful to not
>>> initialize the memblock ranges ahead of the .bss zeroing.
>>
>> Reviewed-by: Juergen Gross <jgross@suse.com>
>>
>> I'd prefer using 73db3abdca58c for the fixes tag. Otherwise you'd need to
>> add this patch to the stable branches, too, which is technically not really
>> needed.
>>
>> Additionally I'd like to drop the Fixes: tag from the prereq patch, as this
>> one doesn't really fix anything.
>>
>> I can do both while committing.
> 
> I was unsure myself (as noted in the post-commit notes) about which
> "Fixes:" tag to use.
> 
> Is there anyway that it can be noted that this commit depends on the
> previous change also being present?  For backport reasons, if anyone
> ends up backporting 73db3abdca58c it would also need to pick the two
> patches here.

This is normal business of backporting. This patch added to a stable kernel
would at least result in a build failure. The person doing the backport could
it either fixup locally in the backported patch, or look which patch did setup
the source to make this patch work.


Juergen

[PATCH 0/2] Stable-6.6 backport x86/xen: fix memblock_reserve() usage on PVH

[Jason Andryuk]

For stable-6.6.  

("x86/xen: fix memblock_reserve() usage on PVH") is the fix, but it
depends on ("x86/xen: move xen_reserve_extra_memory()") as a
prerequisite.  Both need fixups as they predate the removal of the Xen
hypercall_page.

Roger Pau Monne (2):
  x86/xen: move xen_reserve_extra_memory()
  x86/xen: fix memblock_reserve() usage on PVH

 arch/x86/include/asm/xen/hypervisor.h |  5 --
 arch/x86/platform/pvh/enlighten.c     |  3 -
 arch/x86/xen/enlighten_pvh.c          | 93 +++++++++++++++------------
 3 files changed, 51 insertions(+), 50 deletions(-)

-- 
2.49.0

[PATCH 1/2] x86/xen: move xen_reserve_extra_memory()

[Jason Andryuk]

From: Roger Pau Monne <roger.pau@citrix.com>

commit fc05ea89c9ab45e70cb73e70bc0b9cdd403e0ee1 upstream

In preparation for making the function static.

No functional change.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Message-ID: <20240725073116.14626-2-roger.pau@citrix.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
[ Stable backport - move the code as it exists ]
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
For stable-6.6

This patch is code movement, but it doesn't directly apply since it
predates the removal of the Xen hypercall_page.  This version moves
the code as it exists in 6.6.

Pre-req for 4c006734898a113a64a528027274a571b04af95a backport
---
 arch/x86/xen/enlighten_pvh.c | 82 ++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
index 60b358c2f434..89984018141c 100644
--- a/arch/x86/xen/enlighten_pvh.c
+++ b/arch/x86/xen/enlighten_pvh.c
@@ -26,47 +26,6 @@
 bool __ro_after_init xen_pvh;
 EXPORT_SYMBOL_GPL(xen_pvh);
 
-void __init xen_pvh_init(struct boot_params *boot_params)
-{
-	xen_pvh = 1;
-	xen_domain_type = XEN_HVM_DOMAIN;
-	xen_start_flags = pvh_start_info.flags;
-
-	if (xen_initial_domain())
-		x86_init.oem.arch_setup = xen_add_preferred_consoles;
-	x86_init.oem.banner = xen_banner;
-
-	xen_efi_init(boot_params);
-
-	if (xen_initial_domain()) {
-		struct xen_platform_op op = {
-			.cmd = XENPF_get_dom0_console,
-		};
-		int ret = HYPERVISOR_platform_op(&op);
-
-		if (ret > 0)
-			xen_init_vga(&op.u.dom0_console,
-				     min(ret * sizeof(char),
-					 sizeof(op.u.dom0_console)),
-				     &boot_params->screen_info);
-	}
-}
-
-void __init mem_map_via_hcall(struct boot_params *boot_params_p)
-{
-	struct xen_memory_map memmap;
-	int rc;
-
-	memmap.nr_entries = ARRAY_SIZE(boot_params_p->e820_table);
-	set_xen_guest_handle(memmap.buffer, boot_params_p->e820_table);
-	rc = HYPERVISOR_memory_op(XENMEM_memory_map, &memmap);
-	if (rc) {
-		xen_raw_printk("XENMEM_memory_map failed (%d)\n", rc);
-		BUG();
-	}
-	boot_params_p->e820_entries = memmap.nr_entries;
-}
-
 /*
  * Reserve e820 UNUSABLE regions to inflate the memory balloon.
  *
@@ -133,3 +92,44 @@ void __init xen_reserve_extra_memory(struct boot_params *bootp)
 		xen_add_extra_mem(PFN_UP(e->addr), pages);
 	}
 }
+
+void __init xen_pvh_init(struct boot_params *boot_params)
+{
+	xen_pvh = 1;
+	xen_domain_type = XEN_HVM_DOMAIN;
+	xen_start_flags = pvh_start_info.flags;
+
+	if (xen_initial_domain())
+		x86_init.oem.arch_setup = xen_add_preferred_consoles;
+	x86_init.oem.banner = xen_banner;
+
+	xen_efi_init(boot_params);
+
+	if (xen_initial_domain()) {
+		struct xen_platform_op op = {
+			.cmd = XENPF_get_dom0_console,
+		};
+		int ret = HYPERVISOR_platform_op(&op);
+
+		if (ret > 0)
+			xen_init_vga(&op.u.dom0_console,
+				     min(ret * sizeof(char),
+					 sizeof(op.u.dom0_console)),
+				     &boot_params->screen_info);
+	}
+}
+
+void __init mem_map_via_hcall(struct boot_params *boot_params_p)
+{
+	struct xen_memory_map memmap;
+	int rc;
+
+	memmap.nr_entries = ARRAY_SIZE(boot_params_p->e820_table);
+	set_xen_guest_handle(memmap.buffer, boot_params_p->e820_table);
+	rc = HYPERVISOR_memory_op(XENMEM_memory_map, &memmap);
+	if (rc) {
+		xen_raw_printk("XENMEM_memory_map failed (%d)\n", rc);
+		BUG();
+	}
+	boot_params_p->e820_entries = memmap.nr_entries;
+}
-- 
2.49.0

[PATCH 2/2] x86/xen: fix memblock_reserve() usage on PVH

[Jason Andryuk]

From: Roger Pau Monne <roger.pau@citrix.com>

commit 4c006734898a113a64a528027274a571b04af95a upstream

The current usage of memblock_reserve() in init_pvh_bootparams() is done before
the .bss is zeroed, and that used to be fine when
memblock_reserved_init_regions implicitly ended up in the .meminit.data
section.  However after commit 73db3abdca58c memblock_reserved_init_regions
ends up in the .bss section, thus breaking it's usage before the .bss is
cleared.

Move and rename the call to xen_reserve_extra_memory() so it's done in the
x86_init.oem.arch_setup hook, which gets executed after the .bss has been
zeroed, but before calling e820__memory_setup().

Fixes: 73db3abdca58c ("init/modpost: conditionally check section mismatch to __meminit*")
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Message-ID: <20240725073116.14626-3-roger.pau@citrix.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
[ Context fixup for hypercall_page removal ]
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
For stable-6.6

Context fixup is needed to cherry-pick after Xen hypercall_page removal.

The Fixes commit was backported to 6.6, so this is needed to fix booting
for Xen PVH.
---
 arch/x86/include/asm/xen/hypervisor.h |  5 -----
 arch/x86/platform/pvh/enlighten.c     |  3 ---
 arch/x86/xen/enlighten_pvh.c          | 15 ++++++++++++---
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h
index 64fbd2dbc5b7..a9088250770f 100644
--- a/arch/x86/include/asm/xen/hypervisor.h
+++ b/arch/x86/include/asm/xen/hypervisor.h
@@ -62,11 +62,6 @@ void xen_arch_unregister_cpu(int num);
 #ifdef CONFIG_PVH
 void __init xen_pvh_init(struct boot_params *boot_params);
 void __init mem_map_via_hcall(struct boot_params *boot_params_p);
-#ifdef CONFIG_XEN_PVH
-void __init xen_reserve_extra_memory(struct boot_params *bootp);
-#else
-static inline void xen_reserve_extra_memory(struct boot_params *bootp) { }
-#endif
 #endif
 
 /* Lazy mode for batching updates / context switch */
diff --git a/arch/x86/platform/pvh/enlighten.c b/arch/x86/platform/pvh/enlighten.c
index a12117f3d4de..00a92cb2c814 100644
--- a/arch/x86/platform/pvh/enlighten.c
+++ b/arch/x86/platform/pvh/enlighten.c
@@ -74,9 +74,6 @@ static void __init init_pvh_bootparams(bool xen_guest)
 	} else
 		xen_raw_printk("Warning: Can fit ISA range into e820\n");
 
-	if (xen_guest)
-		xen_reserve_extra_memory(&pvh_bootparams);
-
 	pvh_bootparams.hdr.cmd_line_ptr =
 		pvh_start_info.cmdline_paddr;
 
diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
index 89984018141c..ac0a8adb2c50 100644
--- a/arch/x86/xen/enlighten_pvh.c
+++ b/arch/x86/xen/enlighten_pvh.c
@@ -8,6 +8,7 @@
 #include <asm/io_apic.h>
 #include <asm/hypervisor.h>
 #include <asm/e820/api.h>
+#include <asm/setup.h>
 
 #include <xen/xen.h>
 #include <asm/xen/interface.h>
@@ -40,8 +41,9 @@ EXPORT_SYMBOL_GPL(xen_pvh);
  * hypervisor should notify us which memory ranges are suitable for creating
  * foreign mappings, but that's not yet implemented.
  */
-void __init xen_reserve_extra_memory(struct boot_params *bootp)
+static void __init pvh_reserve_extra_memory(void)
 {
+	struct boot_params *bootp = &boot_params;
 	unsigned int i, ram_pages = 0, extra_pages;
 
 	for (i = 0; i < bootp->e820_entries; i++) {
@@ -93,14 +95,21 @@ void __init xen_reserve_extra_memory(struct boot_params *bootp)
 	}
 }
 
+static void __init pvh_arch_setup(void)
+{
+	pvh_reserve_extra_memory();
+
+	if (xen_initial_domain())
+		xen_add_preferred_consoles();
+}
+
 void __init xen_pvh_init(struct boot_params *boot_params)
 {
 	xen_pvh = 1;
 	xen_domain_type = XEN_HVM_DOMAIN;
 	xen_start_flags = pvh_start_info.flags;
 
-	if (xen_initial_domain())
-		x86_init.oem.arch_setup = xen_add_preferred_consoles;
+	x86_init.oem.arch_setup = pvh_arch_setup;
 	x86_init.oem.banner = xen_banner;
 
 	xen_efi_init(boot_params);
-- 
2.49.0

Re: [PATCH 2/2] x86/xen: fix memblock_reserve() usage on PVH

[Sasha Levin]

[ Sasha's backport helper bot ]

Hi,

✅ All tests passed successfully. No issues detected.
No action required from the submitter.

The upstream commit SHA1 provided is correct: 4c006734898a113a64a528027274a571b04af95a

WARNING: Author mismatch between patch and upstream commit:
Backport author: Jason Andryuk<jason.andryuk@amd.com>
Commit author: Roger Pau Monne<roger.pau@citrix.com>

Note: The patch differs from the upstream commit:
---
1:  4c006734898a1 < -:  ------------- x86/xen: fix memblock_reserve() usage on PVH
-:  ------------- > 1:  9bc5c94e278f7 Linux 6.14.2
---

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-5.4.y        |  Success    |  Success   |

Re: [PATCH 1/2] x86/xen: move xen_reserve_extra_memory()

[Sasha Levin]

[ Sasha's backport helper bot ]

Hi,

✅ All tests passed successfully. No issues detected.
No action required from the submitter.

The upstream commit SHA1 provided is correct: fc05ea89c9ab45e70cb73e70bc0b9cdd403e0ee1

WARNING: Author mismatch between patch and upstream commit:
Backport author: Jason Andryuk<jason.andryuk@amd.com>
Commit author: Roger Pau Monne<roger.pau@citrix.com>

Note: The patch differs from the upstream commit:
---
1:  fc05ea89c9ab4 < -:  ------------- x86/xen: move xen_reserve_extra_memory()
-:  ------------- > 1:  9bc5c94e278f7 Linux 6.14.2
---

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-5.4.y        |  Success    |  Success   |