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

[Peng Fan <van.freenix@xxxxxxxxx>]

On Thu, Sep 22, 2016 at 04:23:05PM +0100, Julien Grall wrote:
>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.

Drop it in V4.

>
>>
>>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".

Thanks. Fix in V4.

>
>>  * 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.

Reasonable. Fix in V4.

>
>>         {
>>             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.

Fix in V4.

>
>>         }
>>         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).

Right. Drop it in V4.

>
>>+    {
>>+        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

Fix in V4.

>
>>+        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

Fix in V4.

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

Fix in V4.

>
>>+     * 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.

Ok.

Thanks,
Peng.

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