Re: [PATCH 1/2] UEFI arm64: add noefi boot param

Re: [PATCH 1/2] UEFI arm64: add noefi boot param

[Leif Lindholm]

On Wed, Aug 06, 2014 at 04:38:25PM +0800, Dave Young wrote:
> 
> Adding a noefi boot param like in X86 to disable efi runtime services support.
> 
> This will be useful for debugging uefi problems. Also it will be useful
> for later kexec/kdump work. Kexec on uefi support in X86 depends on a fixed vm
> area specific for uefi runtime 1:1 mapping, kernel will switch to a different
> page table for any uefi runtime callback in virtual mode. In arm64 similar
> work probably is necessary. But kexec boot will just works with 'noefi' with
> the limitaion of lacking runtime services. The runtime services is not critical
> for kdump kernel for now. So as for kexec/kdump just leave the 1:1 mapping a
> future work.

Since this is really turning an x86-specific feature into a generic
one, could it be moved to core code?
Maybe an efi.mask, reusing the efi_enabled defines, with an
efi_disabled macro?

Also, since this patch (and its x86 predecessor) is not really
"noefi", could this be integrated with the "efi=" patch
(http://permalink.gmane.org/gmane.linux.kernel.efi/4405),
as an efi=noruntime option?

On x86, due to CSM, "noefi" was a useful fallback for completely
broken (U)EFI implementations - but on an arm* UEFI system, there will
be no fallback. Could it be wrapped in a kernel hacking config option?

Partial code description, not even compile tested:
---
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 14db1f6..a295a5c 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -386,7 +386,7 @@ static int __init arm64_enter_virtual_mode(void)
 	int count = 0;
 	unsigned long flags;
 
-	if (!efi_enabled(EFI_BOOT)) {
+	if (!efi_enabled(EFI_BOOT) || efi_disabled(EFI_RUNTIME_SERVICES)) {
 		pr_info("EFI services will not be available.\n");
 		return -1;
 	}
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 87fc96b..b59a7c6 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -77,14 +77,6 @@ static efi_config_table_type_t arch_tables[] __initdata = {
 
 u64 efi_setup;		/* efi setup_data physical address */
 
-static bool disable_runtime __initdata = false;
-static int __init setup_noefi(char *arg)
-{
-	disable_runtime = true;
-	return 0;
-}
-early_param("noefi", setup_noefi);
-
 int add_efi_memmap;
 EXPORT_SYMBOL(add_efi_memmap);
 
@@ -764,7 +756,7 @@ void __init efi_init(void)
 	if (!efi_runtime_supported())
 		pr_info("No EFI runtime due to 32/64-bit mismatch with kernel\n");
 	else {
-		if (disable_runtime || efi_runtime_init())
+		if (efi_disabled(EFI_RUNTIME_SERVICES) || efi_runtime_init())
 			return;
 	}
 	if (efi_memmap_init())
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index dc79346..ffc4a45 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -43,6 +43,15 @@ EXPORT_SYMBOL(efi);
 static struct kobject *efi_kobj;
 static struct kobject *efivars_kobj;
 
+static int __init setup_noefi(char *arg)
+{
+#if defined(CONFIG_X86) || defined(CONFIG_EFI_DEBUG)
+	set_bit(EFI_RUNTIME_SERVICES, &efi.mask);
+#endif
+	return 0;
+}
+early_param("noefi", setup_noefi);
+
 /*
  * Let's not leave out systab information that snuck into
  * the efivars driver
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 41bbf8b..9533d0e 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -826,6 +826,7 @@ extern struct efi {
 	efi_set_virtual_address_map_t *set_virtual_address_map;
 	struct efi_memory_map *memmap;
 	unsigned long flags;
+	unsigned long mask;
 } efi;
 
 static inline int
@@ -926,6 +927,10 @@ static inline bool efi_enabled(int feature)
 {
 	return test_bit(feature, &efi.flags) != 0;
 }
+static inline bool efi_disabled(int feature)
+{
+	return test_bit(feature, &efi.mask) == 0;
+}
 #else
 static inline bool efi_enabled(int feature)
 {
---

/
    Leif

> Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm64/kernel/efi.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6/arch/arm64/kernel/efi.c
> ===================================================================
> --- linux-2.6.orig/arch/arm64/kernel/efi.c
> +++ linux-2.6/arch/arm64/kernel/efi.c
> @@ -31,6 +31,14 @@ static efi_runtime_services_t *runtime;
>  
>  static u64 efi_system_table;
>  
> +static bool disable_runtime __initdata = false;
> +static int __init setup_noefi(char *arg)
> +{
> +	disable_runtime = true;
> +	return 0;
> +}
> +early_param("noefi", setup_noefi);
> +
>  static int uefi_debug __initdata;
>  static int __init uefi_debug_setup(char *str)
>  {
> @@ -391,11 +399,14 @@ static int __init arm64_enter_virtual_mo
>  		return -1;
>  	}
>  
> -	pr_info("Remapping and enabling EFI services.\n");
> -
> -	/* replace early memmap mapping with permanent mapping */
>  	mapsize = memmap.map_end - memmap.map;
>  	early_memunmap(memmap.map, mapsize);
> +
> +	if (disable_runtime)
> +		return -ENODEV;
> +
> +	pr_info("Remapping and enabling EFI services.\n");
> +	/* replace early memmap mapping with permanent mapping */
>  	memmap.map = (__force void *)ioremap_cache((phys_addr_t)memmap.phys_map,
>  						   mapsize);
>  	memmap.map_end = memmap.map + mapsize;

Re: [PATCH 1/2] UEFI arm64: add noefi boot param

[Matt Fleming]

On Wed, 06 Aug, at 02:06:23PM, Leif Lindholm wrote:
> On Wed, Aug 06, 2014 at 04:38:25PM +0800, Dave Young wrote:
> > 
> > Adding a noefi boot param like in X86 to disable efi runtime services support.
> > 
> > This will be useful for debugging uefi problems. Also it will be useful
> > for later kexec/kdump work. Kexec on uefi support in X86 depends on a fixed vm
> > area specific for uefi runtime 1:1 mapping, kernel will switch to a different
> > page table for any uefi runtime callback in virtual mode. In arm64 similar
> > work probably is necessary. But kexec boot will just works with 'noefi' with
> > the limitaion of lacking runtime services. The runtime services is not critical
> > for kdump kernel for now. So as for kexec/kdump just leave the 1:1 mapping a
> > future work.
> 
> Since this is really turning an x86-specific feature into a generic
> one, could it be moved to core code?
> Maybe an efi.mask, reusing the efi_enabled defines, with an
> efi_disabled macro?
 
Why the new efi_disabled() and efi.mask? This is all achievable with
efi_enabled() and efi.flags, in fact, this kind of thing is exactly why
they were invented.

> Also, since this patch (and its x86 predecessor) is not really
> "noefi", could this be integrated with the "efi=" patch
> (http://permalink.gmane.org/gmane.linux.kernel.efi/4405),
> as an efi=noruntime option?
> 
> On x86, due to CSM, "noefi" was a useful fallback for completely
> broken (U)EFI implementations - but on an arm* UEFI system, there will
> be no fallback. Could it be wrapped in a kernel hacking config option?
 
I don't mind making "noefi" a synonym for "efi=noruntime" on x86, as
long as we keep "noefi" around with the same semantics it's always had.

And certainly if you're coming at this anew (like on arm(64)), I agree
"efi=noruntime" just makes a ton more sense than "noefi".

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 1/2] UEFI arm64: add noefi boot param

[Leif Lindholm]

On Wed, Aug 06, 2014 at 02:20:21PM +0100, Matt Fleming wrote:
> > Since this is really turning an x86-specific feature into a generic
> > one, could it be moved to core code?
> > Maybe an efi.mask, reusing the efi_enabled defines, with an
> > efi_disabled macro?
>  
> Why the new efi_disabled() and efi.mask? This is all achievable with
> efi_enabled() and efi.flags, in fact, this kind of thing is exactly why
> they were invented.

Because this flag is indicating which facilities we should not try to
enable, rather than which facilities we have enabled.

The EFI_RUNTIME_SERVICES flag is set after successful call to
set_virtual_address_map. The apparent intent of "noefi" is to prevent
that call from being made in the first place.

Anyway, it was just a suggestion - main point was it would make sense
to share the code.
 
> > Also, since this patch (and its x86 predecessor) is not really
> > "noefi", could this be integrated with the "efi=" patch
> > (http://permalink.gmane.org/gmane.linux.kernel.efi/4405),
> > as an efi=noruntime option?
> > 
> > On x86, due to CSM, "noefi" was a useful fallback for completely
> > broken (U)EFI implementations - but on an arm* UEFI system, there will
> > be no fallback. Could it be wrapped in a kernel hacking config option?
>  
> I don't mind making "noefi" a synonym for "efi=noruntime" on x86, as
> long as we keep "noefi" around with the same semantics it's always had.

Yeah, that would be nice.

/
    Leif

Re: [PATCH 1/2] UEFI arm64: add noefi boot param

[Matt Fleming]

On Wed, 06 Aug, at 02:29:41PM, Leif Lindholm wrote:
> On Wed, Aug 06, 2014 at 02:20:21PM +0100, Matt Fleming wrote:
> > > Since this is really turning an x86-specific feature into a generic
> > > one, could it be moved to core code?
> > > Maybe an efi.mask, reusing the efi_enabled defines, with an
> > > efi_disabled macro?
> >  
> > Why the new efi_disabled() and efi.mask? This is all achievable with
> > efi_enabled() and efi.flags, in fact, this kind of thing is exactly why
> > they were invented.
> 
> Because this flag is indicating which facilities we should not try to
> enable, rather than which facilities we have enabled.
> 
> The EFI_RUNTIME_SERVICES flag is set after successful call to
> set_virtual_address_map. The apparent intent of "noefi" is to prevent
> that call from being made in the first place.
> 
> Anyway, it was just a suggestion - main point was it would make sense
> to share the code.

Definitely.

Dave, below is the kind of thing that I had in mind. Please Cc the Xen
and SGI folks when you submit your next version because they're likely
to be hit the hardest by any changes to EFI_RUNTIME_SERVICES and
"noefi".

Obviously the addition of "efi=noruntime" is needed on top, but that's
about 6 lines of code.

---
 arch/arm64/kernel/efi.c     |  4 ++--
 arch/x86/kernel/setup.c     |  4 +++-
 arch/x86/platform/efi/efi.c | 33 +++++++++------------------------
 drivers/firmware/efi/efi.c  |  7 +++++++
 4 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index e72f3100958f..9fdedb721a4e 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -83,6 +83,7 @@ static int __init uefi_init(void)
 
 	set_bit(EFI_BOOT, &efi.flags);
 	set_bit(EFI_64BIT, &efi.flags);
+	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 
 	/*
 	 * Verify the EFI Table
@@ -386,7 +387,7 @@ static int __init arm64_enter_virtual_mode(void)
 	int count = 0;
 	unsigned long flags;
 
-	if (!efi_enabled(EFI_BOOT)) {
+	if (!efi_enabled(EFI_BOOT) || !efi_enabled(EFI_RUNTIME_SERVICES)) {
 		pr_info("EFI services will not be available.\n");
 		return -1;
 	}
@@ -461,7 +462,6 @@ static int __init arm64_enter_virtual_mode(void)
 	/* Set up runtime services function pointers */
 	runtime = efi.systab->runtime;
 	efi_native_runtime_setup();
-	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 
 	return 0;
 
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 41ead8d3bc0b..3e2f820b6007 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -932,8 +932,10 @@ void __init setup_arch(char **cmdline_p)
 		set_bit(EFI_64BIT, &efi.flags);
 	}
 
-	if (efi_enabled(EFI_BOOT))
+	if (efi_enabled(EFI_BOOT)) {
 		efi_memblock_x86_reserve_range();
+		set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
+	}
 #endif
 
 	x86_init.oem.arch_setup();
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index a1f745b0bf1d..bac12cae2a80 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -70,14 +70,6 @@ static efi_config_table_type_t arch_tables[] __initdata = {
 
 u64 efi_setup;		/* efi setup_data physical address */
 
-static bool disable_runtime __initdata = false;
-static int __init setup_noefi(char *arg)
-{
-	disable_runtime = true;
-	return 0;
-}
-early_param("noefi", setup_noefi);
-
 int add_efi_memmap;
 EXPORT_SYMBOL(add_efi_memmap);
 
@@ -397,20 +389,12 @@ static int __init efi_runtime_init(void)
 	 * hypercall which executes relevant EFI functions) and that is why
 	 * they are always enabled.
 	 */
+	if (efi_enabled(EFI_64BIT))
+		rv = efi_runtime_init64();
+	else
+		rv = efi_runtime_init32();
 
-	if (!efi_enabled(EFI_PARAVIRT)) {
-		if (efi_enabled(EFI_64BIT))
-			rv = efi_runtime_init64();
-		else
-			rv = efi_runtime_init32();
-
-		if (rv)
-			return rv;
-	}
-
-	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
-
-	return 0;
+	return rv;
 }
 
 static int __init efi_memmap_init(void)
@@ -489,10 +473,11 @@ void __init efi_init(void)
 	 * that doesn't match the kernel 32/64-bit mode.
 	 */
 
-	if (!efi_runtime_supported())
+	if (!efi_runtime_supported()) {
 		pr_info("No EFI runtime due to 32/64-bit mismatch with kernel\n");
-	else {
-		if (disable_runtime || efi_runtime_init())
+		clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
+	} else {
+		if (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_runtime_init())
 			return;
 	}
 	if (efi_memmap_init())
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 36ffa1747e84..e7584ac22be1 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -415,3 +415,10 @@ int __init efi_get_fdt_params(struct efi_fdt_params *params, int verbose)
 	return of_scan_flat_dt(fdt_find_uefi_params, &info);
 }
 #endif /* CONFIG_EFI_PARAMS_FROM_FDT */
+
+static int __init setup_noefi(char *arg)
+{
+	clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
+	return 0;
+}
+early_param("noefi", setup_noefi);
-- 
1.9.0

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 1/2] UEFI arm64: add noefi boot param

[Ard Biesheuvel]

On 6 August 2014 16:01, Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org> wrote:
> On Wed, 06 Aug, at 02:29:41PM, Leif Lindholm wrote:
>> On Wed, Aug 06, 2014 at 02:20:21PM +0100, Matt Fleming wrote:
>> > > Since this is really turning an x86-specific feature into a generic
>> > > one, could it be moved to core code?
>> > > Maybe an efi.mask, reusing the efi_enabled defines, with an
>> > > efi_disabled macro?
>> >
>> > Why the new efi_disabled() and efi.mask? This is all achievable with
>> > efi_enabled() and efi.flags, in fact, this kind of thing is exactly why
>> > they were invented.
>>
>> Because this flag is indicating which facilities we should not try to
>> enable, rather than which facilities we have enabled.
>>
>> The EFI_RUNTIME_SERVICES flag is set after successful call to
>> set_virtual_address_map. The apparent intent of "noefi" is to prevent
>> that call from being made in the first place.
>>
>> Anyway, it was just a suggestion - main point was it would make sense
>> to share the code.
>
> Definitely.
>
> Dave, below is the kind of thing that I had in mind. Please Cc the Xen
> and SGI folks when you submit your next version because they're likely
> to be hit the hardest by any changes to EFI_RUNTIME_SERVICES and
> "noefi".
>
> Obviously the addition of "efi=noruntime" is needed on top, but that's
> about 6 lines of code.
>
> ---
>  arch/arm64/kernel/efi.c     |  4 ++--
>  arch/x86/kernel/setup.c     |  4 +++-
>  arch/x86/platform/efi/efi.c | 33 +++++++++------------------------
>  drivers/firmware/efi/efi.c  |  7 +++++++
>  4 files changed, 21 insertions(+), 27 deletions(-)
>
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index e72f3100958f..9fdedb721a4e 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -83,6 +83,7 @@ static int __init uefi_init(void)
>
>         set_bit(EFI_BOOT, &efi.flags);
>         set_bit(EFI_64BIT, &efi.flags);
> +       set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
>
>         /*
>          * Verify the EFI Table
> @@ -386,7 +387,7 @@ static int __init arm64_enter_virtual_mode(void)
>         int count = 0;
>         unsigned long flags;
>
> -       if (!efi_enabled(EFI_BOOT)) {
> +       if (!efi_enabled(EFI_BOOT) || !efi_enabled(EFI_RUNTIME_SERVICES)) {
>                 pr_info("EFI services will not be available.\n");
>                 return -1;
>         }
> @@ -461,7 +462,6 @@ static int __init arm64_enter_virtual_mode(void)
>         /* Set up runtime services function pointers */
>         runtime = efi.systab->runtime;
>         efi_native_runtime_setup();
> -       set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
>

Shouldn't we clear the bit here if we failed to enable runtime
services for some reason? Other code may test the bit assuming that it
signifies that runtime services have been enabled successfully, while
this patch changes it to mean that we /intended/ to enable them, which
is not quite the same thing.


>         return 0;
>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 41ead8d3bc0b..3e2f820b6007 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -932,8 +932,10 @@ void __init setup_arch(char **cmdline_p)
>                 set_bit(EFI_64BIT, &efi.flags);
>         }
>
> -       if (efi_enabled(EFI_BOOT))
> +       if (efi_enabled(EFI_BOOT)) {
>                 efi_memblock_x86_reserve_range();
> +               set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> +       }
>  #endif
>
>         x86_init.oem.arch_setup();
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index a1f745b0bf1d..bac12cae2a80 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -70,14 +70,6 @@ static efi_config_table_type_t arch_tables[] __initdata = {
>
>  u64 efi_setup;         /* efi setup_data physical address */
>
> -static bool disable_runtime __initdata = false;
> -static int __init setup_noefi(char *arg)
> -{
> -       disable_runtime = true;
> -       return 0;
> -}
> -early_param("noefi", setup_noefi);
> -
>  int add_efi_memmap;
>  EXPORT_SYMBOL(add_efi_memmap);
>
> @@ -397,20 +389,12 @@ static int __init efi_runtime_init(void)
>          * hypercall which executes relevant EFI functions) and that is why
>          * they are always enabled.
>          */
> +       if (efi_enabled(EFI_64BIT))
> +               rv = efi_runtime_init64();
> +       else
> +               rv = efi_runtime_init32();
>
> -       if (!efi_enabled(EFI_PARAVIRT)) {
> -               if (efi_enabled(EFI_64BIT))
> -                       rv = efi_runtime_init64();
> -               else
> -                       rv = efi_runtime_init32();
> -
> -               if (rv)
> -                       return rv;
> -       }
> -
> -       set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> -
> -       return 0;
> +       return rv;
>  }
>
>  static int __init efi_memmap_init(void)
> @@ -489,10 +473,11 @@ void __init efi_init(void)
>          * that doesn't match the kernel 32/64-bit mode.
>          */
>
> -       if (!efi_runtime_supported())
> +       if (!efi_runtime_supported()) {
>                 pr_info("No EFI runtime due to 32/64-bit mismatch with kernel\n");
> -       else {
> -               if (disable_runtime || efi_runtime_init())
> +               clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> +       } else {
> +               if (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_runtime_init())
>                         return;
>         }
>         if (efi_memmap_init())
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 36ffa1747e84..e7584ac22be1 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -415,3 +415,10 @@ int __init efi_get_fdt_params(struct efi_fdt_params *params, int verbose)
>         return of_scan_flat_dt(fdt_find_uefi_params, &info);
>  }
>  #endif /* CONFIG_EFI_PARAMS_FROM_FDT */
> +
> +static int __init setup_noefi(char *arg)
> +{
> +       clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> +       return 0;
> +}
> +early_param("noefi", setup_noefi);
> --
> 1.9.0
>
> --
> Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 1/2] UEFI arm64: add noefi boot param

