Re: [PATCH for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 09/11/2021 22:29, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
>   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@xxxxxxxxxx>
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