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

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

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;

        acpi_extra_memory...
   }

Does this make sense?

One thing I don't quite understand is that in patch 4
libxl__estimate_acpi_size seems to do allocations as well. You might
want to rename that libxl__allocate_acpi_tables or something.

Wei.

> >Then, you can call libxl_retrieve_domain_configuration to get
> >domain_config, then domain_config->build_info, so that you can derive
> >state from it.
> >
> >Feel free to ask more questions.
> >
> >Without such arrangement, ballooning is going to be broken for ARM
> >guests.
> >
> I see.
> 
> Thanks,
> -- 
> Shannon

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