Re: [PATCH v5 4/7] xen/arm: static memory initialization

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.08.2021 11:50, Penny Zheng wrote:
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -609,6 +609,29 @@ static void __init init_pdx(void)
>      }
>  }
>  
> +#ifdef CONFIG_STATIC_MEMORY
> +/* Static memory initialization */
> +static void __init init_staticmem_pages(void)
> +{
> +    unsigned int bank;
> +
> +    for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
> +    {
> +        if ( bootinfo.reserved_mem.bank[bank].xen_domain )
> +        {
> +            mfn_t bank_start = 
> _mfn(PFN_UP(bootinfo.reserved_mem.bank[bank].start));
> +            unsigned long bank_pages = 
> PFN_DOWN(bootinfo.reserved_mem.bank[bank].size);
> +            mfn_t bank_end = mfn_add(bank_start, bank_pages);
> +
> +            if ( mfn_x(bank_end) <= mfn_x(bank_start) )
> +                return;
> +
> +            free_staticmem_pages(mfn_to_page(bank_start), bank_pages, false);
> +        }
> +    }
> +}
> +#endif

If you moved the #ifdef inside the function body, ...

> @@ -736,6 +759,10 @@ 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));
> +
> +#ifdef CONFIG_STATIC_MEMORY
> +    init_staticmem_pages();
> +#endif
>  }
>  #else /* CONFIG_ARM_64 */
>  static void __init setup_mm(void)
> @@ -789,6 +816,10 @@ static void __init setup_mm(void)
>  
>      setup_frametable_mappings(ram_start, ram_end);
>      max_page = PFN_DOWN(ram_end);
> +
> +#ifdef CONFIG_STATIC_MEMORY
> +    init_staticmem_pages();
> +#endif

... you could avoid this further #ifdef-ary.

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -67,6 +67,23 @@ config MEM_ACCESS
>  config NEEDS_LIBELF
>       bool
>  
> +config STATIC_MEMORY
> +        bool "Static Allocation Support (UNSUPPORTED)" if UNSUPPORTED
> +     depends on ARM

Inconsistent indentation. Both want to use a single tab.

> +     ---help---

We try to phase out "---help---", following Linux. Please use just
"help".

> +       Static Allocation refers to system or sub-system(domains) for
> +       which memory areas are pre-defined by configuration using physical
> +          address ranges.

Inconsistent indentation again. A tab and two spaces, uniformly.


> +       Those pre-defined memory, -- Static Memory, as parts of RAM reserved
> +       during system boot-up, shall never go to heap allocator or boot
> +       allocator for any use.

I find this hard to parse and I'm also unconvinced this belongs here.

> +       When enabled, memory can be statically allocated to a domain using
> +       the property "xen,static-mem" defined in the domain configuration.
> +
> +       If unsure, say Y.

As being unsupported, perhaps rather "N"?

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1519,7 +1519,6 @@ static void free_heap_pages(
>      spin_unlock(&heap_lock);
>  }
>  
> -
>  /*
>   * Following rules applied for page offline:
>   * Once a page is broken, it can't be assigned anymore

Stray change (I don't really mind it, but it doesn't belong here).

Jan