[Matt Fleming]

On Wed, 06 Aug, at 04:10:45PM, Ard Biesheuvel wrote:
> 
> Shouldn't we clear the bit here if we failed to enable runtime
> services for some reason? Other code may test the bit assuming that it
> signifies that runtime services have been enabled successfully, while
> this patch changes it to mean that we /intended/ to enable them, which
> is not quite the same thing.

Yep, good catch. We need to do something similar for efi_runtime_init()
on x86 too.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 1/2] UEFI arm64: add noefi boot param

[Leif Lindholm]

On Wed, Aug 06, 2014 at 03:18:14PM +0100, Matt Fleming wrote:
> On Wed, 06 Aug, at 04:10:45PM, Ard Biesheuvel wrote:
> > 
> > Shouldn't we clear the bit here if we failed to enable runtime
> > services for some reason? Other code may test the bit assuming that it
> > signifies that runtime services have been enabled successfully, while
> > this patch changes it to mean that we /intended/ to enable them, which
> > is not quite the same thing.
> 
> Yep, good catch. We need to do something similar for efi_runtime_init()
> on x86 too.

Since we're now overlaying two different meanings onto the
EFI_RUNTIME_SERVICES bit, could we add comments at set/clear points to
explicitly state the intended action? I.e.:

/* Set to attempt runtime services initialisation */

