Re: [PATCH v2 02/21] xen/arm: make allocate_memory work for non 1:1 mapped guests

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 07/07/18 00:11, Stefano Stabellini wrote:
> Extend allocate_memory to work for non 1:1 mapped domUs. Specifically,
> memory allocated for domU will be mapped into the domU pseudo-physical
> address space at the appropriate addresses according to the guest memory
> map: GUEST_RAM0_BASE and GUEST_RAM1_BASE.
> 
> To do that, insert_11_bank has been extended to deal with non-dom0
> mappings starting from GUEST_RAM0_BASE. insert_11_bank has been renamed
> to insert_bank.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> 
> ---
> Changes in v2:
> - new patch
> ---
>   xen/arch/arm/domain_build.c | 57 ++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 44 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 182e3d5..2a6619a 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -97,27 +97,51 @@ static unsigned int get_allocation_size(paddr_t size)
>    * Returns false if the memory would be below bank 0 or we have run
>    * out of banks. In this case it will free the pages.
>    */
> -static bool insert_11_bank(struct domain *d,
> -                           struct kernel_info *kinfo,
> -                           struct page_info *pg,
> -                           unsigned int order)
> +static bool insert_bank(struct domain *d,
> +                        struct kernel_info *kinfo,
> +                        struct page_info *pg,
> +                        unsigned int order,
> +                        bool map_11)
>   {
> -    int res, i;
> +    int res, i, nr_mem_banks = map_11 ? NR_MEM_BANKS : 2;

nr_mem_banks should be unsigned. I also would drop "mem_" to shorten a 
bit the name.

>       mfn_t smfn;
>       paddr_t start, size;
> +    struct membank *bank;
>   
>       smfn = page_to_mfn(pg);
>       start = mfn_to_maddr(smfn);

The new code is pretty horrible to read. Can you please add some 
comments to help understanding it?

Here is already an example where you set start to MFN. But then override 
after with none comment to understand why.

Also, this code as always assumed MFN == GFN so start was making sense. 
Now, you re-purpose it to just the guest-physical address. So more 
likely you want to rework the code a bit.

>       size = pfn_to_paddr(1UL << order);
> +    if ( !map_11 )

I am not sure why map_11 would mean DomU? I don't see any reason to not 
allow that for Dom0. Note that I am not asking to do it, just having 
clearer name.

> +    {
> +        start = GUEST_RAM0_BASE;
> +        if ( kinfo->mem.nr_banks > 0 )
> +        {
> +            for( i = 0; i < kinfo->mem.nr_banks; i++ )
> +            {
> +                bank = &kinfo->mem.bank[i];
> +                start = bank->start + bank->size;
> +            }
> +            if ( bank->start == GUEST_RAM0_BASE &&
> +                    start + size > (GUEST_RAM0_BASE + GUEST_RAM0_SIZE) )

The indentation looks wrong.

> +                start = GUEST_RAM1_BASE;
> +            if ( bank->start == GUEST_RAM1_BASE &&
> +                    start + size > (GUEST_RAM1_BASE + GUEST_RAM1_SIZE) )
> +            {
> +                D11PRINT("Allocation of domain memory exceeds max amount\n");

This looks quite strange to use D11PRINT here as this related to 
direct-domain mapped.

> +                goto fail;
> +            }
> +        }
> +    }
>   
> -    D11PRINT("Allocated %#"PRIpaddr"-%#"PRIpaddr" (%ldMB/%ldMB, order %d)\n",
> +    D11PRINT("Allocated %#"PRIpaddr"-%#"PRIpaddr":%#"PRIpaddr"-%#"PRIpaddr" (%ldMB/%ldMB, order %d)\n",
> +             mfn_to_maddr(smfn), mfn_to_maddr(smfn) + size,
>                start, start + size,
>                1UL << (order + PAGE_SHIFT - 20),
>                /* Don't want format this as PRIpaddr (16 digit hex) */
>                (unsigned long)(kinfo->unassigned_mem >> 20),
>                order);
>   
> -    if ( kinfo->mem.nr_banks > 0 &&
> +    if ( map_11 && kinfo->mem.nr_banks > 0 &&

Why do you drop that check? It should be harmless for non-direct mapped 
domain.

>            size < MB(128) && >            start + size < kinfo->mem.bank[0].start )
>       {
> @@ -125,7 +149,7 @@ static bool insert_11_bank(struct domain *d,
>           goto fail;
>       }
>   
> -    res = guest_physmap_add_page(d, _gfn(mfn_x(smfn)), smfn, order);
> +    res = guest_physmap_add_page(d, gaddr_to_gfn(start), smfn, order);
>       if ( res )
>           panic("Failed map pages to DOM0: %d", res);
>   
> @@ -141,7 +165,7 @@ static bool insert_11_bank(struct domain *d,
>   
>       for( i = 0; i < kinfo->mem.nr_banks; i++ )
>       {
> -        struct membank *bank = &kinfo->mem.bank[i];
> +        bank = &kinfo->mem.bank[i];
>   
>           /* If possible merge new memory into the start of the bank */
>           if ( bank->start == start+size )
> @@ -164,7 +188,7 @@ static bool insert_11_bank(struct domain *d,
>            * could have inserted the memory into/before we would already
>            * have done so, so this must be the right place.
>            */
> -        if ( start + size < bank->start && kinfo->mem.nr_banks < NR_MEM_BANKS )
> +        if ( start + size < bank->start && kinfo->mem.nr_banks < nr_mem_banks )
>           {
>               memmove(bank + 1, bank,
>                       sizeof(*bank) * (kinfo->mem.nr_banks - i));
> @@ -175,7 +199,7 @@ static bool insert_11_bank(struct domain *d,
>           }
>       }
>   
> -    if ( i == kinfo->mem.nr_banks && kinfo->mem.nr_banks < NR_MEM_BANKS )
> +    if ( i == kinfo->mem.nr_banks && kinfo->mem.nr_banks < nr_mem_banks )
>       {
>           struct membank *bank = &kinfo->mem.bank[kinfo->mem.nr_banks];
>   
> @@ -253,6 +277,7 @@ static void allocate_memory(struct domain *d, struct kernel_info *kinfo)
>       struct page_info *pg;
>       unsigned int order = get_allocation_size(kinfo->unassigned_mem);
>       int i;
> +    bool hwdom = d->domain_id == 0;

You should use is_hardware_domain(...).

>   
>       bool lowmem = true;
>       unsigned int bits;
> @@ -263,6 +288,12 @@ static void allocate_memory(struct domain *d, struct kernel_info *kinfo)
>   
>       kinfo->mem.nr_banks = 0;
>   
> +    if ( !hwdom )
> +    {
> +        lowmem = false;
> +        goto got_bank0;
> +    }

Can you explain why you need this?

> +
>       /*
>        * First try and allocate the largest thing we can as low as
>        * possible to be bank 0.
> @@ -274,7 +305,7 @@ static void allocate_memory(struct domain *d, struct kernel_info *kinfo)
>               pg = alloc_domheap_pages(d, order, MEMF_bits(bits));
>               if ( pg != NULL )
>               {
> -                if ( !insert_11_bank(d, kinfo, pg, order) )
> +                if ( !insert_bank(d, kinfo, pg, order, hwdom) )
>                       BUG(); /* Cannot fail for first bank */
>   
>                   goto got_bank0;
> @@ -319,7 +350,7 @@ static void allocate_memory(struct domain *d, struct kernel_info *kinfo)
>               break;
>           }
>   
> -        if ( !insert_11_bank(d, kinfo, pg, order) )
> +        if ( !insert_bank(d, kinfo, pg, order, hwdom) )
>           {
>               if ( kinfo->mem.nr_banks == NR_MEM_BANKS )
>                   /* Nothing more we can do. */
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel