[Xen-devel] [PATCH-for-4.13] libxl: choose an appropriate default for passthrough...

[Xen-devel] [PATCH-for-4.13] libxl: choose an appropriate default for passthrough...

[Paul Durrant]

...if there is no IOMMU or it is globally disabled.

Without this patch, the following assertion may be hit:

xl: libxl_create.c:589: libxl__domain_make: Assertion `info->passthrough != LIBXL_PASSTHROUGH_ENABLED' failed.

This is because libxl__domain_create_info_setdefault() currently only sets
an appropriate value for 'passthrough' in the case that 'cap_hvm_directio'
is true, which is not the case unless an IOMMU is present and enabled in
the system. This is normally masked by xl choosing a default value, but
that will not happen if xl is not used (e.g. when using libvirt) or when
a stub domain is being created.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Juergen Gross <jgross@suse.com>
---
 tools/libxl/libxl_create.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index b58e030376..3bdb6c51b6 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -68,7 +68,11 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
         c_info->passthrough = ((c_info->type == LIBXL_DOMAIN_TYPE_PV) ||
                                !info.cap_iommu_hap_pt_share) ?
             LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
-    }
+    } else if (!info.cap_hvm_directio)
+        c_info->passthrough = LIBXL_PASSTHROUGH_DISABLED;
+
+    /* An explicit setting should now have been chosen */
+    assert(c_info->passthrough != LIBXL_PASSTHROUGH_ENABLED);
 
     return 0;
 }
-- 
2.20.1.2.gb21ebb671


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH-for-4.13] libxl: choose an appropriate default for passthrough...

[Jürgen Groß]

On 01.10.19 11:12, Paul Durrant wrote:
> ...if there is no IOMMU or it is globally disabled.
> 
> Without this patch, the following assertion may be hit:
> 
> xl: libxl_create.c:589: libxl__domain_make: Assertion `info->passthrough != LIBXL_PASSTHROUGH_ENABLED' failed.
> 
> This is because libxl__domain_create_info_setdefault() currently only sets
> an appropriate value for 'passthrough' in the case that 'cap_hvm_directio'
> is true, which is not the case unless an IOMMU is present and enabled in
> the system. This is normally masked by xl choosing a default value, but
> that will not happen if xl is not used (e.g. when using libvirt) or when
> a stub domain is being created.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH-for-4.13] libxl: choose an appropriate default for passthrough...

[Ian Jackson]

Paul Durrant writes ("[PATCH-for-4.13] libxl: choose an appropriate default for passthrough..."):
> ...if there is no IOMMU or it is globally disabled.
> 
> Without this patch, the following assertion may be hit:
> 
> xl: libxl_create.c:589: libxl__domain_make: Assertion `info->passthrough != LIBXL_PASSTHROUGH_ENABLED' failed.
> 
> This is because libxl__domain_create_info_setdefault() currently only sets
> an appropriate value for 'passthrough' in the case that 'cap_hvm_directio'
> is true, which is not the case unless an IOMMU is present and enabled in
> the system. This is normally masked by xl choosing a default value, but
> that will not happen if xl is not used (e.g. when using libvirt) or when
> a stub domain is being created.

It's weird that after this patch "enabled" can mean DISABLED.  Surely
if you say `passthrough="enabled"' and the host has no PT support (eg
it's disabled in the bios) it should fail ?

Normally libxl config options have an "unknown" or "default" option.

Also it is anomalous that xl is doing the complex work of choosing a
default.  I think all the complex code

