[Xen-devel] [PATCH v1] libxl: fix device_model_version related assert

Olaf Hering posted 1 patch 1 year, 9 months ago
Failed in applying to current master (apply log)
tools/libxl/libxl_create.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)

[Xen-devel] [PATCH v1] libxl: fix device_model_version related assert

Posted by Olaf Hering 1 year, 9 months ago
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-devel

Re: [Xen-devel] [PATCH v1] libxl: fix device_model_version related assert

Posted by Wei Liu 1 year, 9 months ago
On 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

Re: [Xen-devel] [PATCH v1] libxl: fix device_model_version related assert

Posted by Olaf Hering 1 year, 9 months ago
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

Re: [Xen-devel] [PATCH v1] libxl: fix device_model_version related assert

Posted by Wei Liu 1 year, 9 months ago
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

Re: [Xen-devel] [PATCH v1] libxl: fix device_model_version related assert

Posted by Olaf Hering 1 year, 9 months ago
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

Re: [Xen-devel] [PATCH v1] libxl: fix device_model_version related assert

Posted by Wei Liu 1 year, 9 months ago
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

Re: [Xen-devel] [PATCH v1] libxl: fix device_model_version related assert

Posted by Olaf Hering 1 year, 9 months ago
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

Re: [Xen-devel] [PATCH v1] libxl: fix device_model_version related assert

Posted by Wei Liu 1 year, 9 months ago
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

Re: [Xen-devel] [PATCH v1] libxl: fix device_model_version related assert

Posted by Olaf Hering 1 year, 9 months ago
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