diff mbox series

[XEN,for-4.13,v5,10/10] libxl/xl: Overhaul passthrough setting logic

Message ID 20191018150653.24862-11-ian.jackson@eu.citrix.com (mailing list archive)
State Superseded
Headers show
Series libxl memkb & pt defaulting | expand

Commit Message

Ian Jackson Oct. 18, 2019, 3:06 p.m. UTC
LIBXL_PASSTHROUGH_UNKNOWN (aka "ENABLED" in an earlier uncommitted
version of this code) is doing double duty.  We actually need all of
the following to be specifiable:
  * "default": enable PT iff we have devices to
    pass through specified in the initial config file.
  * "enabled" (and fail if the platform doesn't support it).
  * "disabled" (and reject future PT hotplug).
  * "share_pt"/"sync_pt": enable PT and set a specific PT mode.

Defaulting and error checking should be done in libxl.  So, we make
several changes here.

We introduce "enabled", and rename "unknown" to "default".

We move all of the error checking and defaulting code from xl into
libxl.  Now, libxl__domain_config_setdefault has all of the necessary
information to get this right.  So we can do it all there.  Choosing
the specific mode is arch-specific.

We can also arrange to have only one place each which calculates
(i) whether passthrough needs to be enabled because pt devices were
specified (ii) whether pt_share can be used (for each arch).

xl now only has to parse the enum in the same way as it parses all
other enums.

This change fixes a regression from earlier 4.13-pre: until recent
changes, passthrough was only enabled by default if passthrough
devices were specified.  We restore this behaviour.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Andrew Cooper <Andrew.Cooper3@citrix.com>
CC: Paul Durrant <pdurrant@gmail.com>
CC: Jan Beulich <jbeulich@suse.com>

---
v5: Change "unknown" to "default".  In the manpage too.

v4: Fix trailing whitespace
    No longer change "unknown" to "unspecified".

v3: Drop paragraph about masking another osstest regression,
     as that's now fixed.
    Drop redundant "ERROR:" in two log messages.
    Add a comment about the way "enabled" gets changed to a specific value.
    Split passthrough mode defaulting into arch specific functions.
    On ARM, always choose (and insist on) share_pt.
    Reject share_pt for non-HAP guests.
    Reject passthrough for PVH guests.
    Actually document "unspecified" option in xl.cfg(5)
    Rename "unknown" to "unspecified"

v2: New patch in this version of the series.
---
 docs/man/xl.cfg.5.pod.in    |  6 ++++
 tools/libxl/libxl_arch.h    |  6 ++++
 tools/libxl/libxl_arm.c     | 24 ++++++++++++++++
 tools/libxl/libxl_create.c  | 43 ++++++++++++++++++++---------
 tools/libxl/libxl_types.idl |  7 +++--
 tools/libxl/libxl_x86.c     | 41 +++++++++++++++++++++++++++
 tools/xl/xl_parse.c         | 67 ++++-----------------------------------------
 7 files changed, 116 insertions(+), 78 deletions(-)

Comments

Julien Grall Oct. 18, 2019, 3:47 p.m. UTC | #1
Hi Ian,

On 18/10/2019 16:06, Ian Jackson wrote:
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index bf31b9b3ca..2f1ca69431 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -1191,6 +1191,30 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>       libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PVH);
>   }
>   
> +int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
> +                                            uint32_t domid,
> +                                            libxl_domain_config *d_config,
> +                                            const libxl_physinfo *physinfo)
> +{
> +    int rc;
> +    libxl_domain_create_info *const c_info = &d_config->c_info;
> +
> +    if (c_info->passthrough == LIBXL_PASSTHROUGH_ENABLED) {
> +        c_info->passthrough = LIBXL_PASSTHROUGH_SHARE_PT;
> +    }
> +
> +    if (c_info->passthrough == LIBXL_PASSTHROUGH_SYNC_PT) {
> +        LOGD(ERROR, domid,
> +             "passthrough=\"sync_pt\" not supported on ARM\n");
> +        rc = ERROR_INVAL;
> +        goto out;
> +    }

Would it make sense to use a switch case or whitelist here? So we don't end up 
to slip through if a new type is added and unsupported on Arm.

Cheers,
Ian Jackson Oct. 18, 2019, 3:51 p.m. UTC | #2
Julien Grall writes ("Re: [XEN PATCH for-4.13 v5 10/10] libxl/xl: Overhaul passthrough setting logic"):
> On 18/10/2019 16:06, Ian Jackson wrote:
...
> > +    if (c_info->passthrough == LIBXL_PASSTHROUGH_SYNC_PT) {
> > +        LOGD(ERROR, domid,
> > +             "passthrough=\"sync_pt\" not supported on ARM\n");
> > +        rc = ERROR_INVAL;
> > +        goto out;
> > +    }
> 
> Would it make sense to use a switch case or whitelist here? So we don't end up 
> to slip through if a new type is added and unsupported on Arm.

I wouldn't mind changing this to
   c_info->passthrough != LIBXL_PASSTHROUGH_SHARE_PT

Although it's a shame you're raising this now.  This code has been
like this since v3.  Would you mind if we did this as a followup
patch ?

Ian.
Julien Grall Oct. 18, 2019, 3:53 p.m. UTC | #3
Hi Ian,

On 18/10/2019 16:51, Ian Jackson wrote:
> Julien Grall writes ("Re: [XEN PATCH for-4.13 v5 10/10] libxl/xl: Overhaul passthrough setting logic"):
>> On 18/10/2019 16:06, Ian Jackson wrote:
> ...
>>> +    if (c_info->passthrough == LIBXL_PASSTHROUGH_SYNC_PT) {
>>> +        LOGD(ERROR, domid,
>>> +             "passthrough=\"sync_pt\" not supported on ARM\n");
>>> +        rc = ERROR_INVAL;
>>> +        goto out;
>>> +    }
>>
>> Would it make sense to use a switch case or whitelist here? So we don't end up
>> to slip through if a new type is added and unsupported on Arm.
> 
> I wouldn't mind changing this to
>     c_info->passthrough != LIBXL_PASSTHROUGH_SHARE_PT

I think it would need to be

c_info->passthrough != LIBXL_PASSTHROUGH_SHARE_PT && c_info->passthrough != 
LIBXL_PASSTHROUGH_DISABLED.

> 
> Although it's a shame you're raising this now.  This code has been
> like this since v3.  Would you mind if we did this as a followup
> patch ?

Sorry I haven't been really good at reviewing this week :(.

I am happy if this is done in a follow-up.

Cheers,
Ian Jackson Oct. 18, 2019, 3:56 p.m. UTC | #4
Julien Grall writes ("Re: [XEN PATCH for-4.13 v5 10/10] libxl/xl: Overhaul passthrough setting logic"):
> On 18/10/2019 16:51, Ian Jackson wrote:
> > I wouldn't mind changing this to
> >     c_info->passthrough != LIBXL_PASSTHROUGH_SHARE_PT
> 
> I think it would need to be
> 
> c_info->passthrough != LIBXL_PASSTHROUGH_SHARE_PT && c_info->passthrough != 
> LIBXL_PASSTHROUGH_DISABLED.

Err, indeed.  I'll do this or a switch().

Ian.
diff mbox series

Patch

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 64bed30bce..245d3f9472 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -660,6 +660,12 @@  in preference. However, the availability of this option is hardware
 specific. If B<xl info> reports B<virt_caps> containing
 B<iommu_hap_pt_share> then this option may be used.
 
+=item B<default>
+
+The default, which chooses between B<disabled> and B<enabled>
+according to whether passthrough devices are enabled in the config
+file.
+
 =back
 
 =back
diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index d624159e53..ee6641b3e6 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -73,6 +73,12 @@  void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
                                               libxl_domain_build_info *b_info);
 
 _hidden
