From: Stefano Stabellini <stefano.stabellini@xilinx.com>

allocate_bank_memory can be called with a tot_size of zero. In that
case, don't create an empty memory bank, just return immediately without
error. Otherwise a zero-sized memory bank will be added to the domain
device tree.

Fixes: f2931b4233ec "xen/arm: introduce allocate_memory"
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 xen/arch/arm/domain_build.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 9e92b640cd..578ea80e40 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -395,6 +395,9 @@ static bool __init allocate_bank_memory(struct domain *d,
     struct membank *bank;
     unsigned int max_order = ~0;
 
+    if ( tot_size == 0 )
+        return true;
+
     bank = &kinfo->mem.bank[kinfo->mem.nr_banks];
     bank->start = gfn_to_gaddr(sgfn);
     bank->size = tot_size;
-- 
2.25.1

Hi Stefano,

On 09/11/2021 22:29, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> allocate_bank_memory can be called with a tot_size of zero.

Please add some details how this can be called with tot_size == 0. 
AFAIU, this happens when creating the second bank.

> In that
> case, don't create an empty memory bank, just return immediately without
> error. Otherwise a zero-sized memory bank will be added to the domain
> device tree.

There are actually DTs out with zero-size memory bank (see [1]) and, 
AFAIR, Linux is able to cope with it.

Instead, it looks like the issue is some part of Xen may fall over if 
one of the bank is zero-sized. But from the earlier discussion [2], this 
is just latent. So I think this should be clarified in the commit message.

> 
> Fixes: f2931b4233ec "xen/arm: introduce allocate_memory"
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
>   xen/arch/arm/domain_build.c | 3 +++
>   1 file changed, 3 insertions(+)

Please explain why you would like to include it in 4.16. In particular 
that as I wrote above, Linux is able to cope with zero-size memory bank.

> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 9e92b640cd..578ea80e40 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -395,6 +395,9 @@ static bool __init allocate_bank_memory(struct domain *d,
>       struct membank *bank;
>       unsigned int max_order = ~0;
>   
> +    if ( tot_size == 0 )
> +        return true;

One may argue this is a bug. So I think it would be worth explaining in 
a comment that we can safely ignore empty bank and why.

Cheers,

[1] commit 5a37207df52066efefe419c677b089a654d37afc
Author: Julien Grall <jgrall@amazon.com>
Date:   Fri Sep 18 18:11:16 2020 +0100

     xen/arm: bootfdt: Ignore empty memory bank

     At the moment, Xen will stop processing the Device Tree if a memory
     bank is empty (size == 0).

     Unfortunately, some of the Device Tree (such as on Colibri imx8qxp)
     may contain such a bank. This means Xen will not be able to boot
     properly.

     Relax the check to just ignore the banks. FWIW this also seems to 
be the
     behavior adopted by Linux.

[2] alpine.DEB.2.22.394.2111091423020.440530@ubuntu-linux-20-04-desktop

-- 
Julien Grall

Julien Grall writes ("Re: [PATCH for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero"):
> Instead, it looks like the issue is some part of Xen may fall over if 
> one of the bank is zero-sized. But from the earlier discussion [2], this 
> is just latent. So I think this should be clarified in the commit message.

Yes, please.

Indeed, this is part of:

> Please explain why you would like to include it in 4.16. In particular 
> that as I wrote above, Linux is able to cope with zero-size memory bank.

How is 4.16 bad without this patch ?  And, what might it break ?

What mistakes could have been made in analysis and preparation ?
(In this case the preparation of the patch seems very simple but the
analysis much less so.)

Ian.