/* Clear to indicate runtime services will not be available */

/
    Leif

Re: [PATCH 1/2] UEFI arm64: add noefi boot param

[Matt Fleming]

On Wed, 06 Aug, at 03:48:39PM, Leif Lindholm wrote:
> 
> Since we're now overlaying two different meanings onto the
> EFI_RUNTIME_SERVICES bit, could we add comments at set/clear points to
> explicitly state the intended action? I.e.:
> 
> /* Set to attempt runtime services initialisation */
> 
> /* Clear to indicate runtime services will not be available */

Good idea.

My patch was only a hack to show how it's possible to use efi_enabled().
It may require some finessing ;-)

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 1/2] UEFI arm64: add noefi boot param

[Dave Young]

On 08/06/14 at 03:18pm, Matt Fleming wrote:
> On Wed, 06 Aug, at 04:10:45PM, Ard Biesheuvel wrote:
> > 
> > Shouldn't we clear the bit here if we failed to enable runtime
> > services for some reason? Other code may test the bit assuming that it
> > signifies that runtime services have been enabled successfully, while
> > this patch changes it to mean that we /intended/ to enable them, which
> > is not quite the same thing.
> 
> Yep, good catch. We need to do something similar for efi_runtime_init()
> on x86 too.

The current efi_runtime_init() enables the bit after getting the efi callback
phyaddr of SetVirtualAddressMap.

