[PATCH] create multiple banks for dom0 in 1:1 mapping if necessary

[PATCH] create multiple banks for dom0 in 1:1 mapping if necessary

[Karim Raslan]

Create multiple banks to hold dom0 memory in case of 1:1 mapping
if we failed to find 1 large contiguous memory to hold the whole
thing.

Signed-off-by: Karim Raslan <karim.allah.ahmed@gmail.com>
---
 xen/arch/arm/domain_build.c |   74 ++++++++++++++++++++++++++-----------
 xen/arch/arm/kernel.c       |   86 ++++++++++++++++++++++++++++++++++---------
 2 files changed, 121 insertions(+), 39 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index faff88e..bb44cdd 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -69,39 +69,71 @@ static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo)
 {
     paddr_t start;
     paddr_t size;
+    unsigned int cur_order, cur_bank, nr_banks = 1, index = 0;
     struct page_info *pg = NULL;
-    unsigned int order = get_order_from_bytes(dom0_mem);
+    unsigned int order = get_order_from_bytes(kinfo->unassigned_mem);
     int res;
     paddr_t spfn;
     unsigned int bits;
 
-    for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
+#define MIN_BANK_ORDER 10
+
+    kinfo->mem.nr_banks = 0;
+
+    /*
+     * We always first try to allocate all dom0 memory in 1 bank.
+     * However, if we failed to allocate physically contiguous memory
+     * from the allocator, then we try to create more than one bank.
+     */
+    for ( cur_order = order; cur_order > MIN_BANK_ORDER; cur_order--)
     {
-        pg = alloc_domheap_pages(d, order, MEMF_bits(bits));
-        if ( pg != NULL )
-            break;
+        for( cur_bank = 1; cur_bank <= nr_banks; cur_bank++ )
+        {
+            for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
+			{
+				pg = alloc_domheap_pages(d, cur_order, MEMF_bits(bits));
+				if ( pg != NULL )
+					break;
+			}
+
+			if ( !pg )
+				break;
+
+			spfn = page_to_mfn(pg);
+			start = pfn_to_paddr(spfn);
+			size = pfn_to_paddr((1 << cur_order));
+
+		    kinfo->mem.bank[index].start = start;
+		    kinfo->mem.bank[index].size = size;
+		    index++;
+		    kinfo->mem.nr_banks++;
+    	}
+
+    	if( pg )
+    		break;
+
+    	nr_banks = (nr_banks - cur_bank + 1) << 1;
     }
 
-    if ( !pg )
-        panic("Failed to allocate contiguous memory for dom0");
+	if ( !pg )
+		panic("Failed to allocate contiguous memory for dom0");
 
-    spfn = page_to_mfn(pg);
-    start = pfn_to_paddr(spfn);
-    size = pfn_to_paddr((1 << order));
+	for ( index = 0; index < kinfo->mem.nr_banks; index++ )
+	{
+	    start = kinfo->mem.bank[index].start;
+	    size = kinfo->mem.bank[index].size;
+	    spfn = paddr_to_pfn(start);
+	    order = get_order_from_bytes(size);
 
-    // 1:1 mapping
-    printk("Populate P2M %#"PRIx64"->%#"PRIx64" (1:1 mapping for dom0)\n",
-           start, start + size);
-    res = guest_physmap_add_page(d, spfn, spfn, order);
-
-    if ( res )
-        panic("Unable to add pages in DOM0: %d", res);
+	    printk("Populate P2M %#"PRIx64"->%#"PRIx64" (1:1 mapping for dom0 - order : %i)\n",
+	            start, start + size, order);
+	    res = guest_physmap_add_page(d, spfn, spfn, order);
 
-    kinfo->mem.bank[0].start = start;
-    kinfo->mem.bank[0].size = size;
-    kinfo->mem.nr_banks = 1;
+	    if ( res )
+	        panic("Unable to add pages in DOM0: %d", res);
 
-    kinfo->unassigned_mem -= size;
+	    kinfo->unassigned_mem -= size;
+	}
 }
 
 static void allocate_memory(struct domain *d, struct kernel_info *kinfo)
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 6a5772b..658c3de 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -79,15 +79,43 @@ static void place_modules(struct kernel_info *info,
     const paddr_t total = initrd_len + dtb_len;
 
     /* Convenient */
-    const paddr_t mem_start = info->mem.bank[0].start;
-    const paddr_t mem_size = info->mem.bank[0].size;
-    const paddr_t mem_end = mem_start + mem_size;
-    const paddr_t kernel_size = kernel_end - kernel_start;
+    unsigned int i, min_i = -1;
+    bool_t same_bank = false;
+
+    paddr_t mem_start, mem_end, mem_size;
+    paddr_t kernel_size;
 
     paddr_t addr;
 
-    if ( total + kernel_size > mem_size )
-        panic("Not enough memory in the first bank for the dtb+initrd");
+    kernel_size = kernel_end - kernel_start;
+
+    for ( i = 0; i < info->mem.nr_banks; i++ )
+    {
+        mem_start = info->mem.bank[i].start;
+        mem_size = info->mem.bank[i].size;
+        mem_end = mem_start + mem_size - 1;
+
+        if ( (kernel_end > mem_start) && (kernel_end <= mem_end) )
+            same_bank = true;
+        else
+            same_bank = false;
+
+        if ( same_bank && ((total + kernel_size) < mem_size) )
+            min_i = i;
+        else if ( (!same_bank) && (total < mem_size) )
+            min_i = i;
+        else
+            continue;
+
+        break;
+    }
+
+    if ( min_i == -1 )
+        panic("Not enough memory for the dtb+initrd");
+
+    mem_start = info->mem.bank[min_i].start;
+    mem_size = info->mem.bank[min_i].size;
+    mem_end = mem_start + mem_size;
 
     /*
      * DTB must be loaded such that it does not conflict with the
@@ -104,17 +132,25 @@ static void place_modules(struct kernel_info *info,
      * just after the kernel, if there is room, otherwise just before.
      */
 
-    if ( kernel_end < MIN(mem_start + MB(128), mem_end - total) )
-        addr = MIN(mem_start + MB(128), mem_end - total);
-    else if ( mem_end - ROUNDUP(kernel_end, MB(2)) >= total )
-        addr = ROUNDUP(kernel_end, MB(2));
-    else if ( kernel_start - mem_start >= total )
-        addr = kernel_start - total;
-    else
+    if ( same_bank )
     {
-        panic("Unable to find suitable location for dtb+initrd");
-        return;
-    }
+        if ( kernel_end < MIN(mem_start + MB(128), mem_end - total) )
+            addr = MIN(mem_start + MB(128), mem_end - total);
+        if ( mem_end - ROUNDUP(kernel_end, MB(2)) >= total )
+            addr = ROUNDUP(kernel_end, MB(2));
+        else if ( kernel_start - mem_start >= total )
+            addr = kernel_start - total;
+        else
+        {
+            /*
+             * We should never hit this condition because we've already
+             * done the check while choosing the bank.
+             */
+            panic("Unable to find suitable location for dtb+initrd");
+            return;
+        }
+    } else
+        addr = ROUNDUP(mem_end - total, MB(2));
 
     info->dtb_paddr = addr;
     info->initrd_paddr = info->dtb_paddr + dtb_len;
