Re: [Xen-devel] [PATCH V3] xen/arm: domain_build: allocate lowmem for dom0 as much as possible

[Julien Grall <julien.grall@xxxxxxx>]

Hello Peng,

On 22/09/16 10:16, Peng Fan wrote:
> On AArch64 SoCs, some IPs may only have the capability to access
> 32 bits address space. The physical memory assigned for Dom0 maybe
> not in 4GB address space, then the IPs will not work properly.
> So need to allocate memory under 4GB for Dom0.
>
> There is no restriction that how much lowmem needs to be allocated for
> Dom0. Dom0 now use 1:1 mapping, but DomU does not use 1:1 mapping,
> there is no need to reserve lowmem for DomU, so allocate lowmem as much
> as possible for Dom0.
The last sentence is confusing. I would avoid to mention domU use case here.

> This patch does not affect 32-bit domain, because Variable "lowmem" is
> set to true at the beginning. If failed to allocate bank0 under 4GB,
> need to panic for 32-bit domain, because 32-bit domain requires bank0
> be allocated under 4GB.
>
> For 64-bit domain, set "lowmem" to false, and continue allocating
> memory from higher memory space.
>
> Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> ---
>
> This patch is to resolve the issue mentioned in
> https://lists.xen.org/archives/html/xen-devel/2016-09/msg00235.html
>
> V3:
>  Add more commit log
>  Add more comments
>  Add back panic if failed to allocate bank0 under 4GB for 32-bit domain.
>
> V2:
>  Remove the bootargs dom0_lowmem introduced in V1.
>  Following 
> "https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg01459.html";
>  to allocate as much as possible lowmem.
>
>  Tested results:
>  (XEN) Allocating 1:1 mappings totalling 2048MB for dom0:
>  (XEN) BANK[0] 0x00000088000000-0x000000f8000000 (1792MB)
>  (XEN) BANK[1] 0x000009e0000000-0x000009f0000000 (256MB)
>  1792M allocated in 4GB address space.
>
>  xen/arch/arm/domain_build.c | 44 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 35ab08d..a9c37c4 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -195,9 +195,10 @@ fail:
>   *    bank. Partly this is just easier for us to deal with, but also
>   *    the ramdisk and DTB must be placed within a certain proximity of
>   *    the kernel within RAM.
> - * 3. For 32-bit dom0 we want to place as much of the RAM as we
> - *    reasonably can below 4GB, so that it can be used by non-LPAE
> - *    enabled kernels.
> + * 3. For dom0 we want to place as much of the RAM as we reasonably can
> + *    below 4GB, so that the devices have the limitation to access 64 bits
> + *    address space and work properly and it can be used by non-LPAE enabled
> + *    kernels.
I would say: "For dom0 we want to place as much of the RAM as we 
reasonably can below 4GB, so that it can be used by non-LPAE enabled 
kernels (32-bit) or when a device assigned to dom0 can only do 32-bit 
DMA access".
>   * 4. For 32-bit dom0 the kernel must be located below 4GB.
>   * 5. We want to have a few largers banks rather than many smaller ones.
>   *
> @@ -230,7 +231,8 @@ fail:
>   * we give up.
>   *
>   * For 32-bit domain we require that the initial allocation for the
> - * first bank is under 4G. Then for the subsequent allocations we
> + * first bank is under 4G. For 64-bit domain, the first bank is preferred
> + * to be allocated under 4G. Then for the subsequent allocations we
>   * initially allocate memory only from below 4GB. Once that runs out
>   * (as described above) we allow higher allocations and continue until
>   * that runs out (or we have allocated sufficient dom0 memory).
> @@ -240,11 +242,11 @@ static void allocate_memory(struct domain *d, struct 
> kernel_info *kinfo)
>      const unsigned int min_low_order =
>          get_order_from_bytes(min_t(paddr_t, dom0_mem, MB(128)));
>      const unsigned int min_order = get_order_from_bytes(MB(4));
> -    struct page_info *pg;
> +    struct page_info *pg = NULL;
>      unsigned int order = get_11_allocation_size(kinfo->unassigned_mem);
>      int i;
>
> -    bool_t lowmem = is_32bit_domain(d);
> +    bool_t lowmem = true;
>      unsigned int bits;
>
>      /*
> @@ -265,24 +267,40 @@ static void allocate_memory(struct domain *d, struct 
> kernel_info *kinfo)
>       */
>      while ( order >= min_low_order )
>      {
> -        for ( bits = order ; bits <= (lowmem ? 32 : PADDR_BITS); bits++ )
> +        for ( bits = order ; bits <= 32 ; bits++ )
Why this change? If we ever decide to set lowmen to false, it will not 
work anymore.
>          {
>              pg = alloc_domheap_pages(d, order, MEMF_bits(bits));
>              if ( pg != NULL )
> +            {
> +                if ( !insert_11_bank(d, kinfo, pg, order) )
> +                    BUG(); /* Cannot fail for first bank */
> +
>                  goto got_bank0;
> +           }
The indentation looks wrong here.

>          }
>          order--;
>      }
>
> -    panic("Unable to allocate first memory bank");
> -
> - got_bank0:
> +    /* Failed to allocate bank0 under 4GB */
> +    if ( pg == NULL )
This check is not necessary because you can only be here when (pg == NULL).

> +    {
> +        if ( is_32bit_domain(d) )
> +            panic("Unable to allocate first memory bank.");
>
> -    if ( !insert_11_bank(d, kinfo, pg, order) )
> -        BUG(); /* Cannot fail for first bank */
> +        /* Try allocating higher memory */
Try to allocate higher memory

> +        printk(XENLOG_INFO "No bank has been allocated below 4GB.\n");
> +        lowmem = false;
> +    }
>
> -    /* Now allocate more memory and fill in additional banks */
> + got_bank0:
>
> +    /*
> +     * If failed to allocate bank0 under 4GB, continue allocating
If we failed to allocate

> +     * memory from higher memory space and fill in banks.
"higher" is not clear. So I would say "from above 4GB"

> +     * Otherwise allocate more memory under 4GB and fill in additional
> +     * banks. If there is not enough memory under 4GB, use higher
Ditto.

> +     * memory.
> +     */
Although, I would just drop the second sentence. It is a reword of the 
big description on top of the function.
>      order = get_11_allocation_size(kinfo->unassigned_mem);
>      while ( kinfo->unassigned_mem && kinfo->mem.nr_banks < NR_MEM_BANKS )
>      {
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel