Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Henry,

On 24/08/2022 09:31, Henry Wang wrote:
> 
> This commit firstly adds a global variable `reserved_heap`.
> This newly introduced global variable is set at the device tree
> parsing time if the reserved heap ranges are defined in the device
> tree chosen node.
> 
Did you consider putting reserved_heap into bootinfo structure?
It would help to avoid introducing new global variables that are only used
in places making use of the bootinfo anyway.

> For Arm32, In `setup_mm`, if the reserved heap is enabled, we use
> the reserved heap region for both domheap and xenheap allocation.
> 
> For Arm64, In `setup_mm`, if the reserved heap is enabled and used,
> we make sure that only these reserved heap pages are added to the
> boot allocator. These reserved heap pages in the boot allocator are
> added to the heap allocator at `end_boot_allocator()`.
> 
> If the reserved heap is disabled, we stick to current page allocation
> strategy at boot time.
> 
> Also, take the chance to correct a "double not" print in Arm32
> `setup_mm()`.
> 
> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
> ---
> With reserved heap enabled, for Arm64, naming of global variables such
> as `xenheap_mfn_start` and `xenheap_mfn_end` seems to be ambiguous,
> wondering if we should rename these variables.
> ---
> Changes from RFC to v1:
> - Rebase on top of latest `setup_mm()` changes.
> - Added Arm32 logic in `setup_mm()`.
> ---
>  xen/arch/arm/bootfdt.c           |  2 +
>  xen/arch/arm/include/asm/setup.h |  2 +
>  xen/arch/arm/setup.c             | 79 +++++++++++++++++++++++++-------
>  3 files changed, 67 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 33704ca487..ab73b6e212 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -325,6 +325,8 @@ static int __init process_chosen_node(const void *fdt, 
> int node,
>                                       true);
>          if ( rc )
>              return rc;
> +
> +        reserved_heap = true;
>      }
> 
>      printk("Checking for initrd in /chosen\n");
> diff --git a/xen/arch/arm/include/asm/setup.h 
> b/xen/arch/arm/include/asm/setup.h
> index e80f3d6201..00536a6d55 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -92,6 +92,8 @@ extern struct bootinfo bootinfo;
> 
>  extern domid_t max_init_domid;
> 
> +extern bool reserved_heap;
> +
>  void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
> 
>  size_t estimate_efi_size(unsigned int mem_nr_banks);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 500307edc0..fe76cf6325 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -73,6 +73,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes);
> 
>  domid_t __read_mostly max_init_domid;
> 
> +bool __read_mostly reserved_heap;
> +
>  static __used void init_done(void)
>  {
>      /* Must be done past setting system_state. */
> @@ -699,8 +701,10 @@ static void __init populate_boot_allocator(void)
>  #ifdef CONFIG_ARM_32
>  static void __init setup_mm(void)
>  {
> -    paddr_t ram_start, ram_end, ram_size, e;
> -    unsigned long ram_pages;
> +    paddr_t ram_start, ram_end, ram_size, e, bank_start, bank_end, bank_size;
> +    paddr_t reserved_heap_start = ~0, reserved_heap_end = 0,
> +            reserved_heap_size = 0;
> +    unsigned long ram_pages, reserved_heap_pages = 0;
>      unsigned long heap_pages, xenheap_pages, domheap_pages;
>      unsigned int i;
>      const uint32_t ctr = READ_CP32(CTR);
> @@ -720,9 +724,9 @@ static void __init setup_mm(void)
> 
>      for ( i = 1; i < bootinfo.mem.nr_banks; i++ )
>      {
> -        paddr_t bank_start = bootinfo.mem.bank[i].start;
> -        paddr_t bank_size = bootinfo.mem.bank[i].size;
> -        paddr_t bank_end = bank_start + bank_size;
> +        bank_start = bootinfo.mem.bank[i].start;
> +        bank_size = bootinfo.mem.bank[i].size;
> +        bank_end = bank_start + bank_size;
> 
>          ram_size  = ram_size + bank_size;
>          ram_start = min(ram_start,bank_start);
> @@ -731,6 +735,25 @@ static void __init setup_mm(void)
> 
>      total_pages = ram_pages = ram_size >> PAGE_SHIFT;
> 
> +    if ( reserved_heap )
> +    {
> +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> +        {
> +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
> +            {
> +                bank_start = bootinfo.reserved_mem.bank[i].start;
> +                bank_size = bootinfo.reserved_mem.bank[i].size;
> +                bank_end = bank_start + bank_size;
> +
> +                reserved_heap_size += bank_size;
> +                reserved_heap_start = min(reserved_heap_start, bank_start);
You do not need reserved_heap_start as you do not use it at any place later on.
In your current implementation you just need reserved_heap_size and 
reserved_heap_end.

> +                reserved_heap_end = max(reserved_heap_end, bank_end);
> +            }
> +        }
> +
> +        reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT;
> +    }
> +
>      /*
>       * If the user has not requested otherwise via the command line
>       * then locate the xenheap using these constraints:
> @@ -743,7 +766,8 @@ static void __init setup_mm(void)
>       * We try to allocate the largest xenheap possible within these
>       * constraints.
>       */
> -    heap_pages = ram_pages;
> +    heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages;
I must say that the reverted logic is harder to read. This is a matter of taste 
but
please consider the following:
heap_pages = reserved_heap ? reserved_heap_pages : ram_pages;
The same applies to ...

> +
>      if ( opt_xenheap_megabytes )
>          xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
>      else
> @@ -755,17 +779,21 @@ static void __init setup_mm(void)
> 
>      do
>      {
> -        e = consider_modules(ram_start, ram_end,
> +        e = !reserved_heap ?
... here.

> +            consider_modules(ram_start, ram_end,
>                               pfn_to_paddr(xenheap_pages),
> -                             32<<20, 0);
> +                             32<<20, 0) :
> +            reserved_heap_end;
> +
>          if ( e )
>              break;
> 
>          xenheap_pages >>= 1;
>      } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) 
> );
> 
> -    if ( ! e )
> -        panic("Not not enough space for xenheap\n");
> +    if ( ! e ||
> +         ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )
I'm not sure about this. You are checking if the size of the reserved heap is 
less than 32MB
and this has nothing to do with the following panic message.

> +        panic("Not enough space for xenheap\n");
> 
>      domheap_pages = heap_pages - xenheap_pages;
> 
> @@ -810,9 +838,9 @@ static void __init setup_mm(void)
>  static void __init setup_mm(void)
>  {
>      const struct meminfo *banks = &bootinfo.mem;
> -    paddr_t ram_start = ~0;
> -    paddr_t ram_end = 0;
> -    paddr_t ram_size = 0;
> +    paddr_t ram_start = ~0, bank_start = ~0;
> +    paddr_t ram_end = 0, bank_end = 0;
> +    paddr_t ram_size = 0, bank_size = 0;
>      unsigned int i;
> 
>      init_pdx();
> @@ -821,17 +849,36 @@ static void __init setup_mm(void)
>       * We need some memory to allocate the page-tables used for the xenheap
>       * mappings. But some regions may contain memory already allocated
>       * for other uses (e.g. modules, reserved-memory...).
> -     *
> +     * If reserved heap regions are properly defined, (only) add these 
> regions
How can you say at this stage whether the reserved heap regions are defined 
properly?

> +     * in the boot allocator.
> +     */
> +    if ( reserved_heap )
> +    {
> +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> +        {
> +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
> +            {
> +                bank_start = bootinfo.reserved_mem.bank[i].start;
> +                bank_size = bootinfo.reserved_mem.bank[i].size;
> +                bank_end = bank_start + bank_size;
> +
> +                init_boot_pages(bank_start, bank_end);
> +            }
> +        }
> +    }
> +    /*
> +     * No reserved heap regions:
>       * For simplicity, add all the free regions in the boot allocator.
>       */
> -    populate_boot_allocator();
> +    else
> +        populate_boot_allocator();
> 
>      total_pages = 0;
> 
>      for ( i = 0; i < banks->nr_banks; i++ )
>      {
>          const struct membank *bank = &banks->bank[i];
> -        paddr_t bank_end = bank->start + bank->size;
> +        bank_end = bank->start + bank->size;
> 
>          ram_size = ram_size + bank->size;
>          ram_start = min(ram_start, bank->start);
> --
> 2.17.1
> 
> 

~Michal