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

[Shannon Zhao <zhaoshenglong@xxxxxxxxxx>]

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@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>>  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()?

Thanks,
-- 
Shannon


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