| Message ID | 1474548753-12596-17-git-send-email-zhaoshenglong@huawei.com |
|---|---|
| State | New |
| Headers | show |
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 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 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 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 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,
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,