[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 3/5] xen/domain: Stricter configuration checking



Hi Andrew,

On 12/11/2018 16:16, Andrew Cooper wrote:
Currently, a number of options passed for domain creation are ignored, or have
implicit fallback behaviour.  This is bad for forwards compatibility, and for
end users to be certain that they got the configuration they asked for.

With this change:
  * ARM now strictly requires that XEN_DOMCTL_CDF_hap is passed.  Previously,
    only XEN_DOMCTL_CDF_hvm_guest was checked.
  * For x86, requesting HAP without HVM is now prohibited, as the combination
    makes no sense.
  * For x86, requesting HAP on a non-HAP capable system will fail, rather than
    silently fall back to Shadow.

Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

For the Arm bits:

Acked-by: Julien Grall <julien.grall@xxxxxxx>

Cheers,

---
CC: Jan Beulich <JBeulich@xxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
CC: Julien Grall <julien.grall@xxxxxxx>
CC: Ian Jackson <Ian.Jackson@xxxxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>

Semi RFC because this may cause a user-visible change in behaviour.  However,
if the user has gone to the effort of specifying hap=1, silently falling back
to shadow is unexpected, and IMO, a bug.

Alternatively, if this proves to be controversial, it can be dropped from the
series to avoid blocking the main bugfix.

v2:
  * New
---
  xen/arch/arm/domain.c |  7 +++++++
  xen/arch/x86/domain.c | 40 ++++++++++++++++++++++++++++++++++++++++
  xen/common/domain.c   | 34 +++-------------------------------
  3 files changed, 50 insertions(+), 31 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index c24ace6..08ba412 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -601,6 +601,13 @@ void vcpu_switch_to_aarch64_mode(struct vcpu *v)
int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
  {
+    if ( !(config->flags & XEN_DOMCTL_CDF_hvm_guest) ||
+         !(config->flags & XEN_DOMCTL_CDF_hap) )
+    {
+        dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", config->flags);
+        return -EINVAL;
+    }
+
      /* Fill in the native GIC version, passed back to the toolstack. */
      if ( config->arch.gic_version == XEN_DOMCTL_CONFIG_GIC_NATIVE )
      {
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 28a145a..f47ad04 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -420,6 +420,46 @@ void arch_vcpu_destroy(struct vcpu *v)
int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
  {
+    bool hvm;
+
+    if ( !IS_ENABLED(CONFIG_PV) && !(config->flags & XEN_DOMCTL_CDF_hvm_guest) 
)
+    {
+        dprintk(XENLOG_INFO, "PV support not available\n");
+        return -EINVAL;
+    }
+
+    if ( !hvm_enabled && (config->flags & XEN_DOMCTL_CDF_hvm_guest) )
+    {
+        dprintk(XENLOG_INFO, "HVM support not available\n");
+        return -EINVAL;
+    }
+
+    hvm = config->flags & XEN_DOMCTL_CDF_hvm_guest;
+
+    if ( !hvm )
+    {
+        if ( config->flags & XEN_DOMCTL_CDF_hap )
+        {
+            dprintk(XENLOG_INFO, "HAP inapplicable for PV guests\n");
+            return -EINVAL;
+        }
+    }
+    else
+    {
+        if ( !IS_ENABLED(CONFIG_SHADOW_PAGING) &&
+             !(config->flags & XEN_DOMCTL_CDF_hap) )
+        {
+            dprintk(XENLOG_INFO, "SHADOW support not available\n");
+            return -EINVAL;
+        }
+
+        if ( !hvm_hap_supported() && (config->flags & XEN_DOMCTL_CDF_hap) )
+        {
+            dprintk(XENLOG_INFO, "HAP support not available\n");
+            return -EINVAL;
+        }
+    }
+
      return 0;
  }
diff --git a/xen/common/domain.c b/xen/common/domain.c
index ddaf74a..f69f405 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -339,37 +339,9 @@ struct domain *domain_create(domid_t domid,
          hardware_domain = d;
      }
- /* Sort out our idea of is_{pv,hvm}_domain(). */
-    if ( config )
-    {
-        if ( config->flags & XEN_DOMCTL_CDF_hvm_guest )
-        {
-#ifdef CONFIG_HVM
-            d->guest_type = guest_type_hvm;
-#else
-            err = -EINVAL;
-            goto fail;
-#endif
-        }
-        else
-        {
-#ifdef CONFIG_PV
-            d->guest_type = guest_type_pv;
-#else
-            err = -EINVAL;
-            goto fail;
-#endif
-        }
-    }
-    else
-    {
-        /*
-         * At least the idle domain should be treated as PV domain
-         * because it uses PV context switch functions. To err on the
-         * safe side, leave all system domains to be guest_type_pv.
-         */
-        d->guest_type = guest_type_pv;
-    }
+    /* Sort out our idea of is_{pv,hvm}_domain().  All system domains are PV. 
*/
+    d->guest_type = ((config && (config->flags & XEN_DOMCTL_CDF_hvm_guest))
+                     ? guest_type_hvm : guest_type_pv);
TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.