Re: [PATCH v4 1/5] efi: Export boot-services code and data as debugfs-blobs

[Ard Biesheuvel]

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?

If so, after the flag rename:

Acked-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>

(for the series)