+int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
+                                            uint32_t domid,
+                                            libxl_domain_config *d_config,
+                                            const libxl_physinfo *physinfo);
+
+_hidden
 int libxl__arch_extra_memory(libxl__gc *gc,
                              const libxl_domain_build_info *info,
                              uint64_t *out);
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index bf31b9b3ca..2f1ca69431 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -1191,6 +1191,30 @@  void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
     libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PVH);
 }
 
+int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
+                                            uint32_t domid,
+                                            libxl_domain_config *d_config,
+                                            const libxl_physinfo *physinfo)
+{
+    int rc;
+    libxl_domain_create_info *const c_info = &d_config->c_info;
+
+    if (c_info->passthrough == LIBXL_PASSTHROUGH_ENABLED) {
+        c_info->passthrough = LIBXL_PASSTHROUGH_SHARE_PT;
+    }
+
+    if (c_info->passthrough == LIBXL_PASSTHROUGH_SYNC_PT) {
+        LOGD(ERROR, domid,
+             "passthrough=\"sync_pt\" not supported on ARM\n");
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    rc = 0;
+ out:
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 7869d54b32..d59ac2ab05 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -57,18 +57,6 @@  int libxl__domain_create_info_setdefault(libxl__gc *gc,
     if (!c_info->ssidref)
         c_info->ssidref = SECINITSID_DOMU;
 
-    if (info->cap_hvm_directio &&
-        (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;
-    } 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;
 }
 
@@ -591,7 +579,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_UNKNOWN);
+        assert(info->passthrough != LIBXL_PASSTHROUGH_DEFAULT);
         LOG(DETAIL, "passthrough: %s",
             libxl_passthrough_to_string(info->passthrough));
 
@@ -908,6 +896,7 @@  int libxl__domain_config_setdefault(libxl__gc *gc,
     libxl_ctx *ctx = libxl__gc_owner(gc);
     int ret;
     bool pod_enabled = false;
+    libxl_domain_create_info *c_info = &d_config->c_info;
 
     libxl_physinfo physinfo;
     ret = libxl_get_physinfo(CTX, &physinfo);
@@ -979,6 +968,34 @@  int libxl__domain_config_setdefault(libxl__gc *gc,
         goto error_out;
     }
 
+    bool need_pt = d_config->num_pcidevs || d_config->num_dtdevs;
+    if (c_info->passthrough == LIBXL_PASSTHROUGH_DEFAULT) {
+        c_info->passthrough = need_pt
+            ? LIBXL_PASSTHROUGH_ENABLED : LIBXL_PASSTHROUGH_DISABLED;
+    }
+
+    bool iommu_enabled = physinfo.cap_hvm_directio;
+    if (c_info->passthrough != LIBXL_PASSTHROUGH_DISABLED && !iommu_enabled) {
+        LOGD(ERROR, domid,
+             "passthrough not supported on this platform\n");
+        ret = ERROR_INVAL;
+        goto error_out;
+    }
+
+    if (c_info->passthrough == LIBXL_PASSTHROUGH_DISABLED && need_pt) {
+        LOGD(ERROR, domid,
+             "passthrough disabled but devices are specified");
+        ret = ERROR_INVAL;
+        goto error_out;
+    }
+
+    ret = libxl__arch_passthrough_mode_setdefault(gc,domid,d_config,&physinfo);
+    if (ret) goto error_out;
+
+    /* An explicit setting should now have been chosen */
+    assert(c_info->passthrough != LIBXL_PASSTHROUGH_DEFAULT);
+    assert(c_info->passthrough != LIBXL_PASSTHROUGH_ENABLED);
+
     /* If target_memkb is smaller than max_memkb, the subsequent call
      * to libxc when building HVM domain will enable PoD mode.
      */
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 3ac9494b80..0546d7865a 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -264,10 +264,11 @@  libxl_vkb_backend = Enumeration("vkb_backend", [
     ])
 
 libxl_passthrough = Enumeration("passthrough", [
-    (0, "unknown"),
+    (0, "default"),
     (1, "disabled"),
-    (2, "sync_pt"),
-    (3, "share_pt"),
+    (2, "enabled"), # becomes {sync,share}_pt once defaults are evaluated
+    (3, "sync_pt"),
+    (4, "share_pt"),
     ])
 
 #
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index c0f88a7eaa..8b804537ba 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -631,6 +631,47 @@  void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
     libxl_defbool_setdefault(&b_info->acpi, true);
 }
 
+int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
+                                            uint32_t domid,
+                                            libxl_domain_config *d_config,
+                                            const libxl_physinfo *physinfo)
+{
+    int rc;
+    libxl_domain_create_info *const c_info = &d_config->c_info;
+
+    if (c_info->passthrough != LIBXL_PASSTHROUGH_DISABLED &&
+        c_info->type == LIBXL_DOMAIN_TYPE_PVH) {
+        LOGD(ERROR, domid,
+             "passthrough not yet supported for x86 PVH guests\n");
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    const char *whynot_pt_share =
+        c_info->type == LIBXL_DOMAIN_TYPE_PV ? "not valid for PV domain" :
+        !physinfo->cap_iommu_hap_pt_share ? "not supported on this platform" :
+        !libxl_defbool_val(d_config->c_info.hap) ?"only valid for HAP guests":
+        NULL;
+
+    if (c_info->passthrough == LIBXL_PASSTHROUGH_ENABLED) {
+        c_info->passthrough = whynot_pt_share
+            ? LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
+    }
+
+    if (c_info->passthrough == LIBXL_PASSTHROUGH_SHARE_PT && whynot_pt_share) {
+        LOGD(ERROR, domid,
+             "passthrough=\"share_pt\" %s\n",
+             whynot_pt_share);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    rc = 0;
+ out:
+    return rc;
+}
+
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 79871c22d0..112f8ee026 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1222,7 +1222,6 @@  void parse_config_data(const char *config_source,
     int pci_seize = 0;
     int i, e;
     char *kernel_basename;
-    bool iommu_enabled, iommu_hap_pt_share;
 
     libxl_domain_create_info *c_info = &d_config->c_info;
     libxl_domain_build_info *b_info = &d_config->b_info;
@@ -1234,8 +1233,6 @@  void parse_config_data(const char *config_source,
         exit(EXIT_FAILURE);
     }
 
-    iommu_enabled = physinfo.cap_hvm_directio;
-    iommu_hap_pt_share = physinfo.cap_iommu_hap_pt_share;
     libxl_physinfo_dispose(&physinfo);
 
     config= xlu_cfg_init(stderr, config_source);
@@ -1509,67 +1506,13 @@  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_UNKNOWN : LIBXL_PASSTHROUGH_DISABLED;
-    } else {
-        if (!strcasecmp("enabled", buf))
-            c_info->passthrough = LIBXL_PASSTHROUGH_UNKNOWN;
-        else {
-            libxl_passthrough o;
-
-            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;
-        }
-    }
-
-    switch (c_info->passthrough) {
-    case LIBXL_PASSTHROUGH_UNKNOWN:
-        /*
-         * Choose a suitable default. libxl would also do this but
-         * choosing here allows the code calculating 'iommu_memkb'
-         * below make an informed decision.
-         */
-        c_info->passthrough =
-            (c_info->type == LIBXL_DOMAIN_TYPE_PV) || !iommu_hap_pt_share
-            ? LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
-        break;
-
-    case LIBXL_PASSTHROUGH_DISABLED:
-        if (d_config->num_pcidevs || d_config->num_dtdevs) {
+    if (!xlu_cfg_get_string(config, "passthrough", &buf, 0)) {
+        if (libxl_passthrough_from_string(buf, &c_info->passthrough)) {
             fprintf(stderr,
-                    "ERROR: passthrough disabled but devices are specified\n");
-            exit(-ERROR_FAIL);
-        }
-        break;
-    case LIBXL_PASSTHROUGH_SHARE_PT:
-        if (c_info->type == LIBXL_DOMAIN_TYPE_PV) {
-            fprintf(stderr,
-                    "ERROR: passthrough=\"share_pt\" not valid for PV domain\n");
-            exit(-ERROR_FAIL);
-        } else if (!iommu_hap_pt_share) {
-            fprintf(stderr,
-                    "ERROR: passthrough=\"share_pt\" not supported on this platform\n");
-            exit(-ERROR_FAIL);
+                    "ERROR: unknown passthrough option '%s'\n",
+                    buf);
+            exit(1);
         }
-        break;
-    case LIBXL_PASSTHROUGH_SYNC_PT:
-        break;
-    }
-
-    if ((c_info->passthrough != LIBXL_PASSTHROUGH_DISABLED) &&
-        !iommu_enabled) {
-        fprintf(stderr,
-                "ERROR: passthrough not supported on this platform\n");
-        exit(-ERROR_FAIL);
     }
 
     if (!xlu_cfg_get_long(config, "shadow_memory", &l, 0))