| Message ID | 20220907083643.20152-4-Henry.Wang@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce static heap | expand |
Hi Henry, On 07/09/2022 09:36, Henry Wang wrote: > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index 7f5b317d3e..4a70ed2986 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -132,12 +132,12 @@ uint64_t init_ttbr; > static paddr_t phys_offset; > > /* Limits of the Xen heap */ > -mfn_t xenheap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER; > -mfn_t xenheap_mfn_end __read_mostly; > -vaddr_t xenheap_virt_end __read_mostly; > +mfn_t directmap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER; > +mfn_t directmap_mfn_end __read_mostly; > +vaddr_t directmap_virt_end __read_mostly; > #ifdef CONFIG_ARM_64 > -vaddr_t xenheap_virt_start __read_mostly; > -unsigned long xenheap_base_pdx __read_mostly; > +vaddr_t directmap_virt_start __read_mostly; > +unsigned long directmap_base_pdx __read_mostly; > #endif > > unsigned long frametable_base_pdx __read_mostly; > @@ -609,7 +609,7 @@ void __init setup_xenheap_mappings(unsigned long base_mfn, I think the function also want to be renamed to match the code below. > panic("Unable to setup the xenheap mappings.\n"); Likely, I think this wants to be s/xenheap/directmap/ > > /* Record where the xenheap is, for translation routines. */ Same here because you set directmap_virt_end. > - xenheap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE; > + directmap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE; I would be OK to keep "XENHEAP_*" for now. > } > #else /* CONFIG_ARM_64 */ > void __init setup_xenheap_mappings(unsigned long base_mfn, > @@ -618,12 +618,12 @@ void __init setup_xenheap_mappings(unsigned long base_mfn, > int rc; > > /* First call sets the xenheap physical and virtual offset. */ s/xenheap/directmap/ I haven't looked if there are other instances in the function. Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Subject: Re: [PATCH v3 3/4] xen/arm: mm: Rename xenheap_* variable to > directmap_* > > Hi Henry, > > On 07/09/2022 09:36, Henry Wang wrote: > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > > index 7f5b317d3e..4a70ed2986 100644 > > --- a/xen/arch/arm/mm.c > > +++ b/xen/arch/arm/mm.c > > @@ -132,12 +132,12 @@ uint64_t init_ttbr; > > static paddr_t phys_offset; > > > > /* Limits of the Xen heap */ > > -mfn_t xenheap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER; > > -mfn_t xenheap_mfn_end __read_mostly; > > -vaddr_t xenheap_virt_end __read_mostly; > > +mfn_t directmap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER; > > +mfn_t directmap_mfn_end __read_mostly; > > +vaddr_t directmap_virt_end __read_mostly; > > #ifdef CONFIG_ARM_64 > > -vaddr_t xenheap_virt_start __read_mostly; > > -unsigned long xenheap_base_pdx __read_mostly; > > +vaddr_t directmap_virt_start __read_mostly; > > +unsigned long directmap_base_pdx __read_mostly; > > #endif > > > > unsigned long frametable_base_pdx __read_mostly; > > @@ -609,7 +609,7 @@ void __init setup_xenheap_mappings(unsigned > long base_mfn, > > I think the function also want to be renamed to match the code below. Hmmm, renaming the name to "setup_directmap_mappings" would somehow lead me to think of we are getting rid of the name "xenheap" completely in the code, which seems a little bit scary to me... But I just checked there is a comment "/* Set up the xenheap: up to 1GB of contiguous, always-mapped memory." above the function and the declaration so I guess we are fine? > > > panic("Unable to setup the xenheap mappings.\n"); > > Likely, I think this wants to be s/xenheap/directmap/ Ok. > > > > > /* Record where the xenheap is, for translation routines. */ > > Same here because you set directmap_virt_end. Ok. > > > - xenheap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE; > > + directmap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE; > > I would be OK to keep "XENHEAP_*" for now. Thanks for your confirmation. > > > } > > #else /* CONFIG_ARM_64 */ > > void __init setup_xenheap_mappings(unsigned long base_mfn, > > @@ -618,12 +618,12 @@ void __init setup_xenheap_mappings(unsigned > long base_mfn, > > int rc; > > > > /* First call sets the xenheap physical and virtual offset. */ > > s/xenheap/directmap/ I haven't looked if there are other instances in > the function. Don't bother, I will take care of the rest. Kind regards, Henry > > Cheers, > > -- > Julien Grall
On 07/09/2022 11:53, Henry Wang wrote: >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Subject: Re: [PATCH v3 3/4] xen/arm: mm: Rename xenheap_* variable to >> directmap_* >> >> Hi Henry, >> >> On 07/09/2022 09:36, Henry Wang wrote: >>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >>> index 7f5b317d3e..4a70ed2986 100644 >>> --- a/xen/arch/arm/mm.c >>> +++ b/xen/arch/arm/mm.c >>> @@ -132,12 +132,12 @@ uint64_t init_ttbr; >>> static paddr_t phys_offset; >>> >>> /* Limits of the Xen heap */ >>> -mfn_t xenheap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER; >>> -mfn_t xenheap_mfn_end __read_mostly; >>> -vaddr_t xenheap_virt_end __read_mostly; >>> +mfn_t directmap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER; >>> +mfn_t directmap_mfn_end __read_mostly; >>> +vaddr_t directmap_virt_end __read_mostly; >>> #ifdef CONFIG_ARM_64 >>> -vaddr_t xenheap_virt_start __read_mostly; >>> -unsigned long xenheap_base_pdx __read_mostly; >>> +vaddr_t directmap_virt_start __read_mostly; >>> +unsigned long directmap_base_pdx __read_mostly; >>> #endif >>> >>> unsigned long frametable_base_pdx __read_mostly; >>> @@ -609,7 +609,7 @@ void __init setup_xenheap_mappings(unsigned >> long base_mfn, >> >> I think the function also want to be renamed to match the code below. > > Hmmm, renaming the name to "setup_directmap_mappings" would > somehow lead me to think of we are getting rid of the name "xenheap" > completely in the code, which seems a little bit scary to me... > > But I just checked there is a comment > "/* Set up the xenheap: up to 1GB of contiguous, always-mapped memory." > above the function and the declaration so I guess we are fine? We are not getting rid of "xenheap". In fact the common code will continue to use the concept. What we make clear is this function is not only here to map the xenheap but other memory (e.g. static domain memory on arm64). Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > >>> @@ -609,7 +609,7 @@ void __init setup_xenheap_mappings(unsigned > >> long base_mfn, > >> > >> I think the function also want to be renamed to match the code below. > > > > Hmmm, renaming the name to "setup_directmap_mappings" would > > somehow lead me to think of we are getting rid of the name "xenheap" > > completely in the code, which seems a little bit scary to me... > > > > But I just checked there is a comment > > "/* Set up the xenheap: up to 1GB of contiguous, always-mapped > memory." > > above the function and the declaration so I guess we are fine? > > We are not getting rid of "xenheap". In fact the common code will > continue to use the concept. Ack. > > What we make clear is this function is not only here to map the xenheap > but other memory (e.g. static domain memory on arm64). In that case I think the comment in function declaration (attached below) ``` /* Set up the xenheap: up to 1GB of contiguous, always-mapped memory. * Base must be 32MB aligned and size a multiple of 32MB. */ extern void setup_xenheap_mappings(unsigned long base_mfn, unsigned long nr_mfns); ``` would also need changes, as I think it only refers to the Arm32. How about /* * For Arm32, set up the xenheap: up to 1GB of contiguous, * always-mapped memory. Base must be 32MB aligned and size * a multiple of 32MB. * For Arm64, set up the directmap area of memory. */ Kind regards, Henry > > Cheers, > > -- > Julien Grall
On 07/09/2022 12:09, Henry Wang wrote: > Hi Julien, Hi Henry, >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >>>>> @@ -609,7 +609,7 @@ void __init setup_xenheap_mappings(unsigned >>>> long base_mfn, >>>> >>>> I think the function also want to be renamed to match the code below. >>> >>> Hmmm, renaming the name to "setup_directmap_mappings" would >>> somehow lead me to think of we are getting rid of the name "xenheap" >>> completely in the code, which seems a little bit scary to me... >>> >>> But I just checked there is a comment >>> "/* Set up the xenheap: up to 1GB of contiguous, always-mapped >> memory." >>> above the function and the declaration so I guess we are fine? >> >> We are not getting rid of "xenheap". In fact the common code will >> continue to use the concept. > > Ack. > >> >> What we make clear is this function is not only here to map the xenheap >> but other memory (e.g. static domain memory on arm64). > > In that case I think the comment in function declaration (attached below) > ``` > /* Set up the xenheap: up to 1GB of contiguous, always-mapped memory. > * Base must be 32MB aligned and size a multiple of 32MB. */ > extern void setup_xenheap_mappings(unsigned long base_mfn, unsigned long nr_mfns); > ``` > would also need changes, as I think it only refers to the Arm32. > > How about > /* > * For Arm32, set up the xenheap: up to 1GB of contiguous, > * always-mapped memory. Base must be 32MB aligned and size > * a multiple of 32MB. > * For Arm64, set up the directmap area of memory. One remark. I would say: "map the region in the directmap area" Cheers,
Hi Julien, > -----Original Message----- > > How about > > /* > > * For Arm32, set up the xenheap: up to 1GB of contiguous, > > * always-mapped memory. Base must be 32MB aligned and size > > * a multiple of 32MB. > > * For Arm64, set up the directmap area of memory. > > One remark. I would say: "map the region in the directmap area" No problem :)) Thanks! Kind regards, Henry > > Cheers, > > -- > Julien Grall
diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h index 2fafb9f228..0fefed1b8a 100644 --- a/xen/arch/arm/include/asm/config.h +++ b/xen/arch/arm/include/asm/config.h @@ -160,7 +160,7 @@ #define DIRECTMAP_SIZE (SLOT0_ENTRY_SIZE * (265-256)) #define DIRECTMAP_VIRT_END (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE - 1) -#define XENHEAP_VIRT_START xenheap_virt_start +#define XENHEAP_VIRT_START directmap_virt_start #define HYPERVISOR_VIRT_END DIRECTMAP_VIRT_END diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h index 749fbefa0c..7b4f6ce233 100644 --- a/xen/arch/arm/include/asm/mm.h +++ b/xen/arch/arm/include/asm/mm.h @@ -154,19 +154,19 @@ struct page_info #define _PGC_need_scrub _PGC_allocated #define PGC_need_scrub PGC_allocated -extern mfn_t xenheap_mfn_start, xenheap_mfn_end; -extern vaddr_t xenheap_virt_end; +extern mfn_t directmap_mfn_start, directmap_mfn_end; +extern vaddr_t directmap_virt_end; #ifdef CONFIG_ARM_64 -extern vaddr_t xenheap_virt_start; -extern unsigned long xenheap_base_pdx; +extern vaddr_t directmap_virt_start; +extern unsigned long directmap_base_pdx; #endif #ifdef CONFIG_ARM_32 #define is_xen_heap_page(page) is_xen_heap_mfn(page_to_mfn(page)) #define is_xen_heap_mfn(mfn) ({ \ unsigned long mfn_ = mfn_x(mfn); \ - (mfn_ >= mfn_x(xenheap_mfn_start) && \ - mfn_ < mfn_x(xenheap_mfn_end)); \ + (mfn_ >= mfn_x(directmap_mfn_start) && \ + mfn_ < mfn_x(directmap_mfn_end)); \ }) #else #define is_xen_heap_page(page) ((page)->count_info & PGC_xen_heap) @@ -267,16 +267,16 @@ static inline paddr_t __virt_to_maddr(vaddr_t va) static inline void *maddr_to_virt(paddr_t ma) { ASSERT(is_xen_heap_mfn(maddr_to_mfn(ma))); - ma -= mfn_to_maddr(xenheap_mfn_start); + ma -= mfn_to_maddr(directmap_mfn_start); return (void *)(unsigned long) ma + XENHEAP_VIRT_START; } #else static inline void *maddr_to_virt(paddr_t ma) { - ASSERT((mfn_to_pdx(maddr_to_mfn(ma)) - xenheap_base_pdx) < + ASSERT((mfn_to_pdx(maddr_to_mfn(ma)) - directmap_base_pdx) < (DIRECTMAP_SIZE >> PAGE_SHIFT)); return (void *)(XENHEAP_VIRT_START - - (xenheap_base_pdx << PAGE_SHIFT) + + (directmap_base_pdx << PAGE_SHIFT) + ((ma & ma_va_bottom_mask) | ((ma & ma_top_mask) >> pfn_pdx_hole_shift))); } @@ -319,10 +319,10 @@ static inline struct page_info *virt_to_page(const void *v) unsigned long pdx; ASSERT(va >= XENHEAP_VIRT_START); - ASSERT(va < xenheap_virt_end); + ASSERT(va < directmap_virt_end); pdx = (va - XENHEAP_VIRT_START) >> PAGE_SHIFT; - pdx += mfn_to_pdx(xenheap_mfn_start); + pdx += mfn_to_pdx(directmap_mfn_start); return frame_table + pdx - frametable_base_pdx; } diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 7f5b317d3e..4a70ed2986 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -132,12 +132,12 @@ uint64_t init_ttbr; static paddr_t phys_offset; /* Limits of the Xen heap */ -mfn_t xenheap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER; -mfn_t xenheap_mfn_end __read_mostly; -vaddr_t xenheap_virt_end __read_mostly; +mfn_t directmap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER; +mfn_t directmap_mfn_end __read_mostly; +vaddr_t directmap_virt_end __read_mostly; #ifdef CONFIG_ARM_64 -vaddr_t xenheap_virt_start __read_mostly; -unsigned long xenheap_base_pdx __read_mostly; +vaddr_t directmap_virt_start __read_mostly; +unsigned long directmap_base_pdx __read_mostly; #endif unsigned long frametable_base_pdx __read_mostly; @@ -609,7 +609,7 @@ void __init setup_xenheap_mappings(unsigned long base_mfn, panic("Unable to setup the xenheap mappings.\n"); /* Record where the xenheap is, for translation routines. */ - xenheap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE; + directmap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE; } #else /* CONFIG_ARM_64 */ void __init setup_xenheap_mappings(unsigned long base_mfn, @@ -618,12 +618,12 @@ void __init setup_xenheap_mappings(unsigned long base_mfn, int rc; /* First call sets the xenheap physical and virtual offset. */ - if ( mfn_eq(xenheap_mfn_start, INVALID_MFN) ) + if ( mfn_eq(directmap_mfn_start, INVALID_MFN) ) { unsigned long mfn_gb = base_mfn & ~((FIRST_SIZE >> PAGE_SHIFT) - 1); - xenheap_mfn_start = _mfn(base_mfn); - xenheap_base_pdx = mfn_to_pdx(_mfn(base_mfn)); + directmap_mfn_start = _mfn(base_mfn); + directmap_base_pdx = mfn_to_pdx(_mfn(base_mfn)); /* * The base address may not be aligned to the first level * size (e.g. 1GB when using 4KB pages). This would prevent @@ -633,13 +633,13 @@ void __init setup_xenheap_mappings(unsigned long base_mfn, * Prevent that by offsetting the start of the xenheap virtual * address. */ - xenheap_virt_start = DIRECTMAP_VIRT_START + + directmap_virt_start = DIRECTMAP_VIRT_START + (base_mfn - mfn_gb) * PAGE_SIZE; } - if ( base_mfn < mfn_x(xenheap_mfn_start) ) + if ( base_mfn < mfn_x(directmap_mfn_start) ) panic("cannot add xenheap mapping at %lx below heap start %lx\n", - base_mfn, mfn_x(xenheap_mfn_start)); + base_mfn, mfn_x(directmap_mfn_start)); rc = map_pages_to_xen((vaddr_t)__mfn_to_virt(base_mfn), _mfn(base_mfn), nr_mfns, diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 3c36c050bf..4a8334c268 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -697,11 +697,11 @@ static void __init populate_boot_allocator(void) #ifdef CONFIG_ARM_32 /* Avoid the xenheap */ - if ( s < mfn_to_maddr(xenheap_mfn_end) && - mfn_to_maddr(xenheap_mfn_start) < e ) + if ( s < mfn_to_maddr(directmap_mfn_end) && + mfn_to_maddr(directmap_mfn_start) < e ) { - e = mfn_to_maddr(xenheap_mfn_start); - n = mfn_to_maddr(xenheap_mfn_end); + e = mfn_to_maddr(directmap_mfn_start); + n = mfn_to_maddr(directmap_mfn_end); } #endif @@ -793,15 +793,16 @@ static void __init setup_mm(void) * We need some memory to allocate the page-tables used for the * xenheap mappings. So populate the boot allocator first. * - * This requires us to set xenheap_mfn_{start, end} first so the Xenheap + * Note that currently xenheap is direct mapped on Arm32. + * This requires us to set directmap_mfn_{start, end} first so the Xenheap * region can be avoided. */ - xenheap_mfn_start = _mfn((e >> PAGE_SHIFT) - xenheap_pages); - xenheap_mfn_end = mfn_add(xenheap_mfn_start, xenheap_pages); + directmap_mfn_start = _mfn((e >> PAGE_SHIFT) - xenheap_pages); + directmap_mfn_end = mfn_add(directmap_mfn_start, xenheap_pages); populate_boot_allocator(); - setup_xenheap_mappings(mfn_x(xenheap_mfn_start), xenheap_pages); + setup_xenheap_mappings(mfn_x(directmap_mfn_start), xenheap_pages); /* Frame table covers all of RAM region, including holes */ setup_frametable_mappings(ram_start, ram_end); @@ -816,8 +817,8 @@ static void __init setup_mm(void) smp_processor_id()); /* Add xenheap memory that was not already added to the boot allocator. */ - init_xenheap_pages(mfn_to_maddr(xenheap_mfn_start), - mfn_to_maddr(xenheap_mfn_end)); + init_xenheap_pages(mfn_to_maddr(directmap_mfn_start), + mfn_to_maddr(directmap_mfn_end)); init_staticmem_pages(); } @@ -858,9 +859,9 @@ static void __init setup_mm(void) total_pages += ram_size >> PAGE_SHIFT; - xenheap_virt_end = XENHEAP_VIRT_START + ram_end - ram_start; - xenheap_mfn_start = maddr_to_mfn(ram_start); - xenheap_mfn_end = maddr_to_mfn(ram_end); + directmap_virt_end = XENHEAP_VIRT_START + ram_end - ram_start; + directmap_mfn_start = maddr_to_mfn(ram_start); + directmap_mfn_end = maddr_to_mfn(ram_end); setup_frametable_mappings(ram_start, ram_end); max_page = PFN_DOWN(ram_end);
With the static heap setup, keep using xenheap_* in the function setup_xenheap_mappings() will make the code confusing to read, because we always need to map the full RAM on Arm64. Therefore, renaming all "xenheap_*" variables to "directmap_*" to make clear the area is used to access the RAM easily. On Arm32, only the xenheap is direct mapped today. So the renaming to "directmap_*" would be still valid for Arm32. No functional change is intended. Signed-off-by: Henry Wang <Henry.Wang@arm.com> --- Changes from v2 to v3: - Adjust the order of this patch, make it #3. Changes from v1 to v2: - New commit. --- xen/arch/arm/include/asm/config.h | 2 +- xen/arch/arm/include/asm/mm.h | 22 +++++++++++----------- xen/arch/arm/mm.c | 24 ++++++++++++------------ xen/arch/arm/setup.c | 27 ++++++++++++++------------- 4 files changed, 38 insertions(+), 37 deletions(-)