Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1557252 > unrolled thread
| Started by | Dave Young <dyoung@redhat.com> |
|---|---|
| First post | 2017-01-12 10:50 +0100 |
| Last post | 2017-01-13 09:20 +0100 |
| Articles | 6 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas Dave Young <dyoung@redhat.com> - 2017-01-12 10:50 +0100
Re: [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas Nicolai Stange <nicstange@gmail.com> - 2017-01-12 12:20 +0100
Re: [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas Dave Young <dyoung@redhat.com> - 2017-01-12 22:40 +0100
Re: [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-01-12 17:20 +0100
Re: [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas Dave Young <dyoung@redhat.com> - 2017-01-12 22:30 +0100
Re: [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas Dave Young <dyoung@redhat.com> - 2017-01-13 09:20 +0100
| From | Dave Young <dyoung@redhat.com> |
|---|---|
| Date | 2017-01-12 10:50 +0100 |
| Subject | [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas |
| Message-ID | <sYKb7-2kU-15@gated-at.bofh.it> |
There are memory ranges like below when I testing early efi_mem_reserve:
efi: mem62: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
efi: mem63: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
efi: mem64: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
efi: mem65: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
efi: mem66: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
efi: mem67: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
So efi_memmap_insert will run into inserting same region multiple times,
also because efi_memmap_insert does not consider the duplicate ranges it will
cause memmap buffer overflow due to the size is pre-calculated, and kernel boot
fail with a panic. We did not detect such issue because current users of
efi_mem_insert do it very late after switching to virtual mode, at that time
the new cooked efi.memmap contains only runtime needed memory ranges.
efi_mem_reserve cares only about boot services regions and maybe loader areas.
So add a new argument to efi_memmap_insert for this purpose.
Later patches depend on this one for moving bgrt reservation to early code.
Signed-off-by: Dave Young <dyoung@redhat.com>
---
arch/x86/platform/efi/quirks.c | 2 +-
drivers/firmware/efi/fake_mem.c | 3 ++-
drivers/firmware/efi/memmap.c | 8 +++++++-
include/linux/efi.h | 4 ++--
4 files changed, 12 insertions(+), 5 deletions(-)
--- linux-x86.orig/drivers/firmware/efi/memmap.c
+++ linux-x86/drivers/firmware/efi/memmap.c
@@ -213,7 +213,7 @@ int __init efi_memmap_split_count(efi_me
* to see how large @buf needs to be.
*/
void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
- struct efi_mem_range *mem)
+ struct efi_mem_range *mem, bool boot_only)
{
u64 m_start, m_end, m_attr;
efi_memory_desc_t *md;
@@ -246,6 +246,12 @@ void __init efi_memmap_insert(struct efi
start = md->phys_addr;
end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;
+ if (boot_only && !(md->type == EFI_LOADER_DATA ||
+ md->type == EFI_LOADER_CODE ||
+ md->type == EFI_BOOT_SERVICES_CODE ||
+ md->type == EFI_BOOT_SERVICES_DATA))
+ continue;
+
if (m_start <= start && end <= m_end)
md->attribute |= m_attr;
--- linux-x86.orig/arch/x86/platform/efi/quirks.c
+++ linux-x86/arch/x86/platform/efi/quirks.c
@@ -226,7 +226,7 @@ void __init efi_arch_mem_reserve(phys_ad
return;
}
- efi_memmap_insert(&efi.memmap, new, &mr);
+ efi_memmap_insert(&efi.memmap, new, &mr, true);
early_memunmap(new, new_size);
efi_memmap_install(new_phys, num_entries);
--- linux-x86.orig/drivers/firmware/efi/fake_mem.c
+++ linux-x86/drivers/firmware/efi/fake_mem.c
@@ -85,7 +85,8 @@ void __init efi_fake_memmap(void)
}
for (i = 0; i < nr_fake_mem; i++)
- efi_memmap_insert(&efi.memmap, new_memmap, &fake_mems[i]);
+ efi_memmap_insert(&efi.memmap, new_memmap, &fake_mems[i],
+ false);
/* swap into new EFI memmap */
early_memunmap(new_memmap, efi.memmap.desc_size * new_nr_map);
--- linux-x86.orig/include/linux/efi.h
+++ linux-x86/include/linux/efi.h
@@ -957,8 +957,8 @@ extern int __init efi_memmap_install(phy
extern int __init efi_memmap_split_count(efi_memory_desc_t *md,
struct range *range);
extern void __init efi_memmap_insert(struct efi_memory_map *old_memmap,
- void *buf, struct efi_mem_range *mem);
-
+ void *buf, struct efi_mem_range *mem,
+ bool boot_only);
extern int efi_config_init(efi_config_table_type_t *arch_tables);
#ifdef CONFIG_EFI_ESRT
extern void __init efi_esrt_init(void);
[toc] | [next] | [standalone]
| From | Nicolai Stange <nicstange@gmail.com> |
|---|---|
| Date | 2017-01-12 12:20 +0100 |
| Message-ID | <sYLAd-3jp-5@gated-at.bofh.it> |
| In reply to | #1557252 |
Hi Dave,
On Thu, Jan 12 2017, Dave Young wrote:
> efi_mem_reserve cares only about boot services regions and maybe loader areas.
> So add a new argument to efi_memmap_insert for this purpose.
Please see below.
> --- linux-x86.orig/drivers/firmware/efi/memmap.c
> +++ linux-x86/drivers/firmware/efi/memmap.c
> @@ -213,7 +213,7 @@ int __init efi_memmap_split_count(efi_me
> * to see how large @buf needs to be.
> */
> void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
> - struct efi_mem_range *mem)
> + struct efi_mem_range *mem, bool boot_only)
> {
> u64 m_start, m_end, m_attr;
> efi_memory_desc_t *md;
> @@ -246,6 +246,12 @@ void __init efi_memmap_insert(struct efi
> start = md->phys_addr;
> end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;
>
> + if (boot_only && !(md->type == EFI_LOADER_DATA ||
> + md->type == EFI_LOADER_CODE ||
> + md->type == EFI_BOOT_SERVICES_CODE ||
> + md->type == EFI_BOOT_SERVICES_DATA))
> + continue;
> +
Actually, the efi_mem_desc_lookup() called from
efi_arch_memmap_reserve() will only return mds not satisfying the
following condition:
if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
md->type != EFI_BOOT_SERVICES_DATA &&
md->type != EFI_RUNTIME_SERVICES_DATA) {
continue;
}
Furthermore, efi_arch_mem_reserve() will only accept ranges fully
contained within such a region.
I think we can make efi_arch_mem_reserve() return early if
EFI_MEMORY_RUNTIME has been set already and thus, neglect this case in
efi_memmap_insert().
I suppose that we don't want to reserve within EFI_RUNTIME_SERVICES_DATA
regions in efi_mem_reserve() either -- these won't ever get made
available as general memory anyway [1]. So efi_arch_mem_reserve() should
return early here as well imo.
So, what would remain to be handled from efi_memmap_insert() in case of
boot_only would be EFI_BOOT_SERVICES_DATA only?
(As a sidenote, Matt pointed out at [1] that the EFI_LOADER_* regions
should be reserved early through memblock_reserve() and not through
efi_mem_reserve()).
Thanks,
Nicolai
[1] http://lkml.kernel.org/r/20170109130702.GI16838@codeblueprint.co.uk
[toc] | [prev] | [next] | [standalone]
| From | Dave Young <dyoung@redhat.com> |
|---|---|
| Date | 2017-01-12 22:40 +0100 |
| Subject | Re: [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas |
| Message-ID | <sYVge-Ak-25@gated-at.bofh.it> |
| In reply to | #1557369 |
On 01/12/17 at 12:15pm, Nicolai Stange wrote:
> Hi Dave,
>
> On Thu, Jan 12 2017, Dave Young wrote:
>
> > efi_mem_reserve cares only about boot services regions and maybe loader areas.
> > So add a new argument to efi_memmap_insert for this purpose.
>
> Please see below.
>
>
> > --- linux-x86.orig/drivers/firmware/efi/memmap.c
> > +++ linux-x86/drivers/firmware/efi/memmap.c
> > @@ -213,7 +213,7 @@ int __init efi_memmap_split_count(efi_me
> > * to see how large @buf needs to be.
> > */
> > void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
> > - struct efi_mem_range *mem)
> > + struct efi_mem_range *mem, bool boot_only)
> > {
> > u64 m_start, m_end, m_attr;
> > efi_memory_desc_t *md;
> > @@ -246,6 +246,12 @@ void __init efi_memmap_insert(struct efi
> > start = md->phys_addr;
> > end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;
> >
> > + if (boot_only && !(md->type == EFI_LOADER_DATA ||
> > + md->type == EFI_LOADER_CODE ||
> > + md->type == EFI_BOOT_SERVICES_CODE ||
> > + md->type == EFI_BOOT_SERVICES_DATA))
> > + continue;
> > +
>
>
> Actually, the efi_mem_desc_lookup() called from
> efi_arch_memmap_reserve() will only return mds not satisfying the
> following condition:
>
> if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
> md->type != EFI_BOOT_SERVICES_DATA &&
> md->type != EFI_RUNTIME_SERVICES_DATA) {
> continue;
> }
>
> Furthermore, efi_arch_mem_reserve() will only accept ranges fully
> contained within such a region.
>
> I think we can make efi_arch_mem_reserve() return early if
> EFI_MEMORY_RUNTIME has been set already and thus, neglect this case in
> efi_memmap_insert().
>
> I suppose that we don't want to reserve within EFI_RUNTIME_SERVICES_DATA
> regions in efi_mem_reserve() either -- these won't ever get made
> available as general memory anyway [1]. So efi_arch_mem_reserve() should
> return early here as well imo.
>
> So, what would remain to be handled from efi_memmap_insert() in case of
> boot_only would be EFI_BOOT_SERVICES_DATA only?
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?
>
> (As a sidenote, Matt pointed out at [1] that the EFI_LOADER_* regions
> should be reserved early through memblock_reserve() and not through
> efi_mem_reserve()).
>
> Thanks,
>
> Nicolai
>
>
> [1] http://lkml.kernel.org/r/20170109130702.GI16838@codeblueprint.co.uk
Thanks
Dave
[toc] | [prev] | [next] | [standalone]
| From | Ard Biesheuvel <ard.biesheuvel@linaro.org> |
|---|---|
| Date | 2017-01-12 17:20 +0100 |
| Subject | Re: [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas |
| Message-ID | <sYQgx-66R-3@gated-at.bofh.it> |
| In reply to | #1557252 |
Hello Dave,
On 12 January 2017 at 09:41, Dave Young <dyoung@redhat.com> wrote:
> There are memory ranges like below when I testing early efi_mem_reserve:
>
> efi: mem62: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> efi: mem63: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> efi: mem64: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> efi: mem65: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> efi: mem66: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> efi: mem67: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
>
Did you spot Peter's patch to prune invalid memmap entries?
https://git.kernel.org/cgit/linux/kernel/git/efi/efi.git/commit/?h=next&id=b2a91a35445229
I would expect this patch to no longer be necessary with that in place, no?
> So efi_memmap_insert will run into inserting same region multiple times,
> also because efi_memmap_insert does not consider the duplicate ranges it will
> cause memmap buffer overflow due to the size is pre-calculated, and kernel boot
> fail with a panic. We did not detect such issue because current users of
> efi_mem_insert do it very late after switching to virtual mode, at that time
> the new cooked efi.memmap contains only runtime needed memory ranges.
>
> efi_mem_reserve cares only about boot services regions and maybe loader areas.
> So add a new argument to efi_memmap_insert for this purpose.
>
> Later patches depend on this one for moving bgrt reservation to early code.
>
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
> arch/x86/platform/efi/quirks.c | 2 +-
> drivers/firmware/efi/fake_mem.c | 3 ++-
> drivers/firmware/efi/memmap.c | 8 +++++++-
> include/linux/efi.h | 4 ++--
> 4 files changed, 12 insertions(+), 5 deletions(-)
>
> --- linux-x86.orig/drivers/firmware/efi/memmap.c
> +++ linux-x86/drivers/firmware/efi/memmap.c
> @@ -213,7 +213,7 @@ int __init efi_memmap_split_count(efi_me
> * to see how large @buf needs to be.
> */
> void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
> - struct efi_mem_range *mem)
> + struct efi_mem_range *mem, bool boot_only)
> {
> u64 m_start, m_end, m_attr;
> efi_memory_desc_t *md;
> @@ -246,6 +246,12 @@ void __init efi_memmap_insert(struct efi
> start = md->phys_addr;
> end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;
>
> + if (boot_only && !(md->type == EFI_LOADER_DATA ||
> + md->type == EFI_LOADER_CODE ||
> + md->type == EFI_BOOT_SERVICES_CODE ||
> + md->type == EFI_BOOT_SERVICES_DATA))
> + continue;
> +
> if (m_start <= start && end <= m_end)
> md->attribute |= m_attr;
>
> --- linux-x86.orig/arch/x86/platform/efi/quirks.c
> +++ linux-x86/arch/x86/platform/efi/quirks.c
> @@ -226,7 +226,7 @@ void __init efi_arch_mem_reserve(phys_ad
> return;
> }
>
> - efi_memmap_insert(&efi.memmap, new, &mr);
> + efi_memmap_insert(&efi.memmap, new, &mr, true);
> early_memunmap(new, new_size);
>
> efi_memmap_install(new_phys, num_entries);
> --- linux-x86.orig/drivers/firmware/efi/fake_mem.c
> +++ linux-x86/drivers/firmware/efi/fake_mem.c
> @@ -85,7 +85,8 @@ void __init efi_fake_memmap(void)
> }
>
> for (i = 0; i < nr_fake_mem; i++)
> - efi_memmap_insert(&efi.memmap, new_memmap, &fake_mems[i]);
> + efi_memmap_insert(&efi.memmap, new_memmap, &fake_mems[i],
> + false);
>
> /* swap into new EFI memmap */
> early_memunmap(new_memmap, efi.memmap.desc_size * new_nr_map);
> --- linux-x86.orig/include/linux/efi.h
> +++ linux-x86/include/linux/efi.h
> @@ -957,8 +957,8 @@ extern int __init efi_memmap_install(phy
> extern int __init efi_memmap_split_count(efi_memory_desc_t *md,
> struct range *range);
> extern void __init efi_memmap_insert(struct efi_memory_map *old_memmap,
> - void *buf, struct efi_mem_range *mem);
> -
> + void *buf, struct efi_mem_range *mem,
> + bool boot_only);
> extern int efi_config_init(efi_config_table_type_t *arch_tables);
> #ifdef CONFIG_EFI_ESRT
> extern void __init efi_esrt_init(void);
>
>
[toc] | [prev] | [next] | [standalone]
| From | Dave Young <dyoung@redhat.com> |
|---|---|
| Date | 2017-01-12 22:30 +0100 |
| Subject | Re: [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas |
| Message-ID | <sYV6y-xa-15@gated-at.bofh.it> |
| In reply to | #1557563 |
On 01/12/17 at 04:15pm, Ard Biesheuvel wrote: > Hello Dave, > > On 12 January 2017 at 09:41, Dave Young <dyoung@redhat.com> wrote: > > There are memory ranges like below when I testing early efi_mem_reserve: > > > > efi: mem62: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB) > > efi: mem63: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB) > > efi: mem64: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB) > > efi: mem65: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB) > > efi: mem66: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB) > > efi: mem67: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB) > > > > Did you spot Peter's patch to prune invalid memmap entries? > > https://git.kernel.org/cgit/linux/kernel/git/efi/efi.git/commit/?h=next&id=b2a91a35445229 > > I would expect this patch to no longer be necessary with that in place, no? Ard, good suggestion, I did not notice that patch, will try. Actually I'm not sure about fake_mem handling, if we can filter out the invalid ranges then it will be natural to drop this one after a test. Thanks Dave
[toc] | [prev] | [next] | [standalone]
| From | Dave Young <dyoung@redhat.com> |
|---|---|
| Date | 2017-01-13 09:20 +0100 |
| Subject | Re: [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas |
| Message-ID | <sZ5fA-6Wf-7@gated-at.bofh.it> |
| In reply to | #1557823 |
On 01/13/17 at 05:20am, Dave Young wrote: > On 01/12/17 at 04:15pm, Ard Biesheuvel wrote: > > Hello Dave, > > > > On 12 January 2017 at 09:41, Dave Young <dyoung@redhat.com> wrote: > > > There are memory ranges like below when I testing early efi_mem_reserve: > > > > > > efi: mem62: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB) > > > efi: mem63: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB) > > > efi: mem64: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB) > > > efi: mem65: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB) > > > efi: mem66: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB) > > > efi: mem67: [Reserved | | | | | | | | | | | | ] range=[0x0000000000000000-0xffffffffffffffff] (0MB) > > > > > > > Did you spot Peter's patch to prune invalid memmap entries? > > > > https://git.kernel.org/cgit/linux/kernel/git/efi/efi.git/commit/?h=next&id=b2a91a35445229 > > > > I would expect this patch to no longer be necessary with that in place, no? > > Ard, good suggestion, I did not notice that patch, will try. Actually > I'm not sure about fake_mem handling, if we can filter out the invalid > ranges then it will be natural to drop this one after a test. The commit works for me. I updated the series. I addressed comments known, moved patch 1 to 4/4 with only boot service (dropped loader areas checking), I plan to send them out next Monday see if there are more comments. If anyone who want to try them now, feel free to download from below url: http://people.redhat.com/~ruyang/efi-bgrt/ Thanks Dave
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web