+    switch (c_info->passthrough) {
+    case LIBXL_PASSTHROUGH_ENABLED:

in xl_parse.c should be in libxl.  (Some of it is there already.)

I'm sorry that I wasn't didn't review babde47a3fed...

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH-for-4.13] libxl: choose an appropriate default for passthrough...

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 01 October 2019 11:39
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Anthony Perard <anthony.perard@citrix.com>;
> Juergen Gross <jgross@suse.com>
> Subject: Re: [PATCH-for-4.13] libxl: choose an appropriate default for passthrough...
> 
> Paul Durrant writes ("[PATCH-for-4.13] libxl: choose an appropriate default for passthrough..."):
> > ...if there is no IOMMU or it is globally disabled.
> >
> > Without this patch, the following assertion may be hit:
> >
> > xl: libxl_create.c:589: libxl__domain_make: Assertion `info->passthrough !=
> LIBXL_PASSTHROUGH_ENABLED' failed.
> >
> > This is because libxl__domain_create_info_setdefault() currently only sets
> > an appropriate value for 'passthrough' in the case that 'cap_hvm_directio'
> > is true, which is not the case unless an IOMMU is present and enabled in
> > the system. This is normally masked by xl choosing a default value, but
> > that will not happen if xl is not used (e.g. when using libvirt) or when
> > a stub domain is being created.
> 
> It's weird that after this patch "enabled" can mean DISABLED. Surely
> if you say `passthrough="enabled"' and the host has no PT support (eg
> it's disabled in the bios) it should fail ?

Indeed, and xl will do exactly that. 

> 
> Normally libxl config options have an "unknown" or "default" option.
> 
> Also it is anomalous that xl is doing the complex work of choosing a
> default.  I think all the complex code
> 
> +    switch (c_info->passthrough) {
> +    case LIBXL_PASSTHROUGH_ENABLED:
> 
> in xl_parse.c should be in libxl.  (Some of it is there already.)
> 
> I'm sorry that I wasn't didn't review babde47a3fed...
> 

So, would you advocate an 'unknown' value then? That's essentially just a rename operation on 'enabled'.

The code in xl_parse.c is there for a reason though; the appropriate amount of extra memory for the IOMMU page tables has to be determined there because the 'build' parts of libxl seem to be largely firewalled from the 'create' parts and thus the relevant information is not available to decide the appropriate overhead.

  Paul

> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH-for-4.13] libxl: choose an appropriate default for passthrough...

[Paul Durrant]

Ping? Can I get a response on this (w.r.t. 'enabled' vs. 'unknown')
before doing a v2? This issue is currently blocking a push, I believe.

On Tue, 1 Oct 2019 at 11:48, Paul Durrant <Paul.Durrant@citrix.com> wrote:
>
> > -----Original Message-----
> > From: Ian Jackson <ian.jackson@citrix.com>
> > Sent: 01 October 2019 11:39
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Anthony Perard <anthony.perard@citrix.com>;
> > Juergen Gross <jgross@suse.com>
> > Subject: Re: [PATCH-for-4.13] libxl: choose an appropriate default for passthrough...
> >
> > Paul Durrant writes ("[PATCH-for-4.13] libxl: choose an appropriate default for passthrough..."):
> > > ...if there is no IOMMU or it is globally disabled.
> > >
> > > Without this patch, the following assertion may be hit:
> > >
> > > xl: libxl_create.c:589: libxl__domain_make: Assertion `info->passthrough !=
> > LIBXL_PASSTHROUGH_ENABLED' failed.
> > >
> > > This is because libxl__domain_create_info_setdefault() currently only sets
> > > an appropriate value for 'passthrough' in the case that 'cap_hvm_directio'
> > > is true, which is not the case unless an IOMMU is present and enabled in
> > > the system. This is normally masked by xl choosing a default value, but
> > > that will not happen if xl is not used (e.g. when using libvirt) or when
> > > a stub domain is being created.
> >
> > It's weird that after this patch "enabled" can mean DISABLED. Surely
> > if you say `passthrough="enabled"' and the host has no PT support (eg
> > it's disabled in the bios) it should fail ?
>
> Indeed, and xl will do exactly that.
>
> >
> > Normally libxl config options have an "unknown" or "default" option.
> >
> > Also it is anomalous that xl is doing the complex work of choosing a
> > default.  I think all the complex code
> >
> > +    switch (c_info->passthrough) {
> > +    case LIBXL_PASSTHROUGH_ENABLED:
> >
> > in xl_parse.c should be in libxl.  (Some of it is there already.)
> >
> > I'm sorry that I wasn't didn't review babde47a3fed...
> >
>
> So, would you advocate an 'unknown' value then? That's essentially just a rename operation on 'enabled'.
>
> The code in xl_parse.c is there for a reason though; the appropriate amount of extra memory for the IOMMU page tables has to be determined there because the 'build' parts of libxl seem to be largely firewalled from the 'create' parts and thus the relevant information is not available to decide the appropriate overhead.
>
>   Paul
>
> > Ian.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH-for-4.13] libxl: choose an appropriate default for passthrough...

