1. Patchew
  2. Xen
  3. View series

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

Paul Durrant posted 1 patch 6 years, 1 month ago
Failed in applying to current master (apply log)
tools/libxl/libxl_create.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
[Xen-devel] [PATCH-for-4.13] libxl: choose an appropriate default for passthrough...
Posted by Paul Durrant 6 years, 1 month ago 
...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...
Posted by Ian Jackson 6 years, 1 month ago 
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...
Posted by Paul Durrant 6 years, 1 month ago 
> -----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...
Posted by Paul Durrant 6 years, 1 month ago 
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...
Posted by Ian Jackson 6 years, 1 month ago 
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
Re: [Xen-devel] [PATCH-for-4.13] libxl: choose an appropriate default for passthrough...
Posted by Jürgen Groß 6 years, 1 month ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.