ACK/Cmnt: [PATCH] arm64: Fix /proc/iomem for reserved but not memory regions

[Stefan Bader]

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 at 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 at linaro.org>
> CC: Akashi Takahiro <takahiro.akashi at linaro.org>
> CC: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> Signed-off-by: James Morse <james.morse at arm.com>
> Tested-by: John Stultz <john.stultz at linaro.org>

(cherry picked from https://www.spinics.net/lists/arm-kernel/msg675580.html)
> Signed-off-by: Paolo Pisati <paolo.pisati at canonical.comAcked-by: Stefan Bader <stefan.bader at 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;
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20181011/c0e8de06/attachment.sig>