[Ian Jackson]

Paul Durrant writes ("Re: [Xen-devel] [PATCH-for-4.13] libxl: choose an appropriate default for passthrough..."):
> Ping? Can I get a response on this (w.r.t. 'enabled' vs. 'unknown')
> before doing a v2? This issue is currently blocking a push, I believe.

I definitely think we should introduce "unknown".

> > So, would you advocate an 'unknown' value then? That's essentially just a rename operation on 'enabled'.

I think we probably want "enabled" as well but that can wait.  If for
now you rename "unknown" that will do.

> > The code in xl_parse.c is there for a reason though; the appropriate amount of extra memory for the IOMMU page tables has to be determined there because the 'build' parts of libxl seem to be largely firewalled from the 'create' parts and thus the relevant information is not available to decide the appropriate overhead.

I want to look into this more deeply.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH-for-4.13 v2 0/2] libxl: fix assertion failure

[Paul Durrant]

This was originally a single patch, which is now patch #2 of this series.

Paul Durrant (2):
  libxl: replace 'enabled' with 'unknown' in libxl_passthrough
    enumeration
  libxl: choose an appropriate default for passthrough...

 tools/libxl/libxl_create.c  | 10 +++++++---
 tools/libxl/libxl_types.idl |  2 +-
 tools/xl/xl_parse.c         | 26 +++++++++++++++-----------
 3 files changed, 23 insertions(+), 15 deletions(-)
---
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Wei Liu <wl@xen.org>
-- 
2.20.1.2.gb21ebb671


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH-for-4.13 v2 1/2] libxl: replace 'enabled' with 'unknown' in libxl_passthrough enumeration

[Paul Durrant]

This is mostly a cosmetic patch to avoid the default enumeration value
being 'enabled'. The only non-cosmetic parts are in xl_parse.c where it now
becomes necessary to explicitly parse the 'enabled' value for xl.cfg
'passthrough' option, and error on the value 'unknown', because there is no
longer a direct mapping between valid xl.cfg values and the enumeration.

Suggested-by: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Juergen Gross <jgross@suse.com>

v2:
 - new in v2
---
 tools/libxl/libxl_create.c  |  4 ++--
 tools/libxl/libxl_types.idl |  2 +-
 tools/xl/xl_parse.c         | 26 +++++++++++++++-----------
 3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index b58e030376..5f2972bc03 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -64,7 +64,7 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
         c_info->ssidref = SECINITSID_DOMU;
 
     if (info.cap_hvm_directio &&
-        (c_info->passthrough == LIBXL_PASSTHROUGH_ENABLED)) {
+        (c_info->passthrough == LIBXL_PASSTHROUGH_UNKNOWN)) {
         c_info->passthrough = ((c_info->type == LIBXL_DOMAIN_TYPE_PV) ||
                                !info.cap_iommu_hap_pt_share) ?
             LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
@@ -586,7 +586,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
                 libxl_defbool_val(info->oos) ? 0 : XEN_DOMCTL_CDF_oos_off;
         }
 
