On 27 April 2018 at 10:13, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > Hi, > > > On 26-04-18 23:35, Ard Biesheuvel wrote: >> >> On 26 April 2018 at 23:02, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >>> >>> Hi, >>> >>> >>> On 26-04-18 18:51, Ard Biesheuvel wrote: >>>> >>>> >>>> On 26 April 2018 at 14:06, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >>>>> >>>>> >>>>> Sometimes it is useful to be able to dump the efi boot-services code >>>>> and >>>>> data. This commit adds these as debugfs-blobs to /sys/kernel/debug/efi, >>>>> but only if efi=debug is passed on the kernel-commandline as this >>>>> requires >>>>> not freeing those memory-regions, which costs 20+ MB of RAM. >>>>> >>>>> Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> >>>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> >>>>> --- >>>>> Changes in v4: >>>>> -Add new EFI_BOOT_SERVICES flag and use it to determine if the >>>>> boot-services >>>>> memory segments are available (and thus if it makes sense to >>>>> register >>>>> the >>>>> debugfs bits for them) >>>>> >>>>> Changes in v2: >>>>> -Do not call pr_err on debugfs call failures >>>>> --- >>>>> arch/x86/platform/efi/efi.c | 1 + >>>>> arch/x86/platform/efi/quirks.c | 4 +++ >>>>> drivers/firmware/efi/efi.c | 53 >>>>> ++++++++++++++++++++++++++++++++++ >>>>> include/linux/efi.h | 1 + >>>>> 4 files changed, 59 insertions(+) >>>>> >>>>> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c >>>>> index 9061babfbc83..568b7ee3d323 100644 >>>>> --- a/arch/x86/platform/efi/efi.c >>>>> +++ b/arch/x86/platform/efi/efi.c >>>>> @@ -208,6 +208,7 @@ int __init efi_memblock_x86_reserve_range(void) >>>>> efi.memmap.desc_version); >>>>> >>>>> memblock_reserve(pmap, efi.memmap.nr_map * >>>>> efi.memmap.desc_size); >>>>> + set_bit(EFI_BOOT_SERVICES, &efi.flags); >>>>> >>>> >>>> I think it would be better if the flag conveys whether boot services >>>> regions are being preserved, because they will always exist when >>>> EFI_BOOT is set. >>>> The name should then reflect that as well, e.g., >>>> EFI_PRESERVE_BS_REGIONS. >>> >>> >>> >>> Ok, I will rename the flag to EFI_PRESERVE_BS_REGIONS for v5 >>> (I'm going to wait a bit with sending out v5 to give others a change >>> to comment on v4). >>> >>>>> return 0; >>>>> } >>>>> diff --git a/arch/x86/platform/efi/quirks.c >>>>> b/arch/x86/platform/efi/quirks.c >>>>> index 36c1f8b9f7e0..16bdb9e3b343 100644 >>>>> --- a/arch/x86/platform/efi/quirks.c >>>>> +++ b/arch/x86/platform/efi/quirks.c >>>>> @@ -376,6 +376,10 @@ void __init efi_free_boot_services(void) >>>>> int num_entries = 0; >>>>> void *new, *new_md; >>>>> >>>>> + /* Keep all regions for /sys/kernel/debug/efi */ >>>>> + if (efi_enabled(EFI_DBG)) >>>>> + return; >>>>> + >>>> >>>> >>>> >>>> Why is this only necessary when EFI_DBG is enabled? How are you >>>> ensuring that the firmware is still in memory when the subsequent >>>> patches start relying on that? >>> >>> >>> >>> The 2nd patch in this series makes init/main.c call >>> efi_check_for_embedded_firmwares() before efi_free_boot_services(), >>> efi_check_for_embedded_firmwares() then walks the dmi_system_id-s >>> "registered" (its a static list) by drivers and if their is a dmi_match >>> searches for the firmware described by the dmi_system_id.driver_data >>> ptr. If a firmware gets found it gets memdup-ed, so that we do not >>> have to keep all of the boot-services code around. >>> >> >> Right, thanks for clearing that up. >> >> So that means that preserving the boot regions is really only >> necessary if you want to inspect them via debugfs, and the firmware >> loader does not rely on that. I missed that part. >> >> That means the only reason we have the new flag is to ensure that the >> shared debugfs code only exposes the boot services regions if they >> were preserved to begin with by the arch code, right? > > > Mostly right, since we have the flag now anyways I'm also using > it as a condition to call efi_check_for_embedded_firmwares(), > efi_check_for_embedded_firmwares() needs to happen after mm_init() (*) > and on non x86 the boot-services code/data is long gone then so > there is nothing for efi_check_for_embedded_firmwares() to look at. > Ah, of course, yes that was the whole point. >> If so, after the flag rename: >> >> Acked-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> > > > I assume that the "mostly right" is good enough and I'm going to > add your Acked-by for the next version. Let me know if you've > any objections against that. > Yes that's fine. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
