Re: [v7][PATCH 13/16] libxl: construct e820 map with RDM information for HVM guest

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

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.