Re: [PATCH for-4.6] libxl: move calling libxl__arch_domain_construct_memmap to right place

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

On Wed, 2015-08-05 at 11:43 +0100, Wei Liu wrote:
> On Wed, Aug 05, 2015 at 11:38:54AM +0100, Ian Campbell wrote:
> > On Wed, 2015-08-05 at 11:23 +0100, Wei Liu wrote:
> > > This function was called in the wrong place, because both
> > > libxl__vnuma_build_vmemrange_hvm and xc_hvm_build rely on its output.
> > 
> > What is the effect of this call being in the wrong place? Presumably 
> > one or
> > the other of those functions reaches the wrong conclusion?
> 
> Originally, by the time that function got called, all guest pages were
> already populated.  The end result is E820 map disagrees with what vNUMA
> says and what address ranges memory actually resides, i.e. risk of guest
> accessing region that doesn't have backing pages.

Ouch. This should certainly be explained in the commit message.

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

Although perhaps we should wait for confirmation this fix doesn't regress
RMRR somehow?

> Move the call of said function to the right place -- before the other
> > > two functions which reply on its output.
> > > 
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > > Cc: "Chen, Tiejun" <tiejun.chen@intel.com>
> > > Cc: Ian Campbell <ian.campbell@citrix.com>
> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > > 
> > > Discovered this issue by code inspection. This issue is not 
> > > discovered
> > > by osstest because we don't have hardware or test case to test that
> > > code path.
> > > 
> > > Tiejun, can you confirm this is the right fix? Can you test this
> > > change?
> > > ---
> > >  tools/libxl/libxl_dom.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > > index 5555fea..811f7da 100644
> > > --- a/tools/libxl/libxl_dom.c
> > > +++ b/tools/libxl/libxl_dom.c
> > > @@ -960,6 +960,11 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t 
> > > domid,
> > >          goto out;
> > >      }
> > >  
> > > +    if (libxl__arch_domain_construct_memmap(gc, d_config, domid, 
> > > &args)) 
> > > {
> > > +        LOG(ERROR, "setting domain memory map failed");
> > > +        goto out;
> > > +    }
> > > +
> > >      if (info->num_vnuma_nodes != 0) {
> > >          int i;
> > >  
> > > @@ -997,11 +1002,6 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t 
> > > domid,
> > >          goto out;
> > >      }
> > >  
> > > -    if (libxl__arch_domain_construct_memmap(gc, d_config, domid, 
> > > &args)) 
> > > {
> > > -        LOG(ERROR, "setting domain memory map failed");
> > > -        goto out;
> > > -    }
> > > -
> > >      ret = hvm_build_set_params(ctx->xch, domid, info, state
> > > ->store_port,
> > >                                 &state->store_mfn, state
> > > ->console_port,
> > >                                 &state->console_mfn, state
> > > ->store_domid,