| Message ID | 1474548753-12596-17-git-send-email-zhaoshenglong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) > LOGE(ERROR, "Couldn't set max memory"); > return ERROR_FAIL; > } > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > index cb6d9e0..8366fee 100644 > --- a/tools/libxl/libxl_internal.h > +++ b/tools/libxl/libxl_internal.h > @@ -128,6 +128,8 @@ > #define ROUNDUP(_val, _order) \ > (((unsigned long)(_val)+(1UL<<(_order))-1) & ~((1UL<<(_order))-1)) > > +#define DIV_ROUNDUP(n, d) (((n) + (d) - 1) / (d)) > + > #define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) > #define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) > > diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c > index f69f3c2..0a86040 100644 > --- a/tools/libxl/libxl_x86.c > +++ b/tools/libxl/libxl_x86.c > @@ -358,6 +358,12 @@ out: > return ret; > } > > +int libxl__arch_memory_constant(libxl__gc *gc, libxl_domain_build_info *info, > + libxl__domain_build_state *state) > +{ > + return 0; > +} > + > int libxl__arch_domain_init_hw_description(libxl__gc *gc, > libxl_domain_build_info *info, > libxl__domain_build_state *state, > -- > 2.0.4 > >
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.
On 2016/9/22 22:32, Wei Liu wrote: > 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. > Ok, thanks for your review. > No need to resend just yet unless there is something substantial that > you need to change. So far do I need to resend this series for comments? Thanks,
On Mon, Sep 26, 2016 at 03:08:43PM +0800, Shannon Zhao wrote: > > > On 2016/9/22 22:32, Wei Liu wrote: > >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. > > > Ok, thanks for your review. > > >No need to resend just yet unless there is something substantial that > >you need to change. > So far do I need to resend this series for comments? No, you don't have to resend this series at the moment. Please address Jan's comment on patch 14 first. Wei. > > Thanks, > -- > Shannon
On 2016/9/26 2:05, Wei Liu wrote: > On Mon, Sep 26, 2016 at 03:08:43PM +0800, Shannon Zhao wrote: >> > >> > >> > On 2016/9/22 22:32, Wei Liu wrote: >>> > >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. >>> > > >> > Ok, thanks for your review. >> > >>> > >No need to resend just yet unless there is something substantial that >>> > >you need to change. >> > So far do I need to resend this series for comments? > No, you don't have to resend this series at the moment. Please address > Jan's comment on patch 14 first. Ok, I see. Do I need to resend the patch or add new one on top of that? Thanks,
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()? Thanks,
On Mon, Sep 26, 2016 at 11:30:53AM -0700, Shannon Zhao wrote: > > > On 2016/9/26 2:05, Wei Liu wrote: > >On Mon, Sep 26, 2016 at 03:08:43PM +0800, Shannon Zhao wrote: > >>> > >>> > >>> On 2016/9/22 22:32, Wei Liu wrote: > >>>> >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. > >>>> > > >>> Ok, thanks for your review. > >>> > >>>> >No need to resend just yet unless there is something substantial that > >>>> >you need to change. > >>> So far do I need to resend this series for comments? > >No, you don't have to resend this series at the moment. Please address > >Jan's comment on patch 14 first. > Ok, I see. Do I need to resend the patch or add new one on top of that? > What Jan said in his reply. Please resend that patch to address his comment. Sorry I didn't make that clear from the beginning. Wei. > Thanks, > -- > Shannon
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. Wei. > Thanks, > -- > Shannon
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? Thanks,
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. 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. Wei. > Thanks, > -- > Shannon
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. > 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,
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
On 2016/9/28 3:00, Wei Liu wrote: > 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; > If user doesn't specify gic_version in xl config, the d_config->b_info.arch_arm.gic_version will be LIBXL_GIC_VERSION_DEFAULT, so we can't know the exact gic_version which will be constructed later. Since the gic_version is now only used to determine if it should include acpi_madt_generic_redistributor size, can we add a function libxl__get_acpi_max_size which doesn't care about the gic_version and just returns the max acpi size. And this max size is just for setting the target maxmem and not for allocating the acpi tables. What do you think about this? Thanks,
On Wed, Sep 28, 2016 at 06:11:53AM -0700, Shannon Zhao wrote: > > > On 2016/9/28 3:00, Wei Liu wrote: > >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; > > > If user doesn't specify gic_version in xl config, the > d_config->b_info.arch_arm.gic_version will be LIBXL_GIC_VERSION_DEFAULT, so > we can't know the exact gic_version which will be constructed later. > First, can you confirm if it really can't be retrieved? libxl__arch_domain_save_config updates that field after the domain is constructed, so you might have a determined gic version to hand. > Since the gic_version is now only used to determine if it should include > acpi_madt_generic_redistributor size, can we add a function > libxl__get_acpi_max_size which doesn't care about the gic_version and just > returns the max acpi size. Fine by me. But ... > And this max size is just for setting the target > maxmem and not for allocating the acpi tables. > please use that everywhere -- including table allocation. Wei. > What do you think about this? > > Thanks, > -- > Shannon
Hi, On 28/09/2016 06:17, Wei Liu wrote: > On Wed, Sep 28, 2016 at 06:11:53AM -0700, Shannon Zhao wrote: >>> 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; >>> >> If user doesn't specify gic_version in xl config, the >> d_config->b_info.arch_arm.gic_version will be LIBXL_GIC_VERSION_DEFAULT, so >> we can't know the exact gic_version which will be constructed later. >> > > First, can you confirm if it really can't be retrieved? > > libxl__arch_domain_save_config updates that field after the domain is > constructed, so you might have a determined gic version to hand. The target memory of the domain is created after the domain has been created. So we should have the correct GIC version is hand. Regards,
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); + #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); + 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) { LOGE(ERROR, "Couldn't set max memory"); return ERROR_FAIL; } diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index cb6d9e0..8366fee 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -128,6 +128,8 @@ #define ROUNDUP(_val, _order) \ (((unsigned long)(_val)+(1UL<<(_order))-1) & ~((1UL<<(_order))-1)) +#define DIV_ROUNDUP(n, d) (((n) + (d) - 1) / (d)) + #define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) #define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c index f69f3c2..0a86040 100644 --- a/tools/libxl/libxl_x86.c +++ b/tools/libxl/libxl_x86.c @@ -358,6 +358,12 @@ out: return ret; } +int libxl__arch_memory_constant(libxl__gc *gc, libxl_domain_build_info *info, + libxl__domain_build_state *state) +{ + return 0; +} + int libxl__arch_domain_init_hw_description(libxl__gc *gc, libxl_domain_build_info *info, libxl__domain_build_state *state,