-        assert(info->passthrough != LIBXL_PASSTHROUGH_ENABLED);
+        assert(info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN);
         LOG(DETAIL, "passthrough: %s",
             libxl_passthrough_to_string(info->passthrough));
 
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 54ac685f50..3ac9494b80 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -264,7 +264,7 @@ libxl_vkb_backend = Enumeration("vkb_backend", [
     ])
 
 libxl_passthrough = Enumeration("passthrough", [
-    (0, "enabled"),
+    (0, "unknown"),
     (1, "disabled"),
     (2, "sync_pt"),
     (3, "share_pt"),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index c2e61b680f..03a2c54dd2 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1512,23 +1512,27 @@ void parse_config_data(const char *config_source,
     if (xlu_cfg_get_string(config, "passthrough", &buf, 0)) {
         c_info->passthrough =
             (d_config->num_pcidevs || d_config->num_dtdevs)
-            ? LIBXL_PASSTHROUGH_ENABLED : LIBXL_PASSTHROUGH_DISABLED;
+            ? LIBXL_PASSTHROUGH_UNKNOWN : LIBXL_PASSTHROUGH_DISABLED;
     } else {
-        libxl_passthrough o;
+        if (!strcasecmp("enabled", buf))
+            c_info->passthrough = LIBXL_PASSTHROUGH_UNKNOWN;
+        else {
+            libxl_passthrough o;
 
-        e = libxl_passthrough_from_string(buf, &o);
-        if (e) {
-            fprintf(stderr,
-                    "ERROR: unknown passthrough option '%s'\n",
-                    buf);
-            exit(-ERROR_FAIL);
-        }
+            e = libxl_passthrough_from_string(buf, &o);
+            if (e || !strcasecmp("unknown", buf)) {
+                fprintf(stderr,
+                        "ERROR: unknown passthrough option '%s'\n",
+                        buf);
+                exit(-ERROR_FAIL);
+            }
 
-        c_info->passthrough = o;
+            c_info->passthrough = o;
+        }
     }
 
     switch (c_info->passthrough) {
-    case LIBXL_PASSTHROUGH_ENABLED:
+    case LIBXL_PASSTHROUGH_UNKNOWN:
         /*
          * Choose a suitable default. libxl would also do this but
          * choosing here allows the code calculating 'iommu_memkb'
-- 
2.20.1.2.gb21ebb671


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH-for-4.13 v2 2/2] libxl: choose an appropriate default for passthrough...

[Paul Durrant]

...if there is no IOMMU or it is globally disabled.

Without this patch, the following assertion may be hit:

xl: libxl_create.c:589: libxl__domain_make: Assertion `info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN' failed.

This is because libxl__domain_create_info_setdefault() currently only sets
an appropriate value for 'passthrough' in the case that 'cap_hvm_directio'
is true, which is not the case unless an IOMMU is present and enabled in
the system. This is normally masked by xl choosing a default value, but
that will not happen if xl is not used (e.g. when using libvirt) or when
a stub domain is being created.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Juergen Gross <jgross@suse.com>

v2:
 - re-base
---
 tools/libxl/libxl_create.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 5f2972bc03..62e13f3e7c 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -68,7 +68,11 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
         c_info->passthrough = ((c_info->type == LIBXL_DOMAIN_TYPE_PV) ||
                                !info.cap_iommu_hap_pt_share) ?
             LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
-    }
+    } else if (!info.cap_hvm_directio)
+        c_info->passthrough = LIBXL_PASSTHROUGH_DISABLED;
+
+    /* An explicit setting should now have been chosen */
+    assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN);
 
     return 0;
 }
-- 
2.20.1.2.gb21ebb671


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH-for-4.13 v2 0/2] libxl: fix assertion failure

[Jürgen Groß]

On 01.10.19 16:57, Paul Durrant wrote:
> This was originally a single patch, which is now patch #2 of this series.
> 
> Paul Durrant (2):
>    libxl: replace 'enabled' with 'unknown' in libxl_passthrough
>      enumeration
>    libxl: choose an appropriate default for passthrough...
> 
>   tools/libxl/libxl_create.c  | 10 +++++++---
>   tools/libxl/libxl_types.idl |  2 +-
>   tools/xl/xl_parse.c         | 26 +++++++++++++++-----------
>   3 files changed, 23 insertions(+), 15 deletions(-)

For the series:

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH-for-4.13 v2 0/2] libxl: fix assertion failure

