[RFC PATCH] x86/efi: Auto enable EFI memmap on SGI UV systems

[RFC PATCH] x86/efi: Auto enable EFI memmap on SGI UV systems

[Joseph Thelen]

The intent here is to automatically enable the EFI memory map entries
on SGI UV systems, which can have more than 128 E820 entries and
will cause headaches if the EFI memory map entries are not added.

The assumption is made that the community would prefer that this change
be a UV specific quirk, as opposed to automatically adding the EFI
memory map entries whenever more then 128 E820 entries exist. If this
assumption is incorrect, the patch can be changed to check the number
of E820 entries rather than for whether or not the system is a UV.

Cc: Alex Thorlton <athorlton@sgi.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: linux-efi@vger.kernel.org

Joseph Thelen (1):
  x86/efi: Auto enable EFI memmap on SGI UV systems

 arch/x86/platform/efi/efi.c | 43 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)

-- 
1.8.5.6

[PATCH] x86/efi: Auto enable EFI memmap on SGI UV systems

[Joseph Thelen]

Currently, the EFI memory map entries are disabled by default and must
be enabled by passing the kernel boot option:

add_efi_memmap

The EFI memory map entries should be enabled on systems with more
than 128 E820 entries, which includes many UV systems. Check if
we're on a UV system by chekcing the uv system table.
Enable the EFI memory map entries if we're on a UV system.

This change is backward compatible because the EFI memory map entries are
still disabled by default on non-UV systems, and it maintains the previous
behavior of the kernel boot option. In addition, it allows the EFI memory
map entries to be explicitly disabled (will not be automatically enabled)
by setting the add_efi_memmap boot option to anything that kstringtobool
will determine to be false.