Thinking more about it, since SetVirtualAddressMap() could fail somehow it
seems better to set EFI_RUNTIME_SERIVCES bit only when enter virtual mode
return EFI_SUCCESS.

Does it make sense to you, Matt?

Thanks
Dave

Re: [PATCH 1/2] UEFI arm64: add noefi boot param

[Matt Fleming]

On Thu, 07 Aug, at 02:19:45PM, Dave Young wrote:
> 
> The current efi_runtime_init() enables the bit after getting the efi
> callback phyaddr of SetVirtualAddressMap.
> 
> Thinking more about it, since SetVirtualAddressMap() could fail
> somehow it seems better to set EFI_RUNTIME_SERIVCES bit only when
> enter virtual mode return EFI_SUCCESS.
> 
> Does it make sense to you, Matt?

If you're going ahead with the scheme I proposed yesterday you'd
actually want to *clear* the EFI_RUNTIME_SERVICES bit if
SetVirtualAddressMap() fails, since we would have set it by default for
EFI_BOOT.

However, I still think we want to panic() if SetVirtualAddressMap()
fails because we really never expect that function to return an error
under normal circumstances. Also, I'm not sure it's safe to make any
assumptions about the state of the system if SetVirtualAddressMap()
fails.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 1/2] UEFI arm64: add noefi boot param

