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

From: Wei Liu <wei.liu2@citrix.com>
To: Julien Grall <julien.grall@arm.com>
Cc: sstabellini@kernel.org, wei.liu2@citrix.com,
	ian.jackson@eu.citrix.com, peter.huangpeng@huawei.com,
	xen-devel@lists.xen.org, shannon.zhao@linaro.org,
	Shannon Zhao <zhaoshenglong@huawei.com>,
	boris.ostrovsky@oracle.com
Subject: Re: [PATCH v5 16/16] libxl/arm: Add the size of ACPI tables to maxmem
Date: Thu, 15 Sep 2016 11:46:51 +0100	[thread overview]
Message-ID: <20160915104651.GJ15958@citrix.com> (raw)
In-Reply-To: <1923dc3f-67f0-34ec-f5c2-ffef2a982979@arm.com>

On Tue, Sep 13, 2016 at 11:38:35AM +0100, Julien Grall wrote:
> Hi Shannon,
> 
> On 13/09/16 08:03, Shannon Zhao wrote:
> >
> >
> >On 2016/9/12 23:18, Julien Grall wrote:
> >>Hi Shannon,
> >>
> >>On 02/09/16 03:55, Shannon Zhao wrote:
> >>>From: Shannon Zhao <shannon.zhao@linaro.org>
> >>>
> >>>Here it adds the ACPI tables size to set the target maxmem to avoid
> >>>providing less available memory for guest.
> >>>
> >>>Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> >>>---
> >>> tools/libxl/libxl_arch.h        |  2 +-
> >>> tools/libxl/libxl_arm.c         | 18 +++++++++++++++++-
> >>> tools/libxl/libxl_arm.h         |  4 ++++
> >>> tools/libxl/libxl_arm_acpi.c    | 20 ++++++++++++++++++++
> >>> tools/libxl/libxl_arm_no_acpi.c |  6 ++++++
> >>> tools/libxl/libxl_dom.c         |  2 +-
> >>> tools/libxl/libxl_x86.c         |  2 +-
> >>> 7 files changed, 50 insertions(+), 4 deletions(-)
> >>>
> >>>diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
> >>>index 337061f..d62fa4c 100644
> >>>--- a/tools/libxl/libxl_arch.h
> >>>+++ b/tools/libxl/libxl_arch.h
> >>>@@ -30,7 +30,7 @@ int libxl__arch_domain_save_config(libxl__gc *gc,
> >>> /* arch specific internal domain creation function */
> >>> _hidden
> >>> int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config
> >>>*d_config,
> >>>-               uint32_t domid);
> >>>+                              libxl__domain_build_state *state,
> >>>uint32_t domid);
> >>>
> >>> /* setup arch specific hardware description, i.e. DTB on ARM */
> >>> _hidden
> >>>diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> >>>index e73d65e..c7d4f65 100644
> >>>--- a/tools/libxl/libxl_arm.c
> >>>+++ b/tools/libxl/libxl_arm.c
> >>>@@ -101,8 +101,24 @@ int libxl__arch_domain_save_config(libxl__gc *gc,
> >>> }
> >>>
> >>> int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config
> >>>*d_config,
> >>>-                              uint32_t domid)
> >>>+                              libxl__domain_build_state *state,
> >>>uint32_t domid)
> >>> {
> >>>+    libxl_domain_build_info *const info = &d_config->b_info;
> >>>+    libxl_ctx *ctx = libxl__gc_owner(gc);
> >>>+    int size;
> >>>+
> >>>+    /* Add the size of ACPI tables to maxmem if ACPI is enabled for
> >>>guest. */
> >>>+    if (libxl_defbool_val(info->acpi)) {
> >>>+        size = libxl__get_acpi_size(gc, info, state);
> >>>+        if (size < 0)
> >>>+            return ERROR_FAIL;
> >>>+        if (xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
> >>>+                                LIBXL_MAXMEM_CONSTANT + (size + 1023)
> >>>/ 1024)) {
> >>
> >>I still have some concern about use info->target_memkb +
> >>LIBXL_MAXMEM_CONSTANT here. What if the generic code decide to change
> >>the computation?
> >>
> >>We may forgot to replicate here. My suggestion on the previous version
> >>was to have in the common code
> >>
> >>xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
> >>LIBXL_MAXMEM_CONSTANT + libxl__arch_memory_constant());
> >>
> >>Or a similar name.
> >>
> >Just to confirm, do you mean that having a arch function to get the size
> >each arch needs and adding the size in libxl__build_pre()?
> 
> That's correct. Wei, Ian, do you have any opinions on this?
> 

I'm fine with that.

Wei.

> Regards,
> 
> -- 
> Julien Grall

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