| Message ID | 20140806140155.GC15082@console-pimps.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 6 August 2014 16:01, Matt Fleming <matt@console-pimps.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
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.
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
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 ;-)
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
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
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@oracle.com> For SGI, Russ Anderson <rja@sgi.com> and Alex Thorlton <athorlton@sgi.com>.
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.
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
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);