From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH for-4.6] libxl: move calling libxl__arch_domain_construct_memmap to right place Date: Wed, 5 Aug 2015 11:48:55 +0100 Message-ID: <1438771735.9747.54.camel@citrix.com> References: <1438770198-5581-1-git-send-email-wei.liu2@citrix.com> <1438771134.9747.51.camel@citrix.com> <20150805104342.GH26074@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZMwFp-0005U3-JJ for xen-devel@lists.xenproject.org; Wed, 05 Aug 2015 10:49:01 +0000 In-Reply-To: <20150805104342.GH26074@zion.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: xen-devel@lists.xenproject.org, Ian Jackson , "Chen, Tiejun" List-Id: xen-devel@lists.xenproject.org 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 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 > > > --- > > > Cc: "Chen, Tiejun" > > > Cc: Ian Campbell > > > Cc: Ian Jackson > > > > > > 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,