From: Andrew Jones <drjones@redhat.com>
To: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: kvm@vger.kernel.org, nikos.nikoleris@arm.com,
andre.przywara@arm.com, eric.auger@redhat.com
Subject: Re: [PATCH kvm-unit-tests v2 6/8] arm/arm64: setup: Consolidate memory layout assumptions
Date: Mon, 26 Apr 2021 11:18:55 +0200 [thread overview]
Message-ID: <20210426091855.hnl2nqx6a7myqxpo@gator> (raw)
In-Reply-To: <76d8685f-8028-873e-849b-1fbc0d33e95e@arm.com>
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@arm.com>
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> > 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
next prev parent reply other threads:[~2021-04-26 9:19 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-20 18:59 [PATCH kvm-unit-tests v2 0/8] arm/arm64: Prepare for target-efi Andrew Jones
2021-04-20 18:59 ` [PATCH kvm-unit-tests v2 1/8] arm/arm64: Reorganize cstart assembler Andrew Jones
2021-04-22 15:37 ` Alexandru Elisei
2021-04-20 18:59 ` [PATCH kvm-unit-tests v2 2/8] arm/arm64: Move setup_vm into setup Andrew Jones
2021-04-20 18:59 ` [PATCH kvm-unit-tests v2 3/8] pci-testdev: ioremap regions Andrew Jones
2021-04-26 15:03 ` Andre Przywara
2021-04-26 16:25 ` Andrew Jones
2021-04-20 18:59 ` [PATCH kvm-unit-tests v2 4/8] arm/arm64: mmu: Stop mapping an assumed IO region Andrew Jones
2021-04-23 16:10 ` Alexandru Elisei
2021-04-26 9:13 ` Andrew Jones
2021-04-20 18:59 ` [PATCH kvm-unit-tests v2 5/8] arm/arm64: mmu: Remove memory layout assumptions Andrew Jones
2021-04-23 16:31 ` Alexandru Elisei
2021-04-20 19:00 ` [PATCH kvm-unit-tests v2 6/8] arm/arm64: setup: Consolidate " Andrew Jones
2021-04-21 6:40 ` Andrew Jones
2021-04-22 16:12 ` Andrew Jones
2021-04-25 10:35 ` Alexandru Elisei
2021-04-25 10:35 ` Alexandru Elisei
2021-04-26 9:18 ` Andrew Jones [this message]
2021-04-20 19:00 ` [PATCH kvm-unit-tests v2 7/8] chr-testdev: Silently fail init Andrew Jones
2021-04-20 19:00 ` [PATCH kvm-unit-tests v2 8/8] arm/arm64: psci: don't assume method is hvc Andrew Jones
2021-04-21 7:02 ` Andrew Jones
2021-04-22 16:17 ` Andrew Jones
2021-04-26 14:57 ` Alexandru Elisei
2021-04-26 16:35 ` Andrew Jones
2021-04-27 15:47 ` Alexandru Elisei
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210426091855.hnl2nqx6a7myqxpo@gator \
--to=drjones@redhat.com \
--cc=alexandru.elisei@arm.com \
--cc=andre.przywara@arm.com \
--cc=eric.auger@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=nikos.nikoleris@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.