@@ -264,10 +300,24 @@ static int kernel_try_zimage32_prepare(struct kernel_info *info,
      */
     if (start == 0)
     {
+        unsigned int i, min_i = 0, min_start = -1;
         paddr_t load_end;
 
-        load_end = info->mem.bank[0].start + info->mem.bank[0].size;
-        load_end = MIN(info->mem.bank[0].start + MB(128), load_end);
+        /*
+         * Load kernel at the lowest possible bank
+         * ( check commit 6c21cb36e263de2db8716b477157a5b6cd531e1e for reason behind that )
+         */
+        for ( i = 0; i < info->mem.nr_banks; i++ )
+        {
+            if( (unsigned int)info->mem.bank[i].start < min_start )
+            {
+                min_start = info->mem.bank[i].start;
+                min_i = i;
+            }
+        }
+
+        load_end = info->mem.bank[min_i].start + info->mem.bank[min_i].size;
+        load_end = MIN(info->mem.bank[min_i].start + MB(128), load_end);
 
         info->zimage.load_addr = load_end - end;
         /* Align to 2MB */
-- 
1.7.9.5

Re: [PATCH] create multiple banks for dom0 in 1:1 mapping if necessary

[Ian Campbell]

On Fri, 2014-01-10 at 04:12 +0000, Karim Raslan wrote:
> Create multiple banks to hold dom0 memory in case of 1:1 mapping
> if we failed to find 1 large contiguous memory to hold the whole
> thing.

Thanks. While with my ARM maintainer hat on I'd love for this to go in
for 4.4 with my acting release manager hat on I think if I have to be
honest this is too big a change for 4.4 at this stage, which is a
pity :-(


> 
> Signed-off-by: Karim Raslan <karim.allah.ahmed@gmail.com>
> ---
>  xen/arch/arm/domain_build.c |   74 ++++++++++++++++++++++++++-----------
>  xen/arch/arm/kernel.c       |   86 ++++++++++++++++++++++++++++++++++---------
>  2 files changed, 121 insertions(+), 39 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index faff88e..bb44cdd 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -69,39 +69,71 @@ static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo)
>  {
>      paddr_t start;
>      paddr_t size;
> +    unsigned int cur_order, cur_bank, nr_banks = 1, index = 0;
>      struct page_info *pg = NULL;
> -    unsigned int order = get_order_from_bytes(dom0_mem);
> +    unsigned int order = get_order_from_bytes(kinfo->unassigned_mem);
>      int res;
>      paddr_t spfn;
>      unsigned int bits;
>  
> -    for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
> +#define MIN_BANK_ORDER 10

2^10 is < PAGE_SIZE (PAGE_SHIFT is 12). 12 is the lowest allocation size
which can be made, but I think in practice the lowest useful bank size
is going to be somewhat larger than that.

NR_MEM_BANKS is 8 so we also need to consider that.

A 64M dom0 would mean 8M per bank, which seems like a reasonable minimum
bank size. That would be a MIN_BANK_ORDER of 23. Please include a
comment explaining where this number comes from.

The other way to look at this would be to calculate it dynamically as
get_order_from_bytes(dom0_mem / NR_MEM_BANKS).

> +
> +    kinfo->mem.nr_banks = 0;
> +
> +    /*
> +     * We always first try to allocate all dom0 memory in 1 bank.
> +     * However, if we failed to allocate physically contiguous memory
> +     * from the allocator, then we try to create more than one bank.
> +     */
> +    for ( cur_order = order; cur_order > MIN_BANK_ORDER; cur_order--)

I think this can be just 
	for( order = get_order_from_bytes(...) ; order > MIN_BANK_ORDER ; order-- )
(maybe order >= ?) or
	while (order > MIN_BANK_ORDER )
	{
		...
		order--;
	}
I think the first works better.

This does away with the need for cur_order vs order. I think order is
mostly unused after this patch, also not renaming cur_order would
hopefully reduce the diff and therefore the "scariness" of the patch wrt
4.4 (although that may not be sufficient).

>      {
> -        pg = alloc_domheap_pages(d, order, MEMF_bits(bits));
> -        if ( pg != NULL )
> -            break;
> +        for( cur_bank = 1; cur_bank <= nr_banks; cur_bank++ )

Is cur_bank redundant with index? Also kinfo->mem.nr_banks tells you how
many banks are filled in.

> +        {
> +            for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
> +			{

There something a bit odd going on with the whitespace here and in the
rest of this loop. Perhaps some hard tabs snuck in?

> +				pg = alloc_domheap_pages(d, cur_order, MEMF_bits(bits));
> +				if ( pg != NULL )
> +					break;
> +			}
> +
> +			if ( !pg )
> +				break;
> +
> +			spfn = page_to_mfn(pg);
> +			start = pfn_to_paddr(spfn);
> +			size = pfn_to_paddr((1 << cur_order));
> +
> +		    kinfo->mem.bank[index].start = start;
> +		    kinfo->mem.bank[index].size = size;
> +		    index++;
> +		    kinfo->mem.nr_banks++;
> +    	}
> +
> +    	if( pg )
> +    		break;
> +
> +    	nr_banks = (nr_banks - cur_bank + 1) << 1;

<<1 ? 

Is this not just kinfo->mem.nr_banks?

The basic structure here is:

for ( cur_order = order; cur_order > MIN_BANK_ORDER; cur_order--)
        for( cur_bank = 1; cur_bank <= nr_banks; cur_bank++ )
                for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
                
Shouldn't the iteration over bank be the outer one?

The banks might be different sizes, right?

Also with either approach then depending on where memory is available
this may result in the memory not being allocated in and/or that they
are not in increasing order (in fact, because Xen prefer to allocate
higher memory first it seems likely that it will be in reverse order).

I don't know if either of those things matter. What does ePAPR have to
say on the matter?

I'd expect that the ordering might matter from the point of view of
putting the kernel in the first bank -- since that may no longer be the
lowest address.

You don't seem to reference kinfo->unassigned_mem anywhere after the
initial order calculation -- I think you need to subtract memory from it
on each iteration, or else I'm not sure you will actually get the right
amount allocated in all cases.

>      }
>  
> -    if ( !pg )
> -        panic("Failed to allocate contiguous memory for dom0");
> +	if ( !pg )
> +		panic("Failed to allocate contiguous memory for dom0");
>  
> -    spfn = page_to_mfn(pg);
> -    start = pfn_to_paddr(spfn);
> -    size = pfn_to_paddr((1 << order));
> +	for ( index = 0; index < kinfo->mem.nr_banks; index++ )
> +	{
> +	    start = kinfo->mem.bank[index].start;
> +	    size = kinfo->mem.bank[index].size;
> +	    spfn = paddr_to_pfn(start);
> +	    order = get_order_from_bytes(size);
>  
> -    // 1:1 mapping
> -    printk("Populate P2M %#"PRIx64"->%#"PRIx64" (1:1 mapping for dom0)\n",
> -           start, start + size);
> -    res = guest_physmap_add_page(d, spfn, spfn, order);
> -
> -    if ( res )
> -        panic("Unable to add pages in DOM0: %d", res);
> +	    printk("Populate P2M %#"PRIx64"->%#"PRIx64" (1:1 mapping for dom0 - order : %i)\n",
> +	            start, start + size, order);
> +	    res = guest_physmap_add_page(d, spfn, spfn, order);

Can this not be done as it is allocated rather than in a second pass?

>  
> -    kinfo->mem.bank[0].start = start;
> -    kinfo->mem.bank[0].size = size;
> -    kinfo->mem.nr_banks = 1;
> +	    if ( res )
> +	        panic("Unable to add pages in DOM0: %d", res);
>  
> -    kinfo->unassigned_mem -= size;
> +	    kinfo->unassigned_mem -= size;
> +	}
>  }
>  
>  static void allocate_memory(struct domain *d, struct kernel_info *kinfo)
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 6a5772b..658c3de 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -79,15 +79,43 @@ static void place_modules(struct kernel_info *info,
>      const paddr_t total = initrd_len + dtb_len;
>  
>      /* Convenient */

If you are going to remove all of the following convenience variables
then this comment is no longer correct.

(Convenient here means a shorter local name for something complex)

> -    const paddr_t mem_start = info->mem.bank[0].start;
> -    const paddr_t mem_size = info->mem.bank[0].size;
> -    const paddr_t mem_end = mem_start + mem_size;
> -    const paddr_t kernel_size = kernel_end - kernel_start;
> +    unsigned int i, min_i = -1;
> +    bool_t same_bank = false;
> +
> +    paddr_t mem_start, mem_end, mem_size;
> +    paddr_t kernel_size;
>  
>      paddr_t addr;
>  
> -    if ( total + kernel_size > mem_size )
> -        panic("Not enough memory in the first bank for the dtb+initrd");
> +    kernel_size = kernel_end - kernel_start;
> +
> +    for ( i = 0; i < info->mem.nr_banks; i++ )
> +    {
> +        mem_start = info->mem.bank[i].start;
> +        mem_size = info->mem.bank[i].size;
> +        mem_end = mem_start + mem_size - 1;
> +
> +        if ( (kernel_end > mem_start) && (kernel_end <= mem_end) )
> +            same_bank = true;
> +        else
> +            same_bank = false;
> +
> +        if ( same_bank && ((total + kernel_size) < mem_size) )
> +            min_i = i;
> +        else if ( (!same_bank) && (total < mem_size) )
> +            min_i = i;
> +        else
> +            continue;

What is all this doing?

> +
> +        break;
> +    }
> +
> +    if ( min_i == -1 )
> +        panic("Not enough memory for the dtb+initrd");
> +
> +    mem_start = info->mem.bank[min_i].start;
> +    mem_size = info->mem.bank[min_i].size;
> +    mem_end = mem_start + mem_size;
>  
>      /*
>       * DTB must be loaded such that it does not conflict with the
> @@ -104,17 +132,25 @@ static void place_modules(struct kernel_info *info,
>       * just after the kernel, if there is room, otherwise just before.
>       */
>  
> -    if ( kernel_end < MIN(mem_start + MB(128), mem_end - total) )
> -        addr = MIN(mem_start + MB(128), mem_end - total);
> -    else if ( mem_end - ROUNDUP(kernel_end, MB(2)) >= total )
> -        addr = ROUNDUP(kernel_end, MB(2));
> -    else if ( kernel_start - mem_start >= total )
> -        addr = kernel_start - total;
> -    else
> +    if ( same_bank )
>      {
> -        panic("Unable to find suitable location for dtb+initrd");
> -        return;
> -    }
> +        if ( kernel_end < MIN(mem_start + MB(128), mem_end - total) )
> +            addr = MIN(mem_start + MB(128), mem_end - total);
> +        if ( mem_end - ROUNDUP(kernel_end, MB(2)) >= total )
> +            addr = ROUNDUP(kernel_end, MB(2));
> +        else if ( kernel_start - mem_start >= total )
> +            addr = kernel_start - total;
> +        else
> +        {
> +            /*
> +             * We should never hit this condition because we've already
> +             * done the check while choosing the bank.
> +             */
> +            panic("Unable to find suitable location for dtb+initrd");
> +            return;
> +        }
> +    } else
> +        addr = ROUNDUP(mem_end - total, MB(2));
>  
>      info->dtb_paddr = addr;
>      info->initrd_paddr = info->dtb_paddr + dtb_len;
> @@ -264,10 +300,24 @@ static int kernel_try_zimage32_prepare(struct kernel_info *info,
>       */
>      if (start == 0)
>      {
> +        unsigned int i, min_i = 0, min_start = -1;
>          paddr_t load_end;
>  
> -        load_end = info->mem.bank[0].start + info->mem.bank[0].size;
> -        load_end = MIN(info->mem.bank[0].start + MB(128), load_end);
> +        /*
> +         * Load kernel at the lowest possible bank
> +         * ( check commit 6c21cb36e263de2db8716b477157a5b6cd531e1e for reason behind that )

That commit says nothing about loading in the lowest possible bank,
though. If there is some relevant factor which is worth commenting on
please do so directly.

Actually now that the kernel is fixed upstream we don't need the
behaviour of that commit at all. Although there are still restrictions
based on load address vs start of RAM (See booting.txt in the kernel
docs)

Ian.

> +         */
> +        for ( i = 0; i < info->mem.nr_banks; i++ )
> +        {
> +            if( (unsigned int)info->mem.bank[i].start < min_start )
> +            {
> +                min_start = info->mem.bank[i].start;
> +                min_i = i;
> +            }
> +        }
> +
> +        load_end = info->mem.bank[min_i].start + info->mem.bank[min_i].size;
> +        load_end = MIN(info->mem.bank[min_i].start + MB(128), load_end);
>  
>          info->zimage.load_addr = load_end - end;
>          /* Align to 2MB */

Re: [PATCH] create multiple banks for dom0 in 1:1 mapping if necessary

[karim.allah.ahmed]

On Fri, Jan 10, 2014 at 3:48 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Fri, 2014-01-10 at 04:12 +0000, Karim Raslan wrote:
>> Create multiple banks to hold dom0 memory in case of 1:1 mapping
>> if we failed to find 1 large contiguous memory to hold the whole
>> thing.
>
> Thanks. While with my ARM maintainer hat on I'd love for this to go in
> for 4.4 with my acting release manager hat on I think if I have to be
> honest this is too big a change for 4.4 at this stage, which is a
> pity :-(
>
>
>>
>> Signed-off-by: Karim Raslan <karim.allah.ahmed@gmail.com>
>> ---
>>  xen/arch/arm/domain_build.c |   74 ++++++++++++++++++++++++++-----------
>>  xen/arch/arm/kernel.c       |   86 ++++++++++++++++++++++++++++++++++---------
>>  2 files changed, 121 insertions(+), 39 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index faff88e..bb44cdd 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -69,39 +69,71 @@ static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo)
>>  {
>>      paddr_t start;
>>      paddr_t size;
>> +    unsigned int cur_order, cur_bank, nr_banks = 1, index = 0;
>>      struct page_info *pg = NULL;
>> -    unsigned int order = get_order_from_bytes(dom0_mem);
>> +    unsigned int order = get_order_from_bytes(kinfo->unassigned_mem);
>>      int res;
>>      paddr_t spfn;
>>      unsigned int bits;
>>
>> -    for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
>> +#define MIN_BANK_ORDER 10
>
> 2^10 is < PAGE_SIZE (PAGE_SHIFT is 12). 12 is the lowest allocation size
> which can be made, but I think in practice the lowest useful bank size
> is going to be somewhat larger than that.

MIN_BANK_ORDER is in pages, so it 2^10 pages not 2^10 bytes which
makes the minimum 4 Mbyte

>
> NR_MEM_BANKS is 8 so we also need to consider that.
>
> A 64M dom0 would mean 8M per bank, which seems like a reasonable minimum
> bank size. That would be a MIN_BANK_ORDER of 23. Please include a
> comment explaining where this number comes from.
>
> The other way to look at this would be to calculate it dynamically as
> get_order_from_bytes(dom0_mem / NR_MEM_BANKS).

Yup, you're right. That sounds more reasonable.

>
>> +
>> +    kinfo->mem.nr_banks = 0;
>> +
>> +    /*
>> +     * We always first try to allocate all dom0 memory in 1 bank.
>> +     * However, if we failed to allocate physically contiguous memory
>> +     * from the allocator, then we try to create more than one bank.
>> +     */
>> +    for ( cur_order = order; cur_order > MIN_BANK_ORDER; cur_order--)
>
> I think this can be just
>         for( order = get_order_from_bytes(...) ; order > MIN_BANK_ORDER ; order-- )
> (maybe order >= ?) or
>         while (order > MIN_BANK_ORDER )
>         {
>                 ...
>                 order--;
>         }
> I think the first works better.
>
> This does away with the need for cur_order vs order. I think order is
> mostly unused after this patch, also not renaming cur_order would
> hopefully reduce the diff and therefore the "scariness" of the patch wrt
> 4.4 (although that may not be sufficient).

Yes, that's correct, however I'm intentionally using a different
variable because I thought that this is going to make things more
obvious. If you think it's better to use the same variable, then I'll
update it.

>
>>      {
>> -        pg = alloc_domheap_pages(d, order, MEMF_bits(bits));
>> -        if ( pg != NULL )
>> -            break;
>> +        for( cur_bank = 1; cur_bank <= nr_banks; cur_bank++ )
>
> Is cur_bank redundant with index? Also kinfo->mem.nr_banks tells you how
> many banks are filled in.
>
>> +        {
>> +            for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
>> +                     {
>
> There something a bit odd going on with the whitespace here and in the
> rest of this loop. Perhaps some hard tabs snuck in?

Yes, I noticed that when the patch appeared in the mailing list :)

>
>> +                             pg = alloc_domheap_pages(d, cur_order, MEMF_bits(bits));
>> +                             if ( pg != NULL )
>> +                                     break;
>> +                     }
>> +
>> +                     if ( !pg )
>> +                             break;
>> +
>> +                     spfn = page_to_mfn(pg);
>> +                     start = pfn_to_paddr(spfn);
>> +                     size = pfn_to_paddr((1 << cur_order));
>> +
>> +                 kinfo->mem.bank[index].start = start;
>> +                 kinfo->mem.bank[index].size = size;
>> +                 index++;
>> +                 kinfo->mem.nr_banks++;
>> +     }
>> +
>> +     if( pg )
>> +             break;
>> +
>> +     nr_banks = (nr_banks - cur_bank + 1) << 1;
>
> <<1 ?

* 2 :)

>
> Is this not just kinfo->mem.nr_banks?

No, In this equation I'm calculating how much more memory will be
needed to satisfy the memory size of dom0.
So at the end of the iteration, I check how much memory has been
allocated (=cur_bank * cur_order) and how much memory was needed
(=nr_banks * cur_order), so nr_unallocated_banks = (nr_banks -
cur_bank + 1) * cur_order
So cur_order is decremented and nr_unallocated_banks is doubled ( <<1
) and we do another iteration to satisfy the rest of unallocated
memory.

>
> The basic structure here is:
>
> for ( cur_order = order; cur_order > MIN_BANK_ORDER; cur_order--)
>         for( cur_bank = 1; cur_bank <= nr_banks; cur_bank++ )
>                 for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
>
> Shouldn't the iteration over bank be the outer one?
>
> The banks might be different sizes, right?
>

The outer loop will define the order of the allocated bank[s] while
the inner one will define how many banks of that order will be needed.

So you try to allocate one big bank, if it succeeds you're done. If
not, you double the number of required banks and retry with smaller
order (order - 1).

The code can indeed allocate banks of different sizes. So, if we
failed to allocate 1 big bank, we will try to allocate two banks of
(order = order -1) in that case we might only allocate the first bank
and fail to allocate the second one. So, we try to allocate the
required memory for the second one in two banks.

So the logic is always: In the end of the outer loop calculate how
many banks we need to allocate for next iteration as well as the
required order. So all allocation that occur in the same iteration is
equal, while each iteration changes the nr banks and order depending
on how much memory we still need to allocate

> Also with either approach then depending on where memory is available
> this may result in the memory not being allocated in and/or that they
> are not in increasing order (in fact, because Xen prefer to allocate
> higher memory first it seems likely that it will be in reverse order).

Yes, there's no restriction what so ever on the order of the
addresses. However each allocation is trying to allocate the bank as
early as possible

for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
{
    pg = alloc_domheap_pages(d, cur_order, MEMF_bits(bits));
    if ( pg != NULL )
        break;
}

>
> I don't know if either of those things matter. What does ePAPR have to
> say on the matter?

There is no mention of address range ordering ( at least in section
3.4 "Memory Node" )

> I'd expect that the ordering might matter from the point of view of
> putting the kernel in the first bank -- since that may no longer be the
> lowest address.
>
In the patch, when I set the loadaddr of the image I search through
the banks for the lowest address bank not the first one anyway.


> You don't seem to reference kinfo->unassigned_mem anywhere after the
> initial order calculation -- I think you need to subtract memory from it
> on each iteration, or else I'm not sure you will actually get the right
> amount allocated in all cases.

It's being properly calculated in the external mapping loop.

>
>>      }
>>
>> -    if ( !pg )
>> -        panic("Failed to allocate contiguous memory for dom0");
>> +     if ( !pg )
>> +             panic("Failed to allocate contiguous memory for dom0");
>>
>> -    spfn = page_to_mfn(pg);
>> -    start = pfn_to_paddr(spfn);
>> -    size = pfn_to_paddr((1 << order));
>> +     for ( index = 0; index < kinfo->mem.nr_banks; index++ )
>> +     {
>> +         start = kinfo->mem.bank[index].start;
>> +         size = kinfo->mem.bank[index].size;
>> +         spfn = paddr_to_pfn(start);
>> +         order = get_order_from_bytes(size);
>>
>> -    // 1:1 mapping
>> -    printk("Populate P2M %#"PRIx64"->%#"PRIx64" (1:1 mapping for dom0)\n",
>> -           start, start + size);
>> -    res = guest_physmap_add_page(d, spfn, spfn, order);
>> -
>> -    if ( res )
>> -        panic("Unable to add pages in DOM0: %d", res);
>> +         printk("Populate P2M %#"PRIx64"->%#"PRIx64" (1:1 mapping for dom0 - order : %i)\n",
>> +                 start, start + size, order);
>> +         res = guest_physmap_add_page(d, spfn, spfn, order);
>
> Can this not be done as it is allocated rather than in a second pass?
>
Yes, that's possible.

>>
>> -    kinfo->mem.bank[0].start = start;
>> -    kinfo->mem.bank[0].size = size;
>> -    kinfo->mem.nr_banks = 1;
>> +         if ( res )
>> +             panic("Unable to add pages in DOM0: %d", res);
>>
>> -    kinfo->unassigned_mem -= size;
>> +         kinfo->unassigned_mem -= size;
>> +     }
>>  }
>>
>>  static void allocate_memory(struct domain *d, struct kernel_info *kinfo)
>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>> index 6a5772b..658c3de 100644
>> --- a/xen/arch/arm/kernel.c
>> +++ b/xen/arch/arm/kernel.c
>> @@ -79,15 +79,43 @@ static void place_modules(struct kernel_info *info,
>>      const paddr_t total = initrd_len + dtb_len;
>>
>>      /* Convenient */
>
> If you are going to remove all of the following convenience variables
> then this comment is no longer correct.
>
> (Convenient here means a shorter local name for something complex)

It still applies to these variables except probably "unsigned int i,
min_i = -1;", so I'll push the comment one line down.

>
>> -    const paddr_t mem_start = info->mem.bank[0].start;
>> -    const paddr_t mem_size = info->mem.bank[0].size;
>> -    const paddr_t mem_end = mem_start + mem_size;
>> -    const paddr_t kernel_size = kernel_end - kernel_start;
>> +    unsigned int i, min_i = -1;
>> +    bool_t same_bank = false;
>> +
>> +    paddr_t mem_start, mem_end, mem_size;
>> +    paddr_t kernel_size;
>>
>>      paddr_t addr;
>>
>> -    if ( total + kernel_size > mem_size )
>> -        panic("Not enough memory in the first bank for the dtb+initrd");
>> +    kernel_size = kernel_end - kernel_start;
>> +
>> +    for ( i = 0; i < info->mem.nr_banks; i++ )
>> +    {
>> +        mem_start = info->mem.bank[i].start;
>> +        mem_size = info->mem.bank[i].size;
>> +        mem_end = mem_start + mem_size - 1;
>> +
>> +        if ( (kernel_end > mem_start) && (kernel_end <= mem_end) )
>> +            same_bank = true;
>> +        else
>> +            same_bank = false;
>> +
>> +        if ( same_bank && ((total + kernel_size) < mem_size) )
>> +            min_i = i;
>> +        else if ( (!same_bank) && (total < mem_size) )
>> +            min_i = i;
>> +        else
>> +            continue;
>
> What is all this doing?

Search through the banks for the bank that fits the initrd and dtb.
Calculation is slightly different depending on whether I ended up
using the same bank as the one used by the kernel or not. ( as
mentioned previously, I don't just blindly choose the first bank for
the kernel. I search through all of the banks for the lowest banks -
address-wise - ). In the same_bank case, I just use the old
calculations that was already there in the code, however in the
!same_bank case I just start at the end of the bank.

>
>> +
>> +        break;
>> +    }
>> +
>> +    if ( min_i == -1 )
>> +        panic("Not enough memory for the dtb+initrd");
>> +
>> +    mem_start = info->mem.bank[min_i].start;
>> +    mem_size = info->mem.bank[min_i].size;
>> +    mem_end = mem_start + mem_size;
>>
>>      /*
>>       * DTB must be loaded such that it does not conflict with the
>> @@ -104,17 +132,25 @@ static void place_modules(struct kernel_info *info,
>>       * just after the kernel, if there is room, otherwise just before.
>>       */
>>
>> -    if ( kernel_end < MIN(mem_start + MB(128), mem_end - total) )
>> -        addr = MIN(mem_start + MB(128), mem_end - total);
>> -    else if ( mem_end - ROUNDUP(kernel_end, MB(2)) >= total )
>> -        addr = ROUNDUP(kernel_end, MB(2));
>> -    else if ( kernel_start - mem_start >= total )
>> -        addr = kernel_start - total;
>> -    else
>> +    if ( same_bank )
>>      {
>> -        panic("Unable to find suitable location for dtb+initrd");
>> -        return;
>> -    }
>> +        if ( kernel_end < MIN(mem_start + MB(128), mem_end - total) )
>> +            addr = MIN(mem_start + MB(128), mem_end - total);
>> +        if ( mem_end - ROUNDUP(kernel_end, MB(2)) >= total )
>> +            addr = ROUNDUP(kernel_end, MB(2));
>> +        else if ( kernel_start - mem_start >= total )
>> +            addr = kernel_start - total;
>> +        else
>> +        {
>> +            /*
>> +             * We should never hit this condition because we've already
>> +             * done the check while choosing the bank.
>> +             */
>> +            panic("Unable to find suitable location for dtb+initrd");
>> +            return;
>> +        }
>> +    } else
>> +        addr = ROUNDUP(mem_end - total, MB(2));
>>
>>      info->dtb_paddr = addr;
>>      info->initrd_paddr = info->dtb_paddr + dtb_len;
>> @@ -264,10 +300,24 @@ static int kernel_try_zimage32_prepare(struct kernel_info *info,
>>       */
>>      if (start == 0)
>>      {
>> +        unsigned int i, min_i = 0, min_start = -1;
>>          paddr_t load_end;
>>
>> -        load_end = info->mem.bank[0].start + info->mem.bank[0].size;
>> -        load_end = MIN(info->mem.bank[0].start + MB(128), load_end);
>> +        /*
>> +         * Load kernel at the lowest possible bank
>> +         * ( check commit 6c21cb36e263de2db8716b477157a5b6cd531e1e for reason behind that )
>
> That commit says nothing about loading in the lowest possible bank,
> though. If there is some relevant factor which is worth commenting on
> please do so directly.

Loading at the lowest bank is safer because of the:
"The current solution in Linux assuming that the delta physical
address - virtual address is always negative".
This delta is being calculated based on where the kernel was loaded (
using "adr" to find physical address ).
So loading it as early as possible is a good idea to avoid this problem.

However, I'm not quite sure why not just load it at the beginning of
the memory then! I think I'll look at booting.txt for that, maybe it's
a decompresser limitation or something!

>
> Actually now that the kernel is fixed upstream we don't need the
> behaviour of that commit at all. Although there are still restrictions
> based on load address vs start of RAM (See booting.txt in the kernel
> docs)
Ah, I see. I'm using an allwinner branch of the kernel (
https://github.com/bjzhang/linux-allwinner.git ). I'll take a look at
the latest thing.

>
> Ian.
>
>> +         */
>> +        for ( i = 0; i < info->mem.nr_banks; i++ )
>> +        {
>> +            if( (unsigned int)info->mem.bank[i].start < min_start )
>> +            {
>> +                min_start = info->mem.bank[i].start;
>> +                min_i = i;
>> +            }
>> +        }
>> +
>> +        load_end = info->mem.bank[min_i].start + info->mem.bank[min_i].size;
>> +        load_end = MIN(info->mem.bank[min_i].start + MB(128), load_end);
>>
>>          info->zimage.load_addr = load_end - end;
>>          /* Align to 2MB */
>
>
>



-- 
Karim Allah Ahmed.
LinkedIn

Re: [PATCH] create multiple banks for dom0 in 1:1 mapping if necessary

[Ian Campbell]

On Sat, 2014-01-11 at 01:58 +0000, karim.allah.ahmed@gmail.com wrote:
> On Fri, Jan 10, 2014 at 3:48 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Fri, 2014-01-10 at 04:12 +0000, Karim Raslan wrote:
> >> Create multiple banks to hold dom0 memory in case of 1:1 mapping
> >> if we failed to find 1 large contiguous memory to hold the whole
> >> thing.
> >
> > Thanks. While with my ARM maintainer hat on I'd love for this to go in
> > for 4.4 with my acting release manager hat on I think if I have to be
> > honest this is too big a change for 4.4 at this stage, which is a
> > pity :-(
> >
> >
> >>
> >> Signed-off-by: Karim Raslan <karim.allah.ahmed@gmail.com>
> >> ---
> >>  xen/arch/arm/domain_build.c |   74 ++++++++++++++++++++++++++-----------
> >>  xen/arch/arm/kernel.c       |   86 ++++++++++++++++++++++++++++++++++---------
> >>  2 files changed, 121 insertions(+), 39 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> >> index faff88e..bb44cdd 100644
> >> --- a/xen/arch/arm/domain_build.c
> >> +++ b/xen/arch/arm/domain_build.c
> >> @@ -69,39 +69,71 @@ static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo)
> >>  {
> >>      paddr_t start;
> >>      paddr_t size;
> >> +    unsigned int cur_order, cur_bank, nr_banks = 1, index = 0;
> >>      struct page_info *pg = NULL;
> >> -    unsigned int order = get_order_from_bytes(dom0_mem);
> >> +    unsigned int order = get_order_from_bytes(kinfo->unassigned_mem);
> >>      int res;
> >>      paddr_t spfn;
> >>      unsigned int bits;
> >>
> >> -    for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
> >> +#define MIN_BANK_ORDER 10
> >
> > 2^10 is < PAGE_SIZE (PAGE_SHIFT is 12). 12 is the lowest allocation size
> > which can be made, but I think in practice the lowest useful bank size
> > is going to be somewhat larger than that.
> 
> MIN_BANK_ORDER is in pages, so it 2^10 pages not 2^10 bytes which
> makes the minimum 4 Mbyte

My mistake. Please add a comment though.

> >> +
> >> +    kinfo->mem.nr_banks = 0;
> >> +
> >> +    /*
> >> +     * We always first try to allocate all dom0 memory in 1 bank.
> >> +     * However, if we failed to allocate physically contiguous memory
> >> +     * from the allocator, then we try to create more than one bank.
> >> +     */
> >> +    for ( cur_order = order; cur_order > MIN_BANK_ORDER; cur_order--)
> >
> > I think this can be just
> >         for( order = get_order_from_bytes(...) ; order > MIN_BANK_ORDER ; order-- )
> > (maybe order >= ?) or
> >         while (order > MIN_BANK_ORDER )
> >         {
> >                 ...
> >                 order--;
> >         }
> > I think the first works better.
> >
> > This does away with the need for cur_order vs order. I think order is
> > mostly unused after this patch, also not renaming cur_order would
> > hopefully reduce the diff and therefore the "scariness" of the patch wrt
> > 4.4 (although that may not be sufficient).
> 
> Yes, that's correct, however I'm intentionally using a different
> variable because I thought that this is going to make things more
> obvious. If you think it's better to use the same variable, then I'll
> update it.

Personally I found it confusing to read as it is. Although reading
further down I'm still confused and I'm not sure that my suggestion
would actually help.

> >
> >> +                             pg = alloc_domheap_pages(d, cur_order, MEMF_bits(bits));
> >> +                             if ( pg != NULL )
> >> +                                     break;
> >> +                     }
> >> +
> >> +                     if ( !pg )
> >> +                             break;
> >> +
> >> +                     spfn = page_to_mfn(pg);
> >> +                     start = pfn_to_paddr(spfn);
> >> +                     size = pfn_to_paddr((1 << cur_order));
> >> +
> >> +                 kinfo->mem.bank[index].start = start;
> >> +                 kinfo->mem.bank[index].size = size;
> >> +                 index++;
> >> +                 kinfo->mem.nr_banks++;
> >> +     }
> >> +
> >> +     if( pg )
> >> +             break;
> >> +
> >> +     nr_banks = (nr_banks - cur_bank + 1) << 1;
> >
> > <<1 ?
> 
> * 2 :)

I know that ;-) But why are you doubling the number of banks at all?

> > Is this not just kinfo->mem.nr_banks?
> 
> No, In this equation I'm calculating how much more memory will be
> needed to satisfy the memory size of dom0.
> So at the end of the iteration, I check how much memory has been
> allocated (=cur_bank * cur_order) and how much memory was needed
> (=nr_banks * cur_order), so nr_unallocated_banks = (nr_banks -
> cur_bank + 1) * cur_order
> So cur_order is decremented and nr_unallocated_banks is doubled ( <<1
> ) and we do another iteration to satisfy the rest of unallocated
> memory.

Hrm, I'm still a bit confused. I think perhaps choosing better names for
the variables (e.g. it seems like you are saying nr_banks is actually
nr_unallocated_banks?) and adding some comments might help.

But is this algorithm not more complex than it needs to be? Why not just
allocate memory, subtracting it from kinfo->unassigned_mem as you go and
adding a new bank each time? The result would the same wouldn't it?
e.g.:

	while ( kinfo->unassigned_mem )
	{
		order = get_order_of_bytes(kinfo->unsigned_mem)
		do {
			pg = alloc_domheap_pages(... order ...)
		} while(!pg && order>>=1 > MIN_BANK_ORDER)

		if (pg)
			urk!

		kinfo->mem.bank[kinfo->mem.nr_banks].{start,size} = (stuff based on pg, order etc)
		kinfo->mem.nr_banks++
		kinfo->unassigned_mem -= /*whatever it is*/

		/* maybe do guest_physmap_add_page here too */
	}

I think that will produce the same result, won't it?

In either algorithm there needs to be a check for NR_MEM_BANKS somewhere
or else it will run off the end of kinfo->mem.bank[].

> 
> >
> > The basic structure here is:
> >
> > for ( cur_order = order; cur_order > MIN_BANK_ORDER; cur_order--)
> >         for( cur_bank = 1; cur_bank <= nr_banks; cur_bank++ )
> >                 for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
> >
> > Shouldn't the iteration over bank be the outer one?
> >
> > The banks might be different sizes, right?
> >
> 
> The outer loop will define the order of the allocated bank[s] while
> the inner one will define how many banks of that order will be needed.
> 
> So you try to allocate one big bank, if it succeeds you're done. If
> not, you double the number of required banks and retry with smaller
> order (order - 1).
> 
> The code can indeed allocate banks of different sizes. So, if we
> failed to allocate 1 big bank, we will try to allocate two banks of
> (order = order -1) in that case we might only allocate the first bank
> and fail to allocate the second one. So, we try to allocate the
> required memory for the second one in two banks.
> 
> So the logic is always: In the end of the outer loop calculate how
> many banks we need to allocate for next iteration as well as the
> required order. So all allocation that occur in the same iteration is
> equal, while each iteration changes the nr banks and order depending
> on how much memory we still need to allocate

I think I get it now, but it still seems complicated. Am I missing
something with the simpler loop I suggested?

> > Also with either approach then depending on where memory is available
> > this may result in the memory not being allocated in and/or that they
> > are not in increasing order (in fact, because Xen prefer to allocate
> > higher memory first it seems likely that it will be in reverse order).
> 
> Yes, there's no restriction what so ever on the order of the
> addresses. However each allocation is trying to allocate the bank as
> early as possible
> 
> for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
> {
>     pg = alloc_domheap_pages(d, cur_order, MEMF_bits(bits));
>     if ( pg != NULL )
>         break;
> }
> 
> >
> > I don't know if either of those things matter. What does ePAPR have to
> > say on the matter?
> 
> There is no mention of address range ordering ( at least in section
> 3.4 "Memory Node" )

OK. It'll be interesting to see whether the implementations match up to
that!

Since max banks is necessarily small I do wonder if it might be easier
to just do a quick insertion sort as we go rather than risking it. I
suppose there will be plenty of time in the 4.5 cycle (which this work
is now almost certainly targeting) to hit any problem cases.

> > I'd expect that the ordering might matter from the point of view of
> > putting the kernel in the first bank -- since that may no longer be the
> > lowest address.
> >
> In the patch, when I set the loadaddr of the image I search through
> the banks for the lowest address bank not the first one anyway.

OK, I think that makes sense since the requirement is wrt position
relative to the start of RAM, not the first bank.

> > You don't seem to reference kinfo->unassigned_mem anywhere after the
> > initial order calculation -- I think you need to subtract memory from it
> > on each iteration, or else I'm not sure you will actually get the right
> > amount allocated in all cases.
> 
> It's being properly calculated in the external mapping loop.

Hrm yes with all that doubling etc.

> >>
> >> -    kinfo->mem.bank[0].start = start;
> >> -    kinfo->mem.bank[0].size = size;
> >> -    kinfo->mem.nr_banks = 1;
> >> +         if ( res )
> >> +             panic("Unable to add pages in DOM0: %d", res);
> >>
> >> -    kinfo->unassigned_mem -= size;
> >> +         kinfo->unassigned_mem -= size;
> >> +     }
> >>  }
> >>
> >>  static void allocate_memory(struct domain *d, struct kernel_info *kinfo)
> >> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> >> index 6a5772b..658c3de 100644
> >> --- a/xen/arch/arm/kernel.c
> >> +++ b/xen/arch/arm/kernel.c
> >> @@ -79,15 +79,43 @@ static void place_modules(struct kernel_info *info,
> >>      const paddr_t total = initrd_len + dtb_len;
> >>
> >>      /* Convenient */
> >
> > If you are going to remove all of the following convenience variables
> > then this comment is no longer correct.
> >
> > (Convenient here means a shorter local name for something complex)
> 
> It still applies to these variables except probably "unsigned int i,
> min_i = -1;", so I'll push the comment one line down.

No it doesn't AFAICT. "Convenient" here means "these are shorthand for
something longer and inconvenient to type", if the variables aren't
const then they almost certainly are not a convenience in the intended
sense. same_bank, mem_* and kernel_size are all just regular variables I
think.

> >> -    const paddr_t mem_start = info->mem.bank[0].start;
> >> -    const paddr_t mem_size = info->mem.bank[0].size;
> >> -    const paddr_t mem_end = mem_start + mem_size;
> >> -    const paddr_t kernel_size = kernel_end - kernel_start;
> >> +    unsigned int i, min_i = -1;
> >> +    bool_t same_bank = false;
> >> +
> >> +    paddr_t mem_start, mem_end, mem_size;
> >> +    paddr_t kernel_size;
> >>
> >>      paddr_t addr;
> >>
> >> -    if ( total + kernel_size > mem_size )
> >> -        panic("Not enough memory in the first bank for the dtb+initrd");
> >> +    kernel_size = kernel_end - kernel_start;
> >> +
> >> +    for ( i = 0; i < info->mem.nr_banks; i++ )
> >> +    {
> >> +        mem_start = info->mem.bank[i].start;
> >> +        mem_size = info->mem.bank[i].size;
> >> +        mem_end = mem_start + mem_size - 1;
> >> +
> >> +        if ( (kernel_end > mem_start) && (kernel_end <= mem_end) )
> >> +            same_bank = true;
> >> +        else
> >> +            same_bank = false;
> >> +
> >> +        if ( same_bank && ((total + kernel_size) < mem_size) )
> >> +            min_i = i;
> >> +        else if ( (!same_bank) && (total < mem_size) )
> >> +            min_i = i;
> >> +        else
> >> +            continue;
> >
> > What is all this doing?
> 
> Search through the banks for the bank that fits the initrd and dtb.
> Calculation is slightly different depending on whether I ended up
> using the same bank as the one used by the kernel or not. ( as
> mentioned previously, I don't just blindly choose the first bank for
> the kernel. I search through all of the banks for the lowest banks -
> address-wise - ).

Where is the address comparison before assigning min_i?

>  In the same_bank case, I just use the old
> calculations that was already there in the code, however in the
> !same_bank case I just start at the end of the bank.

Would it be clearer to do
	if ( dtb+initrd fits in kernel's back )
		ok
	else
		search
?

There is also a requirement that the dtb and initrd are within certain
ranges of the start of RAM (see the booting.txt and Booting in
Documentation/arm*/), does this implement this? It doesn't look to be
considered during the search or when placing in the !same_bank case.

This would all be simpler if we sorted the banks too. Which is another
argument for doing so IMHO.

> >> -        load_end = info->mem.bank[0].start + info->mem.bank[0].size;
> >> -        load_end = MIN(info->mem.bank[0].start + MB(128), load_end);
> >> +        /*
> >> +         * Load kernel at the lowest possible bank
> >> +         * ( check commit 6c21cb36e263de2db8716b477157a5b6cd531e1e for reason behind that )
> >
> > That commit says nothing about loading in the lowest possible bank,
> > though. If there is some relevant factor which is worth commenting on
> > please do so directly.
> 
> Loading at the lowest bank is safer because of the:
> "The current solution in Linux assuming that the delta physical
> address - virtual address is always negative".
> This delta is being calculated based on where the kernel was loaded (
> using "adr" to find physical address ).
> So loading it as early as possible is a good idea to avoid this problem.

I think this problem is now fixed upstream, the intention was to
eventually revert the workaround (Julien was going to tell me when it
had gone into stable etc, but this is now a 4.5 era revert candidate).

> However, I'm not quite sure why not just load it at the beginning of
> the memory then! I think I'll look at booting.txt for that, maybe it's
> a decompresser limitation or something!

Yes, it's all a bit complicated.

> > Actually now that the kernel is fixed upstream we don't need the
> > behaviour of that commit at all. Although there are still restrictions
> > based on load address vs start of RAM (See booting.txt in the kernel
> > docs)
> Ah, I see. I'm using an allwinner branch of the kernel (
> https://github.com/bjzhang/linux-allwinner.git ). I'll take a look at
> the latest thing.

git://github.com/linux-sunxi/linux-sunxi.git either sunxi-next or
sunxi-devel branches are pretty good baselines nowadays. I've got an
outstanding TODO to update the wiki...

Ian.

Re: [PATCH] create multiple banks for dom0 in 1:1 mapping if necessary

[karim.allah.ahmed]

On Tue, Jan 14, 2014 at 10:51 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Sat, 2014-01-11 at 01:58 +0000, karim.allah.ahmed@gmail.com wrote:
>> On Fri, Jan 10, 2014 at 3:48 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Fri, 2014-01-10 at 04:12 +0000, Karim Raslan wrote:
>> >> Create multiple banks to hold dom0 memory in case of 1:1 mapping
>> >> if we failed to find 1 large contiguous memory to hold the whole
>> >> thing.
>> >
>> > Thanks. While with my ARM maintainer hat on I'd love for this to go in
>> > for 4.4 with my acting release manager hat on I think if I have to be
>> > honest this is too big a change for 4.4 at this stage, which is a
>> > pity :-(
>> >
>> >
>> >>
>> >> Signed-off-by: Karim Raslan <karim.allah.ahmed@gmail.com>
>> >> ---
>> >>  xen/arch/arm/domain_build.c |   74 ++++++++++++++++++++++++++-----------
>> >>  xen/arch/arm/kernel.c       |   86 ++++++++++++++++++++++++++++++++++---------
>> >>  2 files changed, 121 insertions(+), 39 deletions(-)
>> >>
>> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> >> index faff88e..bb44cdd 100644
>> >> --- a/xen/arch/arm/domain_build.c
>> >> +++ b/xen/arch/arm/domain_build.c
>> >> @@ -69,39 +69,71 @@ static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo)
>> >>  {
>> >>      paddr_t start;
>> >>      paddr_t size;
>> >> +    unsigned int cur_order, cur_bank, nr_banks = 1, index = 0;
>> >>      struct page_info *pg = NULL;
>> >> -    unsigned int order = get_order_from_bytes(dom0_mem);
>> >> +    unsigned int order = get_order_from_bytes(kinfo->unassigned_mem);
>> >>      int res;
>> >>      paddr_t spfn;
>> >>      unsigned int bits;
>> >>
>> >> -    for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
>> >> +#define MIN_BANK_ORDER 10
>> >
>> > 2^10 is < PAGE_SIZE (PAGE_SHIFT is 12). 12 is the lowest allocation size
>> > which can be made, but I think in practice the lowest useful bank size
>> > is going to be somewhat larger than that.
>>
>> MIN_BANK_ORDER is in pages, so it 2^10 pages not 2^10 bytes which
>> makes the minimum 4 Mbyte
>
> My mistake. Please add a comment though.
>
>> >> +
>> >> +    kinfo->mem.nr_banks = 0;
>> >> +
>> >> +    /*
>> >> +     * We always first try to allocate all dom0 memory in 1 bank.
>> >> +     * However, if we failed to allocate physically contiguous memory
>> >> +     * from the allocator, then we try to create more than one bank.
>> >> +     */
>> >> +    for ( cur_order = order; cur_order > MIN_BANK_ORDER; cur_order--)
>> >
>> > I think this can be just
>> >         for( order = get_order_from_bytes(...) ; order > MIN_BANK_ORDER ; order-- )
>> > (maybe order >= ?) or
>> >         while (order > MIN_BANK_ORDER )
>> >         {
>> >                 ...
>> >                 order--;
>> >         }
>> > I think the first works better.
>> >
>> > This does away with the need for cur_order vs order. I think order is
>> > mostly unused after this patch, also not renaming cur_order would
>> > hopefully reduce the diff and therefore the "scariness" of the patch wrt
>> > 4.4 (although that may not be sufficient).
>>
>> Yes, that's correct, however I'm intentionally using a different
>> variable because I thought that this is going to make things more
>> obvious. If you think it's better to use the same variable, then I'll
>> update it.
>
> Personally I found it confusing to read as it is. Although reading
> further down I'm still confused and I'm not sure that my suggestion
> would actually help.
>
>> >
>> >> +                             pg = alloc_domheap_pages(d, cur_order, MEMF_bits(bits));
>> >> +                             if ( pg != NULL )
>> >> +                                     break;
>> >> +                     }
>> >> +
>> >> +                     if ( !pg )
>> >> +                             break;
>> >> +
>> >> +                     spfn = page_to_mfn(pg);
>> >> +                     start = pfn_to_paddr(spfn);
>> >> +                     size = pfn_to_paddr((1 << cur_order));
>> >> +
>> >> +                 kinfo->mem.bank[index].start = start;
>> >> +                 kinfo->mem.bank[index].size = size;
>> >> +                 index++;
>> >> +                 kinfo->mem.nr_banks++;
>> >> +     }
>> >> +
>> >> +     if( pg )
>> >> +             break;
>> >> +
>> >> +     nr_banks = (nr_banks - cur_bank + 1) << 1;
>> >
>> > <<1 ?
>>
>> * 2 :)
>
> I know that ;-) But why are you doubling the number of banks at all?
>
>> > Is this not just kinfo->mem.nr_banks?
>>
>> No, In this equation I'm calculating how much more memory will be
>> needed to satisfy the memory size of dom0.
>> So at the end of the iteration, I check how much memory has been
>> allocated (=cur_bank * cur_order) and how much memory was needed
>> (=nr_banks * cur_order), so nr_unallocated_banks = (nr_banks -
>> cur_bank + 1) * cur_order
>> So cur_order is decremented and nr_unallocated_banks is doubled ( <<1
>> ) and we do another iteration to satisfy the rest of unallocated
>> memory.
>
> Hrm, I'm still a bit confused. I think perhaps choosing better names for
> the variables (e.g. it seems like you are saying nr_banks is actually
> nr_unallocated_banks?) and adding some comments might help.
>
> But is this algorithm not more complex than it needs to be? Why not just
> allocate memory, subtracting it from kinfo->unassigned_mem as you go and
> adding a new bank each time? The result would the same wouldn't it?
> e.g.:
>
>         while ( kinfo->unassigned_mem )
>         {
>                 order = get_order_of_bytes(kinfo->unsigned_mem)
>                 do {
>                         pg = alloc_domheap_pages(... order ...)
>                 } while(!pg && order>>=1 > MIN_BANK_ORDER)
>
>                 if (pg)
>                         urk!
>
>                 kinfo->mem.bank[kinfo->mem.nr_banks].{start,size} = (stuff based on pg, order etc)
>                 kinfo->mem.nr_banks++
>                 kinfo->unassigned_mem -= /*whatever it is*/
>
>                 /* maybe do guest_physmap_add_page here too */
>         }
>
> I think that will produce the same result, won't it?
>
> In either algorithm there needs to be a check for NR_MEM_BANKS somewhere
> or else it will run off the end of kinfo->mem.bank[].
>
>>
>> >
>> > The basic structure here is:
>> >
>> > for ( cur_order = order; cur_order > MIN_BANK_ORDER; cur_order--)
>> >         for( cur_bank = 1; cur_bank <= nr_banks; cur_bank++ )
>> >                 for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
>> >
>> > Shouldn't the iteration over bank be the outer one?
>> >
>> > The banks might be different sizes, right?
>> >
>>
>> The outer loop will define the order of the allocated bank[s] while
>> the inner one will define how many banks of that order will be needed.
>>
>> So you try to allocate one big bank, if it succeeds you're done. If
>> not, you double the number of required banks and retry with smaller
>> order (order - 1).
>>
>> The code can indeed allocate banks of different sizes. So, if we
>> failed to allocate 1 big bank, we will try to allocate two banks of
>> (order = order -1) in that case we might only allocate the first bank
>> and fail to allocate the second one. So, we try to allocate the
>> required memory for the second one in two banks.
>>
>> So the logic is always: In the end of the outer loop calculate how
>> many banks we need to allocate for next iteration as well as the
>> required order. So all allocation that occur in the same iteration is
>> equal, while each iteration changes the nr banks and order depending
>> on how much memory we still need to allocate
>
> I think I get it now, but it still seems complicated. Am I missing
> something with the simpler loop I suggested?
>
>> > Also with either approach then depending on where memory is available
>> > this may result in the memory not being allocated in and/or that they
>> > are not in increasing order (in fact, because Xen prefer to allocate
>> > higher memory first it seems likely that it will be in reverse order).
>>
>> Yes, there's no restriction what so ever on the order of the
>> addresses. However each allocation is trying to allocate the bank as
>> early as possible
>>
>> for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
>> {
>>     pg = alloc_domheap_pages(d, cur_order, MEMF_bits(bits));
>>     if ( pg != NULL )
>>         break;
>> }
>>
>> >
>> > I don't know if either of those things matter. What does ePAPR have to
>> > say on the matter?
>>
>> There is no mention of address range ordering ( at least in section
>> 3.4 "Memory Node" )
>
> OK. It'll be interesting to see whether the implementations match up to
> that!
>
> Since max banks is necessarily small I do wonder if it might be easier
> to just do a quick insertion sort as we go rather than risking it. I
> suppose there will be plenty of time in the 4.5 cycle (which this work
> is now almost certainly targeting) to hit any problem cases.
>
>> > I'd expect that the ordering might matter from the point of view of
>> > putting the kernel in the first bank -- since that may no longer be the
>> > lowest address.
>> >
>> In the patch, when I set the loadaddr of the image I search through
>> the banks for the lowest address bank not the first one anyway.
>
> OK, I think that makes sense since the requirement is wrt position
> relative to the start of RAM, not the first bank.
>
>> > You don't seem to reference kinfo->unassigned_mem anywhere after the
>> > initial order calculation -- I think you need to subtract memory from it
>> > on each iteration, or else I'm not sure you will actually get the right
>> > amount allocated in all cases.
>>
>> It's being properly calculated in the external mapping loop.
>
> Hrm yes with all that doubling etc.
>
>> >>
>> >> -    kinfo->mem.bank[0].start = start;
>> >> -    kinfo->mem.bank[0].size = size;
>> >> -    kinfo->mem.nr_banks = 1;
>> >> +         if ( res )
>> >> +             panic("Unable to add pages in DOM0: %d", res);
>> >>
>> >> -    kinfo->unassigned_mem -= size;
>> >> +         kinfo->unassigned_mem -= size;
>> >> +     }
>> >>  }
>> >>
>> >>  static void allocate_memory(struct domain *d, struct kernel_info *kinfo)
>> >> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>> >> index 6a5772b..658c3de 100644
>> >> --- a/xen/arch/arm/kernel.c
>> >> +++ b/xen/arch/arm/kernel.c
>> >> @@ -79,15 +79,43 @@ static void place_modules(struct kernel_info *info,
>> >>      const paddr_t total = initrd_len + dtb_len;
>> >>
>> >>      /* Convenient */
>> >
>> > If you are going to remove all of the following convenience variables
>> > then this comment is no longer correct.
>> >
>> > (Convenient here means a shorter local name for something complex)
>>
>> It still applies to these variables except probably "unsigned int i,
>> min_i = -1;", so I'll push the comment one line down.
>
> No it doesn't AFAICT. "Convenient" here means "these are shorthand for
> something longer and inconvenient to type", if the variables aren't
> const then they almost certainly are not a convenience in the intended
> sense. same_bank, mem_* and kernel_size are all just regular variables I
> think.
>
>> >> -    const paddr_t mem_start = info->mem.bank[0].start;
>> >> -    const paddr_t mem_size = info->mem.bank[0].size;
>> >> -    const paddr_t mem_end = mem_start + mem_size;
>> >> -    const paddr_t kernel_size = kernel_end - kernel_start;
>> >> +    unsigned int i, min_i = -1;
>> >> +    bool_t same_bank = false;
>> >> +
>> >> +    paddr_t mem_start, mem_end, mem_size;
>> >> +    paddr_t kernel_size;
>> >>
>> >>      paddr_t addr;
>> >>
>> >> -    if ( total + kernel_size > mem_size )
>> >> -        panic("Not enough memory in the first bank for the dtb+initrd");
>> >> +    kernel_size = kernel_end - kernel_start;
>> >> +
>> >> +    for ( i = 0; i < info->mem.nr_banks; i++ )
>> >> +    {
>> >> +        mem_start = info->mem.bank[i].start;
>> >> +        mem_size = info->mem.bank[i].size;
>> >> +        mem_end = mem_start + mem_size - 1;
>> >> +
>> >> +        if ( (kernel_end > mem_start) && (kernel_end <= mem_end) )
>> >> +            same_bank = true;
>> >> +        else
>> >> +            same_bank = false;
>> >> +
>> >> +        if ( same_bank && ((total + kernel_size) < mem_size) )
>> >> +            min_i = i;
>> >> +        else if ( (!same_bank) && (total < mem_size) )
>> >> +            min_i = i;
>> >> +        else
>> >> +            continue;
>> >
>> > What is all this doing?
>>
>> Search through the banks for the bank that fits the initrd and dtb.
>> Calculation is slightly different depending on whether I ended up
>> using the same bank as the one used by the kernel or not. ( as
>> mentioned previously, I don't just blindly choose the first bank for
>> the kernel. I search through all of the banks for the lowest banks -
>> address-wise - ).
>
> Where is the address comparison before assigning min_i?
>
>>  In the same_bank case, I just use the old
>> calculations that was already there in the code, however in the
>> !same_bank case I just start at the end of the bank.
>
> Would it be clearer to do
>         if ( dtb+initrd fits in kernel's back )
>                 ok
>         else
>                 search
> ?
>
> There is also a requirement that the dtb and initrd are within certain
> ranges of the start of RAM (see the booting.txt and Booting in
> Documentation/arm*/), does this implement this? It doesn't look to be
> considered during the search or when placing in the !same_bank case.
>
> This would all be simpler if we sorted the banks too. Which is another
> argument for doing so IMHO.
>
>> >> -        load_end = info->mem.bank[0].start + info->mem.bank[0].size;
>> >> -        load_end = MIN(info->mem.bank[0].start + MB(128), load_end);
>> >> +        /*
>> >> +         * Load kernel at the lowest possible bank
>> >> +         * ( check commit 6c21cb36e263de2db8716b477157a5b6cd531e1e for reason behind that )
>> >
>> > That commit says nothing about loading in the lowest possible bank,
>> > though. If there is some relevant factor which is worth commenting on
>> > please do so directly.
>>
>> Loading at the lowest bank is safer because of the:
>> "The current solution in Linux assuming that the delta physical
>> address - virtual address is always negative".
>> This delta is being calculated based on where the kernel was loaded (
>> using "adr" to find physical address ).
>> So loading it as early as possible is a good idea to avoid this problem.
>
> I think this problem is now fixed upstream, the intention was to
> eventually revert the workaround (Julien was going to tell me when it
> had gone into stable etc, but this is now a 4.5 era revert candidate).
>
>> However, I'm not quite sure why not just load it at the beginning of
>> the memory then! I think I'll look at booting.txt for that, maybe it's
>> a decompresser limitation or something!
>
> Yes, it's all a bit complicated.
>
>> > Actually now that the kernel is fixed upstream we don't need the
>> > behaviour of that commit at all. Although there are still restrictions
>> > based on load address vs start of RAM (See booting.txt in the kernel
>> > docs)
>> Ah, I see. I'm using an allwinner branch of the kernel (
>> https://github.com/bjzhang/linux-allwinner.git ). I'll take a look at
>> the latest thing.
>
> git://github.com/linux-sunxi/linux-sunxi.git either sunxi-next or
> sunxi-devel branches are pretty good baselines nowadays. I've got an
> outstanding TODO to update the wiki...
Sorry for the delay!

Okay, I'm going to update my system with this tree and refresh the
patch based on your comments and resend this weekend.

>
> Ian.
>



-- 
Karim Allah Ahmed.

Re: [PATCH] create multiple banks for dom0 in 1:1 mapping if necessary

[Julien Grall]

On 01/14/2014 10:51 AM, Ian Campbell wrote:
> I think this problem is now fixed upstream, the intention was to
> eventually revert the workaround (Julien was going to tell me when it
> had gone into stable etc, but this is now a 4.5 era revert candidate).

The patch was merged for 3.13.

-- 
Julien Grall

Re: [PATCH] create multiple banks for dom0 in 1:1 mapping if necessary

[Ian Campbell]

On Fri, 2014-01-24 at 17:11 +0000, Julien Grall wrote:
> On 01/14/2014 10:51 AM, Ian Campbell wrote:
> > I think this problem is now fixed upstream, the intention was to
> > eventually revert the workaround (Julien was going to tell me when it
> > had gone into stable etc, but this is now a 4.5 era revert candidate).
> 
> The patch was merged for 3.13.

Thanks. I don't think we can revert for 4.4 now, but lets revisit for
4.5.

Ian.

[PATCH] Revert "xen/arm: Allocate memory for dom0 from the bottom with the 1:1 Workaround"

[Ian Campbell]

On Mon, 2014-01-27 at 14:06 +0000, Ian Campbell wrote:
> On Fri, 2014-01-24 at 17:11 +0000, Julien Grall wrote:
> > On 01/14/2014 10:51 AM, Ian Campbell wrote:
> > > I think this problem is now fixed upstream, the intention was to
> > > eventually revert the workaround (Julien was going to tell me when it
> > > had gone into stable etc, but this is now a 4.5 era revert candidate).
> > 
> > The patch was merged for 3.13.
> 
> Thanks. I don't think we can revert for 4.4 now, but lets revisit for
> 4.5.

So should we revert

        commit 6c21cb36e263de2db8716b477157a5b6cd531e1e
        Author: Julien Grall <julien.grall@linaro.org>
        Date:   Tue Oct 22 11:51:48 2013 +0100
        
            xen/arm: Allocate memory for dom0 from the bottom with the 1:1 Workaround

Now?

------8<----------------

>From 2e688b06954663f8ff7bc72e5bcf6a823090af9a Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Wed, 2 Apr 2014 13:03:36 +0100
Subject: [PATCH] Revert "xen/arm: Allocate memory for dom0 from the bottom
 with the 1:1 Workaround"

This reverts commit 6c21cb36e263de2db8716b477157a5b6cd531e1e.

The Linux = issue which this works around was fixed in v3.13.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/domain_build.c |   11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 502db84..a0b73d2 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -69,19 +69,12 @@ static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo)
 {
     paddr_t start;
     paddr_t size;
-    struct page_info *pg = NULL;
+    struct page_info *pg;
     unsigned int order = get_order_from_bytes(dom0_mem);
     int res;
     paddr_t spfn;
-    unsigned int bits;
-
-    for ( bits = PAGE_SHIFT + 1; bits < PADDR_BITS; bits++ )
-    {
-        pg = alloc_domheap_pages(d, order, MEMF_bits(bits));
-        if ( pg != NULL )
-            break;
-    }
 
+    pg = alloc_domheap_pages(d, order, 0);
     if ( !pg )
         panic("Failed to allocate contiguous memory for dom0");
 
-- 
1.7.10.4

Re: [PATCH] create multiple banks for dom0 in 1:1 mapping if necessary

[Ian Campbell]

Hi Karim,

On Fri, 2014-01-24 at 10:21 +0000, karim.allah.ahmed@gmail.com wrote:
> Sorry for the delay!
> 
> Okay, I'm going to update my system with this tree and refresh the
> patch based on your comments and resend this weekend.

Do you still plan to work on this? 4.4 out so 4.5 development is in full
swing, it would be nice to get this issue resolved.

Ian.

Re: [PATCH] Revert "xen/arm: Allocate memory for dom0 from the bottom with the 1:1 Workaround"

[Julien Grall]

On 04/02/2014 01:04 PM, Ian Campbell wrote:
> On Mon, 2014-01-27 at 14:06 +0000, Ian Campbell wrote:
>> On Fri, 2014-01-24 at 17:11 +0000, Julien Grall wrote:
>>> On 01/14/2014 10:51 AM, Ian Campbell wrote:
>>>> I think this problem is now fixed upstream, the intention was to
>>>> eventually revert the workaround (Julien was going to tell me when it
>>>> had gone into stable etc, but this is now a 4.5 era revert candidate).
>>>
>>> The patch was merged for 3.13.
>>
>> Thanks. I don't think we can revert for 4.4 now, but lets revisit for
>> 4.5.
> 
> So should we revert
> 
>         commit 6c21cb36e263de2db8716b477157a5b6cd531e1e
>         Author: Julien Grall <julien.grall@linaro.org>
>         Date:   Tue Oct 22 11:51:48 2013 +0100
>         
>             xen/arm: Allocate memory for dom0 from the bottom with the 1:1 Workaround
> 
> Now?

Sorry, I completely forgot this stuff. Let me try on the arndale with
this patch.

Regards,

-- 
Julien Grall

Re: [PATCH] Revert "xen/arm: Allocate memory for dom0 from the bottom with the 1:1 Workaround"

[Julien Grall]

On 04/02/2014 01:04 PM, Ian Campbell wrote:
> On Mon, 2014-01-27 at 14:06 +0000, Ian Campbell wrote:
>> On Fri, 2014-01-24 at 17:11 +0000, Julien Grall wrote:
>>> On 01/14/2014 10:51 AM, Ian Campbell wrote:
>>>> I think this problem is now fixed upstream, the intention was to
>>>> eventually revert the workaround (Julien was going to tell me when it
>>>> had gone into stable etc, but this is now a 4.5 era revert candidate).
>>>
>>> The patch was merged for 3.13.
>>
>> Thanks. I don't think we can revert for 4.4 now, but lets revisit for
>> 4.5.
> 
> So should we revert
> 
>         commit 6c21cb36e263de2db8716b477157a5b6cd531e1e
>         Author: Julien Grall <julien.grall@linaro.org>
>         Date:   Tue Oct 22 11:51:48 2013 +0100
>         
>             xen/arm: Allocate memory for dom0 from the bottom with the 1:1 Workaround
> 
> Now?

Reverting this commit doesn't change anything in the memory allocation.
I'm sure why...

Anyway it works on the arndale! I will have to update the wiki page (the
instructions are for Linux 3.10).

> ------8<----------------
> 
> From 2e688b06954663f8ff7bc72e5bcf6a823090af9a Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Wed, 2 Apr 2014 13:03:36 +0100
> Subject: [PATCH] Revert "xen/arm: Allocate memory for dom0 from the bottom
>  with the 1:1 Workaround"
> 
> This reverts commit 6c21cb36e263de2db8716b477157a5b6cd531e1e.
> 
> The Linux = issue which this works around was fixed in v3.13.

Linux commit: f52bb722547f43caeaecbcc62db9f3c3b80ead9b "ARM: mm: Correct
virt_to_phys patching for 64 bit physical addresses"

With the commit mentioned:
Acked-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

Re: [PATCH] Revert "xen/arm: Allocate memory for dom0 from the bottom with the 1:1 Workaround"

[Ian Campbell]

On Wed, 2014-04-02 at 14:21 +0100, Julien Grall wrote:

> Linux commit: f52bb722547f43caeaecbcc62db9f3c3b80ead9b "ARM: mm: Correct
> virt_to_phys patching for 64 bit physical addresses"
> 
> With the commit mentioned:

Done and applied, thanks.

> Acked-by: Julien Grall <julien.grall@linaro.org>
> 
> Regards,
> 

Re: [PATCH] create multiple banks for dom0 in 1:1 mapping if necessary

[karim.allah.ahmed]

On Wed, Apr 2, 2014 at 1:05 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> Hi Karim,
>
> On Fri, 2014-01-24 at 10:21 +0000, karim.allah.ahmed@gmail.com wrote:
>> Sorry for the delay!
>>
>> Okay, I'm going to update my system with this tree and refresh the
>> patch based on your comments and resend this weekend.
>
> Do you still plan to work on this? 4.4 out so 4.5 development is in full
> swing, it would be nice to get this issue resolved.
Ooops! I got swamped with work and completely forgot about this patch :)
Okay, I'll get my hands on the Cubieboard again and resubmit the patch.

>
> Ian.
>



-- 
Karim Allah Ahmed.

Re: [PATCH] create multiple banks for dom0 in 1:1 mapping if necessary

[Ian Campbell]

On Fri, 2014-04-04 at 16:04 +0100, karim.allah.ahmed@gmail.com wrote:
> On Wed, Apr 2, 2014 at 1:05 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > Hi Karim,
> >
> > On Fri, 2014-01-24 at 10:21 +0000, karim.allah.ahmed@gmail.com wrote:
> >> Sorry for the delay!
> >>
> >> Okay, I'm going to update my system with this tree and refresh the
> >> patch based on your comments and resend this weekend.
> >
> > Do you still plan to work on this? 4.4 out so 4.5 development is in full
> > swing, it would be nice to get this issue resolved.
> Ooops! I got swamped with work and completely forgot about this patch :)
> Okay, I'll get my hands on the Cubieboard again and resubmit the patch.

Brilliant, thanks!

FYI http://thread.gmane.org/gmane.comp.emulators.xen.devel/194383
changes some bits in this area -- likely they will go into staging next
week.

Ian.

Re: [PATCH] create multiple banks for dom0 in 1:1 mapping if necessary

[Ian Campbell]

On Fri, 2014-04-04 at 16:04 +0100, karim.allah.ahmed@gmail.com wrote:
> On Wed, Apr 2, 2014 at 1:05 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > Hi Karim,
> >
> > On Fri, 2014-01-24 at 10:21 +0000, karim.allah.ahmed@gmail.com wrote:
> >> Sorry for the delay!
> >>
> >> Okay, I'm going to update my system with this tree and refresh the
> >> patch based on your comments and resend this weekend.
> >
> > Do you still plan to work on this? 4.4 out so 4.5 development is in full
> > swing, it would be nice to get this issue resolved.
> Ooops! I got swamped with work and completely forgot about this patch :)
> Okay, I'll get my hands on the Cubieboard again and resubmit the patch.

FYI I've picked this up and am working on a patch.

Ian.