[Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Ian Jackson <ian.jackson@eu.citrix.com>]

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 specificable:
  * default ("unknown"): 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 we document "unknown".)

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, in one
place.

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.

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.

This will hide, from the point of view of libvirt tests in osstest, a
separate hypervisor regression which prevents migration of a domain
with passthrough enabled but without actual PT devices.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
v2: New patch in this version of the series.
---
 tools/libxl/libxl_create.c  | 57 ++++++++++++++++++++++++++++++--------
 tools/libxl/libxl_types.idl |  5 ++--
 tools/xl/xl_parse.c         | 67 ++++-----------------------------------------
 3 files changed, 53 insertions(+), 76 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 69971c97b6..fccb6a6271 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;
 }
 
@@ -904,6 +892,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);
@@ -968,6 +957,50 @@ 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_UNKNOWN) {
+        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,
+             "ERROR: 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;
+    }
+
+    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" :
+        NULL;
+
+    if (c_info->passthrough == LIBXL_PASSTHROUGH_ENABLED) {
+        assert(iommu_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,
+             "ERROR: passthrough=\"share_pt\" %s\n",
+             whynot_pt_share);
+        ret = ERROR_INVAL;
+        goto error_out;
+    }
+
+    /* An explicit setting should now have been chosen */
+    assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN);
+    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..2441c0c233 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -266,8 +266,9 @@ libxl_vkb_backend = Enumeration("vkb_backend", [
 libxl_passthrough = Enumeration("passthrough", [
     (0, "unknown"),
     (1, "disabled"),
-    (2, "sync_pt"),
-    (3, "share_pt"),
+    (2, "enabled"),
+    (3, "sync_pt"),
+    (4, "share_pt"),
     ])
 
 #
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))
-- 
2.11.0


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