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

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

On Thu, Sep 22, 2016 at 03:10:46PM +0100, Wei Liu wrote:
> On Thu, Sep 22, 2016 at 08:52:33PM +0800, z00226004 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        |  4 ++++
> >  tools/libxl/libxl_arm.c         | 16 ++++++++++++++++
> >  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         |  9 ++++++++-
> >  tools/libxl/libxl_internal.h    |  2 ++
> >  tools/libxl/libxl_x86.c         |  6 ++++++
> >  8 files changed, 66 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
> > index 8cb9ba7..a066bbd 100644
> > --- a/tools/libxl/libxl_arch.h
> > +++ b/tools/libxl/libxl_arch.h
> > @@ -66,6 +66,10 @@ _hidden
> >  void libxl__arch_domain_build_info_acpi_setdefault(
> >                                          libxl_domain_build_info *b_info);
> >  
> > +_hidden
> > +int libxl__arch_memory_constant(libxl__gc *gc, libxl_domain_build_info *info,
> > +                                libxl__domain_build_state *state);
> 
> I think the prototype should change a bit.
> 
> _hidden
> int libxl__arch_extra_memory(libxl__gc, *gc,
>                              const libxl_domain_build_info *info,
>                              const libxl__domain_build_state *state,
>                              uint64_t *out);
> 
> The important bit is to not overload the return value to carry the
> output value.
> 
> > +
> >  #if defined(__i386__) || defined(__x86_64__)
> >  
> >  #define LAPIC_BASE_ADDRESS  0xfee00000
> > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> > index 913f401..932e674 100644
> > --- a/tools/libxl/libxl_arm.c
> > +++ b/tools/libxl/libxl_arm.c
> > @@ -106,6 +106,22 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
> >      return 0;
> >  }
> >  
> > +int libxl__arch_memory_constant(libxl__gc *gc, libxl_domain_build_info *info,
> > +                                libxl__domain_build_state *state)
> > +{
> > +    int size;
> > +
> > +    if (libxl_defbool_val(info->acpi)) {
> > +        size = libxl__get_acpi_size(gc, info, state);
> > +        if (size < 0)
> > +            return ERROR_FAIL;
> > +
> > +        return DIV_ROUNDUP(size, 1024);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static struct arch_info {
> >      const char *guest_type;
> >      const char *timer_compat;
> > diff --git a/tools/libxl/libxl_arm.h b/tools/libxl/libxl_arm.h
> > index a91ff93..37b1f15 100644
> > --- a/tools/libxl/libxl_arm.h
> > +++ b/tools/libxl/libxl_arm.h
> > @@ -24,6 +24,10 @@ int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
> >                          libxl__domain_build_state *state,
> >                          struct xc_dom_image *dom);
> >  
> > +_hidden
> > +int libxl__get_acpi_size(libxl__gc *gc, libxl_domain_build_info *info,
> > +                         libxl__domain_build_state *state);
> > +
> 
> Same here, don't overload the return value for output.
> 
> >  static inline uint64_t libxl__compute_mpdir(unsigned int cpuid)
> >  {
> >      /*
> > diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c
> > index 9c4005f..0d4092d 100644
> > --- a/tools/libxl/libxl_arm_acpi.c
> > +++ b/tools/libxl/libxl_arm_acpi.c
> > @@ -94,6 +94,26 @@ static int libxl__estimate_madt_size(libxl__gc *gc,
> >      return rc;
> >  }
> >  
> > +int libxl__get_acpi_size(libxl__gc *gc, libxl_domain_build_info *info,
> > +                         libxl__domain_build_state *state)
> > +{
> > +    int size;
> > +
> > +    size = libxl__estimate_madt_size(gc, info, &state->config);
> > +    if (size < 0)
> > +        goto out;
> > +
> > +    size = ROUNDUP(size, 3) +
> > +           ROUNDUP(sizeof(struct acpi_table_rsdp), 3) +
> > +           ROUNDUP(sizeof(struct acpi_table_xsdt), 3) +
> > +           ROUNDUP(sizeof(struct acpi_table_gtdt), 3) +
> > +           ROUNDUP(sizeof(struct acpi_table_fadt), 3) +
> > +           ROUNDUP(sizeof(dsdt_anycpu_arm_len), 3);
> > +
> > +out:
> > +    return size;
> > +}
> > +
> >  static int libxl__estimate_acpi_size(libxl__gc *gc,
> >                                       libxl_domain_build_info *info,
> >                                       struct xc_dom_image *dom,
> > diff --git a/tools/libxl/libxl_arm_no_acpi.c b/tools/libxl/libxl_arm_no_acpi.c
> > index e7f7411..5eeb825 100644
> > --- a/tools/libxl/libxl_arm_no_acpi.c
> > +++ b/tools/libxl/libxl_arm_no_acpi.c
> > @@ -25,6 +25,12 @@ int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
> >      return ERROR_FAIL;
> >  }
> >  
> > +int libxl__get_acpi_size(libxl__gc *gc, libxl_domain_build_info *info,
> > +                         libxl__domain_build_state *state)
> > +{
> > +    return ERROR_FAIL;
> > +}
> > +
> >  /*
> >   * Local variables:
> >   * mode: C
> > 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)
> 

FAOD:

I think all the issues I found so far in this patch and other patch(es)
are mostly cosmetic. I would be happy to accept incremental patches on
top of this series to make those changes.

No need to resend just yet unless there is something substantial that
you need to change.

Wei.

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