linux-kernel - Re: [PATCH 6/7] x86/hyperv: Reserve real mode when ACPI wakeup mailbox is available

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240824001742.GA424@yjiang5-mobl.amr.corp.intel.com>
Date: Fri, 23 Aug 2024 17:17:42 -0700
From: Yunhong Jiang <yunhong.jiang@...ux.intel.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
	x86@...nel.org, hpa@...or.com, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, kys@...rosoft.com, haiyangz@...rosoft.com,
	wei.liu@...nel.org, decui@...rosoft.com, rafael@...nel.org,
	lenb@...nel.org, kirill.shutemov@...ux.intel.com,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	linux-hyperv@...r.kernel.org, linux-acpi@...r.kernel.org
Subject: Re: [PATCH 6/7] x86/hyperv: Reserve real mode when ACPI wakeup
 mailbox is available

On Wed, Aug 07, 2024 at 07:33:26PM +0200, Thomas Gleixner wrote:
> On Tue, Aug 06 2024 at 15:12, Yunhong Jiang wrote:
> > +static void __init hv_reserve_real_mode(void)
> > +{
> > +	phys_addr_t mem;
> > +	size_t size = real_mode_size_needed();
> > +
> > +	/*
> > +	 * We only need the memory to be <4GB since the 64-bit trampoline goes
> > +	 * down to 32-bit mode.
> > +	 */
> > +	mem = memblock_phys_alloc_range(size, PAGE_SIZE, 0, SZ_4G);
> > +	if (!mem)
> > +		panic("No sub-4G memory is available for the trampoline\n");
> > +	set_real_mode_mem(mem);
> > +}
> 
> We really don't need another copy of reserve_real_mode(). See uncompiled
> patch below. It does not panic when the allocation fails, but why do you
> want to panic in that case? If it's not there then the system boots with
> a single CPU, so what.

I created a separated patch on my v2 patch set with "Originally-by:" tag
following suggestion at
https://lore.kernel.org/lkml/20240711081317.GD4587@noisy.programming.kicks-ass.net/

Please let me know if that's not the appropriate solution.
 
> 
> >  void __init hv_vtl_init_platform(void)
> >  {
> >  	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
> >  
> >  	if (wakeup_mailbox_addr) {
> >  		x86_platform.hyper.is_private_mmio = hv_is_private_mmio_tdx;
> > +		x86_platform.realmode_reserve = hv_reserve_real_mode;
> >  	} else {
> >  		x86_platform.realmode_reserve = x86_init_noop;
> >  		x86_platform.realmode_init = x86_init_noop;
> > @@ -259,7 +276,8 @@ int __init hv_vtl_early_init(void)
> >  		panic("XSAVE has to be disabled as it is not supported by this module.\n"
> >  			  "Please add 'noxsave' to the kernel command line.\n");
> >  
> > -	real_mode_header = &hv_vtl_real_mode_header;
> > +	if (!wakeup_mailbox_addr)
> > +		real_mode_header = &hv_vtl_real_mode_header;
> 
> Why is that not suffient to be done in hv_vtl_init_platform() inside the
> condition which clears x86_platform.realmode_reserve/init?
> 
> x86_platform.realmode_init() is invoked from an early initcall while
> hv_vtl_init_platform() is called during early boot.

I created a separated patch for this change on my v2 patch set, since it's a
a separated logic change. I added a Suggested-by: tag to the patch, hope it's
ok.

Thanks
--jyh

> 
> Thanks,
> 
>         tglx
> ---
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -31,12 +31,18 @@ struct x86_init_mpparse {
>   *				platform
>   * @memory_setup:		platform specific memory setup
>   * @dmi_setup:			platform specific DMI setup
> + * @realmode_limit:		platform specific address limit for the realmode trampoline
> + *				(default 1M)
> + * @reserve_bios:		platform specific address limit for reserving the BIOS area
> + *				(default 1M)
>   */
>  struct x86_init_resources {
>  	void (*probe_roms)(void);
>  	void (*reserve_resources)(void);
>  	char *(*memory_setup)(void);
>  	void (*dmi_setup)(void);
> +	unsigned long realmode_limit;
> +	unsigned long reserve_bios;
>  };
>  
>  /**
> --- a/arch/x86/kernel/x86_init.c
> +++ b/arch/x86/kernel/x86_init.c
> @@ -8,6 +8,7 @@
>  #include <linux/ioport.h>
>  #include <linux/export.h>
>  #include <linux/pci.h>
> +#include <linux/sizes.h>
>  
>  #include <asm/acpi.h>
>  #include <asm/bios_ebda.h>
> @@ -68,6 +69,8 @@ struct x86_init_ops x86_init __initdata
>  		.reserve_resources	= reserve_standard_io_resources,
>  		.memory_setup		= e820__memory_setup_default,
>  		.dmi_setup		= dmi_setup,
> +		.realmode_limit		= SZ_1M,
> +		.reserve_bios		= SZ_1M,
>  	},
>  
>  	.mpparse = {
> --- a/arch/x86/realmode/init.c
> +++ b/arch/x86/realmode/init.c
> @@ -45,7 +45,7 @@ void load_trampoline_pgtable(void)
>  
>  void __init reserve_real_mode(void)
>  {
> -	phys_addr_t mem;
> +	phys_addr_t mem, limit = x86_init.resources.realmode_limit;
>  	size_t size = real_mode_size_needed();
>  
>  	if (!size)
> @@ -54,17 +54,15 @@ void __init reserve_real_mode(void)
>  	WARN_ON(slab_is_available());
>  
>  	/* Has to be under 1M so we can execute real-mode AP code. */
> -	mem = memblock_phys_alloc_range(size, PAGE_SIZE, 0, 1<<20);
> +	mem = memblock_phys_alloc_range(size, PAGE_SIZE, 0, limit);
>  	if (!mem)
> -		pr_info("No sub-1M memory is available for the trampoline\n");
> +		pr_info("No memory below %lluM for the real-mode trampoline\n", limit >> 20);
>  	else
>  		set_real_mode_mem(mem);
>  
> -	/*
> -	 * Unconditionally reserve the entire first 1M, see comment in
> -	 * setup_arch().
> -	 */
> -	memblock_reserve(0, SZ_1M);
> +	/* Reserve the entire first 1M, if enabled. See comment in setup_arch(). */
> +	if (x86_init.resources.reserve_bios)
> +		memblock_reserve(0, x86_init.resources.reserve_bios);
>  }
>  
>  static void __init sme_sev_setup_real_mode(struct trampoline_header *th)