arm64: Fix /proc/iomem for reserved but not memory regions

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
Related show

Commit Message

Paolo Pisati Oct. 10, 2018, 2:30 p.m.
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(-)

Comments

Colin Ian King Oct. 10, 2018, 3:35 p.m. | #1
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>
Stefan Bader Oct. 11, 2018, 8:38 a.m. | #2
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;
>
Paolo Pisati Oct. 12, 2018, 10:34 a.m. | #3
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?

Patch

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;