From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [v7][PATCH 13/16] libxl: construct e820 map with RDM information for HVM guest Date: Fri, 10 Jul 2015 10:18:09 +0100 Message-ID: <1436519889.23508.185.camel@citrix.com> References: <1436420047-25356-1-git-send-email-tiejun.chen@intel.com> <1436420047-25356-14-git-send-email-tiejun.chen@intel.com> <21918.47807.595901.751327@mariner.uk.xensource.com> <559F5AB0.5040506@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <559F5AB0.5040506@intel.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: "Chen, Tiejun" Cc: Wei Liu , Stefano Stabellini , Ian Jackson , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Fri, 2015-07-10 at 13:40 +0800, Chen, Tiejun wrote: > >> tools/libxl/libxl_dom.c | 5 +++ > >> tools/libxl/libxl_internal.h | 24 +++++++++++++ > >> tools/libxl/libxl_x86.c | 83 ++++++++++++++++++++++++++++++++++++++++++++ > > ... > >> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c > >> index 62ef120..41da479 100644 > >> --- a/tools/libxl/libxl_dom.c > >> +++ b/tools/libxl/libxl_dom.c > >> @@ -1004,6 +1004,11 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid, > >> goto out; > >> } > >> > >> + if (libxl__domain_construct_e820(gc, d_config, domid, &args)) { > >> + LOG(ERROR, "setting domain memory map failed"); > >> + goto out; > >> + } > > > > This is platform-independent code, isn't it ? In which case this will > > break the build on ARM, I think. > > > > Would an ARM maintainer please confirm. > > > > I think you're right. I should make this specific to arch since here > we're talking e820. > > So I tried to refactor this patch, This approach looks like it should work, and I think given the point in the release it would be acceptable for 4.6. However long term I think it might make sense to try and reuse one of the existing libxl__arch hooks, i.e. libxl__arch_domain_init_hw_description or libxl__arch_domain_finalise_hw_description. On ARM these are to do with setting the Device Tree Blob, which included the memory map, so it is somewhat morally equivalent to configuring the e820 on x86, I think. Those hooks are only called from libxl__build_pv today, but calling them from libxl__build_hvm seems like it would be good too. In particular I think a call to libxl__arch_domain_finalise_hw_description could be inserted just before xc_hvm_build, which is similar to PV where it precedes xc_dom_build_image, and is where you would want to setup the e820. libxl__arch_domain_init_hw_description I think would still be a NOP on x86, but it should probably go either just after the call to libxl__domain_firmware. Tiejun, would you be willing to commit to refactoring this and the issues which Ian raised in response to #11 and #16 a subsequent clean up series? I don't think it would even need to wait for the freeze to be over to be posted (although it may need to wait to be applied). Ian.