| Message ID | 1460155828-8690-2-git-send-email-ddaney.cavm@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 08, 2016 at 03:50:23PM -0700, David Daney wrote: > From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > There are two problems with the UEFI stub DT memory node removal > routine: > - it deletes nodes as it traverses the tree, which happens to work > but is not supported, as deletion invalidates the node iterator; > - deleting memory nodes entirely may discard annotations in the form > of additional properties on the nodes. > > Since the discovery of DT memory nodes occurs strictly before the > UEFI init sequence, we can simply clear the memblock memory table > before parsing the UEFI memory map. This way, it is no longer > necessary to remove the nodes, so we can remove that logic from the > stub as well. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Signed-off-by: David Daney <david.daney@cavium.com> > --- > drivers/firmware/efi/arm-init.c | 8 ++++++++ > drivers/firmware/efi/libstub/fdt.c | 24 +----------------------- > 2 files changed, 9 insertions(+), 23 deletions(-) > > diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c > index aa1f743..5d6945b 100644 > --- a/drivers/firmware/efi/arm-init.c > +++ b/drivers/firmware/efi/arm-init.c > @@ -143,6 +143,14 @@ static __init void reserve_regions(void) > if (efi_enabled(EFI_DBG)) > pr_info("Processing EFI memory map:\n"); > > + /* > + * Discard memblocks discovered so far: if there are any at this > + * point, they originate from memory nodes in the DT, and UEFI > + * uses its own memory map instead. > + */ > + memblock_dump_all(); > + memblock_remove(0, ULLONG_MAX); > + Does this change need to be applied to any other architectures given that deletion code has been removed from libstub below? Cheers,
On 14 April 2016 at 13:02, Steve Capper <steve.capper@arm.com> wrote: > On Fri, Apr 08, 2016 at 03:50:23PM -0700, David Daney wrote: >> From: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> >> There are two problems with the UEFI stub DT memory node removal >> routine: >> - it deletes nodes as it traverses the tree, which happens to work >> but is not supported, as deletion invalidates the node iterator; >> - deleting memory nodes entirely may discard annotations in the form >> of additional properties on the nodes. >> >> Since the discovery of DT memory nodes occurs strictly before the >> UEFI init sequence, we can simply clear the memblock memory table >> before parsing the UEFI memory map. This way, it is no longer >> necessary to remove the nodes, so we can remove that logic from the >> stub as well. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Signed-off-by: David Daney <david.daney@cavium.com> >> --- >> drivers/firmware/efi/arm-init.c | 8 ++++++++ >> drivers/firmware/efi/libstub/fdt.c | 24 +----------------------- >> 2 files changed, 9 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c >> index aa1f743..5d6945b 100644 >> --- a/drivers/firmware/efi/arm-init.c >> +++ b/drivers/firmware/efi/arm-init.c >> @@ -143,6 +143,14 @@ static __init void reserve_regions(void) >> if (efi_enabled(EFI_DBG)) >> pr_info("Processing EFI memory map:\n"); >> >> + /* >> + * Discard memblocks discovered so far: if there are any at this >> + * point, they originate from memory nodes in the DT, and UEFI >> + * uses its own memory map instead. >> + */ >> + memblock_dump_all(); >> + memblock_remove(0, ULLONG_MAX); >> + > > Does this change need to be applied to any other architectures given > that deletion code has been removed from libstub below? > The 'generic' libstub code below is only used by ARM, so we're safe here in that regard. >> for_each_efi_memory_desc(&memmap, md) { >> paddr = md->phys_addr; >> npages = md->num_pages; >> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c >> index 6dba78a..e58abfa 100644 >> --- a/drivers/firmware/efi/libstub/fdt.c >> +++ b/drivers/firmware/efi/libstub/fdt.c >> @@ -24,7 +24,7 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt, >> unsigned long map_size, unsigned long desc_size, >> u32 desc_ver) >> { >> - int node, prev, num_rsv; >> + int node, num_rsv; >> int status; >> u32 fdt_val32; >> u64 fdt_val64; >> @@ -54,28 +54,6 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt, >> goto fdt_set_fail; >> >> /* >> - * Delete any memory nodes present. We must delete nodes which >> - * early_init_dt_scan_memory may try to use. >> - */ >> - prev = 0; >> - for (;;) { >> - const char *type; >> - int len; >> - >> - node = fdt_next_node(fdt, prev, NULL); >> - if (node < 0) >> - break; >> - >> - type = fdt_getprop(fdt, node, "device_type", &len); >> - if (type && strncmp(type, "memory", len) == 0) { >> - fdt_del_node(fdt, node); >> - continue; >> - } >> - >> - prev = node; >> - } >> - >> - /* >> * Delete all memory reserve map entries. When booting via UEFI, >> * kernel will use the UEFI memory map to find reserved regions. >> */ >> -- >> 1.8.3.1 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>
On Thu, Apr 14, 2016 at 01:10:35PM +0200, Ard Biesheuvel wrote: > On 14 April 2016 at 13:02, Steve Capper <steve.capper@arm.com> wrote: > > On Fri, Apr 08, 2016 at 03:50:23PM -0700, David Daney wrote: > >> From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> > >> There are two problems with the UEFI stub DT memory node removal > >> routine: > >> - it deletes nodes as it traverses the tree, which happens to work > >> but is not supported, as deletion invalidates the node iterator; > >> - deleting memory nodes entirely may discard annotations in the form > >> of additional properties on the nodes. > >> > >> Since the discovery of DT memory nodes occurs strictly before the > >> UEFI init sequence, we can simply clear the memblock memory table > >> before parsing the UEFI memory map. This way, it is no longer > >> necessary to remove the nodes, so we can remove that logic from the > >> stub as well. > >> > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> Signed-off-by: David Daney <david.daney@cavium.com> > >> --- > >> drivers/firmware/efi/arm-init.c | 8 ++++++++ > >> drivers/firmware/efi/libstub/fdt.c | 24 +----------------------- > >> 2 files changed, 9 insertions(+), 23 deletions(-) > >> > >> diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c > >> index aa1f743..5d6945b 100644 > >> --- a/drivers/firmware/efi/arm-init.c > >> +++ b/drivers/firmware/efi/arm-init.c > >> @@ -143,6 +143,14 @@ static __init void reserve_regions(void) > >> if (efi_enabled(EFI_DBG)) > >> pr_info("Processing EFI memory map:\n"); > >> > >> + /* > >> + * Discard memblocks discovered so far: if there are any at this > >> + * point, they originate from memory nodes in the DT, and UEFI > >> + * uses its own memory map instead. > >> + */ > >> + memblock_dump_all(); > >> + memblock_remove(0, ULLONG_MAX); > >> + > > > > Does this change need to be applied to any other architectures given > > that deletion code has been removed from libstub below? > > > > The 'generic' libstub code below is only used by ARM, so we're safe > here in that regard. Thanks Ard, In that case, FWIW: Acked-by: Steve Capper <steve.capper@arm.com> Cheers,
On Fri, Apr 08, 2016 at 03:50:23PM -0700, David Daney wrote: > From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > There are two problems with the UEFI stub DT memory node removal > routine: > - it deletes nodes as it traverses the tree, which happens to work > but is not supported, as deletion invalidates the node iterator; > - deleting memory nodes entirely may discard annotations in the form > of additional properties on the nodes. > > Since the discovery of DT memory nodes occurs strictly before the > UEFI init sequence, we can simply clear the memblock memory table > before parsing the UEFI memory map. This way, it is no longer > necessary to remove the nodes, so we can remove that logic from the > stub as well. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Signed-off-by: David Daney <david.daney@cavium.com> > --- > drivers/firmware/efi/arm-init.c | 8 ++++++++ > drivers/firmware/efi/libstub/fdt.c | 24 +----------------------- > 2 files changed, 9 insertions(+), 23 deletions(-) Matt, are you ok with me taking this through the arm64 tree? (since the NUMA patches depend on it). If so, please can I have your ack? Will > diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c > index aa1f743..5d6945b 100644 > --- a/drivers/firmware/efi/arm-init.c > +++ b/drivers/firmware/efi/arm-init.c > @@ -143,6 +143,14 @@ static __init void reserve_regions(void) > if (efi_enabled(EFI_DBG)) > pr_info("Processing EFI memory map:\n"); > > + /* > + * Discard memblocks discovered so far: if there are any at this > + * point, they originate from memory nodes in the DT, and UEFI > + * uses its own memory map instead. > + */ > + memblock_dump_all(); > + memblock_remove(0, ULLONG_MAX); > + > for_each_efi_memory_desc(&memmap, md) { > paddr = md->phys_addr; > npages = md->num_pages; > diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c > index 6dba78a..e58abfa 100644 > --- a/drivers/firmware/efi/libstub/fdt.c > +++ b/drivers/firmware/efi/libstub/fdt.c > @@ -24,7 +24,7 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt, > unsigned long map_size, unsigned long desc_size, > u32 desc_ver) > { > - int node, prev, num_rsv; > + int node, num_rsv; > int status; > u32 fdt_val32; > u64 fdt_val64; > @@ -54,28 +54,6 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt, > goto fdt_set_fail; > > /* > - * Delete any memory nodes present. We must delete nodes which > - * early_init_dt_scan_memory may try to use. > - */ > - prev = 0; > - for (;;) { > - const char *type; > - int len; > - > - node = fdt_next_node(fdt, prev, NULL); > - if (node < 0) > - break; > - > - type = fdt_getprop(fdt, node, "device_type", &len); > - if (type && strncmp(type, "memory", len) == 0) { > - fdt_del_node(fdt, node); > - continue; > - } > - > - prev = node; > - } > - > - /* > * Delete all memory reserve map entries. When booting via UEFI, > * kernel will use the UEFI memory map to find reserved regions. > */ > -- > 1.8.3.1 >
On 15 April 2016 at 16:03, Will Deacon <will.deacon@arm.com> wrote: > On Fri, Apr 08, 2016 at 03:50:23PM -0700, David Daney wrote: >> From: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> >> There are two problems with the UEFI stub DT memory node removal >> routine: >> - it deletes nodes as it traverses the tree, which happens to work >> but is not supported, as deletion invalidates the node iterator; >> - deleting memory nodes entirely may discard annotations in the form >> of additional properties on the nodes. >> >> Since the discovery of DT memory nodes occurs strictly before the >> UEFI init sequence, we can simply clear the memblock memory table >> before parsing the UEFI memory map. This way, it is no longer >> necessary to remove the nodes, so we can remove that logic from the >> stub as well. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Signed-off-by: David Daney <david.daney@cavium.com> >> --- >> drivers/firmware/efi/arm-init.c | 8 ++++++++ >> drivers/firmware/efi/libstub/fdt.c | 24 +----------------------- >> 2 files changed, 9 insertions(+), 23 deletions(-) > > Matt, are you ok with me taking this through the arm64 tree? (since the > NUMA patches depend on it). If so, please can I have your ack? > Matt gave his Reviewed-by for v15 http://www.gossamer-threads.com/lists/linux/kernel/2390242
On Fri, 15 Apr, at 04:06:08PM, Ard Biesheuvel wrote: > On 15 April 2016 at 16:03, Will Deacon <will.deacon@arm.com> wrote: > > On Fri, Apr 08, 2016 at 03:50:23PM -0700, David Daney wrote: > >> From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> > >> There are two problems with the UEFI stub DT memory node removal > >> routine: > >> - it deletes nodes as it traverses the tree, which happens to work > >> but is not supported, as deletion invalidates the node iterator; > >> - deleting memory nodes entirely may discard annotations in the form > >> of additional properties on the nodes. > >> > >> Since the discovery of DT memory nodes occurs strictly before the > >> UEFI init sequence, we can simply clear the memblock memory table > >> before parsing the UEFI memory map. This way, it is no longer > >> necessary to remove the nodes, so we can remove that logic from the > >> stub as well. > >> > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> Signed-off-by: David Daney <david.daney@cavium.com> > >> --- > >> drivers/firmware/efi/arm-init.c | 8 ++++++++ > >> drivers/firmware/efi/libstub/fdt.c | 24 +----------------------- > >> 2 files changed, 9 insertions(+), 23 deletions(-) > > > > Matt, are you ok with me taking this through the arm64 tree? (since the > > NUMA patches depend on it). If so, please can I have your ack? > > > > Matt gave his Reviewed-by for v15 > > http://www.gossamer-threads.com/lists/linux/kernel/2390242 Heh, you beat me to it! Will, go ahead and take this through the arm64 tree.
On Fri, Apr 15, 2016 at 04:06:08PM +0200, Ard Biesheuvel wrote: > On 15 April 2016 at 16:03, Will Deacon <will.deacon@arm.com> wrote: > > On Fri, Apr 08, 2016 at 03:50:23PM -0700, David Daney wrote: > >> From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> > >> There are two problems with the UEFI stub DT memory node removal > >> routine: > >> - it deletes nodes as it traverses the tree, which happens to work > >> but is not supported, as deletion invalidates the node iterator; > >> - deleting memory nodes entirely may discard annotations in the form > >> of additional properties on the nodes. > >> > >> Since the discovery of DT memory nodes occurs strictly before the > >> UEFI init sequence, we can simply clear the memblock memory table > >> before parsing the UEFI memory map. This way, it is no longer > >> necessary to remove the nodes, so we can remove that logic from the > >> stub as well. > >> > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> Signed-off-by: David Daney <david.daney@cavium.com> > >> --- > >> drivers/firmware/efi/arm-init.c | 8 ++++++++ > >> drivers/firmware/efi/libstub/fdt.c | 24 +----------------------- > >> 2 files changed, 9 insertions(+), 23 deletions(-) > > > > Matt, are you ok with me taking this through the arm64 tree? (since the > > NUMA patches depend on it). If so, please can I have your ack? > > > > Matt gave his Reviewed-by for v15 > > http://www.gossamer-threads.com/lists/linux/kernel/2390242 Brill, thanks. Looks like it got dropped by accident for the latest posting. Will
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c index aa1f743..5d6945b 100644 --- a/drivers/firmware/efi/arm-init.c +++ b/drivers/firmware/efi/arm-init.c @@ -143,6 +143,14 @@ static __init void reserve_regions(void) if (efi_enabled(EFI_DBG)) pr_info("Processing EFI memory map:\n"); + /* + * Discard memblocks discovered so far: if there are any at this + * point, they originate from memory nodes in the DT, and UEFI + * uses its own memory map instead. + */ + memblock_dump_all(); + memblock_remove(0, ULLONG_MAX); + for_each_efi_memory_desc(&memmap, md) { paddr = md->phys_addr; npages = md->num_pages; diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c index 6dba78a..e58abfa 100644 --- a/drivers/firmware/efi/libstub/fdt.c +++ b/drivers/firmware/efi/libstub/fdt.c @@ -24,7 +24,7 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt, unsigned long map_size, unsigned long desc_size, u32 desc_ver) { - int node, prev, num_rsv; + int node, num_rsv; int status; u32 fdt_val32; u64 fdt_val64; @@ -54,28 +54,6 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt, goto fdt_set_fail; /* - * Delete any memory nodes present. We must delete nodes which - * early_init_dt_scan_memory may try to use. - */ - prev = 0; - for (;;) { - const char *type; - int len; - - node = fdt_next_node(fdt, prev, NULL); - if (node < 0) - break; - - type = fdt_getprop(fdt, node, "device_type", &len); - if (type && strncmp(type, "memory", len) == 0) { - fdt_del_node(fdt, node); - continue; - } - - prev = node; - } - - /* * Delete all memory reserve map entries. When booting via UEFI, * kernel will use the UEFI memory map to find reserved regions. */