Re: [PATCH for-4.6] tools/libxc: arm: Check the index before accessing the bank

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-09-17 at 18:42 +0100, Julien Grall wrote:
> On 17/09/15 18:36, Julien Grall wrote:
> > When creating a guest with more than 3GB of memory, the 2 banks will be
> > used and the loop with overrunning. The code will fail later on because
> > Xen will deny to populate the region:
> > 
> > domainbuilder: detail: xc_dom_devicetree_mem: called
> > domainbuilder: detail: xc_dom_mem_init: mem 3096 MB, pages 0xc1800
> > pages, 4k each
> > domainbuilder: detail: xc_dom_mem_init: 0xc1800 pages
> > domainbuilder: detail: xc_dom_boot_mem_init: called
> > domainbuilder: detail: set_mode: guest xen-3.0-aarch64, address size 64
> > domainbuilder: detail: xc_dom_malloc            : 14384 kB
> > domainbuilder: detail: populate_guest_memory: populating RAM
> > @0000000040000000-0000000100000000 (3072MB)
> > domainbuilder: detail: populate_one_size: populated 0x3/0x3 entries
> > with shift 18
> > domainbuilder: detail: populate_guest_memory: populating RAM
> > @0000000200000000-0000000201800000 (24MB)
> > domainbuilder: detail: populate_one_size: populated 0xc/0xc entries
> > with shift 9
> > domainbuilder: detail: populate_guest_memory: populating RAM
> > @0000007fad41c000-0007fb39dd42c000 (2141954816MB)
> > domainbuilder: detail: populate_one_size: populated 0x100/0x1e4 entries
> > with shift 0
> > domainbuilder: detail: populate_guest_memory: Not enough RAM
> > 
> > This is because we are currently accessing the bank before checking the
> > validity of the index. AFAICT, on  Debian Jessie, the compiler (gcc
> > 4.9.2) is
> > assuming that it's not necessary to verify the index because it's used
> > before. This is a valid assumption because the operand of && are
> > execute from from left to right.
> > 
> > Re-order the checks to verify the validity of the index before
> > accessing
> > the bank.
> > 
> > The problem has been present since the introduction of the multi-bank
> > feature in commit 45d9867837f099e9eed4189dac5ed39d1fe2ed49 " tools:
> > arm:
> > prepare domain builder for multiple banks of guest RAM".
> 
> Hmmmm I forgot my Signed-off-by :(.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

This is a no brainer for 4.6 (and further) IMHO and with Wei not being
around I shall plan to apply later today unless there are objections.

Ian.

> 
> > ---
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > 
> > This patch is a candidate for Xen 4.6 and backport to Xen 4.5. Without
> > it, it's impossible to boot guest using more than 3GB (limit after
> > which
> > the memory bank is used).
> > ---
> >  tools/libxc/xc_dom_arm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> > index b00d667..aeaba54 100644
> > --- a/tools/libxc/xc_dom_arm.c
> > +++ b/tools/libxc/xc_dom_arm.c
> > @@ -460,7 +460,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
> >          dom->p2m_host[pfn] = INVALID_P2M_ENTRY;
> >  
> >      /* setup initial p2m and allocate guest memory */
> > -    for ( i = 0; dom->rambank_size[i] && i < GUEST_RAM_BANKS; i++ )
> > +    for ( i = 0; i < GUEST_RAM_BANKS && dom->rambank_size[i]; i++ )
> >      {
> >          if ((rc = populate_guest_memory(dom,
> >                                          bankbase[i] >> XC_PAGE_SHIFT,
> > 
> 
>