[Dave Young]

On 08/07/14 at 09:26pm, Matt Fleming wrote:
> On Thu, 07 Aug, at 02:19:45PM, Dave Young wrote:
> > 
> > The current efi_runtime_init() enables the bit after getting the efi
> > callback phyaddr of SetVirtualAddressMap.
> > 
> > Thinking more about it, since SetVirtualAddressMap() could fail
> > somehow it seems better to set EFI_RUNTIME_SERIVCES bit only when
> > enter virtual mode return EFI_SUCCESS.
> > 
> > Does it make sense to you, Matt?
> 
> If you're going ahead with the scheme I proposed yesterday you'd
> actually want to *clear* the EFI_RUNTIME_SERVICES bit if
> SetVirtualAddressMap() fails, since we would have set it by default for
> EFI_BOOT.

There's one concern about the noefi early param handling in your proposal.
Your patch depends on the parse_early_param() being called after setting
EFI_RUNTIME_SERVICES bit. It's true in X86 setup.c, but it's not in arm64
setup.c so I'm afraid there's other piece of code need early params and it
will not work.

How about using a variable like runtime_disabled, then add a global function
efi_runtime_disabled() to get the status. 


> 
> However, I still think we want to panic() if SetVirtualAddressMap()
> fails because we really never expect that function to return an error
> under normal circumstances. Also, I'm not sure it's safe to make any
> assumptions about the state of the system if SetVirtualAddressMap()
> fails.

