Messages in this thread | | | Date | Thu, 22 Apr 2021 20:30:46 -0700 (PDT) | Subject | Re: [PATCH v3 3/5] RISC-V: Improve init_resources | From | Palmer Dabbelt <> |
| |
On Mon, 05 Apr 2021 01:57:10 PDT (-0700), mick@ics.forth.gr wrote: > * Kernel region is always present and we know where it is, no > need to look for it inside the loop, just ignore it like the > rest of the reserved regions within system's memory. > > * Don't call memblock_free inside the loop, if called it'll split > the region of pre-allocated resources in two parts, messing things > up, just re-use the previous pre-allocated resource and free any > unused resources after both loops finish. > > * memblock_alloc may add a region when called, so increase the > number of pre-allocated regions by one to be on the safe side > (reported and patched by Geert Uytterhoeven)
IIUC this one has already been fixen on for-next. Either way, it caused a merge conflict. I think I've fixed it up, LMK if something went wrong.
Also: I cleaned up the commit text a bit, as this is an odd way to do it. It's probably best to just have split this into two commits.
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Nick Kossifidis <mick@ics.forth.gr> > --- > arch/riscv/kernel/setup.c | 90 ++++++++++++++++++++------------------- > 1 file changed, 46 insertions(+), 44 deletions(-) > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index e85bacff1..030554bab 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -60,6 +60,7 @@ static DEFINE_PER_CPU(struct cpu, cpu_devices); > * also add "System RAM" regions for compatibility with other > * archs, and the rest of the known regions for completeness. > */ > +static struct resource kimage_res = { .name = "Kernel image", }; > static struct resource code_res = { .name = "Kernel code", }; > static struct resource data_res = { .name = "Kernel data", }; > static struct resource rodata_res = { .name = "Kernel rodata", }; > @@ -80,45 +81,54 @@ static int __init add_resource(struct resource *parent, > return 1; > } > > -static int __init add_kernel_resources(struct resource *res) > +static int __init add_kernel_resources(void) > { > int ret = 0; > > /* > * The memory region of the kernel image is continuous and > - * was reserved on setup_bootmem, find it here and register > - * it as a resource, then register the various segments of > - * the image as child nodes > + * was reserved on setup_bootmem, register it here as a > + * resource, with the various segments of the image as > + * child nodes. > */ > - if (!(res->start <= code_res.start && res->end >= data_res.end)) > - return 0; > > - res->name = "Kernel image"; > - res->flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY; > + code_res.start = __pa_symbol(_text); > + code_res.end = __pa_symbol(_etext) - 1; > + code_res.flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY; > > - /* > - * We removed a part of this region on setup_bootmem so > - * we need to expand the resource for the bss to fit in. > - */ > - res->end = bss_res.end; > + rodata_res.start = __pa_symbol(__start_rodata); > + rodata_res.end = __pa_symbol(__end_rodata) - 1; > + rodata_res.flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY; > > - ret = add_resource(&iomem_resource, res); > + data_res.start = __pa_symbol(_data); > + data_res.end = __pa_symbol(_edata) - 1; > + data_res.flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY; > + > + bss_res.start = __pa_symbol(__bss_start); > + bss_res.end = __pa_symbol(__bss_stop) - 1; > + bss_res.flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY; > + > + kimage_res.start = code_res.start; > + kimage_res.end = bss_res.end; > + kimage_res.flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY; > + > + ret = add_resource(&iomem_resource, &kimage_res); > if (ret < 0) > return ret; > > - ret = add_resource(res, &code_res); > + ret = add_resource(&kimage_res, &code_res); > if (ret < 0) > return ret; > > - ret = add_resource(res, &rodata_res); > + ret = add_resource(&kimage_res, &rodata_res); > if (ret < 0) > return ret; > > - ret = add_resource(res, &data_res); > + ret = add_resource(&kimage_res, &data_res); > if (ret < 0) > return ret; > > - ret = add_resource(res, &bss_res); > + ret = add_resource(&kimage_res, &bss_res); > > return ret; > } > @@ -129,53 +139,42 @@ static void __init init_resources(void) > struct resource *res = NULL; > struct resource *mem_res = NULL; > size_t mem_res_sz = 0; > - int ret = 0, i = 0; > - > - code_res.start = __pa_symbol(_text); > - code_res.end = __pa_symbol(_etext) - 1; > - code_res.flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY; > - > - rodata_res.start = __pa_symbol(__start_rodata); > - rodata_res.end = __pa_symbol(__end_rodata) - 1; > - rodata_res.flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY; > - > - data_res.start = __pa_symbol(_data); > - data_res.end = __pa_symbol(_edata) - 1; > - data_res.flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY; > + int num_resources = 0, res_idx = 0; > + int ret = 0; > > - bss_res.start = __pa_symbol(__bss_start); > - bss_res.end = __pa_symbol(__bss_stop) - 1; > - bss_res.flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY; > + /* + 1 as memblock_alloc() might increase memblock.reserved.cnt */ > + num_resources = memblock.memory.cnt + memblock.reserved.cnt + 1; > + res_idx = num_resources - 1; > > - mem_res_sz = (memblock.memory.cnt + memblock.reserved.cnt) * sizeof(*mem_res); > + mem_res_sz = num_resources * sizeof(*mem_res); > mem_res = memblock_alloc(mem_res_sz, SMP_CACHE_BYTES); > if (!mem_res) > panic("%s: Failed to allocate %zu bytes\n", __func__, mem_res_sz); > + > /* > * Start by adding the reserved regions, if they overlap > * with /memory regions, insert_resource later on will take > * care of it. > */ > + ret = add_kernel_resources(); > + if (ret < 0) > + goto error; > + > for_each_reserved_mem_region(region) { > - res = &mem_res[i++]; > + res = &mem_res[res_idx--]; > > res->name = "Reserved"; > res->flags = IORESOURCE_MEM | IORESOURCE_BUSY; > res->start = __pfn_to_phys(memblock_region_reserved_base_pfn(region)); > res->end = __pfn_to_phys(memblock_region_reserved_end_pfn(region)) - 1; > > - ret = add_kernel_resources(res); > - if (ret < 0) > - goto error; > - else if (ret) > - continue; > - > /* > * Ignore any other reserved regions within > * system memory. > */ > if (memblock_is_memory(res->start)) { > - memblock_free((phys_addr_t) res, sizeof(struct resource)); > + /* Re-use this pre-allocated resource */ > + res_idx++; > continue; > } > > @@ -186,7 +185,7 @@ static void __init init_resources(void) > > /* Add /memory regions to the resource tree */ > for_each_mem_region(region) { > - res = &mem_res[i++]; > + res = &mem_res[res_idx--]; > > if (unlikely(memblock_is_nomap(region))) { > res->name = "Reserved"; > @@ -204,6 +203,9 @@ static void __init init_resources(void) > goto error; > } > > + /* Clean-up any unused pre-allocated resources */ > + mem_res_sz = (num_resources - res_idx + 1) * sizeof(*mem_res); > + memblock_free((phys_addr_t) mem_res, mem_res_sz); > return; > > error:
| |