[Ian Jackson]

Paul Durrant writes ("[PATCH-for-4.13 v2 0/2] libxl: fix assertion failure"):
> This was originally a single patch, which is now patch #2 of this series.
> 
> Paul Durrant (2):
>   libxl: replace 'enabled' with 'unknown' in libxl_passthrough
>     enumeration
>   libxl: choose an appropriate default for passthrough...

Thanks.  I have applied these, and also a style fixup (below).

I am continuing to look at the defaulting and config management here
with a view to getting rid of some of the duplicated code and moving
it all into libxl.

Ian.

From b01b1dc046da70a2621a4d1f032ddb22b0cdde6b Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Wed, 2 Oct 2019 16:55:47 +0100
Subject: [PATCH] libxl: create: style: Add a pair of missing { ]

From CODING_STYLE:

  Every indented statement is braced, but blocks that contain just one
  statement may have the braces omitted.  To avoid confusion, either all
  the blocks in an if...else chain have braces, or none of them do.

CC: Paul Durrant <paul.durrant@citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl_create.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 62e13f3e7c..099761a2d7 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -68,8 +68,9 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
         c_info->passthrough = ((c_info->type == LIBXL_DOMAIN_TYPE_PV) ||
                                !info.cap_iommu_hap_pt_share) ?
             LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
-    } else if (!info.cap_hvm_directio)
+    } else if (!info.cap_hvm_directio) {
         c_info->passthrough = LIBXL_PASSTHROUGH_DISABLED;
+    }
 
     /* An explicit setting should now have been chosen */
     assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH-for-4.13] libxl: choose an appropriate default for passthrough... [and 1 more messages]

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH-for-4.13 v2 0/2] libxl: fix assertion failure"):
> I am continuing to look at the defaulting and config management here
> with a view to getting rid of some of the duplicated code and moving
> it all into libxl.

Specifically, Paul Durrant writes:
> Ok. Specifically libxl_domain_need_memory() only has access to the
> build info, but libxl__domain_build_info_setdefault() does not have
> access to the create info, so it can't choose a sensible default. To
> avoid re-writing lots of code I went with having xl calulate a
> default. (Prior to my patch there was no calculated overhead anyway so
> a host without shared EPT and autoballooning on was already playing
> russian roulette).

So some restructuring may be needed.  I'll see how invasive it looks.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH-for-4.13 v2 0/2] libxl: fix assertion failure

[Paul Durrant]

On Wed, 2 Oct 2019 at 17:04, Ian Jackson <ian.jackson@citrix.com> wrote:
>
> Paul Durrant writes ("[PATCH-for-4.13 v2 0/2] libxl: fix assertion failure"):
> > This was originally a single patch, which is now patch #2 of this series.
> >
> > Paul Durrant (2):
> >   libxl: replace 'enabled' with 'unknown' in libxl_passthrough
> >     enumeration
> >   libxl: choose an appropriate default for passthrough...
>
> Thanks.  I have applied these, and also a style fixup (below).
>

Cool.

> I am continuing to look at the defaulting and config management here
> with a view to getting rid of some of the duplicated code and moving
> it all into libxl.
>

That would indeed be beneficial for the likes of libvirt. Perhaps it
would be reasonable to unify the create and build info at a libxl
level (even though they may feed into distinct domctls underneath for
the moment)?

  Cheers,

    Paul

