lkml.org 
[lkml]   [2018]   [Aug]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRE: [PATCH V1 6/6] x86/efi: Introduce EFI_WARN_ON_ILLEGAL_ACCESSES
Date
> > +config EFI_WARN_ON_ILLEGAL_ACCESSES
>
> How about shortening the name to just 'EFI_WARN_ON_ILLEGAL_ACCESS'?
>

Makes sense.. I will shorten it.

> > + bool "Warn about illegal memory accesses by firmware"
> > + depends on EFI
>
> From a distribution p-o-v, I would suggest that we set this CONFIG option only if
> we are in the EXPERT mode, as this need more thrashing with the behaviour we
> see on old, buggy EFI firmwares available on very old x86 machines. So
> something like:
> bool "Warn about illegal memory accesses by firmware" if EXPERT
>

Agreed. Although the feature is intended to warn about all these buggy firmware's
instead of silently working around (as we are doing presently), it needs more testing.
Hence, as you said, I think it's safe to have it as an EXPERT config option.

> > + help
> > + Enable this debug feature so that the kernel can detect illegal
> > + memory accesses by firmware and issue a warning. Also,
> > + 1. If the illegally accessed region is EFI_BOOT_SERVICES_<CODE/DATA>,
> > + the kernel fixes it up by mapping the requested region.

[....]

> > diff --git a/init/main.c b/init/main.c index
> > 3b4ada11ed52..dce0520861a1 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -730,7 +730,8 @@ asmlinkage __visible void __init start_kernel(void)
> > arch_post_acpi_subsys_init();
> > sfi_init_late();
> >
> > - if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> > + if (efi_enabled(EFI_RUNTIME_SERVICES) &&
> > + !IS_ENABLED(CONFIG_EFI_WARN_ON_ILLEGAL_ACCESSES)) {
>
> Since this is an arch agnostic code leg, do we really want to introduce a generic
> check for CONFIG_EFI_WARN_ON_ILLEGAL_ACCESSES
> here, without checking for whether we are actually running this on a
> x86 hardware, or alternatively we can consider moving
> CONFIG_EFI_WARN_ON_ILLEGAL_ACCESSES out of 'arch/x86/Kconfig' to
> 'arch/Kconfig' so that later it can be used on other archs like ARM64 as well.
>
> I think the later would be more useful..

Thanks for bringing this up. I see your point. I will refrain from polluting architecture
agnostic code. As we don't need efi_free_boot_services() at all, if EFI_WARN_ON_ILLEGAL_ACCESSES
is enabled, probably making changes to include/linux/efi.h would be better, I guess..

Moving this to arch/Kconfig could be too futuristic.. I guess.. because I am not sure
if this problem exists on ARM machines, if so, probably Ard could suggest something here.

Regards,
Sai
\
 
 \ /
  Last update: 2018-08-10 19:34    [W:0.042 / U:55.600 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site