From: Ard Biesheuvel <ard.biesheuvel@linaro.org> To: Matt Fleming <matt@codeblueprint.co.uk> Cc: "Dave Young" <dyoung@redhat.com>, "Nicolai Stange" <nicstange@gmail.com>, "linux-efi@vger.kernel.org" <linux-efi@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "x86@kernel.org" <x86@kernel.org>, "Ingo Molnar" <mingo@kernel.org>, "Thomas Gleixner" <tglx@linutronix.de>, "hpa@zytor.com" <hpa@zytor.com>, "Dan Williams" <dan.j.williams@intel.com>, "Mika Penttilä" <mika.penttila@nextfour.com>, "Bhupesh Sharma" <bhsharma@redhat.com> Subject: Re: [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas Date: Fri, 27 Jan 2017 22:15:26 +0000 [thread overview] Message-ID: <CAKv+Gu_koToJSq1i2TG2bBrMzwHaLpva3GKAbD9jzNN99VfvYw@mail.gmail.com> (raw) In-Reply-To: <20170127221311.GH31613@codeblueprint.co.uk> On 27 January 2017 at 22:13, Matt Fleming <matt@codeblueprint.co.uk> wrote: > On Fri, 27 Jan, at 05:04:50PM, Ard Biesheuvel wrote: >> On 27 January 2017 at 14:48, Matt Fleming <matt@codeblueprint.co.uk> 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 > region. > 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
WARNING: multiple messages have this Message-ID (diff)
From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> To: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> Cc: "Dave Young" <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "Nicolai Stange" <nicstange-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, "linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org" <x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, "Ingo Molnar" <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, "Thomas Gleixner" <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>, "hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>, "Dan Williams" <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>, "Mika Penttilä" <mika.penttila-MRsr7dthA9VWk0Htik3J/w@public.gmane.org>, "Bhupesh Sharma" <bhsharma-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Subject: Re: [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas Date: Fri, 27 Jan 2017 22:15:26 +0000 [thread overview] Message-ID: <CAKv+Gu_koToJSq1i2TG2bBrMzwHaLpva3GKAbD9jzNN99VfvYw@mail.gmail.com> (raw) In-Reply-To: <20170127221311.GH31613-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> On 27 January 2017 at 22:13, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote: > On Fri, 27 Jan, at 05:04:50PM, Ard Biesheuvel wrote: >> On 27 January 2017 at 14:48, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> 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 > region. > 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
next prev parent reply other threads:[~2017-01-27 22:22 UTC|newest] Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-01-12 9:41 [PATCH 0/4] efi/x86: move efi bgrt init code to early init Dave Young 2017-01-12 9:41 ` [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas Dave Young 2017-01-12 9:41 ` Dave Young 2017-01-12 11:15 ` Nicolai Stange 2017-01-12 11:15 ` Nicolai Stange 2017-01-12 21:29 ` Dave Young 2017-01-12 21:29 ` Dave Young 2017-01-27 14:48 ` Matt Fleming 2017-01-27 17:04 ` Ard Biesheuvel 2017-01-27 17:04 ` Ard Biesheuvel 2017-01-27 22:13 ` Matt Fleming 2017-01-27 22:13 ` Matt Fleming 2017-01-27 22:15 ` Ard Biesheuvel [this message] 2017-01-27 22:15 ` Ard Biesheuvel 2017-01-12 16:15 ` Ard Biesheuvel 2017-01-12 16:15 ` Ard Biesheuvel 2017-01-12 21:20 ` Dave Young 2017-01-12 21:20 ` Dave Young 2017-01-13 8:10 ` Dave Young 2017-01-12 9:41 ` [PATCH 2/4] efi/x86: move efi bgrt init code to early init code Dave Young 2017-01-12 9:56 ` Dave Young 2017-01-12 11:54 ` Nicolai Stange 2017-01-12 11:54 ` Nicolai Stange 2017-01-12 21:39 ` Dave Young 2017-01-12 23:11 ` Nicolai Stange 2017-01-12 23:11 ` Nicolai Stange 2017-01-13 2:21 ` Dave Young 2017-01-13 3:04 ` Dave Young 2017-01-13 3:04 ` Dave Young 2017-01-13 12:21 ` Nicolai Stange 2017-01-13 12:21 ` Nicolai Stange 2017-01-16 2:55 ` Dave Young 2017-01-16 2:55 ` Dave Young 2017-01-12 16:20 ` Ard Biesheuvel 2017-01-12 16:20 ` Ard Biesheuvel 2017-01-12 21:33 ` Dave Young 2017-01-12 21:33 ` Dave Young 2017-01-16 15:15 ` Bhupesh Sharma 2017-01-17 17:00 ` Ard Biesheuvel 2017-01-12 9:41 ` [PATCH 3/4] efi/x86: move efi_print_memmap to drivers/firmware/efi/memmap.c Dave Young 2017-01-12 9:41 ` Dave Young 2017-01-12 12:08 ` Nicolai Stange 2017-01-12 21:40 ` Dave Young 2017-01-12 9:41 ` [PATCH 4/4] efi/x86: add debug code to print cooked memmap Dave Young 2017-01-12 16:18 ` 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+Gu_koToJSq1i2TG2bBrMzwHaLpva3GKAbD9jzNN99VfvYw@mail.gmail.com \ --to=ard.biesheuvel@linaro.org \ --cc=bhsharma@redhat.com \ --cc=dan.j.williams@intel.com \ --cc=dyoung@redhat.com \ --cc=hpa@zytor.com \ --cc=linux-efi@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=matt@codeblueprint.co.uk \ --cc=mika.penttila@nextfour.com \ --cc=mingo@kernel.org \ --cc=nicstange@gmail.com \ --cc=tglx@linutronix.de \ --cc=x86@kernel.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.