tools/libxl/libxl_create.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
In commit 3802ecbaa9 ("libxl: add helper function to set
device_model_version") an assert was added to
libxl__domain_build_info_setdefault to make sure callers provide
complete info to this function. This unveiled a flaw in the way how
libxl_domain_build_info is passed to libxl. The public API
libxl_domain_need_memory passes an incomplete libxl_domain_build_info to
libxl__domain_build_info_setdefault, which triggers the assert. Prior to
the change above, device_model_version was hardcoded to QEMU_XEN which
lead to the bugs described in that changeset.
A new libxl API would need to be created to fully populate
libxl_domain_build_info with missing defaults. For existing, unchanged
consumers of libxl the assumption about QEMU_XEN needs to be restored
within this function to properly populate the amount of required memory.
Signed-off-by: Olaf Hering <olaf@aepfle.de>
Reported-by: Juergen Gross <jgross@suse.com>
---
tools/libxl/libxl_create.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 89f99f7f44..030851fb28 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -123,6 +123,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
libxl_domain_build_info *b_info)
{
int i, rc;
+ libxl_device_model_version device_model_version;
if (b_info->type != LIBXL_DOMAIN_TYPE_HVM &&
b_info->type != LIBXL_DOMAIN_TYPE_PV &&
@@ -131,7 +132,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
return ERROR_INVAL;
}
- assert(b_info->device_model_version);
+ device_model_version = b_info->device_model_version;
+ if (!device_model_version)
+ device_model_version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
/* Copy deprecated options to it's new position. */
rc = libxl__domain_build_info_copy_deprecated(CTX, b_info);
@@ -149,7 +152,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
if (!b_info->u.hvm.bios)
- switch (b_info->device_model_version) {
+ switch (device_model_version) {
case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
b_info->u.hvm.bios = LIBXL_BIOS_TYPE_ROMBIOS; break;
case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
@@ -160,7 +163,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
}
/* Enforce BIOS<->Device Model version relationship */
- switch (b_info->device_model_version) {
+ switch (device_model_version) {
case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
if (b_info->u.hvm.bios != LIBXL_BIOS_TYPE_ROMBIOS) {
LOG(ERROR, "qemu-xen-traditional requires bios=rombios.");
@@ -186,7 +189,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
}
if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
- b_info->device_model_version !=
+ device_model_version !=
LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL &&
libxl_defbool_val(b_info->device_model_stubdomain)) {
LOG(ERROR,
@@ -259,7 +262,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
if (!b_info->u.hvm.hdtype)
b_info->u.hvm.hdtype = LIBXL_HDTYPE_IDE;
- switch (b_info->device_model_version) {
+ switch (device_model_version) {
case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
switch (b_info->u.hvm.vga.kind) {
case LIBXL_VGA_INTERFACE_TYPE_NONE:
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develOn Thu, May 16, 2019 at 02:50:00PM +0200, Olaf Hering wrote:
> In commit 3802ecbaa9 ("libxl: add helper function to set
> device_model_version") an assert was added to
> libxl__domain_build_info_setdefault to make sure callers provide
> complete info to this function. This unveiled a flaw in the way how
> libxl_domain_build_info is passed to libxl. The public API
> libxl_domain_need_memory passes an incomplete libxl_domain_build_info to
> libxl__domain_build_info_setdefault, which triggers the assert. Prior to
> the change above, device_model_version was hardcoded to QEMU_XEN which
> lead to the bugs described in that changeset.
>
> A new libxl API would need to be created to fully populate
> libxl_domain_build_info with missing defaults. For existing, unchanged
> consumers of libxl the assumption about QEMU_XEN needs to be restored
> within this function to properly populate the amount of required memory.
>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Reported-by: Juergen Gross <jgross@suse.com>
> ---
> tools/libxl/libxl_create.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 89f99f7f44..030851fb28 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -123,6 +123,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> libxl_domain_build_info *b_info)
> {
> int i, rc;
> + libxl_device_model_version device_model_version;
>
> if (b_info->type != LIBXL_DOMAIN_TYPE_HVM &&
> b_info->type != LIBXL_DOMAIN_TYPE_PV &&
> @@ -131,7 +132,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> return ERROR_INVAL;
> }
>
> - assert(b_info->device_model_version);
> + device_model_version = b_info->device_model_version;
> + if (!device_model_version)
> + device_model_version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
This is a bit more code churn than I like.
Like I said, libxl_domain_need_memory is the only public API which takes
b_info. All other callers to libxl__domain_build_info_setdefault should
have d_config to hand. So changing the code here seems overkilled.
Would the following work?
diff --git a/tools/libxl/libxl_mem.c b/tools/libxl/libxl_mem.c
index 448a2af8fd..12af34f70e 100644
--- a/tools/libxl/libxl_mem.c
+++ b/tools/libxl/libxl_mem.c
@@ -457,6 +457,12 @@ int libxl_domain_need_memory(libxl_ctx *ctx,
libxl_domain_build_info_init(b_info);
libxl_domain_build_info_copy(ctx, b_info, b_info_in);
+ /* Compatibility: if we don't set this, build_info_setdefault will
+ * try to access domain_config, which we don't have here.
+ */
+ if (!b_info->device_model_version)
+ b_info->device_model_version = LIBXL_DEVICE_MODEL_XXX;
+
rc = libxl__domain_build_info_setdefault(gc, b_info);
if (rc) goto out;
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Am Thu, 16 May 2019 14:30:13 +0100 schrieb Wei Liu <wei.liu2@citrix.com>: > @@ -457,6 +457,12 @@ int libxl_domain_need_memory(libxl_ctx *ctx, > + if (!b_info->device_model_version) > + b_info->device_model_version = LIBXL_DEVICE_MODEL_XXX; I think this will work and should be applied to unbreak staging. The proposed libxl_domain_config_finalyze(libxl_domain_config*) in the other thread about the regression will most likely be ugly. Something more elegant has to be found. Olaf _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Fri, May 17, 2019 at 10:24:45AM +0200, Olaf Hering wrote: > Am Thu, 16 May 2019 14:30:13 +0100 > schrieb Wei Liu <wei.liu2@citrix.com>: > > > @@ -457,6 +457,12 @@ int libxl_domain_need_memory(libxl_ctx *ctx, > > + if (!b_info->device_model_version) > > + b_info->device_model_version = LIBXL_DEVICE_MODEL_XXX; > > I think this will work and should be applied to unbreak staging. Okay, let me turn this into a patch. Wei. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Am Thu, 16 May 2019 14:30:13 +0100 schrieb Wei Liu <wei.liu2@citrix.com>: > Would the following work? This is not exactly the same because that copy will end up in libxl__domain_set_device_model, and we are back to square #1. Olaf _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Thu, May 16, 2019 at 03:36:01PM +0200, Olaf Hering wrote: > Am Thu, 16 May 2019 14:30:13 +0100 > schrieb Wei Liu <wei.liu2@citrix.com>: > > > Would the following work? > > This is not exactly the same because that copy will end up in > libxl__domain_set_device_model, and we are back to square #1. I'm not sure I follow. libxl_domain_need_memory and its children don't call libxl__domain_set_device_model. Can you clarify? Wei. > > Olaf _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Am Thu, 16 May 2019 14:46:53 +0100 schrieb Wei Liu <wei.liu2@citrix.com>: > Can you clarify? create_domain has a libxl_domain_config on its stack. This is passed to freemem, and libxl_domain_need_memory modifies it as needed. The modification will go through libxl_domain_create_new, do_domain_create, initiate_domain_create to libxl__domain_set_device_model. This function will return right away because device_model_version is already set. Olaf _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Thu, May 16, 2019 at 03:54:55PM +0200, Olaf Hering wrote: > Am Thu, 16 May 2019 14:46:53 +0100 > schrieb Wei Liu <wei.liu2@citrix.com>: > > > Can you clarify? > > create_domain has a libxl_domain_config on its stack. This is passed to > freemem, and libxl_domain_need_memory modifies it as needed. > The modification will go through libxl_domain_create_new, do_domain_create, > initiate_domain_create to libxl__domain_set_device_model. This function > will return right away because device_model_version is already set. Ah, I know where the confusion is now. Note that the b_info used in libxl_domain_need_memory is a copy, not the original one, so whatever modification you do to it won't get passed back to its caller. Notice the call: libxl_domain_build_info_copy(ctx, b_info, b_info_in); And then later: libxl_domain_build_info_dispose(b_info); Does this make sense? Wei. > > Olaf _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Am Thu, 16 May 2019 16:03:55 +0100 schrieb Wei Liu <wei.liu2@citrix.com>: > Does this make sense? Sure, I was looking at the wrong function. Doing it within libxl_domain_need_memory will certainly work. Thanks! Olaf _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.