diff mbox

[v4,16/16] libxl/arm: Add the size of ACPI tables to maxmem

Message ID 1471343113-10652-17-git-send-email-zhaoshenglong@huawei.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shannon Zhao Aug. 16, 2016, 10:25 a.m. UTC
From: Shannon Zhao <shannon.zhao@linaro.org>

While it defines the maximum size of guest ACPI tables in guest
memory layout, here it adds the 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_arm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Julien Grall Aug. 29, 2016, 7:07 p.m. UTC | #1
Hi Shannon,

On 16/08/2016 06:25, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> While it defines the maximum size of guest ACPI tables in guest
> memory layout, here it adds the 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_arm.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index d436167..75b2589 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -103,6 +103,17 @@ 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_info *const info = &d_config->b_info;
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +
> +    /* Add the size of ACPI tables to maxmem if ACPI is enabled for guest. */
> +    if (libxl_defbool_val(info->acpi) &&
> +        xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
> +        LIBXL_MAXMEM_CONSTANT + GUEST_ACPI_SIZE / 1024) < 0) {

Why can't we use the estimate size here? It would be better than 
increasing by a constant again the max size (I doubt the ACPI tables 
will be 2MB every time).

Also, this looks like quite unsafe. If someone decides to change the 
default size, (s)he would have to replicate the new algo here.

Wei, Ian, do you have any suggestion to avoid duplication?

> +        LOGE(ERROR, "Couldn't set max memory");
> +        return ERROR_FAIL;
> +    }
> +
>      return 0;
>  }
>
>

Regards,
Shannon Zhao Aug. 30, 2016, 1:25 a.m. UTC | #2
On 2016/8/30 3:07, Julien Grall wrote:
> Hi Shannon,
> 
> On 16/08/2016 06:25, Shannon Zhao wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> While it defines the maximum size of guest ACPI tables in guest
>> memory layout, here it adds the 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_arm.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>> index d436167..75b2589 100644
>> --- a/tools/libxl/libxl_arm.c
>> +++ b/tools/libxl/libxl_arm.c
>> @@ -103,6 +103,17 @@ 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_info *const info = &d_config->b_info;
>> +    libxl_ctx *ctx = libxl__gc_owner(gc);
>> +
>> +    /* Add the size of ACPI tables to maxmem if ACPI is enabled for
>> guest. */
>> +    if (libxl_defbool_val(info->acpi) &&
>> +        xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
>> +        LIBXL_MAXMEM_CONSTANT + GUEST_ACPI_SIZE / 1024) < 0) {
> 
> Why can't we use the estimate size here? It would be better than
> increasing by a constant again the max size (I doubt the ACPI tables
> will be 2MB every time).
> 
The estimate action happens after libxl__arch_domain_create(), I think.
So it can't get the estimate size here.

Also, it's better to add the max size so that it doesn't have to change
here when it adds other contents to the ACPI tables.

Thanks,
Wei Liu Aug. 30, 2016, 8:29 a.m. UTC | #3
On Mon, Aug 29, 2016 at 03:07:07PM -0400, Julien Grall wrote:
> Hi Shannon,
> 
> On 16/08/2016 06:25, Shannon Zhao wrote:
> >From: Shannon Zhao <shannon.zhao@linaro.org>
> >
> >While it defines the maximum size of guest ACPI tables in guest
> >memory layout, here it adds the 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_arm.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> >diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> >index d436167..75b2589 100644
> >--- a/tools/libxl/libxl_arm.c
> >+++ b/tools/libxl/libxl_arm.c
> >@@ -103,6 +103,17 @@ 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_info *const info = &d_config->b_info;
> >+    libxl_ctx *ctx = libxl__gc_owner(gc);
> >+
> >+    /* Add the size of ACPI tables to maxmem if ACPI is enabled for guest. */
> >+    if (libxl_defbool_val(info->acpi) &&
> >+        xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
> >+        LIBXL_MAXMEM_CONSTANT + GUEST_ACPI_SIZE / 1024) < 0) {
> 
> Why can't we use the estimate size here? It would be better than increasing
> by a constant again the max size (I doubt the ACPI tables will be 2MB every
> time).
> 
> Also, this looks like quite unsafe. If someone decides to change the default
> size, (s)he would have to replicate the new algo here.
> 
> Wei, Ian, do you have any suggestion to avoid duplication?
> 

Provide a function to calculate the value needed?

> >+        LOGE(ERROR, "Couldn't set max memory");
> >+        return ERROR_FAIL;
> >+    }
> >+
> >     return 0;
> > }
> >
> >
> 
> Regards,
> 
> -- 
> Julien Grall
Shannon Zhao Aug. 30, 2016, 9:20 a.m. UTC | #4
On 2016/8/30 3:07, Julien Grall wrote:
> Hi Shannon,
> 
> On 16/08/2016 06:25, Shannon Zhao wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> While it defines the maximum size of guest ACPI tables in guest
>> memory layout, here it adds the 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_arm.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>> index d436167..75b2589 100644
>> --- a/tools/libxl/libxl_arm.c
>> +++ b/tools/libxl/libxl_arm.c
>> @@ -103,6 +103,17 @@ 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_info *const info = &d_config->b_info;
>> +    libxl_ctx *ctx = libxl__gc_owner(gc);
>> +
>> +    /* Add the size of ACPI tables to maxmem if ACPI is enabled for
>> guest. */
>> +    if (libxl_defbool_val(info->acpi) &&
>> +        xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
>> +        LIBXL_MAXMEM_CONSTANT + GUEST_ACPI_SIZE / 1024) < 0) {
> 
> Why can't we use the estimate size here? It would be better than
> increasing by a constant again the max size (I doubt the ACPI tables
> will be 2MB every time).
> 
Rethink about this. I think it could call the
libxl__estimate_acpi_size() here.

Thanks,
diff mbox

Patch

diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index d436167..75b2589 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -103,6 +103,17 @@  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_info *const info = &d_config->b_info;
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+
+    /* Add the size of ACPI tables to maxmem if ACPI is enabled for guest. */
+    if (libxl_defbool_val(info->acpi) &&
+        xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
+        LIBXL_MAXMEM_CONSTANT + GUEST_ACPI_SIZE / 1024) < 0) {
+        LOGE(ERROR, "Couldn't set max memory");
+        return ERROR_FAIL;
+    }
+
     return 0;
 }