Signed-off-by: Joseph Thelen <jthelen@sgi.com>
Cc: Alex Thorlton <athorlton@sgi.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: linux-efi@vger.kernel.org
---
 arch/x86/platform/efi/efi.c | 43 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index f93545e..26e3ff5 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -66,10 +66,32 @@ static efi_config_table_type_t arch_tables[] __initdata = {
 
 u64 efi_setup;		/* efi setup_data physical address */
 
-static int add_efi_memmap __initdata;
+static __initdata enum {
+	EFI_MEMMAP_DEFAULT,
+	EFI_MEMMAP_ENABLED,
+	EFI_MEMMAP_DISABLED
+} add_efi_memmap;
+
 static int __init setup_add_efi_memmap(char *arg)
 {
-	add_efi_memmap = 1;
+	static bool arg_as_bool;
+	int ret = strtobool(arg, &arg_as_bool);
+
+	/* check for a non-existent arg, to maintain backward compatibility */
+	if (!arg) {
+		add_efi_memmap = EFI_MEMMAP_ENABLED;
+	} else {
+		if (ret) {
+			/* a bad argument was passed... */
+			return ret;
+		} else {
+			if (arg_as_bool)
+				add_efi_memmap = EFI_MEMMAP_ENABLED;
+			else
+				add_efi_memmap = EFI_MEMMAP_DISABLED;
+		}
+	}
+
 	return 0;
 }
 early_param("add_efi_memmap", setup_add_efi_memmap);
@@ -433,6 +455,7 @@ static int __init efi_runtime_init(void)
 static int __init efi_memmap_init(void)
 {
 	unsigned long addr, size;
+	bool is_uv_sys;
 
 	if (efi_enabled(EFI_PARAVIRT))
 		return 0;
@@ -449,8 +472,20 @@ static int __init efi_memmap_init(void)
 
 	efi.memmap.map_end = efi.memmap.map + size;
 
-	if (add_efi_memmap)
-		do_add_efi_memmap();
+	is_uv_sys = !((efi.uv_systab == EFI_INVALID_TABLE_ADDR)
+			|| !efi.uv_systab);
+
+	if (add_efi_memmap != EFI_MEMMAP_DISABLED) {
+		if (add_efi_memmap == EFI_MEMMAP_ENABLED) {
+			do_add_efi_memmap();
+			pr_info("EFI memmap enabled: Explicitly\n");
+		} else if (is_uv_sys) {
+			do_add_efi_memmap();
+			pr_info("EFI memmap enabled: this is a UV system\n");
+		}
+	} else {
+		pr_info("EFI memmap disabled: Explicitly\n");
+	}
 
 	set_bit(EFI_MEMMAP, &efi.flags);
 
-- 
1.8.5.6

Re: [PATCH] x86/efi: Auto enable EFI memmap on SGI UV systems

[Ingo Molnar]

* Joseph Thelen <jthelen@sgi.com> wrote:

>  static int __init setup_add_efi_memmap(char *arg)
>  {
> +	static bool arg_as_bool;

Why hidden inside local variables as static?

> +	int ret = strtobool(arg, &arg_as_bool);
> +
> +	/* check for a non-existent arg, to maintain backward compatibility */
> +	if (!arg) {
> +		add_efi_memmap = EFI_MEMMAP_ENABLED;
> +	} else {
> +		if (ret) {
> +			/* a bad argument was passed... */
> +			return ret;
> +		} else {
> +			if (arg_as_bool)
> +				add_efi_memmap = EFI_MEMMAP_ENABLED;
> +			else
> +				add_efi_memmap = EFI_MEMMAP_DISABLED;
> +		}
> +	}
> +
>  	return 0;

And that's a really weird code flow!

How about something straightforward:

	int val = 0;
	int ret;

	/* Check for a non-existent arg, to maintain backward compatibility: */
	if (!arg) {
		add_efi_memmap = EFI_MEMMAP_ENABLED;
		return 0;
	}

	ret = strtobool(arg, &val);

	/* Was a bad argument passed? */
	if (ret)
		return ret;

	if (val)
		add_efi_memmap = EFI_MEMMAP_ENABLED;
	else
		add_efi_memmap = EFI_MEMMAP_DISABLED;

	return 0;

?

Also note the rename to 'val'.

Thanks,

	Ingo

Re: [PATCH] x86/efi: Auto enable EFI memmap on SGI UV systems

[Matt Fleming]

(Cc'ing people familiar with e820 map woes)

On Thu, 02 Jun, at 03:50:35PM, Joseph Thelen wrote:
> Currently, the EFI memory map entries are disabled by default and must
> be enabled by passing the kernel boot option:
> 
> add_efi_memmap
> 
> The EFI memory map entries should be enabled on systems with more
> than 128 E820 entries, which includes many UV systems. Check if
> we're on a UV system by chekcing the uv system table.
> Enable the EFI memory map entries if we're on a UV system.
> 
> This change is backward compatible because the EFI memory map entries are
> still disabled by default on non-UV systems, and it maintains the previous
> behavior of the kernel boot option. In addition, it allows the EFI memory
> map entries to be explicitly disabled (will not be automatically enabled)
> by setting the add_efi_memmap boot option to anything that kstringtobool
> will determine to be false.
> 
> Signed-off-by: Joseph Thelen <jthelen@sgi.com>
> Cc: Alex Thorlton <athorlton@sgi.com>
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> Cc: linux-efi@vger.kernel.org
> ---
>  arch/x86/platform/efi/efi.c | 43 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 39 insertions(+), 4 deletions(-)
 
What's the ultimate goal here? To not require that users specify the
add_efi_memmap kernel parameter in the future? Presumably they do
today?

FYI, a lot of non-UV systems have more than 128 entries and the way we
handle it is by using the SETUP_E820_EXT setup_data entry in
boot_params in the EFI boot stub (I don't think boot loaders use it
because they largely go via the stub anyhow).

So if you've got control over your boot loader, and assuming SGI UV
systems don't boot using the EFI boot stub, you could look at adding
boot loader support for SETUP_E820_EXT to force enable > 128 entries
automatically without any new kernel code.

Re: [PATCH] x86/efi: Auto enable EFI memmap on SGI UV systems

[Joseph Thelen]

On Wed, Jun 08, 2016 at 01:12:23PM +0200, Ingo Molnar wrote:
> 
> * Joseph Thelen <jthelen@sgi.com> wrote:
> 
> >  static int __init setup_add_efi_memmap(char *arg)
> >  {
> > +	static bool arg_as_bool;
> 
> Why hidden inside local variables as static?

Likely muscle memory of some kind after being forced to do a bunch
of things in Java for awhile. Thanks for pointing that out.

> 
> > +	int ret = strtobool(arg, &arg_as_bool);
> > +
> > +	/* check for a non-existent arg, to maintain backward compatibility */
> > +	if (!arg) {
> > +		add_efi_memmap = EFI_MEMMAP_ENABLED;
> > +	} else {
> > +		if (ret) {
> > +			/* a bad argument was passed... */
> > +			return ret;
> > +		} else {
> > +			if (arg_as_bool)
> > +				add_efi_memmap = EFI_MEMMAP_ENABLED;
> > +			else
> > +				add_efi_memmap = EFI_MEMMAP_DISABLED;
> > +		}
> > +	}
> > +
> >  	return 0;
> 
> And that's a really weird code flow!
> 
> How about something straightforward:
> 
> 	int val = 0;
> 	int ret;
> 
> 	/* Check for a non-existent arg, to maintain backward compatibility: */
> 	if (!arg) {
> 		add_efi_memmap = EFI_MEMMAP_ENABLED;
> 		return 0;
> 	}
> 
> 	ret = strtobool(arg, &val);
> 
> 	/* Was a bad argument passed? */
> 	if (ret)
> 		return ret;
> 
> 	if (val)
> 		add_efi_memmap = EFI_MEMMAP_ENABLED;
> 	else
> 		add_efi_memmap = EFI_MEMMAP_DISABLED;
> 
> 	return 0;
> 
> ?
> 
> Also note the rename to 'val'.
> 
> Thanks,
> 
> 	Ingo

That certainly is much cleaner!
Having only recently come from a place where "extra" returns were to
be avoided at any cost, the fact that they do not appear to be quite so
tabo here is nice.

These changes will be made to whatever version of this comes into being
after considering Matt Fleming's comments.

Thanks,

Joseph Thelen

Re: [PATCH] x86/efi: Auto enable EFI memmap on SGI UV systems

[Joseph Thelen]

On Wed, Jun 08, 2016 at 01:36:23PM +0100, Matt Fleming wrote:
> (Cc'ing people familiar with e820 map woes)
> 
> On Thu, 02 Jun, at 03:50:35PM, Joseph Thelen wrote:
> > Currently, the EFI memory map entries are disabled by default and must
> > be enabled by passing the kernel boot option:
> > 
> > add_efi_memmap
> > 
> > The EFI memory map entries should be enabled on systems with more
> > than 128 E820 entries, which includes many UV systems. Check if
> > we're on a UV system by chekcing the uv system table.
> > Enable the EFI memory map entries if we're on a UV system.
> > 
> > This change is backward compatible because the EFI memory map entries are
> > still disabled by default on non-UV systems, and it maintains the previous
> > behavior of the kernel boot option. In addition, it allows the EFI memory
> > map entries to be explicitly disabled (will not be automatically enabled)
> > by setting the add_efi_memmap boot option to anything that kstringtobool
> > will determine to be false.
> > 
> > Signed-off-by: Joseph Thelen <jthelen@sgi.com>
> > Cc: Alex Thorlton <athorlton@sgi.com>
> > Cc: Matt Fleming <matt@codeblueprint.co.uk>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: x86@kernel.org
> > Cc: linux-efi@vger.kernel.org
> > ---
> >  arch/x86/platform/efi/efi.c | 43 +++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 39 insertions(+), 4 deletions(-)
>  
> What's the ultimate goal here? To not require that users specify the
> add_efi_memmap kernel parameter in the future? Presumably they do
> today?
> 
> FYI, a lot of non-UV systems have more than 128 entries and the way we
> handle it is by using the SETUP_E820_EXT setup_data entry in
> boot_params in the EFI boot stub (I don't think boot loaders use it
> because they largely go via the stub anyhow).
> 
> So if you've got control over your boot loader, and assuming SGI UV
> systems don't boot using the EFI boot stub, you could look at adding
> boot loader support for SETUP_E820_EXT to force enable > 128 entries
> automatically without any new kernel code.

We're still reasonably confident we'll need something like this. Although,
after looking into the stuff you mention, it seems that it should perhaps
be a bit more sophisticated than just checking if we're on a UV system.

We'll get back to you after digging into this a bit more and coming
up with some more specific reasons for the change.

Thanks,

Joseph Thelen

Re: [PATCH] x86/efi: Auto enable EFI memmap on SGI UV systems

[Joseph Thelen]

On Tue, Jun 14, 2016 at 05:34:49PM -0500, Joseph Thelen wrote:
> We're still reasonably confident we'll need something like this. Although,
> after looking into the stuff you mention, it seems that it should perhaps
> be a bit more sophisticated than just checking if we're on a UV system.
> 
> We'll get back to you after digging into this a bit more and coming
> up with some more specific reasons for the change.
> 
> Thanks,
> 
> Joseph Thelen
>

After investigating a bit more, it seems that this is really only
an issue where people are using older bootloaders/managers and kernels, some
of which are still supported / used by big distros like redhat.

Given that this is, as you've pointed out, either a non-issue or easily
remedied without kernel changes now and in the future, we'll try
talking about it directly with any distro where it's still an issue.

Thanks,

Joseph Thelen