> Ian.
>
> From b01b1dc046da70a2621a4d1f032ddb22b0cdde6b Mon Sep 17 00:00:00 2001
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> Date: Wed, 2 Oct 2019 16:55:47 +0100
> Subject: [PATCH] libxl: create: style: Add a pair of missing { ]
>
> From CODING_STYLE:
>
>   Every indented statement is braced, but blocks that contain just one
>   statement may have the braces omitted.  To avoid confusion, either all
>   the blocks in an if...else chain have braces, or none of them do.
>
> CC: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
>  tools/libxl/libxl_create.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 62e13f3e7c..099761a2d7 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -68,8 +68,9 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
>          c_info->passthrough = ((c_info->type == LIBXL_DOMAIN_TYPE_PV) ||
>                                 !info.cap_iommu_hap_pt_share) ?
>              LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
> -    } else if (!info.cap_hvm_directio)
> +    } else if (!info.cap_hvm_directio) {
>          c_info->passthrough = LIBXL_PASSTHROUGH_DISABLED;
> +    }
>
>      /* An explicit setting should now have been chosen */
>      assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN);
> --
> 2.11.0
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH-for-4.13 v2 0/2] libxl: fix assertion failure

[Andrew Cooper]

On 02/10/2019 17:02, Ian Jackson wrote:
> Paul Durrant writes ("[PATCH-for-4.13 v2 0/2] libxl: fix assertion failure"):
>> This was originally a single patch, which is now patch #2 of this series.
>>
>> Paul Durrant (2):
>>   libxl: replace 'enabled' with 'unknown' in libxl_passthrough
>>     enumeration
>>   libxl: choose an appropriate default for passthrough...
> Thanks.  I have applied these, and also a style fixup (below).
>
> I am continuing to look at the defaulting and config management here
> with a view to getting rid of some of the duplicated code and moving
> it all into libxl.
>
> Ian.
>
> From b01b1dc046da70a2621a4d1f032ddb22b0cdde6b Mon Sep 17 00:00:00 2001
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> Date: Wed, 2 Oct 2019 16:55:47 +0100
> Subject: [PATCH] libxl: create: style: Add a pair of missing { ]

s/]/}/

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH-for-4.13 v2 0/2] libxl: fix assertion failure

[Ian Jackson]

Paul Durrant writes ("Re: [Xen-devel] [PATCH-for-4.13 v2 0/2] libxl: fix assertion failure"):
> On Wed, 2 Oct 2019 at 17:04, Ian Jackson <ian.jackson@citrix.com> wrote:
> > I am continuing to look at the defaulting and config management here
> > with a view to getting rid of some of the duplicated code and moving
> > it all into libxl.
> 
> That would indeed be beneficial for the likes of libvirt.

I propose the following plan for 4.13:

 * Move the default calculations of b_info->shadow_memkb and
   b_info->iommu_memkb from xl_vmcontrol.c into libxl, in a new
   function libxl__need_memory_setdefault, called from
   initiate_domain_create.  That has access to the whole of c_info and
   b_info.

 * Change the API/ABI for libxl_domain_need_memory to take a
   libxl_domain_config.  Internally, this will call an implementation
   function libxl__domain_need_memory which takes the b_info and
   c_info separately, and which calls libxl__need_memory_setdefault.
   (This is the only other call site for
   libxl__domain_build_info_setdefault.)

 * There will be the usual backward compatible arrangement: here, a
   function libxl_domain_need_memory_0x040c00, which will pass NULL
   for c_info.  The code in libxl__need_memory_setdefault will use 0
   for the two additional memory amounts when c_info is NULL.

 * The overall effect is that old callers will get the old behaviour.
   New callers get the new right behaviour.  This is the same as the
   present libxl 4.13 code.  Note that libxl_domain_need_memory
   already has an API stability caveat.

 * Consequently, the need for libxl_get_required_shadow_memory and
   libxl_get_required_iommu_memory goes away.  Delete them (they have
   not been in any release so we can just do this).

 * Invent a new value for c_info->passthrough "enabled".  Defaulting
   will be 1. turn "unknown" into "disabled" or "enabled" according to
   the current logic based on pcidevs/dtdefs; 2. turn "enabled" into
   something specific according to the current logic based on type,
   hap_pt_share, etc.  Make sure this is all correct inside libxl.

 * Delete the defaulting code in xl.  xl can just leave settings not
   specified by the user as blank, and libxl will DTRT with them.

