On Sun, Apr 25, 2021 at 11:35:29AM +0100, Alexandru Elisei wrote:
> Hi Drew,
>
> On 4/20/21 8:00 PM, Andrew Jones wrote:
> > Keep as much memory layout assumptions as possible in init::start
> > and a single setup function. This prepares us for calling setup()
> > from different start functions which have been linked with different
> > linker scripts. To do this, stacktop is only referenced from
> > init::start, making freemem_start a parameter to setup(). We also
> > split mem_init() into three parts, one that populates the mem regions
> > per the DT, one that populates the mem regions per assumptions,
> > and one that does the mem init. The concept of a primary region
> > is dropped, but we add a sanity check for the absence of memory
> > holes, because we don't know how to deal with them yet.
> >
> > Reviewed-by: Nikos Nikoleris <nikos.nikoleris@xxxxxxx>
> > Signed-off-by: Andrew Jones <drjones@xxxxxxxxxx>
> > ---
> > arm/cstart.S | 4 +-
> > arm/cstart64.S | 2 +
> > arm/flat.lds | 23 ++++++
> > lib/arm/asm/setup.h | 8 +-
> > lib/arm/mmu.c | 2 -
> > lib/arm/setup.c | 173 ++++++++++++++++++++++++--------------------
> > 6 files changed, 127 insertions(+), 85 deletions(-)
> >
> > diff --git a/arm/cstart.S b/arm/cstart.S
> > index bf3c78157e6a..446966de350d 100644
> > --- a/arm/cstart.S
> > +++ b/arm/cstart.S
> > @@ -80,7 +80,9 @@ start:
> >
> > /* complete setup */
> > pop {r0-r1}
> > - bl setup
> > + mov r3, #0
> > + ldr r2, =stacktop @ r2,r3 is the base of free memory
> > + bl setup @ r0 is the addr of the dtb
> >
> > /* run the test */
> > ldr r0, =__argc
> > diff --git a/arm/cstart64.S b/arm/cstart64.S
> > index 27251fe8b5cd..42ba3a3ca249 100644
> > --- a/arm/cstart64.S
> > +++ b/arm/cstart64.S
> > @@ -92,6 +92,8 @@ start:
> > bl exceptions_init
> >
> > /* complete setup */
> > + adrp x1, stacktop
> > + add x1, x1, :lo12:stacktop // x1 is the base of free memory
> > bl setup // x0 is the addr of the dtb
> >
> > /* run the test */
> > diff --git a/arm/flat.lds b/arm/flat.lds
> > index 6ed377c0eaa0..6fb459efb815 100644
> > --- a/arm/flat.lds
> > +++ b/arm/flat.lds
> > @@ -1,3 +1,26 @@
> > +/*
> > + * init::start will pass stacktop to setup() as the base of free memory.
> > + * setup() will then move the FDT and initrd to that base before calling
> > + * mem_init(). With those movements and this linker script, we'll end up
> > + * having the following memory layout:
> > + *
> > + * +----------------------+ <-- top of physical memory
> > + * | |
> > + * ~ ~
> > + * | |
> > + * +----------------------+ <-- top of initrd
> > + * | |
> > + * +----------------------+ <-- top of FDT
> > + * | |
> > + * +----------------------+ <-- top of cpu0's stack
> > + * | |
> > + * +----------------------+ <-- top of text/data/bss sections
> > + * | |
> > + * | |
> > + * +----------------------+ <-- load address
> > + * | |
> > + * +----------------------+ <-- physical address 0x0
> > + */
> >
> > SECTIONS
> > {
> > diff --git a/lib/arm/asm/setup.h b/lib/arm/asm/setup.h
> > index 210c14f818fb..f0e70b119fb0 100644
> > --- a/lib/arm/asm/setup.h
> > +++ b/lib/arm/asm/setup.h
> > @@ -13,9 +13,8 @@
> > extern u64 cpus[NR_CPUS]; /* per-cpu IDs (MPIDRs) */
> > extern int nr_cpus;
> >
> > -#define MR_F_PRIMARY (1U << 0)
> > -#define MR_F_IO (1U << 1)
> > -#define MR_F_CODE (1U << 2)
> > +#define MR_F_IO (1U << 0)
> > +#define MR_F_CODE (1U << 1)
> > #define MR_F_UNKNOWN (1U << 31)
> >
> > struct mem_region {
> > @@ -26,6 +25,7 @@ struct mem_region {
> > extern struct mem_region *mem_regions;
> > extern phys_addr_t __phys_offset, __phys_end;
> >
> > +extern struct mem_region *mem_region_find(phys_addr_t paddr);
> > extern unsigned int mem_region_get_flags(phys_addr_t paddr);
> >
> > #define PHYS_OFFSET (__phys_offset)
> > @@ -35,6 +35,6 @@ extern unsigned int mem_region_get_flags(phys_addr_t paddr);
> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
> > #define SMP_CACHE_BYTES L1_CACHE_BYTES
> >
> > -void setup(const void *fdt);
> > +void setup(const void *fdt, phys_addr_t freemem_start);
> >
> > #endif /* _ASMARM_SETUP_H_ */
> > diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> > index 4e3cf37e33d0..10df98e7a955 100644
> > --- a/lib/arm/mmu.c
> > +++ b/lib/arm/mmu.c
> > @@ -175,12 +175,10 @@ void *setup_mmu(phys_addr_t phys_end)
> > if (r->flags & MR_F_IO) {
> > continue;
> > } else if (r->flags & MR_F_CODE) {
> > - assert_msg(r->flags & MR_F_PRIMARY, "Unexpected code region");
> > /* armv8 requires code shared between EL1 and EL0 to be read-only */
> > mmu_set_range_ptes(mmu_idmap, r->start, r->start, r->end,
> > __pgprot(PTE_WBWA | PTE_USER | PTE_RDONLY));
> > } else {
> > - assert_msg(r->flags & MR_F_PRIMARY, "Unexpected data region");
> > mmu_set_range_ptes(mmu_idmap, r->start, r->start, r->end,
> > __pgprot(PTE_WBWA | PTE_USER));
> > }
> > diff --git a/lib/arm/setup.c b/lib/arm/setup.c
> > index 7db308b70744..a5ebec3c5a12 100644
> > --- a/lib/arm/setup.c
> > +++ b/lib/arm/setup.c
> > @@ -28,9 +28,10 @@
> >
> > #include "io.h"
> >
> > -#define NR_INITIAL_MEM_REGIONS 16
> > +#define MAX_DT_MEM_REGIONS 16
> > +#define NR_EXTRA_MEM_REGIONS 16
> > +#define NR_INITIAL_MEM_REGIONS (MAX_DT_MEM_REGIONS + NR_EXTRA_MEM_REGIONS)
> >
> > -extern unsigned long stacktop;
> > extern unsigned long etext;
> >
> > struct timer_state __timer_state;
> > @@ -75,28 +76,68 @@ static void cpu_init(void)
> > set_cpu_online(0, true);
> > }
> >
> > -unsigned int mem_region_get_flags(phys_addr_t paddr)
> > +static void mem_region_add(struct mem_region *r)
> > +{
> > + struct mem_region *r_next = mem_regions;
> > + int i = 0;
> > +
> > + for (; r_next->end; ++r_next, ++i)
> > + ;
> > + assert(i != NR_INITIAL_MEM_REGIONS);
>
> Shouldn't that be i < NR_INITIAL_MEM_REGIONS?
Indeed. Will fix.
> I think it conveys intention better,
> and also helps catch situations where we've set in other parts of the code the
> entire mem_regions array, from index 0 to index NR_INITIAL_MEM_REGIONS (at least),
> which is an error.
>
> > +
> > + *r_next = *r;
> > +}
> > +
> > +static void mem_regions_get_dt_regions(void)
> > +{
> > + struct dt_pbus_reg regs[MAX_DT_MEM_REGIONS];
> > + int nr_regs, i;
> > +
> > + nr_regs = dt_get_memory_params(regs, MAX_DT_MEM_REGIONS);
> > + assert(nr_regs > 0);
> > +
> > + for (i = 0; i < nr_regs; ++i) {
> > + mem_region_add(&(struct mem_region){
> > + .start = regs[i].addr,
> > + .end = regs[i].addr + regs[i].size,
> > + });
> > + }
> > +}
> > +
> > +struct mem_region *mem_region_find(phys_addr_t paddr)
> > {
> > struct mem_region *r;
> >
> > - for (r = mem_regions; r->end; ++r) {
> > + for (r = mem_regions; r->end; ++r)
> > if (paddr >= r->start && paddr < r->end)
> > - return r->flags;
> > - }
> > + return r;
> > + return NULL;
> > +}
> >
> > - return MR_F_UNKNOWN;
> > +unsigned int mem_region_get_flags(phys_addr_t paddr)
> > +{
> > + struct mem_region *r = mem_region_find(paddr);
> > + return r ? r->flags : MR_F_UNKNOWN;
> > }
> >
> > -static void mem_init(phys_addr_t freemem_start)
> > +static void mem_regions_add_assumed(void)
> > {
> > phys_addr_t code_end = (phys_addr_t)(unsigned long)&etext;
> > - struct dt_pbus_reg regs[NR_INITIAL_MEM_REGIONS];
> > - struct mem_region mem = {
> > - .start = (phys_addr_t)-1,
> > + struct mem_region *r;
> > +
> > + r = mem_region_find(code_end - 1);
> > + assert(r);
> > +
> > + /* Split the region with the code into two regions; code and data */
> > + mem_region_add(&(struct mem_region){
> > + .start = code_end,
> > + .end = r->end,
> > + });
> > + *r = (struct mem_region){
> > + .start = r->start,
> > + .end = code_end,
> > + .flags = MR_F_CODE,
> > };
> > - struct mem_region *primary = NULL;
> > - phys_addr_t base, top;
> > - int nr_regs, nr_io = 0, i;
> >
> > /*
> > * mach-virt I/O regions:
> > @@ -104,57 +145,47 @@ static void mem_init(phys_addr_t freemem_start)
> > * - 512M at 256G (arm64, arm uses highmem=off)
> > * - 512G at 512G (arm64, arm uses highmem=off)
> > */
> > - mem_regions[nr_io++] = (struct mem_region){ 0, (1ul << 30), MR_F_IO };
> > + mem_region_add(&(struct mem_region){ 0, (1ul << 30), MR_F_IO });
> > #ifdef __aarch64__
> > - mem_regions[nr_io++] = (struct mem_region){ (1ul << 38), (1ul << 38) | (1ul << 29), MR_F_IO };
> > - mem_regions[nr_io++] = (struct mem_region){ (1ul << 39), (1ul << 40), MR_F_IO };
> > + mem_region_add(&(struct mem_region){ (1ul << 38), (1ul << 38) | (1ul << 29), MR_F_IO });
> > + mem_region_add(&(struct mem_region){ (1ul << 39), (1ul << 40), MR_F_IO });
> > #endif
> > +}
> >
> > - nr_regs = dt_get_memory_params(regs, NR_INITIAL_MEM_REGIONS - nr_io);
> > - assert(nr_regs > 0);
> > -
> > - for (i = 0; i < nr_regs; ++i) {
> > - struct mem_region *r = &mem_regions[nr_io + i];
> > +static void mem_init(phys_addr_t freemem_start)
> > +{
> > + phys_addr_t base, top;
> > + struct mem_region *freemem, *r, mem = {
> > + .start = (phys_addr_t)-1,
> > + };
> >
> > - r->start = regs[i].addr;
> > - r->end = regs[i].addr + regs[i].size;
> > + freemem = mem_region_find(freemem_start);
> > + assert(freemem && !(freemem->flags & (MR_F_IO | MR_F_CODE)));
> >
> > - /*
> > - * pick the region we're in for our primary region
> > - */
> > - if (freemem_start >= r->start && freemem_start < r->end) {
> > - r->flags |= MR_F_PRIMARY;
> > - primary = r;
> > + for (r = mem_regions; r->end; ++r) {
> > + if (!(r->flags & MR_F_IO)) {
> > + if (r->start < mem.start)
> > + mem.start = r->start;
> > + if (r->end > mem.end)
> > + mem.end = r->end;
> > }
> > -
> > - /*
> > - * set the lowest and highest addresses found,
> > - * ignoring potential gaps
> > - */
> > - if (r->start < mem.start)
> > - mem.start = r->start;
> > - if (r->end > mem.end)
> > - mem.end = r->end;
> > }
> > - assert(primary);
> > + assert(mem.end);
> > assert(!(mem.start & ~PHYS_MASK) && !((mem.end - 1) & ~PHYS_MASK));
> >
> > - __phys_offset = primary->start; /* PHYS_OFFSET */
> > - __phys_end = primary->end; /* PHYS_END */
> > + /* Check for holes */
> > + r = mem_region_find(mem.start);
> > + while (r && r->end != mem.end)
> > + r = mem_region_find(r->end);
> > + assert(r);
>
> It took me a while to figure this out, what it does is find all memory regions
> adjacent to each other and starting at mem.start and ending precisely at mem.end.
> Looks good.
>
> >
> > - /* Split the primary region into two regions; code and data */
> > - mem_regions[nr_io + i] = (struct mem_region){
> > - .start = code_end,
> > - .end = primary->end,
> > - .flags = MR_F_PRIMARY,
> > - };
> > - *primary = (struct mem_region){
> > - .start = primary->start,
> > - .end = code_end,
> > - .flags = MR_F_PRIMARY | MR_F_CODE,
> > - };
> > + /* Ensure our selected freemem region is somewhere in our full range */
> > + assert(freemem_start >= mem.start && freemem->end <= mem.end);
>
> That looks a bit strange, the comment refers to checking the freemem region, but
> we're actually checking is the subregion from freemem_start to freemem->end. I
> suppose it's because freemem_start comes after data, stack, fdt and initrd in the
> freemem region, and what we consider available memory is [freemem_start,
> freemem->end), right?
Right. I'll change the comment to say 'freemem range' rather than region.
Thanks,
drew
