Hi Julien
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: Friday, August 13, 2021 8:21 PM
> To: Penny Zheng <penny.zh...@arm.com>; xen-devel@lists.xenproject.org;
> sstabell...@kernel.org
> Cc: Bertrand Marquis <bertrand.marq...@arm.com>; Wei Chen
> <wei.c...@arm.com>; nd <n...@arm.com>
> Subject: Re: [PATCH V4 05/10] xen/arm: static memory initialization
>
> Hi Penny,
>
> On 28/07/2021 11:27, Penny Zheng wrote:
> > This patch introduces static memory initialization, during system boot up.
> >
> > The new function init_staticmem_pages is responsible for static memory
> > initialization.
> >
> > Helper free_staticmem_pages is the equivalent of free_heap_pages, to
> > free nr_mfns pages of static memory.
> >
> > This commit also introduces new CONFIG_STATIC_MEMORY to avoid
> bringing
> > dead codes in other archs.
> >
> > Put asynchronous scrubbing for pages of static memory in TODO list.
> >
> > Signed-off-by: Penny Zheng <penny.zh...@arm.com>
> > ---
> > v4 change:
> > - move the option CONFIG_STATIC_MEMORY to common code, and with
> Arm
> > "select"ing it
> > - replace round_pg{down,up}() with PFN_DOWN()/PFN_UP()
> > ---
> > xen/arch/arm/Kconfig | 1 +
> > xen/arch/arm/setup.c | 24 ++++++++++++++++++++++++
> > xen/common/Kconfig | 3 +++
> > xen/common/page_alloc.c | 20 ++++++++++++++++++++
> > xen/include/xen/mm.h | 6 ++++++
> > 5 files changed, 54 insertions(+)
> >
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index
> > ecfa6822e4..cc7a943d27 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -15,6 +15,7 @@ config ARM
> > select HAS_PASSTHROUGH
> > select HAS_PDX
> > select IOMMU_FORCE_PT_SHARE
> > + select STATIC_MEMORY
>
> Given the list of TODOs, I think it would be better if STATIC_MEMORY is user
> selectable and gated by UNSUPPORTED.
>
> We can remove the dependency on UNSUPPORTED once every have been
> addressed.
>
Sure. I'll change it UNSUPPORTED.
config STATIC_ALLOCATION
bool "Static Allocation Support (UNSUPPORTED)" if UNSUPPORTED
> >
> > config ARCH_DEFCONFIG
> > string
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index
> > f569134317..369f6631ee 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -622,6 +622,26 @@ static void __init init_pdx(void)
> > }
> > }
> >
> > +/* Static memory initialization */
> > +static void __init init_staticmem_pages(void) {
> > + unsigned int bank;
> > +
> > + /* TODO: Considering NUMA-support scenario. */
> > + for ( bank = 0 ; bank < bootinfo.static_mem.nr_banks; bank++ )
> > + {
> > + unsigned long bank_start =
> > + PFN_UP(bootinfo.static_mem.bank[bank].start);
>
> I would prefer if bank_start is a mfn_t.
>
Sure, it's more appropriate.
> > + unsigned long bank_size =
> > + PFN_DOWN(bootinfo.static_mem.bank[bank].size);
>
> NIT: I would suggest to name it bank_pages or bank_nr_pages. This would
> make clear in the user that this contains pages.
>
Sure.
> > + unsigned long bank_end = bank_start + bank_size;
>
> mfn_t please.
>
Sure.
> > +
> > + if ( bank_end <= bank_start )
>
> This will mean you will need to use mfn_x() for both. This code would be less
> nice but at least it avoids mixing address and MFN.
>
Sure.
> > + return;
> > +
> > + free_staticmem_pages(mfn_to_page(_mfn(bank_start)),
> > + bank_size, false);
> > + }
> > +}
> > +
> > #ifdef CONFIG_ARM_32
> > static void __init setup_mm(void)
> > {
> > @@ -749,6 +769,8 @@ static void __init setup_mm(void)
> > /* 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_staticmem_pages();
> > }
> > #else /* CONFIG_ARM_64 */
> > static void __init setup_mm(void)
> > @@ -802,6 +824,8 @@ static void __init setup_mm(void)
> >
> > setup_frametable_mappings(ram_start, ram_end);
> > max_page = PFN_DOWN(ram_end);
> > +
> > + init_staticmem_pages();
> > }
> > #endif
> >
> > diff --git a/xen/common/Kconfig b/xen/common/Kconfig index
> > 0ddd18e11a..8f736eea82 100644
> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -67,6 +67,9 @@ config MEM_ACCESS
> > config NEEDS_LIBELF
> > bool
> >
> > +config STATIC_MEMORY
> > + bool
> > +
> > menu "Speculative hardening"
> >
> > config SPECULATIVE_HARDEN_ARRAY
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index
> > a3ee5eca9e..2acb73e323 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -1519,6 +1519,26 @@ static void free_heap_pages(
> > spin_unlock(&heap_lock);
> > }
> >
> > +#ifdef CONFIG_STATIC_MEMORY
> > +/* Equivalent of free_heap_pages to free nr_mfns pages of static
> > +memory. */ void __init free_staticmem_pages(struct page_info *pg,
> unsigned long nr_mfns,
> > + bool need_scrub) {
> > + mfn_t mfn = page_to_mfn(pg);
> > + unsigned long i;
> > +
> > + for ( i = 0; i < nr_mfns; i++ )
> > + {
> > + mark_page_free(&pg[i], mfn_add(mfn, i));
> > +
> > + if ( need_scrub )
> > + {
> > + /* TODO: asynchronous scrubbing for pages of static memory. */
> > + scrub_one_page(pg);
> > + }
> > + }
> > +}
> > +#endif
> >
> > /*
> > * Following rules applied for page offline:
> > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index
> > 667f9dac83..8e8fb5a615 100644
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -85,6 +85,12 @@ bool scrub_free_pages(void);
> > } while ( false )
> > #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
> >
> > +#ifdef CONFIG_STATIC_MEMORY
> > +/* These functions are for static memory */ void
> > +free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> > + bool need_scrub); #endif
> > +
> > /* Map machine page range in Xen virtual address space. */
> > int map_pages_to_xen(
> > unsigned long virt,
> >
>
> Cheers,
>
> --
Cheers
--
> Julien Grall
