| Message ID | 20211109222944.531368-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero | expand |
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 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.
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;