Ok, so the panic is reasonable to me as well.

Thanks
Dave

Re: [PATCH 1/2] UEFI arm64: add noefi boot param

[Dave Young]

On 08/06/14 at 03:01pm, Matt Fleming wrote:
> On Wed, 06 Aug, at 02:29:41PM, Leif Lindholm wrote:
> > On Wed, Aug 06, 2014 at 02:20:21PM +0100, Matt Fleming wrote:
> > > > Since this is really turning an x86-specific feature into a generic
> > > > one, could it be moved to core code?
> > > > Maybe an efi.mask, reusing the efi_enabled defines, with an
> > > > efi_disabled macro?
> > >  
> > > Why the new efi_disabled() and efi.mask? This is all achievable with
> > > efi_enabled() and efi.flags, in fact, this kind of thing is exactly why
> > > they were invented.
> > 
> > Because this flag is indicating which facilities we should not try to
> > enable, rather than which facilities we have enabled.
> > 
> > The EFI_RUNTIME_SERVICES flag is set after successful call to
> > set_virtual_address_map. The apparent intent of "noefi" is to prevent
> > that call from being made in the first place.
> > 
> > Anyway, it was just a suggestion - main point was it would make sense
> > to share the code.
> 
> Definitely.
> 
> Dave, below is the kind of thing that I had in mind. Please Cc the Xen
> and SGI folks when you submit your next version because they're likely
> to be hit the hardest by any changes to EFI_RUNTIME_SERVICES and
> "noefi".
> 
> Obviously the addition of "efi=noruntime" is needed on top, but that's
> about 6 lines of code.

[snip]

Matt, will file a new version based on your code. I will also address the
failure case in enter virtual mode.

Currently in x86, there's cases such as efi_map_regions failure and efi call
set_virtual_address_map, in case those failures it should be better to
unset the EFI_RUNTIME_SERVICES in efi.flags instead of do noting or panic.

As for Xen and SGI people? I have not been following all the efi threads
so I'm not sure who exactly I should cc, could you tell me? 


Thanks
Dave

Re: [PATCH 1/2] UEFI arm64: add noefi boot param

[Matt Fleming]

On Thu, 07 Aug, at 09:27:09AM, Dave Young wrote:
> 
> As for Xen and SGI people? I have not been following all the efi threads
> so I'm not sure who exactly I should cc, could you tell me? 

For Xen, Daniel Kiper <daniel.kiper-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> 

For SGI, Russ Anderson <rja-sJ/iWh9BUns@public.gmane.org> and Alex Thorlton
<athorlton-sJ/iWh9BUns@public.gmane.org>.

-- 
Matt Fleming, Intel Open Source Technology Center