Re: [Xen-devel] [PATCH v6 16/16] libxl/arm: Add the size of ACPI tables to maxmem
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,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 16/16] libxl/arm: Add the size of ACPI tables to maxmem
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
Re: [Xen-devel] [PATCH v6 16/16] libxl/arm: Add the size of ACPI tables to maxmem
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,
--
Shannon
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 16/16] libxl/arm: Add the size of ACPI tables to maxmem
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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 16/16] libxl/arm: Add the size of ACPI tables to maxmem
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,
--
Shannon
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 16/16] libxl/arm: Add the size of ACPI tables to maxmem
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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 16/16] libxl/arm: Add the size of ACPI tables to maxmem
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,
--
Shannon
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 16/16] libxl/arm: Add the size of ACPI tables to maxmem
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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 16/16] libxl/arm: Add the size of ACPI tables to maxmem
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 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 16/16] libxl/arm: Add the size of ACPI tables to maxmem
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,
--
Shannon
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 16/16] libxl/arm: Add the size of ACPI tables to maxmem
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, -- Shannon ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 16/16] libxl/arm: Add the size of ACPI tables to maxmem
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 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 16/16] libxl/arm: Add the size of ACPI tables to maxmem
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, -- Shannon ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 16/16] libxl/arm: Add the size of ACPI tables to maxmem
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
> >
> > 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
> > ---
> > 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 0xfee0
> > 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
Re: [Xen-devel] [PATCH v6 16/16] libxl/arm: Add the size of ACPI tables to maxmem
On Thu, Sep 22, 2016 at 08:52:33PM +0800, z00226004 wrote:
> From: Shannon Zhao
>
> 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
> ---
> 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 0xfee0
> 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,
[Xen-devel] [PATCH v6 16/16] libxl/arm: Add the size of ACPI tables to maxmem
From: Shannon Zhao
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
---
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);
+
#if defined(__i386__) || defined(__x86_64__)
#define LAPIC_BASE_ADDRESS 0xfee0
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<<(_
