| Message ID | 20140806130623.GI4179@bivouac.eciton.net |
|---|---|
| State | New |
| Headers | show |
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".
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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) {