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

[Peng Fan <van.freenix@xxxxxxxxx>]

Hello Julien,

On Tue, Sep 20, 2016 at 10:36:27AM +0200, Julien Grall wrote:
>Hello Peng,
>
>On 20/09/2016 07:52, van.freenix@xxxxxxxxx wrote:
>>From: Peng Fan <peng.fan@xxxxxxx>
>>
>>On AArch64 SoCs, some IPs may only have the capability to access
>>32bits address space. The physical memory assigned for Dom0 maybe
>
>s/32bits/32 bits/

Fix in V3.

>
>>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.
>>
>>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
>>
>>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 | 20 +++++++++++++-------
>> 1 file changed, 13 insertions(+), 7 deletions(-)
>>
>>diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>index 35ab08d..0b9b85c 100644
>>--- a/xen/arch/arm/domain_build.c
>>+++ b/xen/arch/arm/domain_build.c
>>@@ -240,11 +240,11 @@ static void allocate_memory(struct domain *d, struct 
>>kernel_info *kinfo)
>
>As mentioned on V1, please update the comment above this function.

About this part:
* 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.

Changed to this V3:
For dom0 we want to place as much of the RAM as we reasonably can
below 4GB, so the devices have the limitation to access 64 bits
address space can work properly and it can also be used by
non-LPAE enabled kernels for 32-bit dom0.

About
"
 * For 32-bit domain we require that the initial allocation for the
 * first bank is under 4G.
"

Changed to this in V3:

"
For 32-bit domain we require that the initial allocation for the
first bank is under 4G. For 64-bit domain, the first bank is preferred
to be allocated under 4GB.
"

Is this ok?

>
>>     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,22 +265,28 @@ 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++ )
>>         {
>>             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;
>>+         }
>>         }
>>         order--;
>>     }
>>
>>-    panic("Unable to allocate first memory bank");
>>+    if ( pg == NULL )
>>+    {
>>+        printk(XENLOG_INFO "No bank has been allocated below 4GB.\n");
>>+        lowmem = false;
>>+    }
>
>This new behavior should be documented in the commit message, with some
>rationale why it is fine to do it for ARM32.

Consider the comments of the function:
  1. The dom0 kernel should be loaded within the first 128MB of RAM. This
     is necessary at least for Linux zImage kernels, which are all we
     support today.
 4. For 32-bit dom0 the kernel must be located below 4GB.

I think I may need to add back the panic when first bank is failed to be 
allocated
under 4GB.

Will use the following code in V3 and document this in commit log.
    if ( pg == NULL )
    {
        if ( is_32bit_domain(d) )
            panic("Unable to allocate first memory bank");

        printk(XENLOG_INFO "No bank has been allocated below 4GB.\n");
        lowmem = false;
    }


>
>>
>>  got_bank0:
>>
>>-    if ( !insert_11_bank(d, kinfo, pg, order) )
>>-        BUG(); /* Cannot fail for first bank */
>>-
>>     /* Now allocate more memory and fill in additional banks */
>
>This comment should be updated.

Add more comments? I did not modify code below the comment and the comment
is correct I think.

Thanks,
Peng.
>
>>
>>     order = get_11_allocation_size(kinfo->unassigned_mem);
>>
>
>Regards,
>
>-- 
>Julien Grall

-- 

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