| Message ID | 1539181834-21736-2-git-send-email-paolo.pisati@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Fix /proc/iomem for reserved but not memory regions | expand |
On 10/10/18 15:30, Paolo Pisati wrote: > From: James Morse <james.morse@arm.com> > > BugLink: https://bugs.launchpad.net/bugs/1797139 > > commit 50d7ba36b916 ("arm64: export memblock_reserve()d regions via > /proc/iomem") wrongly assumed that memblock_reserve() would not be used to > reserve regions that aren't memory. It turns out, this is exactly what > early_init_dt_reserve_memory_arch() will do if it finds a reservation > that was also carved out of the memory node. > > reserve_memblock_reserved_regions() now needs to cope with reserved regions > that aren't memory, which means we must walk two lists at once. > > We can't use walk_system_ram_res() and reserve_region_with_split() > together, as the former hands its callback a copied resource on > the stack, where as the latter expects the in-tree resource to be > provided. > > Allocate an array of struct resources during request_standard_resources() > so that we have all the 'System RAM' regions on hand. > > Increasing the mem_idx cursor is optional as multiple memblock_reserved() > regions may exist in one System RAM region. > Because adjacent memblock_reserved() regions will be merged, we also need > to consider multiple System RAM regions for one span of memblock_reserved() > address space. > > Fixes: 50d7ba36b916 ("arm64: export memblock_reserve()d regions via /proc/iomem") > Reported-by: John Stultz <john.stultz@linaro.org> > CC: Akashi Takahiro <takahiro.akashi@linaro.org> > CC: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Signed-off-by: James Morse <james.morse@arm.com> > Tested-by: John Stultz <john.stultz@linaro.org> > Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com> > --- > arch/arm64/kernel/setup.c | 50 +++++++++++++++++++++++++++++------------------ > 1 file changed, 31 insertions(+), 19 deletions(-) > > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c > index 5b4fac4..952c2b1 100644 > --- a/arch/arm64/kernel/setup.c > +++ b/arch/arm64/kernel/setup.c > @@ -64,6 +64,9 @@ > #include <asm/xen/hypervisor.h> > #include <asm/mmu_context.h> > > +static int num_standard_resources; > +static struct resource *standard_resources; > + > phys_addr_t __fdt_pointer __initdata; > > /* > @@ -206,14 +209,19 @@ static void __init request_standard_resources(void) > { > struct memblock_region *region; > struct resource *res; > + unsigned long i = 0; > > kernel_code.start = __pa_symbol(_text); > kernel_code.end = __pa_symbol(__init_begin - 1); > kernel_data.start = __pa_symbol(_sdata); > kernel_data.end = __pa_symbol(_end - 1); > > + num_standard_resources = memblock.memory.cnt; > + standard_resources = alloc_bootmem_low(num_standard_resources * > + sizeof(*standard_resources)); > + > for_each_memblock(memory, region) { > - res = alloc_bootmem_low(sizeof(*res)); > + res = &standard_resources[i++]; > if (memblock_is_nomap(region)) { > res->name = "reserved"; > res->flags = IORESOURCE_MEM; > @@ -244,8 +252,11 @@ static void __init request_standard_resources(void) > static int __init reserve_memblock_reserved_regions(void) > { > phys_addr_t start, end, roundup_end = 0; > - struct resource *mem, *res; > - u64 i; > + struct resource *mem; > + u64 i, mem_idx = 0; > + > + if (!standard_resources) > + return 0; > > for_each_reserved_mem_region(i, &start, &end) { > if (end <= roundup_end) > @@ -255,24 +266,25 @@ static int __init reserve_memblock_reserved_regions(void) > end = __pfn_to_phys(PFN_UP(end)) - 1; > roundup_end = end; > > - res = kzalloc(sizeof(*res), GFP_ATOMIC); > - if (WARN_ON(!res)) > - return -ENOMEM; > - res->start = start; > - res->end = end; > - res->name = "reserved"; > - res->flags = IORESOURCE_MEM; > + while (start > standard_resources[mem_idx].end) { > + mem_idx++; > + if (mem_idx >= num_standard_resources) > + return 0; /* no more 'System RAM' */ > + } > + do { > + mem = &standard_resources[mem_idx]; > > - mem = request_resource_conflict(&iomem_resource, res); > - /* > - * We expected memblock_reserve() regions to conflict with > - * memory created by request_standard_resources(). > - */ > - if (WARN_ON_ONCE(!mem)) > - continue; > - kfree(res); > + if (mem->start > end) > + continue; /* doesn't overlap with memory */ > + > + start = max(start, mem->start); > + reserve_region_with_split(mem, start, > + min(end, mem->end), > + "reserved"); > > - reserve_region_with_split(mem, start, end, "reserved"); > + if (mem->end < end) > + mem_idx++; > + } while (mem->end < end && mem_idx < num_standard_resources); > } > > return 0; > Even though this hasn't landed upstream it has been tested John Stultz and it fixes the issue. So, I'm happy to ACK this given the testing. Acked-by: Colin Ian King <colin.king@canonical.com>
Given, this is not upstream I would make this a "UBUNTU: SAUCE: " patch... On 10.10.2018 16:30, Paolo Pisati wrote: > From: James Morse <james.morse@arm.com> > > BugLink: https://bugs.launchpad.net/bugs/1797139 > > commit 50d7ba36b916 ("arm64: export memblock_reserve()d regions via > /proc/iomem") wrongly assumed that memblock_reserve() would not be used to > reserve regions that aren't memory. It turns out, this is exactly what > early_init_dt_reserve_memory_arch() will do if it finds a reservation > that was also carved out of the memory node. > > reserve_memblock_reserved_regions() now needs to cope with reserved regions > that aren't memory, which means we must walk two lists at once. > > We can't use walk_system_ram_res() and reserve_region_with_split() > together, as the former hands its callback a copied resource on > the stack, where as the latter expects the in-tree resource to be > provided. > > Allocate an array of struct resources during request_standard_resources() > so that we have all the 'System RAM' regions on hand. > > Increasing the mem_idx cursor is optional as multiple memblock_reserved() > regions may exist in one System RAM region. > Because adjacent memblock_reserved() regions will be merged, we also need > to consider multiple System RAM regions for one span of memblock_reserved() > address space. > > Fixes: 50d7ba36b916 ("arm64: export memblock_reserve()d regions via /proc/iomem") > Reported-by: John Stultz <john.stultz@linaro.org> > CC: Akashi Takahiro <takahiro.akashi@linaro.org> > CC: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Signed-off-by: James Morse <james.morse@arm.com> > Tested-by: John Stultz <john.stultz@linaro.org> (cherry picked from https://www.spinics.net/lists/arm-kernel/msg675580.html) > Signed-off-by: Paolo Pisati <paolo.pisati@canonical.comAcked-by: Stefan Bader <stefan.bader@canonical.com> > --- Testing was good and with the slight modifications to provenance I do not see reasons to not take this. -Stefan > arch/arm64/kernel/setup.c | 50 +++++++++++++++++++++++++++++------------------ > 1 file changed, 31 insertions(+), 19 deletions(-) > > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c > index 5b4fac4..952c2b1 100644 > --- a/arch/arm64/kernel/setup.c > +++ b/arch/arm64/kernel/setup.c > @@ -64,6 +64,9 @@ > #include <asm/xen/hypervisor.h> > #include <asm/mmu_context.h> > > +static int num_standard_resources; > +static struct resource *standard_resources; > + > phys_addr_t __fdt_pointer __initdata; > > /* > @@ -206,14 +209,19 @@ static void __init request_standard_resources(void) > { > struct memblock_region *region; > struct resource *res; > + unsigned long i = 0; > > kernel_code.start = __pa_symbol(_text); > kernel_code.end = __pa_symbol(__init_begin - 1); > kernel_data.start = __pa_symbol(_sdata); > kernel_data.end = __pa_symbol(_end - 1); > > + num_standard_resources = memblock.memory.cnt; > + standard_resources = alloc_bootmem_low(num_standard_resources * > + sizeof(*standard_resources)); > + > for_each_memblock(memory, region) { > - res = alloc_bootmem_low(sizeof(*res)); > + res = &standard_resources[i++]; > if (memblock_is_nomap(region)) { > res->name = "reserved"; > res->flags = IORESOURCE_MEM; > @@ -244,8 +252,11 @@ static void __init request_standard_resources(void) > static int __init reserve_memblock_reserved_regions(void) > { > phys_addr_t start, end, roundup_end = 0; > - struct resource *mem, *res; > - u64 i; > + struct resource *mem; > + u64 i, mem_idx = 0; > + > + if (!standard_resources) > + return 0; > > for_each_reserved_mem_region(i, &start, &end) { > if (end <= roundup_end) > @@ -255,24 +266,25 @@ static int __init reserve_memblock_reserved_regions(void) > end = __pfn_to_phys(PFN_UP(end)) - 1; > roundup_end = end; > > - res = kzalloc(sizeof(*res), GFP_ATOMIC); > - if (WARN_ON(!res)) > - return -ENOMEM; > - res->start = start; > - res->end = end; > - res->name = "reserved"; > - res->flags = IORESOURCE_MEM; > + while (start > standard_resources[mem_idx].end) { > + mem_idx++; > + if (mem_idx >= num_standard_resources) > + return 0; /* no more 'System RAM' */ > + } > + do { > + mem = &standard_resources[mem_idx]; > > - mem = request_resource_conflict(&iomem_resource, res); > - /* > - * We expected memblock_reserve() regions to conflict with > - * memory created by request_standard_resources(). > - */ > - if (WARN_ON_ONCE(!mem)) > - continue; > - kfree(res); > + if (mem->start > end) > + continue; /* doesn't overlap with memory */ > + > + start = max(start, mem->start); > + reserve_region_with_split(mem, start, > + min(end, mem->end), > + "reserved"); > > - reserve_region_with_split(mem, start, end, "reserved"); > + if (mem->end < end) > + mem_idx++; > + } while (mem->end < end && mem_idx < num_standard_resources); > } > > return 0; >
On Thu, Oct 11, 2018 at 10:38:40AM +0200, Stefan Bader wrote:
> Given, this is not upstream I would make this a "UBUNTU: SAUCE: " patch...
Can you make the change before applying or i do i have to resend?
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index 5b4fac4..952c2b1 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -64,6 +64,9 @@ #include <asm/xen/hypervisor.h> #include <asm/mmu_context.h> +static int num_standard_resources; +static struct resource *standard_resources; + phys_addr_t __fdt_pointer __initdata; /* @@ -206,14 +209,19 @@ static void __init request_standard_resources(void) { struct memblock_region *region; struct resource *res; + unsigned long i = 0; kernel_code.start = __pa_symbol(_text); kernel_code.end = __pa_symbol(__init_begin - 1); kernel_data.start = __pa_symbol(_sdata); kernel_data.end = __pa_symbol(_end - 1); + num_standard_resources = memblock.memory.cnt; + standard_resources = alloc_bootmem_low(num_standard_resources * + sizeof(*standard_resources)); + for_each_memblock(memory, region) { - res = alloc_bootmem_low(sizeof(*res)); + res = &standard_resources[i++]; if (memblock_is_nomap(region)) { res->name = "reserved"; res->flags = IORESOURCE_MEM; @@ -244,8 +252,11 @@ static void __init request_standard_resources(void) static int __init reserve_memblock_reserved_regions(void) { phys_addr_t start, end, roundup_end = 0; - struct resource *mem, *res; - u64 i; + struct resource *mem; + u64 i, mem_idx = 0; + + if (!standard_resources) + return 0; for_each_reserved_mem_region(i, &start, &end) { if (end <= roundup_end) @@ -255,24 +266,25 @@ static int __init reserve_memblock_reserved_regions(void) end = __pfn_to_phys(PFN_UP(end)) - 1; roundup_end = end; - res = kzalloc(sizeof(*res), GFP_ATOMIC); - if (WARN_ON(!res)) - return -ENOMEM; - res->start = start; - res->end = end; - res->name = "reserved"; - res->flags = IORESOURCE_MEM; + while (start > standard_resources[mem_idx].end) { + mem_idx++; + if (mem_idx >= num_standard_resources) + return 0; /* no more 'System RAM' */ + } + do { + mem = &standard_resources[mem_idx]; - mem = request_resource_conflict(&iomem_resource, res); - /* - * We expected memblock_reserve() regions to conflict with - * memory created by request_standard_resources(). - */ - if (WARN_ON_ONCE(!mem)) - continue; - kfree(res); + if (mem->start > end) + continue; /* doesn't overlap with memory */ + + start = max(start, mem->start); + reserve_region_with_split(mem, start, + min(end, mem->end), + "reserved"); - reserve_region_with_split(mem, start, end, "reserved"); + if (mem->end < end) + mem_idx++; + } while (mem->end < end && mem_idx < num_standard_resources); } return 0;