Re: [PATCH v6 16/16] libxl/arm: Add the size of ACPI tables to maxmem

[Wei Liu <wei.liu2@citrix.com>]

On Wed, Sep 28, 2016 at 06:11:53AM -0700, Shannon Zhao wrote:
> 
> 
> On 2016/9/28 3:00, Wei Liu wrote:
> >On Tue, Sep 27, 2016 at 11:43:38AM -0700, Shannon Zhao wrote:
> >>
> >>
> >>On 2016/9/27 9:35, Wei Liu wrote:
> >>>On Tue, Sep 27, 2016 at 09:01:00AM -0700, Shannon Zhao wrote:
> >>>>
> >>>>
> >>>>On 2016/9/27 2:41, Wei Liu wrote:
> >>>>>On Mon, Sep 26, 2016 at 02:54:55PM -0700, Shannon Zhao wrote:
> >>>>>>
> >>>>>>
> >>>>>>On 2016/9/22 7:10, Wei Liu wrote:
> >>>>>>>>diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> >>>>>>>>>index 2924629..118beab 100644
> >>>>>>>>>--- a/tools/libxl/libxl_dom.c
> >>>>>>>>>+++ b/tools/libxl/libxl_dom.c
> >>>>>>>>>@@ -408,8 +408,15 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> >>>>>>>>>       }
> >>>>>>>>>   }
> >>>>>>>>>
> >>>>>>>>>+
> >>>>>>>>>+    rc = libxl__arch_memory_constant(gc, info, state);
> >>>>>>>>>+    if (rc < 0) {
> >>>>>>>>>+        LOGE(ERROR, "Couldn't get arch constant memory size");
> >>>>>>>>>+        return ERROR_FAIL;
> >>>>>>>>>+    }
> >>>>>>>>>+
> >>>>>>>>>   if (xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
> >>>>>>>>>-        LIBXL_MAXMEM_CONSTANT) < 0) {
> >>>>>>>>>+        LIBXL_MAXMEM_CONSTANT + rc) < 0) {
> >>>>>>>I think this LIBXL_MAXMEM_CONSTANT should be pushed to your helper
> >>>>>>>function, too.
> >>>>>>>
> >>>>>>>So that, we can have all LIBXL_MAXMEM_CONSTANT removed in libxl
> >>>>>>>functions (see libxl.c and libxl_dom.c)
> >>>>>>>
> >>>>>>If we push LIBXL_MAXMEM_CONSTANT to the libxl_arch_memory_constant and
> >>>>>>remove it from libxl.c, do we need to call libxl_arch_memory_constant there
> >>>>>>in libxl_set_memory_target()?
> >>>>>>
> >>>>>
> >>>>>Yes, we need to call that function everywhere to get consistent results.
> >>>>>That's the reason I asked you to consolidate it to a function.
> >>>>>
> >>>>Well it's a little awkward I think, since in libxl_domain_setmaxmem() and
> >>>>libxl_set_memory_target() it seems it can't get the parameters info and
> >>>>state for libxl__arch_memory_constant().
> >>>>I'm not sure how to solve it. Wei, any suggestion?
> >>>>
> >>>
> >>>Hmm...
> >>>
> >>>The first question is can state be derived from build_info ? From my
> >>>quick skim of the code the answer is likely yes.
> >>>
> >>I'm not familiar with the relationship between these structures and not sure
> >>how to do this. Please give me some suggestion.
> >>
> >
> >Oh, I was just reading the code in your patch series and existing code
> >in libxl_arm.c.  Here is my analysis of the code, please point out any
> >inaccuracy.
> >
> >In your patch that estimates the size of ACPI table(s), xc_config is
> >needed. In particular, you need to know the gic version -- in fact
> >that's the only thing you need to know as far as I can tell.
> >
> >In libxl_arm.c, the gic version is finally saved to  d_config, which
> >means you should be able to later extract that from d_config.
> >
> >But, as I understand it, you can't use  d_config only while *building*
> >the domain, because the gic version might be determined only after the
> >domain is constructed (_NATIVE case). If you want to do so, you need to
> >move some code around, which might or might not be feasible -- I haven't
> >checked.
> >
> >So based on my analysis, it would make sense to have such function:
> >
> >   libxl__arch_extra_memory(gc, d_config)
> >
> >This is the function that is used in libxl_set_memory_target and
> >friends.
> >
> >Obviously x86 would only need to return a constant in that function.
> >
> >Then, in arm implementation:
> >
> >   libxl__get_acpi_size(gc, info, gic_version /* not build_state anymore */)
> >
> >   /* also fix up libxl__estimate_madt_size */
> >
> >
> >   /* this is the function called when constructing the domain etc, only
> >    * in libxl_arm.c */
> >   static acpi_extra_memory(gc, build_info, gic_version)
> >   {
> >        libxl__get_acpi_size...
> >   }
> >
> >   libxl__arch_extra_memory(gc, d_config)
> >   {
> >        gic_version = d_config->..gic_version;
> >
> If user doesn't specify gic_version in xl config, the
> d_config->b_info.arch_arm.gic_version will be LIBXL_GIC_VERSION_DEFAULT, so
> we can't know the exact gic_version which will be constructed later.
> 

First, can you confirm if it really can't be retrieved?

libxl__arch_domain_save_config updates that field after the domain is
constructed, so you might have a determined gic version to hand.

> Since the gic_version is now only used to determine if it should include
> acpi_madt_generic_redistributor size, can we add a function
> libxl__get_acpi_max_size which doesn't care about the gic_version and just
> returns the max acpi size.

Fine by me. But ...

> And this max size is just for setting the target
> maxmem and not for allocating the acpi tables.
> 

please use that everywhere -- including table allocation.

Wei.

> What do you think about this?
> 


> Thanks,
> -- 
> Shannon

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel