[PATCH 0/8] x86: Untangle Nested virt and CPUID interactions

[PATCH 0/8] x86: Untangle Nested virt and CPUID interactions

[Andrew Cooper]

This is the final bit of untangling for existing CPUID handling, as well as
the logical next step in nested-virt work to start making it usable.

Patch 4 was previously posted in isolation.  It is unchanged in this series.

Andrew Cooper (8):
  tools/libxl: Simplify DOMCTL_CDF_ flags handling in libxl__domain_make()
  xen/domctl: Simplify DOMCTL_CDF_ checking logic
  xen/domctl: Introduce and use XEN_DOMCTL_CDF_nested_virt
  tools/cpuid: Plumb nested_virt down into xc_cpuid_apply_policy()
  x86/hvm: Obsolete the use of HVM_PARAM_NESTEDHVM
  xen/xsm: Drop xsm_hvm_param_nested()
  x86/hvm: Drop restore boolean from hvm_cr4_guest_valid_bits()
  x86/cpuid: Move VMX/SVM out of the default policy

 tools/flask/policy/modules/xen.if           |  2 +-
 tools/libs/ctrl/include/xenctrl.h           |  4 +--
 tools/libs/guest/xg_cpuid_x86.c             | 14 ++++------
 tools/libxl/libxl_cpuid.c                   |  3 +-
 tools/libxl/libxl_create.c                  | 13 ++++++---
 tools/libxl/libxl_x86.c                     |  5 ----
 tools/ocaml/libs/xc/xenctrl.ml              |  1 +
 tools/ocaml/libs/xc/xenctrl.mli             |  1 +
 xen/arch/x86/domain.c                       | 14 ++++++++--
 xen/arch/x86/hvm/domain.c                   |  2 +-
 xen/arch/x86/hvm/hvm.c                      | 41 ++++++---------------------
 xen/arch/x86/hvm/svm/svmdebug.c             |  6 ++--
 xen/arch/x86/hvm/vmx/vmx.c                  |  2 +-
 xen/arch/x86/hvm/vmx/vvmx.c                 |  2 +-
 xen/common/domain.c                         | 43 +++++++++++++++++------------
 xen/include/asm-x86/hvm/hvm.h               |  2 +-
 xen/include/asm-x86/hvm/nestedhvm.h         |  5 ++--
 xen/include/public/arch-x86/cpufeatureset.h |  4 +--
 xen/include/public/domctl.h                 |  4 ++-
 xen/include/public/hvm/params.h             |  4 +--
 xen/include/xsm/dummy.h                     |  6 ----
 xen/include/xsm/xsm.h                       |  6 ----
 xen/xsm/dummy.c                             |  1 -
 xen/xsm/flask/hooks.c                       |  6 ----
 xen/xsm/flask/policy/access_vectors         |  2 --
 25 files changed, 81 insertions(+), 112 deletions(-)

-- 
2.11.0

[PATCH 1/8] tools/libxl: Simplify DOMCTL_CDF_ flags handling in libxl__domain_make()

[Andrew Cooper]

The use of the ternary operator serves only to obfuscate the code.  Rewrite it
in more simple terms, avoiding the need to conditionally OR zero into the
flags.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_create.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 1031b75159..ed671052d7 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -612,10 +612,12 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
 
         if (info->type != LIBXL_DOMAIN_TYPE_PV) {
             create.flags |= XEN_DOMCTL_CDF_hvm;
-            create.flags |=
-                libxl_defbool_val(info->hap) ? XEN_DOMCTL_CDF_hap : 0;
-            create.flags |=
-                libxl_defbool_val(info->oos) ? 0 : XEN_DOMCTL_CDF_oos_off;
+
+            if ( libxl_defbool_val(info->hap) )
+                create.flags |= XEN_DOMCTL_CDF_hap;
+
+            if ( !libxl_defbool_val(info->oos) )
+                create.flags |= XEN_DOMCTL_CDF_oos_off;
         }
 
         assert(info->passthrough != LIBXL_PASSTHROUGH_DEFAULT);
-- 
2.11.0

[PATCH 2/8] xen/domctl: Simplify DOMCTL_CDF_ checking logic

[Andrew Cooper]

Introduce some local variables to make the resulting logic easier to follow.
Join the two IOMMU checks in sanitise_domain_config().  Tweak some of the
terminology for better accuracy.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/domain.c |  7 ++++---
 xen/common/domain.c   | 32 ++++++++++++++++++++------------
 2 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 7e16d49bfd..d8f9be132c 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -629,6 +629,7 @@ void arch_vcpu_destroy(struct vcpu *v)
 int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
 {
     bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
+    bool hap = config->flags & XEN_DOMCTL_CDF_hap;
     unsigned int max_vcpus;
 
     if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
@@ -653,13 +654,13 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }
 
-    if ( (config->flags & XEN_DOMCTL_CDF_hap) && !hvm_hap_supported() )
+    if ( hap && !hvm_hap_supported() )
     {
-        dprintk(XENLOG_INFO, "HAP requested but not supported\n");
+        dprintk(XENLOG_INFO, "HAP requested but not available\n");
         return -EINVAL;
     }
 