What do people think ?  I really want to fix this for 4.13 because the
current 4.13 API is not one I want to support.

> Perhaps it would be reasonable to unify the create and build info at
> a libxl level (even though they may feed into distinct domctls
> underneath for the moment)?

Yes, but we are probably too late for such an API change at this point
for 4.13.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH-for-4.13 v2 0/2] libxl: fix assertion failure

[Anthony PERARD]

On Thu, Oct 03, 2019 at 02:30:31PM +0100, Ian Jackson wrote:
> Paul Durrant writes ("Re: [Xen-devel] [PATCH-for-4.13 v2 0/2] libxl: fix assertion failure"):
> > On Wed, 2 Oct 2019 at 17:04, Ian Jackson <ian.jackson@citrix.com> wrote:
> > > I am continuing to look at the defaulting and config management here
> > > with a view to getting rid of some of the duplicated code and moving
> > > it all into libxl.
> > 
> > That would indeed be beneficial for the likes of libvirt.
> 
> I propose the following plan for 4.13:
> 
>  * Move the default calculations of b_info->shadow_memkb and
>    b_info->iommu_memkb from xl_vmcontrol.c into libxl, in a new
>    function libxl__need_memory_setdefault, called from
>    initiate_domain_create.  That has access to the whole of c_info and
>    b_info.
> 
>  * Change the API/ABI for libxl_domain_need_memory to take a
>    libxl_domain_config.  Internally, this will call an implementation
>    function libxl__domain_need_memory which takes the b_info and
>    c_info separately, and which calls libxl__need_memory_setdefault.
>    (This is the only other call site for
>    libxl__domain_build_info_setdefault.)
> 
>  * There will be the usual backward compatible arrangement: here, a
>    function libxl_domain_need_memory_0x040c00, which will pass NULL
>    for c_info.  The code in libxl__need_memory_setdefault will use 0
>    for the two additional memory amounts when c_info is NULL.
> 
>  * The overall effect is that old callers will get the old behaviour.
>    New callers get the new right behaviour.  This is the same as the
>    present libxl 4.13 code.  Note that libxl_domain_need_memory
>    already has an API stability caveat.
> 
>  * Consequently, the need for libxl_get_required_shadow_memory and
>    libxl_get_required_iommu_memory goes away.  Delete them (they have
>    not been in any release so we can just do this).

libxl_get_required_shadow_memory is old, and libvirt is using.
Only libxl_get_required_iommu_memory is new.

>  * Invent a new value for c_info->passthrough "enabled".  Defaulting
>    will be 1. turn "unknown" into "disabled" or "enabled" according to
>    the current logic based on pcidevs/dtdefs; 2. turn "enabled" into
>    something specific according to the current logic based on type,
>    hap_pt_share, etc.  Make sure this is all correct inside libxl.
> 
>  * Delete the defaulting code in xl.  xl can just leave settings not
>    specified by the user as blank, and libxl will DTRT with them.
> 
> What do people think ?  I really want to fix this for 4.13 because the
> current 4.13 API is not one I want to support.

That plan sound fine to me.

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH-for-4.13 v2 0/2] libxl: fix assertion failure

[Jürgen Groß]

On 03.10.19 15:30, Ian Jackson wrote:
> Paul Durrant writes ("Re: [Xen-devel] [PATCH-for-4.13 v2 0/2] libxl: fix assertion failure"):
>> On Wed, 2 Oct 2019 at 17:04, Ian Jackson <ian.jackson@citrix.com> wrote:
>>> I am continuing to look at the defaulting and config management here
>>> with a view to getting rid of some of the duplicated code and moving
>>> it all into libxl.
>>
>> That would indeed be beneficial for the likes of libvirt.
> 
> I propose the following plan for 4.13:

Fine with me.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel