Re: [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas
From: Ard Biesheuvel
Date: Fri Jan 27 2017 - 17:23:08 EST
On 27 January 2017 at 22:13, Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, 27 Jan, at 05:04:50PM, Ard Biesheuvel wrote:
>> On 27 January 2017 at 14:48, Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx> wrote:
>> > On Fri, 13 Jan, at 05:29:52AM, Dave Young wrote:
>> >> It sounds reasonable though I'm still not sure about EFI_LOADER*.
>> >> The main purpose of this patch is to address the invalid mem ranges
>> >> case. As Ard mentioned I will test with Peter's patch first, if it works
>> >> fine I would like to either drop this patch as a future improvement or add
>> >> it at the end of the next post.
>> >> Matt, what's your opinion about the boot_only check and the EFI_LOADERS*
>> >> question?
>> > The main reason that efi_mem_reserve() isn't used for EFI_LOADER
>> > regions today is because we already have a mechanism for reserving it
>> > via memblock_reserve(), which we do during a very early stage of boot
>> > when parsing all the different types of SETUP_* objects.
>> > It's questionable whether it would make sense to switch to
>> > efi_mem_reserve() for EFI_LOADER regions because then you'd
>> > potentially have different APIs for different SETUP_* objects.
>> > As things stand today, I would suggest triggering a WARN_ON() if
>> > someone tries to efi_mem_reserve() an EFI_LOADER region, until/unless
>> > the day comes when a user exists in the kernel.
>> Hmm, I just queued this. Should we drop it again?
> Does dropping it break the entire series?
> Having had some time to re-read Dave's commit log, it sounds like it
> just papers over a bug, which is that efi_memmap_insert() cannot deal
> with reserved entries, which all look like they describe the same
No, it cannot deal with bogus entries, and Peter already fixed that.
Dave confirmed that Peter's patch (the one we moved from next to
urgen) made the problem go away.
> So I guess my question is: Shouldn't you fix that instead of requiring
> the caller of efi_memmap_insert() to understand what type of entries
> it's mapping?
Indeed. So I don't think the patch is actually needed anymore