-    if ( !(config->flags & XEN_DOMCTL_CDF_hvm) )
+    if ( !hvm )
         /*
          * It is only meaningful for XEN_DOMCTL_CDF_oos_off to be clear
          * for HVM guests.
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 8cfa2e0b6b..cb617dc5aa 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -299,6 +299,10 @@ static void _domain_destroy(struct domain *d)
 
 static int sanitise_domain_config(struct xen_domctl_createdomain *config)
 {
+    bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
+    bool hap = config->flags & XEN_DOMCTL_CDF_hap;
+    bool iommu = config->flags & XEN_DOMCTL_CDF_iommu;
+
     if ( config->flags & ~(XEN_DOMCTL_CDF_hvm |
                            XEN_DOMCTL_CDF_hap |
                            XEN_DOMCTL_CDF_s3_integrity |
@@ -310,30 +314,34 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }
 
-    if ( !(config->flags & XEN_DOMCTL_CDF_iommu) && config->iommu_opts )
-    {
-        dprintk(XENLOG_INFO,
-                "IOMMU options specified but IOMMU not enabled\n");
-        return -EINVAL;
-    }
-
     if ( config->max_vcpus < 1 )
     {
         dprintk(XENLOG_INFO, "No vCPUS\n");
         return -EINVAL;
     }
 
-    if ( !(config->flags & XEN_DOMCTL_CDF_hvm) &&
-         (config->flags & XEN_DOMCTL_CDF_hap) )
+    if ( hap && !hvm )
     {
         dprintk(XENLOG_INFO, "HAP requested for non-HVM guest\n");
         return -EINVAL;
     }
 
-    if ( (config->flags & XEN_DOMCTL_CDF_iommu) && !iommu_enabled )
+    if ( iommu )
     {
-        dprintk(XENLOG_INFO, "IOMMU is not enabled\n");
-        return -EINVAL;
+        if ( !iommu_enabled )
+        {
+            dprintk(XENLOG_INFO, "IOMMU requested but not available\n");
+            return -EINVAL;
+        }
+    }
+    else
+    {
+        if ( config->iommu_opts )
+        {
+            dprintk(XENLOG_INFO,
+                    "IOMMU options specified but IOMMU not requested\n");
+            return -EINVAL;
+        }
     }
 
     return arch_sanitise_domain_config(config);
-- 
2.11.0

[PATCH 3/8] xen/domctl: Introduce and use XEN_DOMCTL_CDF_nested_virt

[Andrew Cooper]

Like other major areas of functionality, nested virt (or not) needs to be
known at domain creation time for sensible CPUID handling, and wants to be
known this early for sensible infrastructure handling in Xen.

Introduce XEN_DOMCTL_CDF_nested_virt and modify libxl to set it appropriately
when creating domains.  There is no need to adjust the ARM logic to reject the
use of this new flag.

No functional change yet.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Christian Lindig <christian.lindig@citrix.com>
CC: Edwin Török <edvin.torok@citrix.com>
CC: Rob Hoes <Rob.Hoes@citrix.com>
---
 tools/libxl/libxl_create.c      |  3 +++
 tools/ocaml/libs/xc/xenctrl.ml  |  1 +
 tools/ocaml/libs/xc/xenctrl.mli |  1 +
 xen/arch/x86/domain.c           |  7 +++++++
 xen/common/domain.c             | 11 +++++------
 xen/include/public/domctl.h     |  4 +++-
 6 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index ed671052d7..51e6809f3c 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -618,6 +618,9 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
 
             if ( !libxl_defbool_val(info->oos) )
                 create.flags |= XEN_DOMCTL_CDF_oos_off;
+
+            if ( libxl_defbool_val(b_info->nested_hvm) )
+                create.flags |= XEN_DOMCTL_CDF_nested_virt;
         }
 
         assert(info->passthrough != LIBXL_PASSTHROUGH_DEFAULT);
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 497ded7ce2..e878699b0a 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -64,6 +64,7 @@ type domain_create_flag =
 	| CDF_OOS_OFF
 	| CDF_XS_DOMAIN
 	| CDF_IOMMU
+	| CDF_NESTED_VIRT
 
 type domain_create_iommu_opts =
 	| IOMMU_NO_SHAREPT
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index f7f6ec570d..e64907df8e 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -57,6 +57,7 @@ type domain_create_flag =
   | CDF_OOS_OFF
   | CDF_XS_DOMAIN
   | CDF_IOMMU
+  | CDF_NESTED_VIRT
 
 type domain_create_iommu_opts =
   | IOMMU_NO_SHAREPT
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index d8f9be132c..5454f94d18 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -630,6 +630,7 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
 {
     bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
     bool hap = config->flags & XEN_DOMCTL_CDF_hap;
+    bool nested_virt = config->flags & XEN_DOMCTL_CDF_nested_virt;
     unsigned int max_vcpus;
 
     if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
@@ -667,6 +668,12 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
          */
         config->flags |= XEN_DOMCTL_CDF_oos_off;
 
+    if ( nested_virt && !hap )
+    {
+        dprintk(XENLOG_INFO, "Nested virt not supported without HAP\n");
+        return -EINVAL;
+    }
+
     return 0;
 }
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index cb617dc5aa..58b62d2fe4 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -303,12 +303,11 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
     bool hap = config->flags & XEN_DOMCTL_CDF_hap;
     bool iommu = config->flags & XEN_DOMCTL_CDF_iommu;
 
-    if ( config->flags & ~(XEN_DOMCTL_CDF_hvm |
-                           XEN_DOMCTL_CDF_hap |
-                           XEN_DOMCTL_CDF_s3_integrity |
-                           XEN_DOMCTL_CDF_oos_off |
-                           XEN_DOMCTL_CDF_xs_domain |
-                           XEN_DOMCTL_CDF_iommu) )
+    if ( config->flags &
+         ~(XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
+           XEN_DOMCTL_CDF_s3_integrity | XEN_DOMCTL_CDF_oos_off |
+           XEN_DOMCTL_CDF_xs_domain | XEN_DOMCTL_CDF_iommu |
+           XEN_DOMCTL_CDF_nested_virt) )
     {
         dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags);
         return -EINVAL;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 791f0a2592..666aeb71bf 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -68,9 +68,11 @@ struct xen_domctl_createdomain {
  /* Should this domain be permitted to use the IOMMU? */
 #define _XEN_DOMCTL_CDF_iommu         5
 #define XEN_DOMCTL_CDF_iommu          (1U<<_XEN_DOMCTL_CDF_iommu)
+#define _XEN_DOMCTL_CDF_nested_virt   6
+#define XEN_DOMCTL_CDF_nested_virt    (1U << _XEN_DOMCTL_CDF_nested_virt)
 
 /* Max XEN_DOMCTL_CDF_* constant.  Used for ABI checking. */
-#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_iommu
+#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_nested_virt
 
     uint32_t flags;
 
-- 
2.11.0

[PATCH 4/8] tools/cpuid: Plumb nested_virt down into xc_cpuid_apply_policy()

[Andrew Cooper]

Nested Virt is the final special case in legacy CPUID handling.  Pass the
(poorly named) nested_hvm setting down into xc_cpuid_apply_policy() to break
the semantic dependency on HVM_PARAM_NESTEDHVM.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libs/ctrl/include/xenctrl.h |  4 ++--
 tools/libs/guest/xg_cpuid_x86.c   | 14 +++++---------
 tools/libxl/libxl_cpuid.c         |  3 ++-
 3 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/tools/libs/ctrl/include/xenctrl.h b/tools/libs/ctrl/include/xenctrl.h
index 73e9535fc8..ba70bec9c4 100644
--- a/tools/libs/ctrl/include/xenctrl.h
+++ b/tools/libs/ctrl/include/xenctrl.h
@@ -1826,7 +1826,7 @@ struct xc_xend_cpuid {
  * cases, and the generated policy must be compatible with a 4.13.
  *
  * Either pass a full new @featureset (and @nr_features), or adjust individual
- * features (@pae, @itsc).
+ * features (@pae, @itsc, @nested_virt).
  *
  * Then (optionally) apply legacy XEND overrides (@xend) to the result.
  */
@@ -1834,7 +1834,7 @@ int xc_cpuid_apply_policy(xc_interface *xch,
                           uint32_t domid, bool restore,
                           const uint32_t *featureset,
                           unsigned int nr_features, bool pae, bool itsc,
-                          const struct xc_xend_cpuid *xend);
+                          bool nested_virt, const struct xc_xend_cpuid *xend);
 int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
 int xc_mca_op_inject_v2(xc_interface *xch, unsigned int flags,
                         xc_cpumap_t cpumap, unsigned int nr_cpus);
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index dc50106975..aae6931a11 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -427,7 +427,7 @@ static int xc_cpuid_xend_policy(
 
 int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
                           const uint32_t *featureset, unsigned int nr_features,
-                          bool pae, bool itsc,
+                          bool pae, bool itsc, bool nested_virt,
                           const struct xc_xend_cpuid *xend)
 {
     int rc;
@@ -559,7 +559,11 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
         p->extd.itsc = itsc;
 
         if ( di.hvm )
+        {
             p->basic.pae = pae;
+            p->basic.vmx = nested_virt;
+            p->extd.svm = nested_virt;
+        }
     }
 
     if ( !di.hvm )
@@ -625,14 +629,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
             }
             break;
         }
-
-        /*
-         * These settings are necessary to cause earlier HVM_PARAM_NESTEDHVM
-         * to be reflected correctly in CPUID.  Xen will discard these bits if
-         * configuration hasn't been set for the domain.
-         */
-        p->basic.vmx = true;
-        p->extd.svm = true;
     }
 
     rc = x86_cpuid_copy_to_buffer(p, leaves, &nr_leaves);
diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index f54eb83a90..08e85dcffb 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -422,6 +422,7 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
 {
     bool pae = true;
     bool itsc;
+    bool nested_virt = libxl_defbool_val(info->nested_hvm);
 
     /*
      * For PV guests, PAE is Xen-controlled (it is the 'p' that differentiates
@@ -452,7 +453,7 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
             info->tsc_mode == LIBXL_TSC_MODE_ALWAYS_EMULATE);
 
     xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
-                          pae, itsc, info->cpuid);
+                          pae, itsc, nested_virt, info->cpuid);
 }
 
 static const char *input_names[2] = { "leaf", "subleaf" };
-- 
2.11.0

[PATCH 5/8] x86/hvm: Obsolete the use of HVM_PARAM_NESTEDHVM

[Andrew Cooper]

With XEN_DOMCTL_CDF_nested_virt now passed properly to domain_create(),
reimplement nestedhvm_enabled() to use the property which is fixed for the
lifetime of the domain.

This makes the call to nestedhvm_vcpu_initialise() from hvm_vcpu_initialise()
no longer dead.  It became logically dead with the Xend => XL transition, as
they initialise HVM_PARAM_NESTEDHVM in opposite orders with respect to
XEN_DOMCTL_max_vcpus.

There is one opencoded user of nestedhvm_enabled() in HVM_PARAM_ALTP2M's
safety check.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Ian Jackson <iwj@xenproject.org>
CC: Anthony PERARD <anthony.perard@citrix.com>

The redunction in complexity of nestedhvm_enabled() has a substantial effect:

  add/remove: 1/0 grow/shrink: 2/37 up/down: 669/-2095 (-1426)
---
 tools/libxl/libxl_x86.c             |  5 -----
 xen/arch/x86/hvm/hvm.c              | 33 ++++-----------------------------
 xen/include/asm-x86/hvm/nestedhvm.h |  5 ++---
 xen/include/public/hvm/params.h     |  4 +---
 4 files changed, 7 insertions(+), 40 deletions(-)

diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 7d95506e00..72777fcaad 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -428,11 +428,6 @@ static int hvm_set_conf_params(libxl__gc *gc, uint32_t domid,
             LOG(ERROR, "Couldn't set HVM_PARAM_TIMER_MODE");
             goto out;
         }
-        if (xc_hvm_param_set(xch, domid, HVM_PARAM_NESTEDHVM,
-                             libxl_defbool_val(info->nested_hvm))) {
-            LOG(ERROR, "Couldn't set HVM_PARAM_NESTEDHVM");
-            goto out;
-        }
         if (xc_hvm_param_set(xch, domid, HVM_PARAM_ALTP2M, altp2m)) {
             LOG(ERROR, "Couldn't set HVM_PARAM_ALTP2M");
             goto out;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 2dfda93e09..101a739952 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4086,6 +4086,7 @@ static int hvm_allow_set_param(struct domain *d,
     case HVM_PARAM_MEMORY_EVENT_CR3:
     case HVM_PARAM_MEMORY_EVENT_CR4:
     case HVM_PARAM_MEMORY_EVENT_INT3:
+    case HVM_PARAM_NESTEDHVM:
     case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
     case HVM_PARAM_BUFIOREQ_EVTCHN:
     case HVM_PARAM_MEMORY_EVENT_MSR:
@@ -4204,39 +4205,12 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
     case HVM_PARAM_ACPI_IOPORTS_LOCATION:
         rc = pmtimer_change_ioport(d, value);
         break;
-    case HVM_PARAM_NESTEDHVM:
-        rc = xsm_hvm_param_nested(XSM_PRIV, d);
-        if ( rc )
-            break;
-        if ( value > 1 )
-            rc = -EINVAL;
-        /*
-         * Remove the check below once we have
-         * shadow-on-shadow.
-         */
-        if ( !paging_mode_hap(d) && value )
-            rc = -EINVAL;
-        if ( value &&
-             d->arch.hvm.params[HVM_PARAM_ALTP2M] )
-            rc = -EINVAL;
-        /* Set up NHVM state for any vcpus that are already up. */
-        if ( value &&
-             !d->arch.hvm.params[HVM_PARAM_NESTEDHVM] )
-            for_each_vcpu(d, v)
-                if ( rc == 0 )
-                    rc = nestedhvm_vcpu_initialise(v);
-        if ( !value || rc )
-            for_each_vcpu(d, v)
-                nestedhvm_vcpu_destroy(v);
-        break;
     case HVM_PARAM_ALTP2M:
         rc = xsm_hvm_param_altp2mhvm(XSM_PRIV, d);
         if ( rc )
             break;
-        if ( value > XEN_ALTP2M_limited )
-            rc = -EINVAL;
-        if ( value &&
-             d->arch.hvm.params[HVM_PARAM_NESTEDHVM] )
+        if ( (value > XEN_ALTP2M_limited) ||
+             (value && nestedhvm_enabled(d)) )
             rc = -EINVAL;
         break;
     case HVM_PARAM_TRIPLE_FAULT_REASON:
@@ -4390,6 +4364,7 @@ static int hvm_allow_get_param(struct domain *d,
     case HVM_PARAM_MEMORY_EVENT_CR3:
     case HVM_PARAM_MEMORY_EVENT_CR4:
     case HVM_PARAM_MEMORY_EVENT_INT3:
+    case HVM_PARAM_NESTEDHVM:
     case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
     case HVM_PARAM_BUFIOREQ_EVTCHN:
     case HVM_PARAM_MEMORY_EVENT_MSR:
diff --git a/xen/include/asm-x86/hvm/nestedhvm.h b/xen/include/asm-x86/hvm/nestedhvm.h
index d9784a2e0b..385cb708a3 100644
--- a/xen/include/asm-x86/hvm/nestedhvm.h
+++ b/xen/include/asm-x86/hvm/nestedhvm.h
@@ -34,10 +34,9 @@ enum nestedhvm_vmexits {
 };
 
 /* Nested HVM on/off per domain */
-static always_inline bool nestedhvm_enabled(const struct domain *d)
+static inline bool nestedhvm_enabled(const struct domain *d)
 {
-    return is_hvm_domain(d) && d->arch.hvm.params &&
-        d->arch.hvm.params[HVM_PARAM_NESTEDHVM];
+    return d->options & XEN_DOMCTL_CDF_nested_virt;
 }
 
 /* Nested VCPU */
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 0a91bfa749..0e3fdca096 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -34,6 +34,7 @@
 #define HVM_PARAM_MEMORY_EVENT_CR3          21
 #define HVM_PARAM_MEMORY_EVENT_CR4          22
 #define HVM_PARAM_MEMORY_EVENT_INT3         23
+#define HVM_PARAM_NESTEDHVM                 24
 #define HVM_PARAM_MEMORY_EVENT_SINGLE_STEP  25
 #define HVM_PARAM_BUFIOREQ_EVTCHN           26
 #define HVM_PARAM_MEMORY_EVENT_MSR          30
@@ -232,9 +233,6 @@
  */
 #define HVM_PARAM_ACPI_IOPORTS_LOCATION 19
 
-/* Boolean: Enable nestedhvm (hvm only) */
-#define HVM_PARAM_NESTEDHVM    24
-
 /* Params for the mem event rings */
 #define HVM_PARAM_PAGING_RING_PFN   27
 #define HVM_PARAM_MONITOR_RING_PFN  28
-- 
2.11.0

[PATCH 6/8] xen/xsm: Drop xsm_hvm_param_nested()

[Andrew Cooper]

The sole caller has been removed.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 tools/flask/policy/modules/xen.if   | 2 +-
 xen/include/xsm/dummy.h             | 6 ------
 xen/include/xsm/xsm.h               | 6 ------
 xen/xsm/dummy.c                     | 1 -
 xen/xsm/flask/hooks.c               | 6 ------
 xen/xsm/flask/policy/access_vectors | 2 --
 6 files changed, 1 insertion(+), 22 deletions(-)

diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if
index 8eb2293a52..5e2aa472b6 100644
--- a/tools/flask/policy/modules/xen.if
+++ b/tools/flask/policy/modules/xen.if
@@ -59,7 +59,7 @@ define(`create_domain_common', `
 	allow $1 $2:mmu { map_read map_write adjust memorymap physmap pinpage mmuext_op updatemp };
 	allow $1 $2:grant setup;
 	allow $1 $2:hvm { getparam hvmctl sethvmc
-			setparam nested altp2mhvm altp2mhvm_op dm };
+			setparam altp2mhvm altp2mhvm_op dm };
 ')
 
 # create_domain(priv, target)
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 2368acebed..7ae3c40eb5 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -551,12 +551,6 @@ static XSM_INLINE int xsm_hvm_control(XSM_DEFAULT_ARG struct domain *d, unsigned
     return xsm_default_action(action, current->domain, d);
 }
 
-static XSM_INLINE int xsm_hvm_param_nested(XSM_DEFAULT_ARG struct domain *d)
-{
-    XSM_ASSERT_ACTION(XSM_PRIV);
-    return xsm_default_action(action, current->domain, d);
-}
-
 static XSM_INLINE int xsm_hvm_param_altp2mhvm(XSM_DEFAULT_ARG struct domain *d)
 {
     XSM_ASSERT_ACTION(XSM_PRIV);
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index a80bcf3e42..7bd03d8817 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -136,7 +136,6 @@ struct xsm_operations {
 
     int (*hvm_param) (struct domain *d, unsigned long op);
     int (*hvm_control) (struct domain *d, unsigned long op);
-    int (*hvm_param_nested) (struct domain *d);
     int (*hvm_param_altp2mhvm) (struct domain *d);
     int (*hvm_altp2mhvm_op) (struct domain *d, uint64_t mode, uint32_t op);
     int (*get_vnumainfo) (struct domain *d);
@@ -564,11 +563,6 @@ static inline int xsm_hvm_control(xsm_default_t def, struct domain *d, unsigned
     return xsm_ops->hvm_control(d, op);
 }
 
-static inline int xsm_hvm_param_nested (xsm_default_t def, struct domain *d)
-{
-    return xsm_ops->hvm_param_nested(d);
-}
-
 static inline int xsm_hvm_param_altp2mhvm (xsm_default_t def, struct domain *d)
 {
     return xsm_ops->hvm_param_altp2mhvm(d);
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index d4cce68089..9e09512144 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -106,7 +106,6 @@ void __init xsm_fixup_ops (struct xsm_operations *ops)
     set_to_dummy_if_null(ops, hypfs_op);
     set_to_dummy_if_null(ops, hvm_param);
     set_to_dummy_if_null(ops, hvm_control);
-    set_to_dummy_if_null(ops, hvm_param_nested);
     set_to_dummy_if_null(ops, hvm_param_altp2mhvm);
     set_to_dummy_if_null(ops, hvm_altp2mhvm_op);
 
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index fab5d30c3a..19b0d9e3eb 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1208,11 +1208,6 @@ static int flask_hvm_param(struct domain *d, unsigned long op)
     return current_has_perm(d, SECCLASS_HVM, perm);
 }
 
-static int flask_hvm_param_nested(struct domain *d)
-{
-    return current_has_perm(d, SECCLASS_HVM, HVM__NESTED);
-}
-
 static int flask_hvm_param_altp2mhvm(struct domain *d)
 {
     return current_has_perm(d, SECCLASS_HVM, HVM__ALTP2MHVM);
@@ -1816,7 +1811,6 @@ static struct xsm_operations flask_ops = {
     .hypfs_op = flask_hypfs_op,
     .hvm_param = flask_hvm_param,
     .hvm_control = flask_hvm_param,
-    .hvm_param_nested = flask_hvm_param_nested,
     .hvm_param_altp2mhvm = flask_hvm_param_altp2mhvm,
     .hvm_altp2mhvm_op = flask_hvm_altp2mhvm_op,
 
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index fde5162c7e..1aa0bb501c 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -272,8 +272,6 @@ class hvm
 #  source = domain whose memory is being shared
 #  target = client domain
     share_mem
-# HVMOP_set_param setting HVM_PARAM_NESTEDHVM
-    nested
 # HVMOP_set_param setting HVM_PARAM_ALTP2MHVM
     altp2mhvm
 # HVMOP_altp2m_set_domain_state HVMOP_altp2m_get_domain_state
-- 
2.11.0

[PATCH 7/8] x86/hvm: Drop restore boolean from hvm_cr4_guest_valid_bits()

[Andrew Cooper]

Previously, migration was reordered so the CPUID data was available before
register state.  nestedhvm_enabled() has recently been made accurate for the
entire lifetime of the domain.

Therefore, we can drop the bodge in hvm_cr4_guest_valid_bits() which existed
previously to tolerate a guests' CR4 being set/restored before
HVM_PARAM_NESTEDHVM.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/domain.c       | 2 +-
 xen/arch/x86/hvm/hvm.c          | 8 ++++----
 xen/arch/x86/hvm/svm/svmdebug.c | 6 ++++--
 xen/arch/x86/hvm/vmx/vmx.c      | 2 +-
 xen/arch/x86/hvm/vmx/vvmx.c     | 2 +-
 xen/include/asm-x86/hvm/hvm.h   | 2 +-
 6 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
index 8e3375265c..0ce132b308 100644
--- a/xen/arch/x86/hvm/domain.c
+++ b/xen/arch/x86/hvm/domain.c
@@ -275,7 +275,7 @@ int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
     if ( v->arch.hvm.guest_efer & EFER_LME )
         v->arch.hvm.guest_efer |= EFER_LMA;
 
-    if ( v->arch.hvm.guest_cr[4] & ~hvm_cr4_guest_valid_bits(d, false) )
+    if ( v->arch.hvm.guest_cr[4] & ~hvm_cr4_guest_valid_bits(d) )
     {
         gprintk(XENLOG_ERR, "Bad CR4 value: %#016lx\n",
                 v->arch.hvm.guest_cr[4]);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 101a739952..54e32e4fe8 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -972,14 +972,14 @@ const char *hvm_efer_valid(const struct vcpu *v, uint64_t value,
         X86_CR0_CD | X86_CR0_PG)))
 
 /* These bits in CR4 can be set by the guest. */
-unsigned long hvm_cr4_guest_valid_bits(const struct domain *d, bool restore)
+unsigned long hvm_cr4_guest_valid_bits(const struct domain *d)
 {
     const struct cpuid_policy *p = d->arch.cpuid;
     bool mce, vmxe;
 
     /* Logic broken out simply to aid readability below. */
     mce  = p->basic.mce || p->basic.mca;
-    vmxe = p->basic.vmx && (restore || nestedhvm_enabled(d));
+    vmxe = p->basic.vmx && nestedhvm_enabled(d);
 
     return ((p->basic.vme     ? X86_CR4_VME | X86_CR4_PVI : 0) |
             (p->basic.tsc     ? X86_CR4_TSD               : 0) |
@@ -1033,7 +1033,7 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
         return -EINVAL;
     }
 
-    if ( ctxt.cr4 & ~hvm_cr4_guest_valid_bits(d, true) )
+    if ( ctxt.cr4 & ~hvm_cr4_guest_valid_bits(d) )
     {
         printk(XENLOG_G_ERR "HVM%d restore: bad CR4 %#" PRIx64 "\n",
                d->domain_id, ctxt.cr4);
@@ -2425,7 +2425,7 @@ int hvm_set_cr4(unsigned long value, bool may_defer)
     struct vcpu *v = current;
     unsigned long old_cr;
 
-    if ( value & ~hvm_cr4_guest_valid_bits(v->domain, false) )
+    if ( value & ~hvm_cr4_guest_valid_bits(v->domain) )
     {
         HVM_DBG_LOG(DBG_LEVEL_1,
                     "Guest attempts to set reserved bit in CR4: %lx",
diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c
index ba26b6a80b..f450391df4 100644
--- a/xen/arch/x86/hvm/svm/svmdebug.c
+++ b/xen/arch/x86/hvm/svm/svmdebug.c
@@ -106,6 +106,7 @@ bool svm_vmcb_isvalid(const char *from, const struct vmcb_struct *vmcb,
     unsigned long cr0 = vmcb_get_cr0(vmcb);
     unsigned long cr3 = vmcb_get_cr3(vmcb);
     unsigned long cr4 = vmcb_get_cr4(vmcb);
+    unsigned long valid;
     uint64_t efer = vmcb_get_efer(vmcb);
 
 #define PRINTF(fmt, args...) do { \
@@ -130,9 +131,10 @@ bool svm_vmcb_isvalid(const char *from, const struct vmcb_struct *vmcb,
            (cr3 >> v->domain->arch.cpuid->extd.maxphysaddr))) )
         PRINTF("CR3: MBZ bits are set (%#"PRIx64")\n", cr3);
 
-    if ( cr4 & ~hvm_cr4_guest_valid_bits(v->domain, false) )
+    valid = hvm_cr4_guest_valid_bits(v->domain);
+    if ( cr4 & ~valid )
         PRINTF("CR4: invalid bits are set (%#"PRIx64", valid: %#"PRIx64")\n",
-               cr4, hvm_cr4_guest_valid_bits(v->domain, false));
+               cr4, valid);
 
     if ( vmcb_get_dr6(vmcb) >> 32 )
         PRINTF("DR6: bits [63:32] are not zero (%#"PRIx64")\n",
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 95d109f962..86b8916a5d 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1529,7 +1529,7 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr,
              */
             v->arch.hvm.vmx.cr4_host_mask =
                 (HVM_CR4_HOST_MASK | X86_CR4_PKE |
-                 ~hvm_cr4_guest_valid_bits(v->domain, false));
+                 ~hvm_cr4_guest_valid_bits(v->domain));
 
             v->arch.hvm.vmx.cr4_host_mask |= v->arch.hvm.vmx.vmx_realmode ?
                                              X86_CR4_VME : 0;
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 1e51689ef3..3a37e9ebea 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -2323,7 +2323,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
         data = X86_CR4_VMXE;
         break;
     case MSR_IA32_VMX_CR4_FIXED1:
-        data = hvm_cr4_guest_valid_bits(d, false);
+        data = hvm_cr4_guest_valid_bits(d);
         break;
     case MSR_IA32_VMX_MISC:
         /* Do not support CR3-target feature now */
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index be0d8b0a4d..334bd573b9 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -334,7 +334,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
 /* Check CR4/EFER values */
 const char *hvm_efer_valid(const struct vcpu *v, uint64_t value,
                            signed int cr0_pg);
-unsigned long hvm_cr4_guest_valid_bits(const struct domain *d, bool restore);
+unsigned long hvm_cr4_guest_valid_bits(const struct domain *d);
 
 int hvm_copy_context_and_params(struct domain *src, struct domain *dst);
 
-- 
2.11.0

[PATCH 8/8] x86/cpuid: Move VMX/SVM out of the default policy

[Andrew Cooper]

Nested virt is still experimental, and requires explicitly opting in to at
domain create time.  The VMX/SVM features should not be visible by default.

Also correct them from all HVM guests, to just HAP-enabled guests.  This has
been the restriction for SVM right from the outset (c/s e006a0e0aaa), while
VMX was first introduced supporting shadow mode (c/s 9122c69c8d3) but later
adjusted to HAP-only (c/s 77751ed79e3).

There is deliberately no adjustment to xc_cpuid_apply_policy() for pre-4.14
migration compatibility.  The migration stream doesn't contain the required
architectural state for either VMX/SVM, and a nested virt VM which migrates
will explode in weird and wonderful ways.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/include/public/arch-x86/cpufeatureset.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index abd18722ee..ef7cca334d 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -127,7 +127,7 @@ XEN_CPUFEATURE(PCLMULQDQ,     1*32+ 1) /*A  Carry-less multiplication */
 XEN_CPUFEATURE(DTES64,        1*32+ 2) /*   64-bit Debug Store */
 XEN_CPUFEATURE(MONITOR,       1*32+ 3) /*   Monitor/Mwait support */
 XEN_CPUFEATURE(DSCPL,         1*32+ 4) /*   CPL Qualified Debug Store */
-XEN_CPUFEATURE(VMX,           1*32+ 5) /*S  Virtual Machine Extensions */
+XEN_CPUFEATURE(VMX,           1*32+ 5) /*h  Virtual Machine Extensions */
 XEN_CPUFEATURE(SMX,           1*32+ 6) /*   Safer Mode Extensions */
 XEN_CPUFEATURE(EIST,          1*32+ 7) /*   Enhanced SpeedStep */
 XEN_CPUFEATURE(TM2,           1*32+ 8) /*   Thermal Monitor 2 */
@@ -166,7 +166,7 @@ XEN_CPUFEATURE(3DNOW,         2*32+31) /*A  3DNow! */
 /* AMD-defined CPU features, CPUID level 0x80000001.ecx, word 3 */
 XEN_CPUFEATURE(LAHF_LM,       3*32+ 0) /*A  LAHF/SAHF in long mode */
 XEN_CPUFEATURE(CMP_LEGACY,    3*32+ 1) /*!A If yes HyperThreading not valid */
-XEN_CPUFEATURE(SVM,           3*32+ 2) /*S  Secure virtual machine */
+XEN_CPUFEATURE(SVM,           3*32+ 2) /*h  Secure virtual machine */
 XEN_CPUFEATURE(EXTAPIC,       3*32+ 3) /*   Extended APIC space */
 XEN_CPUFEATURE(CR8_LEGACY,    3*32+ 4) /*S  CR8 in 32-bit mode */
 XEN_CPUFEATURE(ABM,           3*32+ 5) /*A  Advanced bit manipulation */
-- 
2.11.0

Re: [PATCH 3/8] xen/domctl: Introduce and use XEN_DOMCTL_CDF_nested_virt

[Edwin Torok]

On Wed, 2020-09-30 at 14:42 +0100, Andrew Cooper wrote:
> Like other major areas of functionality, nested virt (or not) needs
> to be
> known at domain creation time for sensible CPUID handling, and wants
> to be
> known this early for sensible infrastructure handling in Xen.
> 
> Introduce XEN_DOMCTL_CDF_nested_virt and modify libxl to set it
> appropriately
> when creating domains.  There is no need to adjust the ARM logic to
> reject the
> use of this new flag.
> 
> No functional change yet.
> [...]
> diff --git a/tools/ocaml/libs/xc/xenctrl.ml
> b/tools/ocaml/libs/xc/xenctrl.ml
> index 497ded7ce2..e878699b0a 100644
> --- a/tools/ocaml/libs/xc/xenctrl.ml
> +++ b/tools/ocaml/libs/xc/xenctrl.ml
> @@ -64,6 +64,7 @@ type domain_create_flag =
>  	| CDF_OOS_OFF
>  	| CDF_XS_DOMAIN
>  	| CDF_IOMMU
> +	| CDF_NESTED_VIRT
>  
>  type domain_create_iommu_opts =
>  	| IOMMU_NO_SHAREPT
> diff --git a/tools/ocaml/libs/xc/xenctrl.mli
> b/tools/ocaml/libs/xc/xenctrl.mli
> index f7f6ec570d..e64907df8e 100644
> --- a/tools/ocaml/libs/xc/xenctrl.mli
> +++ b/tools/ocaml/libs/xc/xenctrl.mli
> @@ -57,6 +57,7 @@ type domain_create_flag =
>    | CDF_OOS_OFF
>    | CDF_XS_DOMAIN
>    | CDF_IOMMU
> +  | CDF_NESTED_VIRT

OCaml changes LGTM.
Just a reminder that we'll need to apply the xenctrl changes to the
mock xenctrl used by the xapi-project CI at 
https://github.com/lindig/xenctrl too.

Best regards,
--Edwin

Re: [PATCH 1/8] tools/libxl: Simplify DOMCTL_CDF_ flags handling in libxl__domain_make()

[Roger Pau Monné]

On Wed, Sep 30, 2020 at 02:42:41PM +0100, Andrew Cooper wrote:
> The use of the ternary operator serves only to obfuscate the code.  Rewrite it
> in more simple terms, avoiding the need to conditionally OR zero into the
> flags.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Might be worth adding to the log that it's a non-functional change.

Thanks, Roger.

Re: [PATCH 2/8] xen/domctl: Simplify DOMCTL_CDF_ checking logic

[Roger Pau Monné]

On Wed, Sep 30, 2020 at 02:42:42PM +0100, Andrew Cooper wrote:
> Introduce some local variables to make the resulting logic easier to follow.
> Join the two IOMMU checks in sanitise_domain_config().  Tweak some of the
> terminology for better accuracy.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

On an unrelated note, we don't seem to sanitize iommu_opts in
sanitise_domain_config like we do for flags.

Thanks, Roger.

Re: [PATCH 3/8] xen/domctl: Introduce and use XEN_DOMCTL_CDF_nested_virt

[Roger Pau Monné]

On Wed, Sep 30, 2020 at 02:42:43PM +0100, Andrew Cooper wrote:
> Like other major areas of functionality, nested virt (or not) needs to be
> known at domain creation time for sensible CPUID handling, and wants to be
> known this early for sensible infrastructure handling in Xen.
> 
> Introduce XEN_DOMCTL_CDF_nested_virt and modify libxl to set it appropriately
> when creating domains.  There is no need to adjust the ARM logic to reject the
> use of this new flag.
> 
> No functional change yet.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 4/8] tools/cpuid: Plumb nested_virt down into xc_cpuid_apply_policy()

[Roger Pau Monné]

On Wed, Sep 30, 2020 at 02:42:44PM +0100, Andrew Cooper wrote:
> Nested Virt is the final special case in legacy CPUID handling.  Pass the
> (poorly named) nested_hvm setting down into xc_cpuid_apply_policy() to break
> the semantic dependency on HVM_PARAM_NESTEDHVM.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 3/8] xen/domctl: Introduce and use XEN_DOMCTL_CDF_nested_virt

[Jan Beulich]

On 30.09.2020 15:42, Andrew Cooper wrote:
> @@ -667,6 +668,12 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>           */
>          config->flags |= XEN_DOMCTL_CDF_oos_off;
>  
> +    if ( nested_virt && !hap )
> +    {
> +        dprintk(XENLOG_INFO, "Nested virt not supported without HAP\n");
> +        return -EINVAL;
> +    }

Initially I was merely puzzled by this not being accompanied by
any removal of code elsewhere. But when I started looking I couldn't
find any such enforcement, but e.g. did find nsvm_vcpu_hostrestore()
covering the shadow mode case. For this to be "No functional change
yet" as the description claims, could you point me at where this
restriction is currently enforced?

Jan

Re: [PATCH 5/8] x86/hvm: Obsolete the use of HVM_PARAM_NESTEDHVM

[Roger Pau Monné]

On Wed, Sep 30, 2020 at 02:42:45PM +0100, Andrew Cooper wrote:
> With XEN_DOMCTL_CDF_nested_virt now passed properly to domain_create(),
> reimplement nestedhvm_enabled() to use the property which is fixed for the
> lifetime of the domain.
> 
> This makes the call to nestedhvm_vcpu_initialise() from hvm_vcpu_initialise()
> no longer dead.  It became logically dead with the Xend => XL transition, as
> they initialise HVM_PARAM_NESTEDHVM in opposite orders with respect to
> XEN_DOMCTL_max_vcpus.
> 
> There is one opencoded user of nestedhvm_enabled() in HVM_PARAM_ALTP2M's
> safety check.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

It's nice that the logic is already present in hvm_vcpu_initialise :).

Thanks, Roger.

Re: [PATCH 1/8] tools/libxl: Simplify DOMCTL_CDF_ flags handling in libxl__domain_make()

[Wei Liu]

On Wed, Sep 30, 2020 at 02:42:41PM +0100, Andrew Cooper wrote:
> The use of the ternary operator serves only to obfuscate the code.  Rewrite it
> in more simple terms, avoiding the need to conditionally OR zero into the
> flags.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Wei Liu <wl@xen.org>

Re: [PATCH 6/8] xen/xsm: Drop xsm_hvm_param_nested()

[Roger Pau Monné]

On Wed, Sep 30, 2020 at 02:42:46PM +0100, Andrew Cooper wrote:
> The sole caller has been removed.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 2/8] xen/domctl: Simplify DOMCTL_CDF_ checking logic

[Wei Liu]

On Wed, Sep 30, 2020 at 02:42:42PM +0100, Andrew Cooper wrote:
> Introduce some local variables to make the resulting logic easier to follow.
> Join the two IOMMU checks in sanitise_domain_config().  Tweak some of the
> terminology for better accuracy.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wl@xen.org>

Re: [PATCH 3/8] xen/domctl: Introduce and use XEN_DOMCTL_CDF_nested_virt

[Wei Liu]

On Wed, Sep 30, 2020 at 02:42:43PM +0100, Andrew Cooper wrote:
> Like other major areas of functionality, nested virt (or not) needs to be
> known at domain creation time for sensible CPUID handling, and wants to be
> known this early for sensible infrastructure handling in Xen.
> 
> Introduce XEN_DOMCTL_CDF_nested_virt and modify libxl to set it appropriately
> when creating domains.  There is no need to adjust the ARM logic to reject the
> use of this new flag.
> 
> No functional change yet.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Wei Liu <wl@xen.org>

Re: [PATCH 4/8] tools/cpuid: Plumb nested_virt down into xc_cpuid_apply_policy()

[Wei Liu]

On Wed, Sep 30, 2020 at 02:42:44PM +0100, Andrew Cooper wrote:
> Nested Virt is the final special case in legacy CPUID handling.  Pass the
> (poorly named) nested_hvm setting down into xc_cpuid_apply_policy() to break
> the semantic dependency on HVM_PARAM_NESTEDHVM.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

This has gone in.

Re: [PATCH 5/8] x86/hvm: Obsolete the use of HVM_PARAM_NESTEDHVM

[Wei Liu]

On Wed, Sep 30, 2020 at 02:42:45PM +0100, Andrew Cooper wrote:
> With XEN_DOMCTL_CDF_nested_virt now passed properly to domain_create(),
> reimplement nestedhvm_enabled() to use the property which is fixed for the
> lifetime of the domain.
> 
> This makes the call to nestedhvm_vcpu_initialise() from hvm_vcpu_initialise()
> no longer dead.  It became logically dead with the Xend => XL transition, as
> they initialise HVM_PARAM_NESTEDHVM in opposite orders with respect to
> XEN_DOMCTL_max_vcpus.
> 
> There is one opencoded user of nestedhvm_enabled() in HVM_PARAM_ALTP2M's
> safety check.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wl@xen.org>

Re: [PATCH 6/8] xen/xsm: Drop xsm_hvm_param_nested()

[Wei Liu]

On Wed, Sep 30, 2020 at 02:42:46PM +0100, Andrew Cooper wrote:
> The sole caller has been removed.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Wei Liu <wl@xen.org>

Re: [PATCH 7/8] x86/hvm: Drop restore boolean from hvm_cr4_guest_valid_bits()

[Roger Pau Monné]

On Wed, Sep 30, 2020 at 02:42:47PM +0100, Andrew Cooper wrote:
> Previously, migration was reordered so the CPUID data was available before
> register state.  nestedhvm_enabled() has recently been made accurate for the
> entire lifetime of the domain.
> 
> Therefore, we can drop the bodge in hvm_cr4_guest_valid_bits() which existed
> previously to tolerate a guests' CR4 being set/restored before
> HVM_PARAM_NESTEDHVM.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, just one nit below.

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> ---
>  xen/arch/x86/hvm/domain.c       | 2 +-
>  xen/arch/x86/hvm/hvm.c          | 8 ++++----
>  xen/arch/x86/hvm/svm/svmdebug.c | 6 ++++--
>  xen/arch/x86/hvm/vmx/vmx.c      | 2 +-
>  xen/arch/x86/hvm/vmx/vvmx.c     | 2 +-
>  xen/include/asm-x86/hvm/hvm.h   | 2 +-
>  6 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
> index 8e3375265c..0ce132b308 100644
> --- a/xen/arch/x86/hvm/domain.c
> +++ b/xen/arch/x86/hvm/domain.c
> @@ -275,7 +275,7 @@ int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
>      if ( v->arch.hvm.guest_efer & EFER_LME )
>          v->arch.hvm.guest_efer |= EFER_LMA;
>  
> -    if ( v->arch.hvm.guest_cr[4] & ~hvm_cr4_guest_valid_bits(d, false) )
> +    if ( v->arch.hvm.guest_cr[4] & ~hvm_cr4_guest_valid_bits(d) )
>      {
>          gprintk(XENLOG_ERR, "Bad CR4 value: %#016lx\n",
>                  v->arch.hvm.guest_cr[4]);
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 101a739952..54e32e4fe8 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -972,14 +972,14 @@ const char *hvm_efer_valid(const struct vcpu *v, uint64_t value,
>          X86_CR0_CD | X86_CR0_PG)))
>  
>  /* These bits in CR4 can be set by the guest. */
> -unsigned long hvm_cr4_guest_valid_bits(const struct domain *d, bool restore)
> +unsigned long hvm_cr4_guest_valid_bits(const struct domain *d)
>  {
>      const struct cpuid_policy *p = d->arch.cpuid;
>      bool mce, vmxe;
>  
>      /* Logic broken out simply to aid readability below. */
>      mce  = p->basic.mce || p->basic.mca;
> -    vmxe = p->basic.vmx && (restore || nestedhvm_enabled(d));
> +    vmxe = p->basic.vmx && nestedhvm_enabled(d);
>  
>      return ((p->basic.vme     ? X86_CR4_VME | X86_CR4_PVI : 0) |
>              (p->basic.tsc     ? X86_CR4_TSD               : 0) |
> @@ -1033,7 +1033,7 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>          return -EINVAL;
>      }
>  
> -    if ( ctxt.cr4 & ~hvm_cr4_guest_valid_bits(d, true) )
> +    if ( ctxt.cr4 & ~hvm_cr4_guest_valid_bits(d) )
>      {
>          printk(XENLOG_G_ERR "HVM%d restore: bad CR4 %#" PRIx64 "\n",
>                 d->domain_id, ctxt.cr4);
> @@ -2425,7 +2425,7 @@ int hvm_set_cr4(unsigned long value, bool may_defer)
>      struct vcpu *v = current;
>      unsigned long old_cr;
>  
> -    if ( value & ~hvm_cr4_guest_valid_bits(v->domain, false) )
> +    if ( value & ~hvm_cr4_guest_valid_bits(v->domain) )
>      {
>          HVM_DBG_LOG(DBG_LEVEL_1,
>                      "Guest attempts to set reserved bit in CR4: %lx",
> diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c
> index ba26b6a80b..f450391df4 100644
> --- a/xen/arch/x86/hvm/svm/svmdebug.c
> +++ b/xen/arch/x86/hvm/svm/svmdebug.c
> @@ -106,6 +106,7 @@ bool svm_vmcb_isvalid(const char *from, const struct vmcb_struct *vmcb,
>      unsigned long cr0 = vmcb_get_cr0(vmcb);
>      unsigned long cr3 = vmcb_get_cr3(vmcb);
>      unsigned long cr4 = vmcb_get_cr4(vmcb);
> +    unsigned long valid;

Could you init valid here at definition time? Also cr4_valid might be
a better name since the sacope of the variable is quite wide.

Thanks, Roger.

Re: [PATCH 3/8] xen/domctl: Introduce and use XEN_DOMCTL_CDF_nested_virt

[Andrew Cooper]

On 01/10/2020 11:23, Jan Beulich wrote:
> On 30.09.2020 15:42, Andrew Cooper wrote:
>> @@ -667,6 +668,12 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>>           */
>>          config->flags |= XEN_DOMCTL_CDF_oos_off;
>>  
>> +    if ( nested_virt && !hap )
>> +    {
>> +        dprintk(XENLOG_INFO, "Nested virt not supported without HAP\n");
>> +        return -EINVAL;
>> +    }
> Initially I was merely puzzled by this not being accompanied by
> any removal of code elsewhere. But when I started looking I couldn't
> find any such enforcement, but e.g. did find nsvm_vcpu_hostrestore()
> covering the shadow mode case. For this to be "No functional change
> yet" as the description claims, could you point me at where this
> restriction is currently enforced?

Currently enforced in the HVM_PARAM_NESTEDHVM write side effect, which
is deleted in patch 5.

~Andrew

Re: [PATCH 8/8] x86/cpuid: Move VMX/SVM out of the default policy

[Roger Pau Monné]

On Wed, Sep 30, 2020 at 02:42:48PM +0100, Andrew Cooper wrote:
> Nested virt is still experimental, and requires explicitly opting in to at
> domain create time.  The VMX/SVM features should not be visible by default.
> 
> Also correct them from all HVM guests, to just HAP-enabled guests.  This has
> been the restriction for SVM right from the outset (c/s e006a0e0aaa), while
> VMX was first introduced supporting shadow mode (c/s 9122c69c8d3) but later
> adjusted to HAP-only (c/s 77751ed79e3).
> 
> There is deliberately no adjustment to xc_cpuid_apply_policy() for pre-4.14
> migration compatibility.  The migration stream doesn't contain the required
> architectural state for either VMX/SVM, and a nested virt VM which migrates
> will explode in weird and wonderful ways.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 3/8] xen/domctl: Introduce and use XEN_DOMCTL_CDF_nested_virt

[Christian Lindig]

--
Acked-by: Christian Lindig <christian.lindig@citrix.com>

________________________________________
From: Andrew Cooper <Andrew.Cooper3@citrix.com>
Sent: 01 October 2020 12:02
To: Jan Beulich
Cc: Xen-devel; Roger Pau Monne; Wei Liu; Ian Jackson; Christian Lindig; Edwin Torok; Rob Hoes
Subject: Re: [PATCH 3/8] xen/domctl: Introduce and use XEN_DOMCTL_CDF_nested_virt

On 01/10/2020 11:23, Jan Beulich wrote:
> On 30.09.2020 15:42, Andrew Cooper wrote:
>> @@ -667,6 +668,12 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>>           */
>>          config->flags |= XEN_DOMCTL_CDF_oos_off;
>>
>> +    if ( nested_virt && !hap )
>> +    {
>> +        dprintk(XENLOG_INFO, "Nested virt not supported without HAP\n");
>> +        return -EINVAL;
>> +    }
> Initially I was merely puzzled by this not being accompanied by
> any removal of code elsewhere. But when I started looking I couldn't
> find any such enforcement, but e.g. did find nsvm_vcpu_hostrestore()
> covering the shadow mode case. For this to be "No functional change
> yet" as the description claims, could you point me at where this
> restriction is currently enforced?

Currently enforced in the HVM_PARAM_NESTEDHVM write side effect, which
is deleted in patch 5.

~Andrew

Re: [PATCH 7/8] x86/hvm: Drop restore boolean from hvm_cr4_guest_valid_bits()

[Andrew Cooper]

On 01/10/2020 12:00, Roger Pau Monné wrote:
> On Wed, Sep 30, 2020 at 02:42:47PM +0100, Andrew Cooper wrote:
>> Previously, migration was reordered so the CPUID data was available before
>> register state.  nestedhvm_enabled() has recently been made accurate for the
>> entire lifetime of the domain.
>>
>> Therefore, we can drop the bodge in hvm_cr4_guest_valid_bits() which existed
>> previously to tolerate a guests' CR4 being set/restored before
>> HVM_PARAM_NESTEDHVM.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks,

>
> Thanks, just one nit below.
>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Jun Nakajima <jun.nakajima@intel.com>
>> CC: Kevin Tian <kevin.tian@intel.com>
>> ---
>>  xen/arch/x86/hvm/domain.c       | 2 +-
>>  xen/arch/x86/hvm/hvm.c          | 8 ++++----
>>  xen/arch/x86/hvm/svm/svmdebug.c | 6 ++++--
>>  xen/arch/x86/hvm/vmx/vmx.c      | 2 +-
>>  xen/arch/x86/hvm/vmx/vvmx.c     | 2 +-
>>  xen/include/asm-x86/hvm/hvm.h   | 2 +-
>>  6 files changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
>> index 8e3375265c..0ce132b308 100644
>> --- a/xen/arch/x86/hvm/domain.c
>> +++ b/xen/arch/x86/hvm/domain.c
>> @@ -275,7 +275,7 @@ int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
>>      if ( v->arch.hvm.guest_efer & EFER_LME )
>>          v->arch.hvm.guest_efer |= EFER_LMA;
>>  
>> -    if ( v->arch.hvm.guest_cr[4] & ~hvm_cr4_guest_valid_bits(d, false) )
>> +    if ( v->arch.hvm.guest_cr[4] & ~hvm_cr4_guest_valid_bits(d) )
>>      {
>>          gprintk(XENLOG_ERR, "Bad CR4 value: %#016lx\n",
>>                  v->arch.hvm.guest_cr[4]);
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 101a739952..54e32e4fe8 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -972,14 +972,14 @@ const char *hvm_efer_valid(const struct vcpu *v, uint64_t value,
>>          X86_CR0_CD | X86_CR0_PG)))
>>  
>>  /* These bits in CR4 can be set by the guest. */
>> -unsigned long hvm_cr4_guest_valid_bits(const struct domain *d, bool restore)
>> +unsigned long hvm_cr4_guest_valid_bits(const struct domain *d)
>>  {
>>      const struct cpuid_policy *p = d->arch.cpuid;
>>      bool mce, vmxe;
>>  
>>      /* Logic broken out simply to aid readability below. */
>>      mce  = p->basic.mce || p->basic.mca;
>> -    vmxe = p->basic.vmx && (restore || nestedhvm_enabled(d));
>> +    vmxe = p->basic.vmx && nestedhvm_enabled(d);
>>  
>>      return ((p->basic.vme     ? X86_CR4_VME | X86_CR4_PVI : 0) |
>>              (p->basic.tsc     ? X86_CR4_TSD               : 0) |
>> @@ -1033,7 +1033,7 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>>          return -EINVAL;
>>      }
>>  
>> -    if ( ctxt.cr4 & ~hvm_cr4_guest_valid_bits(d, true) )
>> +    if ( ctxt.cr4 & ~hvm_cr4_guest_valid_bits(d) )
>>      {
>>          printk(XENLOG_G_ERR "HVM%d restore: bad CR4 %#" PRIx64 "\n",
>>                 d->domain_id, ctxt.cr4);
>> @@ -2425,7 +2425,7 @@ int hvm_set_cr4(unsigned long value, bool may_defer)
>>      struct vcpu *v = current;
>>      unsigned long old_cr;
>>  
>> -    if ( value & ~hvm_cr4_guest_valid_bits(v->domain, false) )
>> +    if ( value & ~hvm_cr4_guest_valid_bits(v->domain) )
>>      {
>>          HVM_DBG_LOG(DBG_LEVEL_1,
>>                      "Guest attempts to set reserved bit in CR4: %lx",
>> diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c
>> index ba26b6a80b..f450391df4 100644
>> --- a/xen/arch/x86/hvm/svm/svmdebug.c
>> +++ b/xen/arch/x86/hvm/svm/svmdebug.c
>> @@ -106,6 +106,7 @@ bool svm_vmcb_isvalid(const char *from, const struct vmcb_struct *vmcb,
>>      unsigned long cr0 = vmcb_get_cr0(vmcb);
>>      unsigned long cr3 = vmcb_get_cr3(vmcb);
>>      unsigned long cr4 = vmcb_get_cr4(vmcb);
>> +    unsigned long valid;
> Could you init valid here at definition time? Also cr4_valid might be
> a better name since the sacope of the variable is quite wide.

I have some further cleanup in mind, which is why I did it like this.

~Andrew