LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Hans de Goede <hdegoede@redhat.com>
Cc: "Luis R . Rodriguez" <mcgrof@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, "H . Peter Anvin" <hpa@zytor.com>,
	Peter Jones <pjones@redhat.com>, Dave Olsthoorn <dave@bewaar.me>,
	Will Deacon <will.deacon@arm.com>,
	Andy Lutomirski <luto@kernel.org>,
	Matt Fleming <matt@codeblueprint.co.uk>,
	David Howells <dhowells@redhat.com>,
	Mimi Zohar <zohar@linux.vnet.ibm.com>,
	Josh Triplett <josh@joshtriplett.org>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Martin Fuzzey <mfuzzey@parkeon.com>,
	Kalle Valo <kvalo@codeaurora.org>,
	Arend Van Spriel <arend.vanspriel@broadcom.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Nicolas Broeking <nbroeking@me.com>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Torsten Duwe <duwe@suse.de>, Kees Cook <keescook@chromium.org>,
	"the arch/x86 maintainers" <x86@kernel.org>,
	linux-efi@vger.kernel.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 1/5] efi: Export boot-services code and data as debugfs-blobs
Date: Thu, 26 Apr 2018 23:35:43 +0200	[thread overview]
Message-ID: <CAKv+Gu9C09Qn6vxo1SaCtEP3KDTT3--JbybkvTaNF+N_G2HyOw@mail.gmail.com> (raw)
In-Reply-To: <d587af46-72cb-8fc7-8432-36473b67de47@redhat.com>

On 26 April 2018 at 23:02, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 26-04-18 18:51, Ard Biesheuvel wrote:
>>
>> On 26 April 2018 at 14:06, Hans de Goede <hdegoede@redhat.com> 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@linuxfoundation.org>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> 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@linaro.org>

(for the series)

  reply	other threads:[~2018-04-26 21:35 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-26 12:06 [PATCH v4 0/5] efi/firmware/platform-x86: Add EFI embedded fw support Hans de Goede
2018-04-26 12:06 ` [PATCH v4 1/5] efi: Export boot-services code and data as debugfs-blobs Hans de Goede
2018-04-26 16:51   ` Ard Biesheuvel
2018-04-26 21:02     ` Hans de Goede
2018-04-26 21:35       ` Ard Biesheuvel [this message]
2018-04-27  8:13         ` Hans de Goede
2018-04-27  8:24           ` Ard Biesheuvel
2018-04-26 12:06 ` [PATCH v4 2/5] efi: Add embedded peripheral firmware support Hans de Goede
2018-04-26 12:06 ` [PATCH v4 3/5] platform/x86: Rename silead_dmi to touchscreen_dmi Hans de Goede
2018-04-26 12:06 ` [PATCH v4 4/5] platform/x86: touchscreen_dmi: Add EFI embedded firmware info support Hans de Goede
2018-04-26 12:06 ` [PATCH v4 5/5] platform/x86: touchscreen_dmi: Add info for the Chuwi Vi8 Plus tablet Hans de Goede
2018-04-26 16:53 ` [PATCH v4 0/5] efi/firmware/platform-x86: Add EFI embedded fw support Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKv+Gu9C09Qn6vxo1SaCtEP3KDTT3--JbybkvTaNF+N_G2HyOw@mail.gmail.com \
    --to=ard.biesheuvel@linaro.org \
    --cc=arend.vanspriel@broadcom.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=dave@bewaar.me \
    --cc=dhowells@redhat.com \
    --cc=dmitry.torokhov@gmail.com \
    --cc=duwe@suse.de \
    --cc=gregkh@linuxfoundation.org \
    --cc=hdegoede@redhat.com \
    --cc=hpa@zytor.com \
    --cc=josh@joshtriplett.org \
    --cc=keescook@chromium.org \
    --cc=kvalo@codeaurora.org \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=matt@codeblueprint.co.uk \
    --cc=mcgrof@kernel.org \
    --cc=mfuzzey@parkeon.com \
    --cc=mingo@redhat.com \
    --cc=nbroeking@me.com \
    --cc=pjones@redhat.com \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=will.deacon@arm.com \
    --cc=x86@kernel.org \
    --cc=zohar@linux.vnet.ibm.com \
    --subject='Re: [PATCH v4 1/5] efi: Export boot-services code and data as debugfs-blobs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).