Re: [Xen-devel] [XEN PATCH for-4.13 v3 05/10] libxl: Move shadow_memkb and iommu_memkb defaulting into libxl
On Mon, Oct 14, 2019 at 06:00:23PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v3 05/10] libxl:
> Move shadow_memkb and iommu_memkb defaulting into libxl"):
> > So maybe libxl__domain_build_info_setdefault() should also set a value
> > to iommu_memkb as it does for shadow_memkb? At least, set iommu_memkb=0
> > if still the default.
>
> I think, this.
>
> Ian.
>
> From 379d8eb69f713cccf7eacb4c2a63f4f5fe944255 Mon Sep 17 00:00:00 2001
> From: Ian Jackson
> Date: Mon, 14 Oct 2019 17:59:00 +0100
> Subject: [PATCH] squash! libxl: Move shadow_memkb and iommu_memkb defaulting
> into libxl
>
> ---
> v4: Provide a fallback default for iommu_memkb too, for old callers.
> ---
> tools/libxl/libxl_create.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 7423bedf7d..b3951a2423 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -229,6 +229,10 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> libxl__arch_domain_build_info_setdefault(gc, b_info);
> libxl_defbool_setdefault(_info->dm_restrict, false);
>
> +if (b_info->iommu_memkb == LIBXL_MEMKB_DEFAULT)
> +/* Normally defaulted in libxl__domain_create_info_setdefault */
> +b_info->iommu_memkb = 0;
> +
> switch (b_info->type) {
> case LIBXL_DOMAIN_TYPE_HVM:
> if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
Acked-by: Anthony PERARD
Thanks,
--
Anthony PERARD
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [XEN PATCH for-4.13 v3 05/10] libxl: Move shadow_memkb and iommu_memkb defaulting into libxl
Anthony PERARD writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v3 05/10] libxl:
Move shadow_memkb and iommu_memkb defaulting into libxl"):
> So maybe libxl__domain_build_info_setdefault() should also set a value
> to iommu_memkb as it does for shadow_memkb? At least, set iommu_memkb=0
> if still the default.
I think, this.
Ian.
From 379d8eb69f713cccf7eacb4c2a63f4f5fe944255 Mon Sep 17 00:00:00 2001
From: Ian Jackson
Date: Mon, 14 Oct 2019 17:59:00 +0100
Subject: [PATCH] squash! libxl: Move shadow_memkb and iommu_memkb defaulting
into libxl
---
v4: Provide a fallback default for iommu_memkb too, for old callers.
---
tools/libxl/libxl_create.c | 4
1 file changed, 4 insertions(+)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 7423bedf7d..b3951a2423 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -229,6 +229,10 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
libxl__arch_domain_build_info_setdefault(gc, b_info);
libxl_defbool_setdefault(_info->dm_restrict, false);
+if (b_info->iommu_memkb == LIBXL_MEMKB_DEFAULT)
+/* Normally defaulted in libxl__domain_create_info_setdefault */
+b_info->iommu_memkb = 0;
+
switch (b_info->type) {
case LIBXL_DOMAIN_TYPE_HVM:
if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [XEN PATCH for-4.13 v3 05/10] libxl: Move shadow_memkb and iommu_memkb defaulting into libxl
Anthony PERARD writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v3 05/10] libxl:
Move shadow_memkb and iommu_memkb defaulting into libxl"):
> There's probably something else that is needed to be done for user of
> the pre-4.13 API. If they call libxl_domain_need_memory_0x041200, there
> is nothing that set iommu_memkb, so the default value stays at -1 and
> libxl_domain_need_memory_0x041200 returns a value that is lower that
> "target_memkb + shadow_memkb". Then, when libxl create the domain, it
> still keep iommu_memkb==-1, because the old API as been used.
>
> I tried to have xl call the 4.12 API and create a guest without
> passthrough, and QEMU crashed (assert() failure).
>
> So maybe libxl__domain_build_info_setdefault() should also set a value
> to iommu_memkb as it does for shadow_memkb? At least, set iommu_memkb=0
> if still the default.
I think you are right. Not sure how I missed that.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [XEN PATCH for-4.13 v3 05/10] libxl: Move shadow_memkb and iommu_memkb defaulting into libxl
On Fri, Oct 11, 2019 at 05:55:44PM +0100, Ian Jackson wrote: > For callers which call libxl_domain_need_memory, and request an old > pre-4.13 libxl API, and which leave one of these memkb settings unset, > we take special measures to preserve the old behaviour. > > This means that they don't get the additional iommu memory and are at > risk of the domain running out of memory as a result of f89f555827a6 > "remove late (on-demand) construction of IOMMU page tables". But this > is no worse than the state just after f89f555827a6, which already > broke such callers in that way. This is perhaps justifiable because > of the API stability warning next to libxl_domain_need_memory. There's probably something else that is needed to be done for user of the pre-4.13 API. If they call libxl_domain_need_memory_0x041200, there is nothing that set iommu_memkb, so the default value stays at -1 and libxl_domain_need_memory_0x041200 returns a value that is lower that "target_memkb + shadow_memkb". Then, when libxl create the domain, it still keep iommu_memkb==-1, because the old API as been used. I tried to have xl call the 4.12 API and create a guest without passthrough, and QEMU crashed (assert() failure). So maybe libxl__domain_build_info_setdefault() should also set a value to iommu_memkb as it does for shadow_memkb? At least, set iommu_memkb=0 if still the default. iommu_memkb is new in 4.13, user of the pre-4.13 API will not set it. -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [XEN PATCH for-4.13 v3 05/10] libxl: Move shadow_memkb and iommu_memkb defaulting into libxl
On Fri, Oct 11, 2019 at 05:55:44PM +0100, Ian Jackson wrote: > Defaulting is supposed to be done by libxl. So these calculations > should be here in libxl. libxl__domain_config_setdefault has all the > necessary information including the values of max_memkb and max_vcpus. > > The overall functional effect depends on the caller: > > For xl, no change. The code moves from xl to libxl. > > For callers who set one or both shadow_memkb and iommu_memkb (whether > from libxl_get_required_shadow_memory or otherwise) before calling > libxl_domain_need_memory (any version): the new code will leave their > setting(s) unchanged. > > For callers who do not call libxl_domain_need_memory at all, and who > fail to set one of these memory values: now they are both are properly > set. The shadow and iommu memory to be properly accounted for as > intended. > > For callers which call libxl_domain_need_memory and request the > current API (4.13) or which track libxl, the default values are also > now right and everything works as intended. > > For callers which call libxl_domain_need_memory, and request an old > pre-4.13 libxl API, and which leave one of these memkb settings unset, > we take special measures to preserve the old behaviour. > > This means that they don't get the additional iommu memory and are at > risk of the domain running out of memory as a result of f89f555827a6 > "remove late (on-demand) construction of IOMMU page tables". But this > is no worse than the state just after f89f555827a6, which already > broke such callers in that way. This is perhaps justifiable because > of the API stability warning next to libxl_domain_need_memory. > > An alternative would be to drop the special-casing of these callers. > That would cause a discrepancy between libxl_domain_need_memory and > libxl_domain_create: the former would not include the iommu memory and > the latter would. That seems worse, but it's debateable. > > Signed-off-by: Ian Jackson > --- > v2: Replace _Bool with bool > Fix logic sense in ok_to_default_memkb_in_create Reviewed-by: Anthony PERARD -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [XEN PATCH for-4.13 v3 05/10] libxl: Move shadow_memkb and iommu_memkb defaulting into libxl
Defaulting is supposed to be done by libxl. So these calculations
should be here in libxl. libxl__domain_config_setdefault has all the
necessary information including the values of max_memkb and max_vcpus.
The overall functional effect depends on the caller:
For xl, no change. The code moves from xl to libxl.
For callers who set one or both shadow_memkb and iommu_memkb (whether
from libxl_get_required_shadow_memory or otherwise) before calling
libxl_domain_need_memory (any version): the new code will leave their
setting(s) unchanged.
For callers who do not call libxl_domain_need_memory at all, and who
fail to set one of these memory values: now they are both are properly
set. The shadow and iommu memory to be properly accounted for as
intended.
For callers which call libxl_domain_need_memory and request the
current API (4.13) or which track libxl, the default values are also
now right and everything works as intended.
For callers which call libxl_domain_need_memory, and request an old
pre-4.13 libxl API, and which leave one of these memkb settings unset,
we take special measures to preserve the old behaviour.
This means that they don't get the additional iommu memory and are at
risk of the domain running out of memory as a result of f89f555827a6
"remove late (on-demand) construction of IOMMU page tables". But this
is no worse than the state just after f89f555827a6, which already
broke such callers in that way. This is perhaps justifiable because
of the API stability warning next to libxl_domain_need_memory.
An alternative would be to drop the special-casing of these callers.
That would cause a discrepancy between libxl_domain_need_memory and
libxl_domain_create: the former would not include the iommu memory and
the latter would. That seems worse, but it's debateable.
Signed-off-by: Ian Jackson
---
v2: Replace _Bool with bool
Fix logic sense in ok_to_default_memkb_in_create
---
tools/libxl/libxl_create.c | 40
tools/libxl/libxl_internal.h | 3 +++
tools/libxl/libxl_mem.c | 4
tools/xl/xl_parse.c | 15 ++-
4 files changed, 49 insertions(+), 13 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index fd8bb22be9..a1b00a8aef 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -250,6 +250,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
switch (b_info->type) {
case LIBXL_DOMAIN_TYPE_HVM:
if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
+/* Normally defaulted in libxl__domain_create_info_setdefault */
b_info->shadow_memkb = 0;
if (b_info->u.hvm.mmio_hole_memkb == LIBXL_MEMKB_DEFAULT)
b_info->u.hvm.mmio_hole_memkb = 0;
@@ -395,6 +396,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
b_info->video_memkb = 0;
if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
+/* Normally defaulted in libxl__domain_create_info_setdefault */
b_info->shadow_memkb = 0;
if (b_info->u.pv.slack_memkb == LIBXL_MEMKB_DEFAULT)
b_info->u.pv.slack_memkb = 0;
@@ -862,6 +864,30 @@ static void domcreate_destruction_cb(libxl__egc *egc,
libxl__domain_destroy_state *dds,
int rc);
+static bool ok_to_default_memkb_in_create(libxl__gc *gc)
+{
+/*
+ * This is a fudge. We are trying to find whether the caller
+ * calls the old version of libxl_domain_need_memory. If they do
+ * then, because it only gets the b_info, and because it can't
+ * update the b_info (because it's const), it will base its
+ * calculations on defaulting shadow_memkb and iommu_memkb to 0
+ * In that case we probably shouldn't default them differently
+ * during libxl_domain_create.
+ *
+ * The result is that the behaviour with old callers is the same
+ * as in 4.13: no additional memory is allocated for shadow and
+ * iommu (unless the caller set shadow_memkb, eg from a call to
+ * libxl_get_required_shadow_memory).
+ */
+return !CTX->libxl_domain_need_memory_0x041200_called ||
+CTX->libxl_domain_need_memory_called;
+/*
+ * Treat mixed callers as new callers. Presumably they know what
+ * they are doing.
+ */
+}
+
int libxl__domain_config_setdefault(libxl__gc *gc,
libxl_domain_config *d_config,
uint32_t domid)
@@ -974,6 +1000,20 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
goto error_out;
}
+if (d_config->b_info.shadow_memkb == LIBXL_MEMKB_DEFAULT
+&& ok_to_default_memkb_in_create(gc))
+d_config->b_info.shadow_memkb =
+libxl_get_required_shadow_memory(d_config->b_info.max_memkb,
+
