[PATCH v2 0/5] xen/domain: Allocate d->vcpu[] earlier during domain construction

[PATCH v2 0/5] xen/domain: Allocate d->vcpu[] earlier during domain construction

[Andrew Cooper]

To fix an order-of-construction issue with gic-v3 on ARM, arrange for
d->max_vcpus to be auditied and set up prior to arch_domain_create()

This is slightly-RFC because all of the interesting changes are in ARM, and
therefore only compile tested by me at this point.

This can be found in git tree from from:

  http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/xen-alloc-vcpus-v2

Andrew Cooper (5):
  xen/domain: Introduce a new sanitise_domain_config() helper
  xen/domain: Introduce a new arch_sanitise_domain_config() helper
  xen/domain: Stricter configuration checking
  xen/domain: Allocate d->vcpu[] earlier during domain_create()
  Revert "xen/arm: vgic-v3: Delay the initialization of the domain information"

 xen/arch/arm/domain.c         | 70 +++++++++++++++++++++++++-----------
 xen/arch/arm/vgic-v2.c        |  1 -
 xen/arch/arm/vgic-v3.c        | 34 ++----------------
 xen/arch/arm/vgic.c           | 22 ++++++++++--
 xen/arch/arm/vgic/vgic-init.c |  3 --
 xen/arch/arm/vgic/vgic.c      |  7 ++--
 xen/arch/x86/domain.c         | 55 ++++++++++++++++++++++++++++
 xen/common/domain.c           | 83 +++++++++++++++++++++----------------------
 xen/common/domctl.c           |  9 -----
 xen/include/asm-arm/domain.h  |  6 ----
 xen/include/asm-arm/vgic.h    |  5 ++-
 xen/include/asm-x86/domain.h  |  2 --
 xen/include/xen/sched.h       |  6 ++++
 13 files changed, 179 insertions(+), 124 deletions(-)

-- 
2.1.4


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

[PATCH v2 1/5] xen/domain: Introduce a new sanitise_domain_config() helper

[Andrew Cooper]

Call it from the head of domain_create() (before doing any memory
allocations), which will apply the checks to dom0 as well as domU's.

For now, just subsume the XEN_DOMCTL_CDF_* check from XEN_DOMCTL_createdomain.

In an effort to aid future developoment, leave a debug printk() identifying
the cause of sanitisation failures.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

v2:
 * Rename to sanitise_domain_config()
 * Leave a dprintk() behind
---
 xen/common/domain.c | 18 ++++++++++++++++++
 xen/common/domctl.c |  9 ---------
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index d6650f0..22aa634 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -288,6 +288,21 @@ static void _domain_destroy(struct domain *d)
     free_domain_struct(d);
 }
 
+static int sanitise_domain_config(struct xen_domctl_createdomain *config)
+{
+    if ( config->flags & ~(XEN_DOMCTL_CDF_hvm_guest |
+                           XEN_DOMCTL_CDF_hap |
+                           XEN_DOMCTL_CDF_s3_integrity |
+                           XEN_DOMCTL_CDF_oos_off |
+                           XEN_DOMCTL_CDF_xs_domain) )
+    {
+        dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags);
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
 struct domain *domain_create(domid_t domid,
                              struct xen_domctl_createdomain *config,
                              bool is_priv)
@@ -297,6 +312,9 @@ struct domain *domain_create(domid_t domid,
            INIT_evtchn = 1u<<3, INIT_gnttab = 1u<<4, INIT_arch = 1u<<5 };
     int err, init_status = 0;
 
+    if ( config && (err = sanitise_domain_config(config)) )
+        return ERR_PTR(err);
+
     if ( (d = alloc_domain_struct()) == NULL )
         return ERR_PTR(-ENOMEM);
 
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index b294881..d08b627 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -498,15 +498,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         domid_t        dom;
         static domid_t rover = 0;
 
-        ret = -EINVAL;
-        if ( (op->u.createdomain.flags &
-             ~(XEN_DOMCTL_CDF_hvm_guest
-               | XEN_DOMCTL_CDF_hap
-               | XEN_DOMCTL_CDF_s3_integrity
-               | XEN_DOMCTL_CDF_oos_off
-               | XEN_DOMCTL_CDF_xs_domain)) )
-            break;
-
         dom = op->domain;
         if ( (dom > 0) && (dom < DOMID_FIRST_RESERVED) )
         {
-- 
2.1.4


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

[PATCH v2 2/5] xen/domain: Introduce a new arch_sanitise_domain_config() helper

[Andrew Cooper]

On the ARM side, lift the code to select the appropriate GIC version when
NATIVE is requested.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Julien Grall <julien.grall@arm.com>
---
CC: Wei Liu <wei.liu2@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>

v2:
 * Rename to arch_sanitise_domain_config()
 * Use ASSERT_UNREACHABLE() in preference to BUG()
---
 xen/arch/arm/domain.c   | 45 +++++++++++++++++++++++++--------------------
 xen/arch/x86/domain.c   |  5 +++++
 xen/common/domain.c     |  2 +-
 xen/include/xen/sched.h |  6 ++++++
 4 files changed, 37 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 8043287..c24ace6 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -599,6 +599,30 @@ void vcpu_switch_to_aarch64_mode(struct vcpu *v)
     v->arch.hcr_el2 |= HCR_RW;
 }
 
+int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
+{
+    /* Fill in the native GIC version, passed back to the toolstack. */
+    if ( config->arch.gic_version == XEN_DOMCTL_CONFIG_GIC_NATIVE )
+    {
+        switch ( gic_hw_version() )
+        {
+        case GIC_V2:
+            config->arch.gic_version = XEN_DOMCTL_CONFIG_GIC_V2;
+            break;
+
+        case GIC_V3:
+            config->arch.gic_version = XEN_DOMCTL_CONFIG_GIC_V3;
+            break;
+
+        default:
+            ASSERT_UNREACHABLE();
+            return -EINVAL;
+        }
+    }
+
+    return 0;
+}
+
 int arch_domain_create(struct domain *d,
                        struct xen_domctl_createdomain *config)
 {
@@ -629,24 +653,6 @@ int arch_domain_create(struct domain *d,
 
     switch ( config->arch.gic_version )
     {
-    case XEN_DOMCTL_CONFIG_GIC_NATIVE:
-        switch ( gic_hw_version () )
-        {
-        case GIC_V2:
-            config->arch.gic_version = XEN_DOMCTL_CONFIG_GIC_V2;
-            d->arch.vgic.version = GIC_V2;
-            break;
-
-        case GIC_V3:
-            config->arch.gic_version = XEN_DOMCTL_CONFIG_GIC_V3;
-            d->arch.vgic.version = GIC_V3;
-            break;
-
-        default:
-            BUG();
-        }
-        break;
-
     case XEN_DOMCTL_CONFIG_GIC_V2:
         d->arch.vgic.version = GIC_V2;
         break;
@@ -656,8 +662,7 @@ int arch_domain_create(struct domain *d,
         break;
 
     default:
-        rc = -EOPNOTSUPP;
-        goto fail;
+        BUG();
     }
 
     if ( (rc = domain_vgic_register(d, &count)) != 0 )
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index f6fe954..28a145a 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -418,6 +418,11 @@ void arch_vcpu_destroy(struct vcpu *v)
         pv_vcpu_destroy(v);
 }
 
+int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
+{
+    return 0;
+}
+
 static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
 {
 #ifdef CONFIG_HVM
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 22aa634..ddaf74a 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -300,7 +300,7 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }
 
-    return 0;
+    return arch_sanitise_domain_config(config);
 }
 
 struct domain *domain_create(domid_t domid,
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 3171eab..d66fdb4 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -539,6 +539,12 @@ int domain_set_node_affinity(struct domain *d, const nodemask_t *affinity);
 void domain_update_node_affinity(struct domain *d);
 
 /*
+ * To be implemented by each architecture, sanity checking the configuration
+ * and filling in any appropriate defaults.
+ */
+int arch_sanitise_domain_config(struct xen_domctl_createdomain *config);
+
+/*
  * Create a domain: the configuration is only necessary for real domain
  * (domid < DOMID_FIRST_RESERVED).
  */
-- 
2.1.4


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

[PATCH v2 3/5] xen/domain: Stricter configuration checking

[Andrew Cooper]

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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

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);
 
-- 
2.1.4


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

[PATCH v2 4/5] xen/domain: Allocate d->vcpu[] earlier during domain_create()

[Andrew Cooper]

The ARM code has a chicken-and-egg problem.  One of the vGIC_v3 emulations
wants to know d->max_vcpus to be able to size itself appropriately, but the
current order of initialisation requires the vGIC to be set up before the
requested number of vcpus can be checked.

Move the range checking of config->max_vcpus into sanitise_domain_config()
path, which allows for the allocation of d->vcpu[] and d->max_vcpus to happen
earlier during create, and in particular, before the call to
arch_domain_create().

The x86 side is fairly easy, and implements the logical equivalent of
domain_max_vcpus() but using XEN_DOMCTL_CDF_hvm_guest rather than
is_hvm_domain().

For the ARM side, re-purpose vgic_max_vcpus() to take a domctl vGIC version,
and return the maximum number of supported vCPUs, reusing 0 for "version not
supported".  To avoid exporting the vgic_ops structures (which are in the
process of being replaced), hard code the upper limits.

This allows for the removal of the domain_max_vcpus() infrastructure, which is
done to prevent it being reused incorrectly in the future.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/domain.c         | 18 ++++++++++++++++++
 xen/arch/arm/vgic-v2.c        |  1 -
 xen/arch/arm/vgic-v3.c        |  5 -----
 xen/arch/arm/vgic.c           | 22 ++++++++++++++++++++--
 xen/arch/arm/vgic/vgic-init.c |  3 ---
 xen/arch/arm/vgic/vgic.c      |  7 ++++---
 xen/arch/x86/domain.c         | 10 ++++++++++
 xen/common/domain.c           | 33 ++++++++++++++++++++-------------
 xen/include/asm-arm/domain.h  |  6 ------
 xen/include/asm-arm/vgic.h    |  5 ++---
 xen/include/asm-x86/domain.h  |  2 --
 11 files changed, 74 insertions(+), 38 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 08ba412..24123ff 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -601,6 +601,8 @@ void vcpu_switch_to_aarch64_mode(struct vcpu *v)
 
 int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
 {
+    unsigned int max_vcpus;
+
     if ( !(config->flags & XEN_DOMCTL_CDF_hvm_guest) ||
          !(config->flags & XEN_DOMCTL_CDF_hap) )
     {
@@ -627,6 +629,22 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
         }
     }
 
+    /* Calculate the maximum number of vcpus from the selected GIC version. */
+    max_vcpus = vgic_max_vcpus(config->arch.gic_version);
+
+    if ( max_vcpus == 0 )
+    {
+        dprintk(XENLOG_INFO, "Unsupported GIC version\n");
+        return -EINVAL;
+    }
+
+    if ( config->max_vcpus > max_vcpus )
+    {
+        dprintk(XENLOG_INFO, "Requested vCPUs (%u) exceeds max (%u)\n",
+                config->max_vcpus, max_vcpus);
+        return -EINVAL;
+    }
+
     return 0;
 }
 
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index bf77899..64b141f 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -725,7 +725,6 @@ static const struct vgic_ops vgic_v2_ops = {
     .domain_free = vgic_v2_domain_free,
     .lpi_to_pending = vgic_v2_lpi_to_pending,
     .lpi_get_priority = vgic_v2_lpi_get_priority,
-    .max_vcpus = 8,
 };
 
 int vgic_v2_init(struct domain *d, int *mmio_count)
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index c14bcd8..519cc72 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1822,11 +1822,6 @@ static const struct vgic_ops v3_ops = {
     .emulate_reg  = vgic_v3_emulate_reg,
     .lpi_to_pending = vgic_v3_lpi_to_pending,
     .lpi_get_priority = vgic_v3_lpi_get_priority,
-    /*
-     * We use both AFF1 and AFF0 in (v)MPIDR. Thus, the max number of CPU
-     * that can be supported is up to 4096(==256*16) in theory.
-     */
-    .max_vcpus = 4096,
 };
 
 int vgic_v3_init(struct domain *d, int *mmio_count)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 5a4f082..892445e 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -667,9 +667,27 @@ void vgic_free_virq(struct domain *d, unsigned int virq)
     clear_bit(virq, d->arch.vgic.allocated_irqs);
 }
 
-unsigned int vgic_max_vcpus(const struct domain *d)
+unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)
 {
-    return min_t(unsigned int, MAX_VIRT_CPUS, d->arch.vgic.handler->max_vcpus);
+    unsigned int max_vcpus;
+
+    switch ( domctl_vgic_version )
+    {
+    case XEN_DOMCTL_CONFIG_GIC_V2:
+        max_vcpus = 8;
+        break;
+
+#ifdef CONFIG_GICV3
+    case XEN_DOMCTL_CONFIG_GIC_V3:
+        max_vcpus = 4092;
+        break;
+#endif
+
+    default:
+        return 0;
+    }
+
+    return min_t(unsigned int, MAX_VIRT_CPUS, max_vcpus);
 }
 
 /*
diff --git a/xen/arch/arm/vgic/vgic-init.c b/xen/arch/arm/vgic/vgic-init.c
index bfd3d09..62ae553 100644
--- a/xen/arch/arm/vgic/vgic-init.c
+++ b/xen/arch/arm/vgic/vgic-init.c
@@ -112,9 +112,6 @@ int domain_vgic_register(struct domain *d, int *mmio_count)
         BUG();
     }
 
-    if ( d->max_vcpus > domain_max_vcpus(d) )
-        return -E2BIG;
-
     d->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
     d->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
     d->arch.vgic.vgic_redist_base = VGIC_ADDR_UNDEF;
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index 7c3cfc5..c7a7b38 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -949,17 +949,18 @@ void vgic_sync_hardware_irq(struct domain *d,
     spin_unlock_irqrestore(&desc->lock, flags);
 }
 
-unsigned int vgic_max_vcpus(const struct domain *d)
+unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)
 {
     unsigned int vgic_vcpu_limit;
 
     switch ( d->arch.vgic.version )
     {
-    case GIC_V2:
+    case XEN_DOMCTL_CONFIG_GIC_V2:
         vgic_vcpu_limit = VGIC_V2_MAX_CPUS;
         break;
+
     default:
-        BUG();
+        return 0;
     }
 
     return min_t(unsigned int, MAX_VIRT_CPUS, vgic_vcpu_limit);
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index f47ad04..8051890 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -421,6 +421,7 @@ void arch_vcpu_destroy(struct vcpu *v)
 int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
 {
     bool hvm;
+    unsigned int max_vcpus;
 
     if ( !IS_ENABLED(CONFIG_PV) && !(config->flags & XEN_DOMCTL_CDF_hvm_guest) )
     {
@@ -460,6 +461,15 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
         }
     }
 
+    max_vcpus = hvm ? HVM_MAX_VCPUS : MAX_VIRT_CPUS;
+
+    if ( config->max_vcpus > max_vcpus )
+    {
+        dprintk(XENLOG_INFO, "Requested vCPUs (%u) exceeds max (%u)\n",
+                config->max_vcpus, max_vcpus);
+        return -EINVAL;
+    }
+
     return 0;
 }
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index f69f405..78cc524 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -300,6 +300,12 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }
 
+    if ( config->max_vcpus < 1 )
+    {
+        dprintk(XENLOG_INFO, "No vCPUS\n");
+        return -EINVAL;
+    }
+
     return arch_sanitise_domain_config(config);
 }
 
@@ -345,6 +351,20 @@ struct domain *domain_create(domid_t domid,
 
     TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
 
+    /*
+     * Allocate d->vcpu[] and set ->max_vcpus up early.  Various per-domain
+     * resources want to be sized based on max_vcpus.
+     */
+    if ( !is_system_domain(d) )
+    {
+        err = -ENOMEM;
+        d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus);
+        if ( !d->vcpu )
+            goto fail;
+
+        d->max_vcpus = config->max_vcpus;
+    }
+
     lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid, "Domain");
 
     if ( (err = xsm_alloc_security_domain(d)) != 0 )
@@ -396,19 +416,6 @@ struct domain *domain_create(domid_t domid,
 
     if ( !is_idle_domain(d) )
     {
-        /* Check d->max_vcpus and allocate d->vcpu[]. */
-        err = -EINVAL;
-        if ( config->max_vcpus < 1 ||
-             config->max_vcpus > domain_max_vcpus(d) )
-            goto fail;
-
-        err = -ENOMEM;
-        d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus);
-        if ( !d->vcpu )
-            goto fail;
-
-        d->max_vcpus = config->max_vcpus;
-
         watchdog_domain_init(d);
         init_status |= INIT_watchdog;
 
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index d682307..175de44 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -208,12 +208,6 @@ void vcpu_show_execution_state(struct vcpu *);
 void vcpu_show_registers(const struct vcpu *);
 void vcpu_switch_to_aarch64_mode(struct vcpu *);
 
-/* On ARM, the number of VCPUs is limited by the type of GIC emulated. */
-static inline unsigned int domain_max_vcpus(const struct domain *d)
-{
-    return vgic_max_vcpus(d);
-}
-
 /*
  * Due to the restriction of GICv3, the number of vCPUs in AFF0 is
  * limited to 16, thus only the first 4 bits of AFF0 are legal. We will
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 56ed5fe..447d24e 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -234,8 +234,6 @@ struct vgic_ops {
     /* lookup the struct pending_irq for a given LPI interrupt */
     struct pending_irq *(*lpi_to_pending)(struct domain *d, unsigned int vlpi);
     int (*lpi_get_priority)(struct domain *d, uint32_t vlpi);
-    /* Maximum number of vCPU supported */
-    const unsigned int max_vcpus;
 };
 
 /* Number of ranks of interrupt registers for a domain */
@@ -350,7 +348,8 @@ extern void vgic_clear_pending_irqs(struct vcpu *v);
 
 extern bool vgic_emulate(struct cpu_user_regs *regs, union hsr hsr);
 
-unsigned int vgic_max_vcpus(const struct domain *d);
+/* Maximum vCPUs for a specific vGIC version, or 0 for unsupported. */
+unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version);
 
 void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
                       paddr_t vbase, uint32_t aliased_offset);
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 643e69a..277f99f 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -664,8 +664,6 @@ unsigned long pv_guest_cr4_to_real_cr4(const struct vcpu *v);
              X86_CR4_OSXSAVE | X86_CR4_SMEP |               \
              X86_CR4_FSGSBASE | X86_CR4_SMAP | X86_CR4_PCIDE))
 
-#define domain_max_vcpus(d) (is_hvm_domain(d) ? HVM_MAX_VCPUS : MAX_VIRT_CPUS)
-
 static inline struct vcpu_guest_context *alloc_vcpu_guest_context(void)
 {
     return vmalloc(sizeof(struct vcpu_guest_context));
-- 
2.1.4


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

[PATCH v2 5/5] Revert "xen/arm: vgic-v3: Delay the initialization of the domain information"

[Andrew Cooper]

This reverts commit 703d9d5ec13a0f487e7415174ba54e0e3ca158db.  The domain
creation logic has been adjusted to set up d->max_vcpus early enough to be
usable in vgic_v3_domain_init().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/vgic-v3.c | 29 ++---------------------------
 1 file changed, 2 insertions(+), 27 deletions(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 519cc72..474be13 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1573,11 +1573,9 @@ static const struct mmio_handler_ops vgic_distr_mmio_handler = {
     .write = vgic_v3_distr_mmio_write,
 };
 
-static int vgic_v3_real_domain_init(struct domain *d);
-
 static int vgic_v3_vcpu_init(struct vcpu *v)
 {
-    int i, rc;
+    int i;
     paddr_t rdist_base;
     struct vgic_rdist_region *region;
     unsigned int last_cpu;
@@ -1586,19 +1584,6 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
     struct domain *d = v->domain;
 
     /*
-     * This is the earliest place where the number of vCPUs is
-     * known. This is required to initialize correctly the vGIC v3
-     * domain structure. We only to do that when vCPU 0 is
-     * initilialized.
-     */
-    if ( v->vcpu_id == 0 )
-    {
-        rc = vgic_v3_real_domain_init(d);
-        if ( rc )
-            return rc;
-    }
-
-    /*
      * Find the region where the re-distributor lives. For this purpose,
      * we look one region ahead as we have only the first CPU in hand.
      */
@@ -1660,7 +1645,7 @@ static inline unsigned int vgic_v3_max_rdist_count(struct domain *d)
                GUEST_GICV3_RDIST_REGIONS;
 }
 
-static int vgic_v3_real_domain_init(struct domain *d)
+static int vgic_v3_domain_init(struct domain *d)
 {
     struct vgic_rdist_region *rdist_regions;
     int rdist_count, i, ret;
@@ -1763,16 +1748,6 @@ static int vgic_v3_real_domain_init(struct domain *d)
     return 0;
 }
 
-static int vgic_v3_domain_init(struct domain *d)
-{
-    /*
-     * The domain initialization for vGIC v3 is delayed until the first vCPU
-     * is created. This because the initialization may require to know the
-     * number of vCPUs that is not known when creating the domain.
-     */
-    return 0;
-}
-
 static void vgic_v3_domain_free(struct domain *d)
 {
     vgic_v3_its_free_domain(d);
-- 
2.1.4


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

Re: [PATCH v2 1/5] xen/domain: Introduce a new sanitise_domain_config() helper

[Jan Beulich]

>>> On 12.11.18 at 17:16, <andrew.cooper3@citrix.com> wrote:
> Call it from the head of domain_create() (before doing any memory
> allocations), which will apply the checks to dom0 as well as domU's.
> 
> For now, just subsume the XEN_DOMCTL_CDF_* check from 
> XEN_DOMCTL_createdomain.

And the needless assignment of a domain ID in case of error here
is deemed sufficiently minor? If so ...

> In an effort to aid future developoment, leave a debug printk() identifying
> the cause of sanitisation failures.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



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

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

[Jan Beulich]

>>> On 12.11.18 at 17:16, <andrew.cooper3@citrix.com> 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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Ian Jackson <Ian.Jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> 
> 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.

My view on this to a fair part depends on whether the tool stack
would guard us from actually getting into such a situation in the
hypervisor. Getting an unspecific -EINVAL back without further
help towards diagnosis by the tool stack would make such a
change undesirable imo. This also extends to other checks you
appear to tighten - for example I wouldn't want to see a PV
guest config with "hap=1" in it to no longer work if currently it
happens to work, at least not without a clear hint towards the
issue.

> --- 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;

Would you mind making this the initializer of the variable and using
the variable in the two if()-s above? Personally I also think the two
if()-s would better be folded, using a conditional expression as its
condition.

Jan



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

Re: [PATCH v2 4/5] xen/domain: Allocate d->vcpu[] earlier during domain_create()

[Jan Beulich]

>>> On 12.11.18 at 17:16, <andrew.cooper3@citrix.com> wrote:
> The ARM code has a chicken-and-egg problem.  One of the vGIC_v3 emulations
> wants to know d->max_vcpus to be able to size itself appropriately, but the
> current order of initialisation requires the vGIC to be set up before the
> requested number of vcpus can be checked.
> 
> Move the range checking of config->max_vcpus into sanitise_domain_config()
> path, which allows for the allocation of d->vcpu[] and d->max_vcpus to happen
> earlier during create, and in particular, before the call to
> arch_domain_create().
> 
> The x86 side is fairly easy, and implements the logical equivalent of
> domain_max_vcpus() but using XEN_DOMCTL_CDF_hvm_guest rather than
> is_hvm_domain().
> 
> For the ARM side, re-purpose vgic_max_vcpus() to take a domctl vGIC version,
> and return the maximum number of supported vCPUs, reusing 0 for "version not
> supported".  To avoid exporting the vgic_ops structures (which are in the
> process of being replaced), hard code the upper limits.
> 
> This allows for the removal of the domain_max_vcpus() infrastructure, which is
> done to prevent it being reused incorrectly in the future.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Non-Arm bits
Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

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

[Wei Liu]

On Tue, Nov 13, 2018 at 07:14:24AM -0700, Jan Beulich wrote:
> >>> On 12.11.18 at 17:16, <andrew.cooper3@citrix.com> 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@citrix.com>
> > ---
> > CC: Jan Beulich <JBeulich@suse.com>
> > CC: Wei Liu <wei.liu2@citrix.com>
> > CC: Stefano Stabellini <sstabellini@kernel.org>
> > CC: Julien Grall <julien.grall@arm.com>
> > CC: Ian Jackson <Ian.Jackson@citrix.com>
> > CC: Wei Liu <wei.liu2@citrix.com>
> > 
> > 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.
> 
> My view on this to a fair part depends on whether the tool stack
> would guard us from actually getting into such a situation in the
> hypervisor. Getting an unspecific -EINVAL back without further
> help towards diagnosis by the tool stack would make such a
> change undesirable imo.

If you want toolstack to tell you what goes wrong, this sanitisation
function should be shared with the toolstack, and presumably with some
if __XEN_TOOLS__ trickeries to return / print out the culprit.

Wei.

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

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

[Andrew Cooper]

On 13/11/2018 14:36, Wei Liu wrote:
> On Tue, Nov 13, 2018 at 07:14:24AM -0700, Jan Beulich wrote:
>>>>> On 12.11.18 at 17:16, <andrew.cooper3@citrix.com> 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@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Wei Liu <wei.liu2@citrix.com>
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Julien Grall <julien.grall@arm.com>
>>> CC: Ian Jackson <Ian.Jackson@citrix.com>
>>> CC: Wei Liu <wei.liu2@citrix.com>
>>>
>>> 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.
>> My view on this to a fair part depends on whether the tool stack
>> would guard us from actually getting into such a situation in the
>> hypervisor. Getting an unspecific -EINVAL back without further
>> help towards diagnosis by the tool stack would make such a
>> change undesirable imo.
> If you want toolstack to tell you what goes wrong, this sanitisation
> function should be shared with the toolstack, and presumably with some
> if __XEN_TOOLS__ trickeries to return / print out the culprit.

Some bits of logic could be shared like that, but some can't.

As a different idea, could we hand back an up-to-128 byte string in the
failure case?  There is space for that in the domctl.u because we've got
no other error information we need to propagate backwards.

~Andrew

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

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

[Wei Liu]

On Tue, Nov 13, 2018 at 02:39:43PM +0000, Andrew Cooper wrote:
> On 13/11/2018 14:36, Wei Liu wrote:
> > On Tue, Nov 13, 2018 at 07:14:24AM -0700, Jan Beulich wrote:
> >>>>> On 12.11.18 at 17:16, <andrew.cooper3@citrix.com> 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@citrix.com>
> >>> ---
> >>> CC: Jan Beulich <JBeulich@suse.com>
> >>> CC: Wei Liu <wei.liu2@citrix.com>
> >>> CC: Stefano Stabellini <sstabellini@kernel.org>
> >>> CC: Julien Grall <julien.grall@arm.com>
> >>> CC: Ian Jackson <Ian.Jackson@citrix.com>
> >>> CC: Wei Liu <wei.liu2@citrix.com>
> >>>
> >>> 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.
> >> My view on this to a fair part depends on whether the tool stack
> >> would guard us from actually getting into such a situation in the
> >> hypervisor. Getting an unspecific -EINVAL back without further
> >> help towards diagnosis by the tool stack would make such a
> >> change undesirable imo.
> > If you want toolstack to tell you what goes wrong, this sanitisation
> > function should be shared with the toolstack, and presumably with some
> > if __XEN_TOOLS__ trickeries to return / print out the culprit.
> 
> Some bits of logic could be shared like that, but some can't.
> 
> As a different idea, could we hand back an up-to-128 byte string in the
> failure case?  There is space for that in the domctl.u because we've got
> no other error information we need to propagate backwards.

This sounds plausible. We can even define per domctl-op error types
instead of using a single string.

Wei.

> 
> ~Andrew

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

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

[Andrew Cooper]

On 13/11/2018 14:14, Jan Beulich wrote:
>>>> On 12.11.18 at 17:16, <andrew.cooper3@citrix.com> 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@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> CC: Ian Jackson <Ian.Jackson@citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>>
>> 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.
> My view on this to a fair part depends on whether the tool stack
> would guard us from actually getting into such a situation in the
> hypervisor. Getting an unspecific -EINVAL back without further
> help towards diagnosis by the tool stack would make such a
> change undesirable imo. This also extends to other checks you
> appear to tighten - for example I wouldn't want to see a PV
> guest config with "hap=1" in it to no longer work if currently it
> happens to work, at least not without a clear hint towards the
> issue.

Hmm - in attempting to answer this, I've discovered that xl issues no
warning/error about hap or nestedhvm with type="pv", and that a PV-shim
Xen binary will be started as a PV guest, despite the domain builder
identifying the binary as PVH.

>
>> --- 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;
> Would you mind making this the initializer of the variable and using
> the variable in the two if()-s above? Personally I also think the two
> if()-s would better be folded, using a conditional expression as its
> condition.

I can move the initialiser, but how do you propose folding the
conditionals given their different contents?

~Andrew

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

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

[Jan Beulich]

>>> On 13.11.18 at 16:07, <andrew.cooper3@citrix.com> wrote:
> On 13/11/2018 14:14, Jan Beulich wrote:
>>>>> On 12.11.18 at 17:16, <andrew.cooper3@citrix.com> wrote:
>>> --- 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;
>> Would you mind making this the initializer of the variable and using
>> the variable in the two if()-s above? Personally I also think the two
>> if()-s would better be folded, using a conditional expression as its
>> condition.
> 
> I can move the initialiser, but how do you propose folding the
> conditionals given their different contents?

    if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
    {
        dprintk(XENLOG_INFO, "%s support not available\n", hvm ? "HVM" : "PV");
        return -EINVAL;
    }

Jan



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

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

[Jan Beulich]

>>> On 13.11.18 at 15:39, <andrew.cooper3@citrix.com> wrote:
> On 13/11/2018 14:36, Wei Liu wrote:
>> On Tue, Nov 13, 2018 at 07:14:24AM -0700, Jan Beulich wrote:
>>>>>> On 12.11.18 at 17:16, <andrew.cooper3@citrix.com> 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@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Wei Liu <wei.liu2@citrix.com>
>>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>>> CC: Julien Grall <julien.grall@arm.com>
>>>> CC: Ian Jackson <Ian.Jackson@citrix.com>
>>>> CC: Wei Liu <wei.liu2@citrix.com>
>>>>
>>>> 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.
>>> My view on this to a fair part depends on whether the tool stack
>>> would guard us from actually getting into such a situation in the
>>> hypervisor. Getting an unspecific -EINVAL back without further
>>> help towards diagnosis by the tool stack would make such a
>>> change undesirable imo.
>> If you want toolstack to tell you what goes wrong, this sanitisation
>> function should be shared with the toolstack, and presumably with some
>> if __XEN_TOOLS__ trickeries to return / print out the culprit.
> 
> Some bits of logic could be shared like that, but some can't.
> 
> As a different idea, could we hand back an up-to-128 byte string in the
> failure case?  There is space for that in the domctl.u because we've got
> no other error information we need to propagate backwards.

I like the idea in general; I'm not entirely sure if this will scale with future
additions, as the caller might legitimately expect an explanatory string to
come back in all cases. (Of course absence of an explanation could be
signaled via an empty string.)

Jan



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

[PATCH v3 3/5] xen/domain: Move guest type checks into the arch_sanitise_domain_config() path

[Andrew Cooper]

This is a more appropriate location for the checks to happen, and cleans up
the common code substantially.

Take the opportunity to make ARM strictly require HVM|HAP for guests, which is
how the toolstack behaves, and leave a dprintk() behind for auditing failures.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

v2:
 * New
v3:
 * Rewrite to retain the existing bug compatibility on the x86 side.
---
 xen/arch/arm/domain.c |  6 ++++++
 xen/arch/x86/domain.c |  8 ++++++++
 xen/common/domain.c   | 34 +++-------------------------------
 3 files changed, 17 insertions(+), 31 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index c24ace6..71ad1f9 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -601,6 +601,12 @@ 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 | 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..272fd84 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -420,6 +420,14 @@ 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_guest;
+
+    if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
+    {
+        dprintk(XENLOG_INFO, "%s support not available\n", hvm ? "HVM" : "PV");
+        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);
 
-- 
2.1.4


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

[PATCH v3 4/5] xen/domain: Allocate d->vcpu[] earlier during domain_create()

[Andrew Cooper]

The ARM code has a chicken-and-egg problem.  One of the vGIC_v3 emulations
wants to know d->max_vcpus to be able to size itself appropriately, but the
current order of initialisation requires the vGIC to be set up before the
requested number of vcpus can be checked.

Move the range checking of config->max_vcpus into sanitise_domain_config()
path, which allows for the allocation of d->vcpu[] and d->max_vcpus to happen
earlier during create, and in particular, before the call to
arch_domain_create().

The x86 side is fairly easy, and implements the logical equivalent of
domain_max_vcpus() but using XEN_DOMCTL_CDF_hvm_guest rather than
is_hvm_domain().

For the ARM side, re-purpose vgic_max_vcpus() to take a domctl vGIC version,
and return the maximum number of supported vCPUs, reusing 0 for "version not
supported".  To avoid exporting the vgic_ops structures (which are in the
process of being replaced), hard code the upper limits.

This allows for the removal of the domain_max_vcpus() infrastructure, which is
done to prevent it being reused incorrectly in the future.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Wei Liu <wei.liu2@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

v3:
 * Fix 4092/4096 typo
 * Fix build in NEW_VGIC case
---
 xen/arch/arm/domain.c         | 18 ++++++++++++++++++
 xen/arch/arm/vgic-v2.c        |  1 -
 xen/arch/arm/vgic-v3.c        |  5 -----
 xen/arch/arm/vgic.c           | 22 ++++++++++++++++++++--
 xen/arch/arm/vgic/vgic-init.c |  3 ---
 xen/arch/arm/vgic/vgic.c      |  9 +++++----
 xen/arch/x86/domain.c         | 10 ++++++++++
 xen/common/domain.c           | 33 ++++++++++++++++++++-------------
 xen/include/asm-arm/domain.h  |  6 ------
 xen/include/asm-arm/vgic.h    |  5 ++---
 xen/include/asm-x86/domain.h  |  2 --
 11 files changed, 75 insertions(+), 39 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 71ad1f9..2c5ff65 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -601,6 +601,8 @@ void vcpu_switch_to_aarch64_mode(struct vcpu *v)
 
 int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
 {
+    unsigned int max_vcpus;
+
     if ( config->flags != (XEN_DOMCTL_CDF_hvm_guest | XEN_DOMCTL_CDF_hap) )
     {
         dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", config->flags);
@@ -626,6 +628,22 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
         }
     }
 
+    /* Calculate the maximum number of vcpus from the selected GIC version. */
+    max_vcpus = vgic_max_vcpus(config->arch.gic_version);
+
+    if ( max_vcpus == 0 )
+    {
+        dprintk(XENLOG_INFO, "Unsupported GIC version\n");
+        return -EINVAL;
+    }
+
+    if ( config->max_vcpus > max_vcpus )
+    {
+        dprintk(XENLOG_INFO, "Requested vCPUs (%u) exceeds max (%u)\n",
+                config->max_vcpus, max_vcpus);
+        return -EINVAL;
+    }
+
     return 0;
 }
 
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index bf77899..64b141f 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -725,7 +725,6 @@ static const struct vgic_ops vgic_v2_ops = {
     .domain_free = vgic_v2_domain_free,
     .lpi_to_pending = vgic_v2_lpi_to_pending,
     .lpi_get_priority = vgic_v2_lpi_get_priority,
-    .max_vcpus = 8,
 };
 
 int vgic_v2_init(struct domain *d, int *mmio_count)
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index c14bcd8..519cc72 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1822,11 +1822,6 @@ static const struct vgic_ops v3_ops = {
     .emulate_reg  = vgic_v3_emulate_reg,
     .lpi_to_pending = vgic_v3_lpi_to_pending,
     .lpi_get_priority = vgic_v3_lpi_get_priority,
-    /*
-     * We use both AFF1 and AFF0 in (v)MPIDR. Thus, the max number of CPU
-     * that can be supported is up to 4096(==256*16) in theory.
-     */
-    .max_vcpus = 4096,
 };
 
 int vgic_v3_init(struct domain *d, int *mmio_count)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 5a4f082..e05b6b6 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -667,9 +667,27 @@ void vgic_free_virq(struct domain *d, unsigned int virq)
     clear_bit(virq, d->arch.vgic.allocated_irqs);
 }
 
-unsigned int vgic_max_vcpus(const struct domain *d)
+unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)
 {
-    return min_t(unsigned int, MAX_VIRT_CPUS, d->arch.vgic.handler->max_vcpus);
+    unsigned int max_vcpus;
+
+    switch ( domctl_vgic_version )
+    {
+    case XEN_DOMCTL_CONFIG_GIC_V2:
+        max_vcpus = 8;
+        break;
+
+#ifdef CONFIG_GICV3
+    case XEN_DOMCTL_CONFIG_GIC_V3:
+        max_vcpus = 4096;
+        break;
+#endif
+
+    default:
+        return 0;
+    }
+
+    return min_t(unsigned int, MAX_VIRT_CPUS, max_vcpus);
 }
 
 /*
diff --git a/xen/arch/arm/vgic/vgic-init.c b/xen/arch/arm/vgic/vgic-init.c
index bfd3d09..62ae553 100644
--- a/xen/arch/arm/vgic/vgic-init.c
+++ b/xen/arch/arm/vgic/vgic-init.c
@@ -112,9 +112,6 @@ int domain_vgic_register(struct domain *d, int *mmio_count)
         BUG();
     }
 
-    if ( d->max_vcpus > domain_max_vcpus(d) )
-        return -E2BIG;
-
     d->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
     d->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
     d->arch.vgic.vgic_redist_base = VGIC_ADDR_UNDEF;
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index 7c3cfc5..b8ff840 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -949,17 +949,18 @@ void vgic_sync_hardware_irq(struct domain *d,
     spin_unlock_irqrestore(&desc->lock, flags);
 }
 
-unsigned int vgic_max_vcpus(const struct domain *d)
+unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)
 {
     unsigned int vgic_vcpu_limit;
 
-    switch ( d->arch.vgic.version )
+    switch ( domctl_vgic_version )
     {
-    case GIC_V2:
+    case XEN_DOMCTL_CONFIG_GIC_V2:
         vgic_vcpu_limit = VGIC_V2_MAX_CPUS;
         break;
+
     default:
-        BUG();
+        return 0;
     }
 
     return min_t(unsigned int, MAX_VIRT_CPUS, vgic_vcpu_limit);
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 272fd84..295b10c 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -421,6 +421,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_guest;
+    unsigned int max_vcpus;
 
     if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
     {
@@ -428,6 +429,15 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }
 
+    max_vcpus = hvm ? HVM_MAX_VCPUS : MAX_VIRT_CPUS;
+
+    if ( config->max_vcpus > max_vcpus )
+    {
+        dprintk(XENLOG_INFO, "Requested vCPUs (%u) exceeds max (%u)\n",
+                config->max_vcpus, max_vcpus);
+        return -EINVAL;
+    }
+
     return 0;
 }
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index f69f405..78cc524 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -300,6 +300,12 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }
 
+    if ( config->max_vcpus < 1 )
+    {
+        dprintk(XENLOG_INFO, "No vCPUS\n");
+        return -EINVAL;
+    }
+
     return arch_sanitise_domain_config(config);
 }
 
@@ -345,6 +351,20 @@ struct domain *domain_create(domid_t domid,
 
     TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
 
+    /*
+     * Allocate d->vcpu[] and set ->max_vcpus up early.  Various per-domain
+     * resources want to be sized based on max_vcpus.
+     */
+    if ( !is_system_domain(d) )
+    {
+        err = -ENOMEM;
+        d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus);
+        if ( !d->vcpu )
+            goto fail;
+
+        d->max_vcpus = config->max_vcpus;
+    }
+
     lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid, "Domain");
 
     if ( (err = xsm_alloc_security_domain(d)) != 0 )
@@ -396,19 +416,6 @@ struct domain *domain_create(domid_t domid,
 
     if ( !is_idle_domain(d) )
     {
-        /* Check d->max_vcpus and allocate d->vcpu[]. */
-        err = -EINVAL;
-        if ( config->max_vcpus < 1 ||
-             config->max_vcpus > domain_max_vcpus(d) )
-            goto fail;
-
-        err = -ENOMEM;
-        d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus);
-        if ( !d->vcpu )
-            goto fail;
-
-        d->max_vcpus = config->max_vcpus;
-
         watchdog_domain_init(d);
         init_status |= INIT_watchdog;
 
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index d682307..175de44 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -208,12 +208,6 @@ void vcpu_show_execution_state(struct vcpu *);
 void vcpu_show_registers(const struct vcpu *);
 void vcpu_switch_to_aarch64_mode(struct vcpu *);
 
-/* On ARM, the number of VCPUs is limited by the type of GIC emulated. */
-static inline unsigned int domain_max_vcpus(const struct domain *d)
-{
-    return vgic_max_vcpus(d);
-}
-
 /*
  * Due to the restriction of GICv3, the number of vCPUs in AFF0 is
  * limited to 16, thus only the first 4 bits of AFF0 are legal. We will
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 56ed5fe..447d24e 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -234,8 +234,6 @@ struct vgic_ops {
     /* lookup the struct pending_irq for a given LPI interrupt */
     struct pending_irq *(*lpi_to_pending)(struct domain *d, unsigned int vlpi);
     int (*lpi_get_priority)(struct domain *d, uint32_t vlpi);
-    /* Maximum number of vCPU supported */
-    const unsigned int max_vcpus;
 };
 
 /* Number of ranks of interrupt registers for a domain */
@@ -350,7 +348,8 @@ extern void vgic_clear_pending_irqs(struct vcpu *v);
 
 extern bool vgic_emulate(struct cpu_user_regs *regs, union hsr hsr);
 
-unsigned int vgic_max_vcpus(const struct domain *d);
+/* Maximum vCPUs for a specific vGIC version, or 0 for unsupported. */
+unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version);
 
 void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
                       paddr_t vbase, uint32_t aliased_offset);
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 643e69a..277f99f 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -664,8 +664,6 @@ unsigned long pv_guest_cr4_to_real_cr4(const struct vcpu *v);
              X86_CR4_OSXSAVE | X86_CR4_SMEP |               \
              X86_CR4_FSGSBASE | X86_CR4_SMAP | X86_CR4_PCIDE))
 
-#define domain_max_vcpus(d) (is_hvm_domain(d) ? HVM_MAX_VCPUS : MAX_VIRT_CPUS)
-
 static inline struct vcpu_guest_context *alloc_vcpu_guest_context(void)
 {
     return vmalloc(sizeof(struct vcpu_guest_context));
-- 
2.1.4


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

Re: [PATCH v2 1/5] xen/domain: Introduce a new sanitise_domain_config() helper

[Julien Grall]

Hi Andrew,

On 12/11/2018 16:16, Andrew Cooper wrote:
> Call it from the head of domain_create() (before doing any memory
> allocations), which will apply the checks to dom0 as well as domU's.
> 
> For now, just subsume the XEN_DOMCTL_CDF_* check from XEN_DOMCTL_createdomain.
> 
> In an effort to aid future developoment, leave a debug printk() identifying
> the cause of sanitisation failures.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheers,

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> 
> v2:
>   * Rename to sanitise_domain_config()
>   * Leave a dprintk() behind
> ---
>   xen/common/domain.c | 18 ++++++++++++++++++
>   xen/common/domctl.c |  9 ---------
>   2 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index d6650f0..22aa634 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -288,6 +288,21 @@ static void _domain_destroy(struct domain *d)
>       free_domain_struct(d);
>   }
>   
> +static int sanitise_domain_config(struct xen_domctl_createdomain *config)
> +{
> +    if ( config->flags & ~(XEN_DOMCTL_CDF_hvm_guest |
> +                           XEN_DOMCTL_CDF_hap |
> +                           XEN_DOMCTL_CDF_s3_integrity |
> +                           XEN_DOMCTL_CDF_oos_off |
> +                           XEN_DOMCTL_CDF_xs_domain) )
> +    {
> +        dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags);
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
>   struct domain *domain_create(domid_t domid,
>                                struct xen_domctl_createdomain *config,
>                                bool is_priv)
> @@ -297,6 +312,9 @@ struct domain *domain_create(domid_t domid,
>              INIT_evtchn = 1u<<3, INIT_gnttab = 1u<<4, INIT_arch = 1u<<5 };
>       int err, init_status = 0;
>   
> +    if ( config && (err = sanitise_domain_config(config)) )
> +        return ERR_PTR(err);
> +
>       if ( (d = alloc_domain_struct()) == NULL )
>           return ERR_PTR(-ENOMEM);
>   
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index b294881..d08b627 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -498,15 +498,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>           domid_t        dom;
>           static domid_t rover = 0;
>   
> -        ret = -EINVAL;
> -        if ( (op->u.createdomain.flags &
> -             ~(XEN_DOMCTL_CDF_hvm_guest
> -               | XEN_DOMCTL_CDF_hap
> -               | XEN_DOMCTL_CDF_s3_integrity
> -               | XEN_DOMCTL_CDF_oos_off
> -               | XEN_DOMCTL_CDF_xs_domain)) )
> -            break;
> -
>           dom = op->domain;
>           if ( (dom > 0) && (dom < DOMID_FIRST_RESERVED) )
>           {
> 

-- 
Julien Grall

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

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

[Julien Grall]

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@citrix.com>

For the Arm bits:

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

Cheers,

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Ian Jackson <Ian.Jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> 
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 3/5] xen/domain: Move guest type checks into the arch_sanitise_domain_config() path

[Julien Grall]

Hi,

On 14/11/2018 17:44, Andrew Cooper wrote:
> This is a more appropriate location for the checks to happen, and cleans up
> the common code substantially.
> 
> Take the opportunity to make ARM strictly require HVM|HAP for guests, which is
> how the toolstack behaves, and leave a dprintk() behind for auditing failures.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I didn't spot you sent a new version. So adding my ack here:

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

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 4/5] xen/domain: Allocate d->vcpu[] earlier during domain_create()

[Julien Grall]

Hi Andrew,

On 12/11/2018 16:16, Andrew Cooper wrote:
> The ARM code has a chicken-and-egg problem.  One of the vGIC_v3 emulations

NIT: s/vGIC_v3/vGICv3/

> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 5a4f082..892445e 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -667,9 +667,27 @@ void vgic_free_virq(struct domain *d, unsigned int virq)
>       clear_bit(virq, d->arch.vgic.allocated_irqs);
>   }
>   
> -unsigned int vgic_max_vcpus(const struct domain *d)
> +unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)
>   {
> -    return min_t(unsigned int, MAX_VIRT_CPUS, d->arch.vgic.handler->max_vcpus);
> +    unsigned int max_vcpus;
> +
> +    switch ( domctl_vgic_version )
> +    {
> +    case XEN_DOMCTL_CONFIG_GIC_V2:
> +        max_vcpus = 8;
> +        break;
> +
> +#ifdef CONFIG_GICV3
> +    case XEN_DOMCTL_CONFIG_GIC_V3:
> +        max_vcpus = 4092;

The previous case was using 4096. Also, can you move the comment to keep the 
rationale for the number?

> +        break;
> +#endif
> +
> +    default:
> +        return 0;
> +    }
> +
> +    return min_t(unsigned int, MAX_VIRT_CPUS, max_vcpus);

How about moving this check in the common code?

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 4/5] xen/domain: Allocate d->vcpu[] earlier during domain_create()

[Andrew Cooper]

On 14/11/2018 18:59, Julien Grall wrote:
> Hi Andrew,
>
> On 12/11/2018 16:16, Andrew Cooper wrote:
>> The ARM code has a chicken-and-egg problem.  One of the vGIC_v3
>> emulations
>
> NIT: s/vGIC_v3/vGICv3/

Will fix.

>
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 5a4f082..892445e 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -667,9 +667,27 @@ void vgic_free_virq(struct domain *d, unsigned
>> int virq)
>>       clear_bit(virq, d->arch.vgic.allocated_irqs);
>>   }
>>   -unsigned int vgic_max_vcpus(const struct domain *d)
>> +unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)
>>   {
>> -    return min_t(unsigned int, MAX_VIRT_CPUS,
>> d->arch.vgic.handler->max_vcpus);
>> +    unsigned int max_vcpus;
>> +
>> +    switch ( domctl_vgic_version )
>> +    {
>> +    case XEN_DOMCTL_CONFIG_GIC_V2:
>> +        max_vcpus = 8;
>> +        break;
>> +
>> +#ifdef CONFIG_GICV3
>> +    case XEN_DOMCTL_CONFIG_GIC_V3:
>> +        max_vcpus = 4092;
>
> The previous case was using 4096. Also, can you move the comment to
> keep the rationale for the number?

Please see the posted v3.  This was a straight typo.

>
>> +        break;
>> +#endif
>> +
>> +    default:
>> +        return 0;
>> +    }
>> +
>> +    return min_t(unsigned int, MAX_VIRT_CPUS, max_vcpus);
>
> How about moving this check in the common code?

Can do.

~Andrew

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

[PATCH v4 4/5] xen/domain: Allocate d->vcpu[] earlier during domain_create()

[Andrew Cooper]

The ARM code has a chicken-and-egg problem.  One of the vGICv3 emulations
wants to know d->max_vcpus to be able to size itself appropriately, but the
current order of initialisation requires the vGIC to be set up before the
requested number of vcpus can be checked.

Move the range checking of config->max_vcpus into sanitise_domain_config()
path, which allows for the allocation of d->vcpu[] and d->max_vcpus to happen
earlier during create, and in particular, before the call to
arch_domain_create().

The x86 side is fairly easy, and implements the logical equivalent of
domain_max_vcpus() but using XEN_DOMCTL_CDF_hvm_guest rather than
is_hvm_domain().

For the ARM side, re-purpose vgic_max_vcpus() to take a domctl vGIC version,
and return the maximum number of supported vCPUs, reusing 0 for "version not
supported".  To avoid exporting the vgic_ops structures (which are in the
process of being replaced), hard code the upper limits.

This allows for the removal of the domain_max_vcpus() infrastructure, which is
done to prevent it being reused incorrectly in the future.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Wei Liu <wei.liu2@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

v3:
 * Fix 4092/4096 typo
 * Fix build in NEW_VGIC case
v4:
 * Move the min() into ARM's common arch_sanitise_domain_config()
   implementation.  Simplfy the min() expression code by making MAX_VIRT_CPUS
   be an unsigned constant.
---
 xen/arch/arm/domain.c         | 18 ++++++++++++++++++
 xen/arch/arm/vgic-v2.c        |  1 -
 xen/arch/arm/vgic-v3.c        |  5 -----
 xen/arch/arm/vgic.c           | 16 ++++++++++++++--
 xen/arch/arm/vgic/vgic-init.c |  3 ---
 xen/arch/arm/vgic/vgic.c      | 16 ++++++----------
 xen/arch/x86/domain.c         | 10 ++++++++++
 xen/common/domain.c           | 33 ++++++++++++++++++++-------------
 xen/include/asm-arm/config.h  |  4 ++--
 xen/include/asm-arm/domain.h  |  6 ------
 xen/include/asm-arm/vgic.h    |  5 ++---
 xen/include/asm-x86/domain.h  |  2 --
 12 files changed, 72 insertions(+), 47 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 71ad1f9..7399989 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -601,6 +601,8 @@ void vcpu_switch_to_aarch64_mode(struct vcpu *v)
 
 int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
 {
+    unsigned int max_vcpus;
+
     if ( config->flags != (XEN_DOMCTL_CDF_hvm_guest | XEN_DOMCTL_CDF_hap) )
     {
         dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", config->flags);
@@ -626,6 +628,22 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
         }
     }
 
+    /* max_vcpus depends on the GIC version, and Xen's compiled limit. */
+    max_vcpus = max(vgic_max_vcpus(config->arch.gic_version), MAX_VIRT_CPUS);
+
+    if ( max_vcpus == 0 )
+    {
+        dprintk(XENLOG_INFO, "Unsupported GIC version\n");
+        return -EINVAL;
+    }
+
+    if ( config->max_vcpus > max_vcpus )
+    {
+        dprintk(XENLOG_INFO, "Requested vCPUs (%u) exceeds max (%u)\n",
+                config->max_vcpus, max_vcpus);
+        return -EINVAL;
+    }
+
     return 0;
 }
 
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index bf77899..64b141f 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -725,7 +725,6 @@ static const struct vgic_ops vgic_v2_ops = {
     .domain_free = vgic_v2_domain_free,
     .lpi_to_pending = vgic_v2_lpi_to_pending,
     .lpi_get_priority = vgic_v2_lpi_get_priority,
-    .max_vcpus = 8,
 };
 
 int vgic_v2_init(struct domain *d, int *mmio_count)
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index c14bcd8..519cc72 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1822,11 +1822,6 @@ static const struct vgic_ops v3_ops = {
     .emulate_reg  = vgic_v3_emulate_reg,
     .lpi_to_pending = vgic_v3_lpi_to_pending,
     .lpi_get_priority = vgic_v3_lpi_get_priority,
-    /*
-     * We use both AFF1 and AFF0 in (v)MPIDR. Thus, the max number of CPU
-     * that can be supported is up to 4096(==256*16) in theory.
-     */
-    .max_vcpus = 4096,
 };
 
 int vgic_v3_init(struct domain *d, int *mmio_count)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 5a4f082..f2608b0 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -667,9 +667,21 @@ void vgic_free_virq(struct domain *d, unsigned int virq)
     clear_bit(virq, d->arch.vgic.allocated_irqs);
 }
 
-unsigned int vgic_max_vcpus(const struct domain *d)
+unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)
 {
-    return min_t(unsigned int, MAX_VIRT_CPUS, d->arch.vgic.handler->max_vcpus);
+    switch ( domctl_vgic_version )
+    {
+    case XEN_DOMCTL_CONFIG_GIC_V2:
+        return 8;
+
+#ifdef CONFIG_GICV3
+    case XEN_DOMCTL_CONFIG_GIC_V3:
+        return 4096;
+#endif
+
+    default:
+        return 0;
+    }
 }
 
 /*
diff --git a/xen/arch/arm/vgic/vgic-init.c b/xen/arch/arm/vgic/vgic-init.c
index bfd3d09..62ae553 100644
--- a/xen/arch/arm/vgic/vgic-init.c
+++ b/xen/arch/arm/vgic/vgic-init.c
@@ -112,9 +112,6 @@ int domain_vgic_register(struct domain *d, int *mmio_count)
         BUG();
     }
 
-    if ( d->max_vcpus > domain_max_vcpus(d) )
-        return -E2BIG;
-
     d->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
     d->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
     d->arch.vgic.vgic_redist_base = VGIC_ADDR_UNDEF;
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index 7c3cfc5..e2844dc 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -949,20 +949,16 @@ void vgic_sync_hardware_irq(struct domain *d,
     spin_unlock_irqrestore(&desc->lock, flags);
 }
 
-unsigned int vgic_max_vcpus(const struct domain *d)
+unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)
 {
-    unsigned int vgic_vcpu_limit;
-
-    switch ( d->arch.vgic.version )
+    switch ( domctl_vgic_version )
     {
-    case GIC_V2:
-        vgic_vcpu_limit = VGIC_V2_MAX_CPUS;
-        break;
+    case XEN_DOMCTL_CONFIG_GIC_V2:
+        return VGIC_V2_MAX_CPUS;
+
     default:
-        BUG();
+        return 0;
     }
-
-    return min_t(unsigned int, MAX_VIRT_CPUS, vgic_vcpu_limit);
 }
 
 #ifdef CONFIG_GICV3
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 272fd84..295b10c 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -421,6 +421,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_guest;
+    unsigned int max_vcpus;
 
     if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
     {
@@ -428,6 +429,15 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }
 
+    max_vcpus = hvm ? HVM_MAX_VCPUS : MAX_VIRT_CPUS;
+
+    if ( config->max_vcpus > max_vcpus )
+    {
+        dprintk(XENLOG_INFO, "Requested vCPUs (%u) exceeds max (%u)\n",
+                config->max_vcpus, max_vcpus);
+        return -EINVAL;
+    }
+
     return 0;
 }
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index f69f405..78cc524 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -300,6 +300,12 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }
 
+    if ( config->max_vcpus < 1 )
+    {
+        dprintk(XENLOG_INFO, "No vCPUS\n");
+        return -EINVAL;
+    }
+
     return arch_sanitise_domain_config(config);
 }
 
@@ -345,6 +351,20 @@ struct domain *domain_create(domid_t domid,
 
     TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
 
+    /*
+     * Allocate d->vcpu[] and set ->max_vcpus up early.  Various per-domain
+     * resources want to be sized based on max_vcpus.
+     */
+    if ( !is_system_domain(d) )
+    {
+        err = -ENOMEM;
+        d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus);
+        if ( !d->vcpu )
+            goto fail;
+
+        d->max_vcpus = config->max_vcpus;
+    }
+
     lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid, "Domain");
 
     if ( (err = xsm_alloc_security_domain(d)) != 0 )
@@ -396,19 +416,6 @@ struct domain *domain_create(domid_t domid,
 
     if ( !is_idle_domain(d) )
     {
-        /* Check d->max_vcpus and allocate d->vcpu[]. */
-        err = -EINVAL;
-        if ( config->max_vcpus < 1 ||
-             config->max_vcpus > domain_max_vcpus(d) )
-            goto fail;
-
-        err = -ENOMEM;
-        d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus);
-        if ( !d->vcpu )
-            goto fail;
-
-        d->max_vcpus = config->max_vcpus;
-
         watchdog_domain_init(d);
         init_status |= INIT_watchdog;
 
diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index cdae8f6..bc89e84 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -41,9 +41,9 @@
 #define OPT_CONSOLE_STR "dtuart"
 
 #ifdef CONFIG_ARM_64
-#define MAX_VIRT_CPUS 128
+#define MAX_VIRT_CPUS 128u
 #else
-#define MAX_VIRT_CPUS 8
+#define MAX_VIRT_CPUS 8u
 #endif
 
 #define INVALID_VCPU_ID MAX_VIRT_CPUS
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index d682307..175de44 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -208,12 +208,6 @@ void vcpu_show_execution_state(struct vcpu *);
 void vcpu_show_registers(const struct vcpu *);
 void vcpu_switch_to_aarch64_mode(struct vcpu *);
 
-/* On ARM, the number of VCPUs is limited by the type of GIC emulated. */
-static inline unsigned int domain_max_vcpus(const struct domain *d)
-{
-    return vgic_max_vcpus(d);
-}
-
 /*
  * Due to the restriction of GICv3, the number of vCPUs in AFF0 is
  * limited to 16, thus only the first 4 bits of AFF0 are legal. We will
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 56ed5fe..447d24e 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -234,8 +234,6 @@ struct vgic_ops {
     /* lookup the struct pending_irq for a given LPI interrupt */
     struct pending_irq *(*lpi_to_pending)(struct domain *d, unsigned int vlpi);
     int (*lpi_get_priority)(struct domain *d, uint32_t vlpi);
-    /* Maximum number of vCPU supported */
-    const unsigned int max_vcpus;
 };
 
 /* Number of ranks of interrupt registers for a domain */
@@ -350,7 +348,8 @@ extern void vgic_clear_pending_irqs(struct vcpu *v);
 
 extern bool vgic_emulate(struct cpu_user_regs *regs, union hsr hsr);
 
-unsigned int vgic_max_vcpus(const struct domain *d);
+/* Maximum vCPUs for a specific vGIC version, or 0 for unsupported. */
+unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version);
 
 void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
                       paddr_t vbase, uint32_t aliased_offset);
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 643e69a..277f99f 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -664,8 +664,6 @@ unsigned long pv_guest_cr4_to_real_cr4(const struct vcpu *v);
              X86_CR4_OSXSAVE | X86_CR4_SMEP |               \
              X86_CR4_FSGSBASE | X86_CR4_SMAP | X86_CR4_PCIDE))
 
-#define domain_max_vcpus(d) (is_hvm_domain(d) ? HVM_MAX_VCPUS : MAX_VIRT_CPUS)
-
 static inline struct vcpu_guest_context *alloc_vcpu_guest_context(void)
 {
     return vmalloc(sizeof(struct vcpu_guest_context));
-- 
2.1.4


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

Re: [PATCH v4 4/5] xen/domain: Allocate d->vcpu[] earlier during domain_create()

[Julien Grall]

On 14/11/2018 19:36, Andrew Cooper wrote:
> +    /* max_vcpus depends on the GIC version, and Xen's compiled limit. */
> +    max_vcpus = max(vgic_max_vcpus(config->arch.gic_version), MAX_VIRT_CPUS);

Did you mean min(vgic, MAX_VIRT_CPUS)?

Cheers,

-- 
Julien Grall

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

Re: [PATCH v4 4/5] xen/domain: Allocate d->vcpu[] earlier during domain_create()

[Andrew Cooper]

On 14/11/2018 19:37, Julien Grall wrote:
>
>
> On 14/11/2018 19:36, Andrew Cooper wrote:
>> +    /* max_vcpus depends on the GIC version, and Xen's compiled
>> limit. */
>> +    max_vcpus = max(vgic_max_vcpus(config->arch.gic_version),
>> MAX_VIRT_CPUS);
>
> Did you mean min(vgic, MAX_VIRT_CPUS)?

Bah - yes I did.

~Andrew


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

[PATCH v5 4/5] xen/domain: Allocate d->vcpu[] earlier during domain_create()

[Andrew Cooper]

The ARM code has a chicken-and-egg problem.  One of the vGICv3 emulations
wants to know d->max_vcpus to be able to size itself appropriately, but the
current order of initialisation requires the vGIC to be set up before the
requested number of vcpus can be checked.

Move the range checking of config->max_vcpus into sanitise_domain_config()
path, which allows for the allocation of d->vcpu[] and d->max_vcpus to happen
earlier during create, and in particular, before the call to
arch_domain_create().

The x86 side is fairly easy, and implements the logical equivalent of
domain_max_vcpus() but using XEN_DOMCTL_CDF_hvm_guest rather than
is_hvm_domain().

For the ARM side, re-purpose vgic_max_vcpus() to take a domctl vGIC version,
and return the maximum number of supported vCPUs, reusing 0 for "version not
supported".  To avoid exporting the vgic_ops structures (which are in the
process of being replaced), hard code the upper limits.

This allows for the removal of the domain_max_vcpus() infrastructure, which is
done to prevent it being reused incorrectly in the future.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Wei Liu <wei.liu2@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

v3:
 * Fix 4092/4096 typo
 * Fix build in NEW_VGIC case
v4:
 * Move the min() into ARM's common arch_sanitise_domain_config()
   implementation.  Simplfy the min() expression code by making MAX_VIRT_CPUS
   be an unsigned constant.
v5:
 * s/max/min
---
 xen/arch/arm/domain.c         | 18 ++++++++++++++++++
 xen/arch/arm/vgic-v2.c        |  1 -
 xen/arch/arm/vgic-v3.c        |  5 -----
 xen/arch/arm/vgic.c           | 16 ++++++++++++++--
 xen/arch/arm/vgic/vgic-init.c |  3 ---
 xen/arch/arm/vgic/vgic.c      | 16 ++++++----------
 xen/arch/x86/domain.c         | 10 ++++++++++
 xen/common/domain.c           | 33 ++++++++++++++++++++-------------
 xen/include/asm-arm/config.h  |  4 ++--
 xen/include/asm-arm/domain.h  |  6 ------
 xen/include/asm-arm/vgic.h    |  5 ++---
 xen/include/asm-x86/domain.h  |  2 --
 12 files changed, 72 insertions(+), 47 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 71ad1f9..1d926dc 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -601,6 +601,8 @@ void vcpu_switch_to_aarch64_mode(struct vcpu *v)
 
 int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
 {
+    unsigned int max_vcpus;
+
     if ( config->flags != (XEN_DOMCTL_CDF_hvm_guest | XEN_DOMCTL_CDF_hap) )
     {
         dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", config->flags);
@@ -626,6 +628,22 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
         }
     }
 
+    /* max_vcpus depends on the GIC version, and Xen's compiled limit. */
+    max_vcpus = min(vgic_max_vcpus(config->arch.gic_version), MAX_VIRT_CPUS);
+
+    if ( max_vcpus == 0 )
+    {
+        dprintk(XENLOG_INFO, "Unsupported GIC version\n");
+        return -EINVAL;
+    }
+
+    if ( config->max_vcpus > max_vcpus )
+    {
+        dprintk(XENLOG_INFO, "Requested vCPUs (%u) exceeds max (%u)\n",
+                config->max_vcpus, max_vcpus);
+        return -EINVAL;
+    }
+
     return 0;
 }
 
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index bf77899..64b141f 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -725,7 +725,6 @@ static const struct vgic_ops vgic_v2_ops = {
     .domain_free = vgic_v2_domain_free,
     .lpi_to_pending = vgic_v2_lpi_to_pending,
     .lpi_get_priority = vgic_v2_lpi_get_priority,
-    .max_vcpus = 8,
 };
 
 int vgic_v2_init(struct domain *d, int *mmio_count)
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index c14bcd8..519cc72 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1822,11 +1822,6 @@ static const struct vgic_ops v3_ops = {
     .emulate_reg  = vgic_v3_emulate_reg,
     .lpi_to_pending = vgic_v3_lpi_to_pending,
     .lpi_get_priority = vgic_v3_lpi_get_priority,
-    /*
-     * We use both AFF1 and AFF0 in (v)MPIDR. Thus, the max number of CPU
-     * that can be supported is up to 4096(==256*16) in theory.
-     */
-    .max_vcpus = 4096,
 };
 
 int vgic_v3_init(struct domain *d, int *mmio_count)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 5a4f082..f2608b0 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -667,9 +667,21 @@ void vgic_free_virq(struct domain *d, unsigned int virq)
     clear_bit(virq, d->arch.vgic.allocated_irqs);
 }
 
-unsigned int vgic_max_vcpus(const struct domain *d)
+unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)
 {
-    return min_t(unsigned int, MAX_VIRT_CPUS, d->arch.vgic.handler->max_vcpus);
+    switch ( domctl_vgic_version )
+    {
+    case XEN_DOMCTL_CONFIG_GIC_V2:
+        return 8;
+
+#ifdef CONFIG_GICV3
+    case XEN_DOMCTL_CONFIG_GIC_V3:
+        return 4096;
+#endif
+
+    default:
+        return 0;
+    }
 }
 
 /*
diff --git a/xen/arch/arm/vgic/vgic-init.c b/xen/arch/arm/vgic/vgic-init.c
index bfd3d09..62ae553 100644
--- a/xen/arch/arm/vgic/vgic-init.c
+++ b/xen/arch/arm/vgic/vgic-init.c
@@ -112,9 +112,6 @@ int domain_vgic_register(struct domain *d, int *mmio_count)
         BUG();
     }
 
-    if ( d->max_vcpus > domain_max_vcpus(d) )
-        return -E2BIG;
-
     d->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
     d->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
     d->arch.vgic.vgic_redist_base = VGIC_ADDR_UNDEF;
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index 7c3cfc5..e2844dc 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -949,20 +949,16 @@ void vgic_sync_hardware_irq(struct domain *d,
     spin_unlock_irqrestore(&desc->lock, flags);
 }
 
-unsigned int vgic_max_vcpus(const struct domain *d)
+unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)
 {
-    unsigned int vgic_vcpu_limit;
-
-    switch ( d->arch.vgic.version )
+    switch ( domctl_vgic_version )
     {
-    case GIC_V2:
-        vgic_vcpu_limit = VGIC_V2_MAX_CPUS;
-        break;
+    case XEN_DOMCTL_CONFIG_GIC_V2:
+        return VGIC_V2_MAX_CPUS;
+
     default:
-        BUG();
+        return 0;
     }
-
-    return min_t(unsigned int, MAX_VIRT_CPUS, vgic_vcpu_limit);
 }
 
 #ifdef CONFIG_GICV3
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 272fd84..295b10c 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -421,6 +421,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_guest;
+    unsigned int max_vcpus;
 
     if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
     {
@@ -428,6 +429,15 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }
 
+    max_vcpus = hvm ? HVM_MAX_VCPUS : MAX_VIRT_CPUS;
+
+    if ( config->max_vcpus > max_vcpus )
+    {
+        dprintk(XENLOG_INFO, "Requested vCPUs (%u) exceeds max (%u)\n",
+                config->max_vcpus, max_vcpus);
+        return -EINVAL;
+    }
+
     return 0;
 }
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index f69f405..78cc524 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -300,6 +300,12 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }
 
+    if ( config->max_vcpus < 1 )
+    {
+        dprintk(XENLOG_INFO, "No vCPUS\n");
+        return -EINVAL;
+    }
+
     return arch_sanitise_domain_config(config);
 }
 
@@ -345,6 +351,20 @@ struct domain *domain_create(domid_t domid,
 
     TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
 
+    /*
+     * Allocate d->vcpu[] and set ->max_vcpus up early.  Various per-domain
+     * resources want to be sized based on max_vcpus.
+     */
+    if ( !is_system_domain(d) )
+    {
+        err = -ENOMEM;
+        d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus);
+        if ( !d->vcpu )
+            goto fail;
+
+        d->max_vcpus = config->max_vcpus;
+    }
+
     lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid, "Domain");
 
     if ( (err = xsm_alloc_security_domain(d)) != 0 )
@@ -396,19 +416,6 @@ struct domain *domain_create(domid_t domid,
 
     if ( !is_idle_domain(d) )
     {
-        /* Check d->max_vcpus and allocate d->vcpu[]. */
-        err = -EINVAL;
-        if ( config->max_vcpus < 1 ||
-             config->max_vcpus > domain_max_vcpus(d) )
-            goto fail;
-
-        err = -ENOMEM;
-        d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus);
-        if ( !d->vcpu )
-            goto fail;
-
-        d->max_vcpus = config->max_vcpus;
-
         watchdog_domain_init(d);
         init_status |= INIT_watchdog;
 
diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index cdae8f6..bc89e84 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -41,9 +41,9 @@
 #define OPT_CONSOLE_STR "dtuart"
 
 #ifdef CONFIG_ARM_64
-#define MAX_VIRT_CPUS 128
+#define MAX_VIRT_CPUS 128u
 #else
-#define MAX_VIRT_CPUS 8
+#define MAX_VIRT_CPUS 8u
 #endif
 
 #define INVALID_VCPU_ID MAX_VIRT_CPUS
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index d682307..175de44 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -208,12 +208,6 @@ void vcpu_show_execution_state(struct vcpu *);
 void vcpu_show_registers(const struct vcpu *);
 void vcpu_switch_to_aarch64_mode(struct vcpu *);
 
-/* On ARM, the number of VCPUs is limited by the type of GIC emulated. */
-static inline unsigned int domain_max_vcpus(const struct domain *d)
-{
-    return vgic_max_vcpus(d);
-}
-
 /*
  * Due to the restriction of GICv3, the number of vCPUs in AFF0 is
  * limited to 16, thus only the first 4 bits of AFF0 are legal. We will
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 56ed5fe..447d24e 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -234,8 +234,6 @@ struct vgic_ops {
     /* lookup the struct pending_irq for a given LPI interrupt */
     struct pending_irq *(*lpi_to_pending)(struct domain *d, unsigned int vlpi);
     int (*lpi_get_priority)(struct domain *d, uint32_t vlpi);
-    /* Maximum number of vCPU supported */
-    const unsigned int max_vcpus;
 };
 
 /* Number of ranks of interrupt registers for a domain */
@@ -350,7 +348,8 @@ extern void vgic_clear_pending_irqs(struct vcpu *v);
 
 extern bool vgic_emulate(struct cpu_user_regs *regs, union hsr hsr);
 
-unsigned int vgic_max_vcpus(const struct domain *d);
+/* Maximum vCPUs for a specific vGIC version, or 0 for unsupported. */
+unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version);
 
 void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
                       paddr_t vbase, uint32_t aliased_offset);
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 643e69a..277f99f 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -664,8 +664,6 @@ unsigned long pv_guest_cr4_to_real_cr4(const struct vcpu *v);
              X86_CR4_OSXSAVE | X86_CR4_SMEP |               \
              X86_CR4_FSGSBASE | X86_CR4_SMAP | X86_CR4_PCIDE))
 
-#define domain_max_vcpus(d) (is_hvm_domain(d) ? HVM_MAX_VCPUS : MAX_VIRT_CPUS)
-
 static inline struct vcpu_guest_context *alloc_vcpu_guest_context(void)
 {
     return vmalloc(sizeof(struct vcpu_guest_context));
-- 
2.1.4


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

Re: [PATCH v5 4/5] xen/domain: Allocate d->vcpu[] earlier during domain_create()

[Julien Grall]

Hi Andrew,

On 14/11/2018 19:48, Andrew Cooper wrote:
> The ARM code has a chicken-and-egg problem.  One of the vGICv3 emulations
> wants to know d->max_vcpus to be able to size itself appropriately, but the
> current order of initialisation requires the vGIC to be set up before the
> requested number of vcpus can be checked.
> 
> Move the range checking of config->max_vcpus into sanitise_domain_config()
> path, which allows for the allocation of d->vcpu[] and d->max_vcpus to happen
> earlier during create, and in particular, before the call to
> arch_domain_create().
> 
> The x86 side is fairly easy, and implements the logical equivalent of
> domain_max_vcpus() but using XEN_DOMCTL_CDF_hvm_guest rather than
> is_hvm_domain().
> 
> For the ARM side, re-purpose vgic_max_vcpus() to take a domctl vGIC version,
> and return the maximum number of supported vCPUs, reusing 0 for "version not
> supported".  To avoid exporting the vgic_ops structures (which are in the
> process of being replaced), hard code the upper limits.
> 
> This allows for the removal of the domain_max_vcpus() infrastructure, which is
> done to prevent it being reused incorrectly in the future.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

Cheers,

> ---
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> 
> v3:
>   * Fix 4092/4096 typo
>   * Fix build in NEW_VGIC case
> v4:
>   * Move the min() into ARM's common arch_sanitise_domain_config()
>     implementation.  Simplfy the min() expression code by making MAX_VIRT_CPUS
>     be an unsigned constant.
> v5:
>   * s/max/min
> ---
>   xen/arch/arm/domain.c         | 18 ++++++++++++++++++
>   xen/arch/arm/vgic-v2.c        |  1 -
>   xen/arch/arm/vgic-v3.c        |  5 -----
>   xen/arch/arm/vgic.c           | 16 ++++++++++++++--
>   xen/arch/arm/vgic/vgic-init.c |  3 ---
>   xen/arch/arm/vgic/vgic.c      | 16 ++++++----------
>   xen/arch/x86/domain.c         | 10 ++++++++++
>   xen/common/domain.c           | 33 ++++++++++++++++++++-------------
>   xen/include/asm-arm/config.h  |  4 ++--
>   xen/include/asm-arm/domain.h  |  6 ------
>   xen/include/asm-arm/vgic.h    |  5 ++---
>   xen/include/asm-x86/domain.h  |  2 --
>   12 files changed, 72 insertions(+), 47 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 71ad1f9..1d926dc 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -601,6 +601,8 @@ void vcpu_switch_to_aarch64_mode(struct vcpu *v)
>   
>   int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>   {
> +    unsigned int max_vcpus;
> +
>       if ( config->flags != (XEN_DOMCTL_CDF_hvm_guest | XEN_DOMCTL_CDF_hap) )
>       {
>           dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", config->flags);
> @@ -626,6 +628,22 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>           }
>       }
>   
> +    /* max_vcpus depends on the GIC version, and Xen's compiled limit. */
> +    max_vcpus = min(vgic_max_vcpus(config->arch.gic_version), MAX_VIRT_CPUS);
> +
> +    if ( max_vcpus == 0 )
> +    {
> +        dprintk(XENLOG_INFO, "Unsupported GIC version\n");
> +        return -EINVAL;
> +    }
> +
> +    if ( config->max_vcpus > max_vcpus )
> +    {
> +        dprintk(XENLOG_INFO, "Requested vCPUs (%u) exceeds max (%u)\n",
> +                config->max_vcpus, max_vcpus);
> +        return -EINVAL;
> +    }
> +
>       return 0;
>   }
>   
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index bf77899..64b141f 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -725,7 +725,6 @@ static const struct vgic_ops vgic_v2_ops = {
>       .domain_free = vgic_v2_domain_free,
>       .lpi_to_pending = vgic_v2_lpi_to_pending,
>       .lpi_get_priority = vgic_v2_lpi_get_priority,
> -    .max_vcpus = 8,
>   };
>   
>   int vgic_v2_init(struct domain *d, int *mmio_count)
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index c14bcd8..519cc72 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1822,11 +1822,6 @@ static const struct vgic_ops v3_ops = {
>       .emulate_reg  = vgic_v3_emulate_reg,
>       .lpi_to_pending = vgic_v3_lpi_to_pending,
>       .lpi_get_priority = vgic_v3_lpi_get_priority,
> -    /*
> -     * We use both AFF1 and AFF0 in (v)MPIDR. Thus, the max number of CPU
> -     * that can be supported is up to 4096(==256*16) in theory.
> -     */
> -    .max_vcpus = 4096,
>   };
>   
>   int vgic_v3_init(struct domain *d, int *mmio_count)
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 5a4f082..f2608b0 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -667,9 +667,21 @@ void vgic_free_virq(struct domain *d, unsigned int virq)
>       clear_bit(virq, d->arch.vgic.allocated_irqs);
>   }
>   
> -unsigned int vgic_max_vcpus(const struct domain *d)
> +unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)
>   {
> -    return min_t(unsigned int, MAX_VIRT_CPUS, d->arch.vgic.handler->max_vcpus);
> +    switch ( domctl_vgic_version )
> +    {
> +    case XEN_DOMCTL_CONFIG_GIC_V2:
> +        return 8;
> +
> +#ifdef CONFIG_GICV3
> +    case XEN_DOMCTL_CONFIG_GIC_V3:
> +        return 4096;
> +#endif
> +
> +    default:
> +        return 0;
> +    }
>   }
>   
>   /*
> diff --git a/xen/arch/arm/vgic/vgic-init.c b/xen/arch/arm/vgic/vgic-init.c
> index bfd3d09..62ae553 100644
> --- a/xen/arch/arm/vgic/vgic-init.c
> +++ b/xen/arch/arm/vgic/vgic-init.c
> @@ -112,9 +112,6 @@ int domain_vgic_register(struct domain *d, int *mmio_count)
>           BUG();
>       }
>   
> -    if ( d->max_vcpus > domain_max_vcpus(d) )
> -        return -E2BIG;
> -
>       d->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
>       d->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
>       d->arch.vgic.vgic_redist_base = VGIC_ADDR_UNDEF;
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> index 7c3cfc5..e2844dc 100644
> --- a/xen/arch/arm/vgic/vgic.c
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -949,20 +949,16 @@ void vgic_sync_hardware_irq(struct domain *d,
>       spin_unlock_irqrestore(&desc->lock, flags);
>   }
>   
> -unsigned int vgic_max_vcpus(const struct domain *d)
> +unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)
>   {
> -    unsigned int vgic_vcpu_limit;
> -
> -    switch ( d->arch.vgic.version )
> +    switch ( domctl_vgic_version )
>       {
> -    case GIC_V2:
> -        vgic_vcpu_limit = VGIC_V2_MAX_CPUS;
> -        break;
> +    case XEN_DOMCTL_CONFIG_GIC_V2:
> +        return VGIC_V2_MAX_CPUS;
> +
>       default:
> -        BUG();
> +        return 0;
>       }
> -
> -    return min_t(unsigned int, MAX_VIRT_CPUS, vgic_vcpu_limit);
>   }
>   
>   #ifdef CONFIG_GICV3
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 272fd84..295b10c 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -421,6 +421,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_guest;
> +    unsigned int max_vcpus;
>   
>       if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
>       {
> @@ -428,6 +429,15 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>           return -EINVAL;
>       }
>   
> +    max_vcpus = hvm ? HVM_MAX_VCPUS : MAX_VIRT_CPUS;
> +
> +    if ( config->max_vcpus > max_vcpus )
> +    {
> +        dprintk(XENLOG_INFO, "Requested vCPUs (%u) exceeds max (%u)\n",
> +                config->max_vcpus, max_vcpus);
> +        return -EINVAL;
> +    }
> +
>       return 0;
>   }
>   
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index f69f405..78cc524 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -300,6 +300,12 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
>           return -EINVAL;
>       }
>   
> +    if ( config->max_vcpus < 1 )
> +    {
> +        dprintk(XENLOG_INFO, "No vCPUS\n");
> +        return -EINVAL;
> +    }
> +
>       return arch_sanitise_domain_config(config);
>   }
>   
> @@ -345,6 +351,20 @@ struct domain *domain_create(domid_t domid,
>   
>       TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
>   
> +    /*
> +     * Allocate d->vcpu[] and set ->max_vcpus up early.  Various per-domain
> +     * resources want to be sized based on max_vcpus.
> +     */
> +    if ( !is_system_domain(d) )
> +    {
> +        err = -ENOMEM;
> +        d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus);
> +        if ( !d->vcpu )
> +            goto fail;
> +
> +        d->max_vcpus = config->max_vcpus;
> +    }
> +
>       lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid, "Domain");
>   
>       if ( (err = xsm_alloc_security_domain(d)) != 0 )
> @@ -396,19 +416,6 @@ struct domain *domain_create(domid_t domid,
>   
>       if ( !is_idle_domain(d) )
>       {
> -        /* Check d->max_vcpus and allocate d->vcpu[]. */
> -        err = -EINVAL;
> -        if ( config->max_vcpus < 1 ||
> -             config->max_vcpus > domain_max_vcpus(d) )
> -            goto fail;
> -
> -        err = -ENOMEM;
> -        d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus);
> -        if ( !d->vcpu )
> -            goto fail;
> -
> -        d->max_vcpus = config->max_vcpus;
> -
>           watchdog_domain_init(d);
>           init_status |= INIT_watchdog;
>   
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index cdae8f6..bc89e84 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -41,9 +41,9 @@
>   #define OPT_CONSOLE_STR "dtuart"
>   
>   #ifdef CONFIG_ARM_64
> -#define MAX_VIRT_CPUS 128
> +#define MAX_VIRT_CPUS 128u
>   #else
> -#define MAX_VIRT_CPUS 8
> +#define MAX_VIRT_CPUS 8u
>   #endif
>   
>   #define INVALID_VCPU_ID MAX_VIRT_CPUS
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index d682307..175de44 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -208,12 +208,6 @@ void vcpu_show_execution_state(struct vcpu *);
>   void vcpu_show_registers(const struct vcpu *);
>   void vcpu_switch_to_aarch64_mode(struct vcpu *);
>   
> -/* On ARM, the number of VCPUs is limited by the type of GIC emulated. */
> -static inline unsigned int domain_max_vcpus(const struct domain *d)
> -{
> -    return vgic_max_vcpus(d);
> -}
> -
>   /*
>    * Due to the restriction of GICv3, the number of vCPUs in AFF0 is
>    * limited to 16, thus only the first 4 bits of AFF0 are legal. We will
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 56ed5fe..447d24e 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -234,8 +234,6 @@ struct vgic_ops {
>       /* lookup the struct pending_irq for a given LPI interrupt */
>       struct pending_irq *(*lpi_to_pending)(struct domain *d, unsigned int vlpi);
>       int (*lpi_get_priority)(struct domain *d, uint32_t vlpi);
> -    /* Maximum number of vCPU supported */
> -    const unsigned int max_vcpus;
>   };
>   
>   /* Number of ranks of interrupt registers for a domain */
> @@ -350,7 +348,8 @@ extern void vgic_clear_pending_irqs(struct vcpu *v);
>   
>   extern bool vgic_emulate(struct cpu_user_regs *regs, union hsr hsr);
>   
> -unsigned int vgic_max_vcpus(const struct domain *d);
> +/* Maximum vCPUs for a specific vGIC version, or 0 for unsupported. */
> +unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version);
>   
>   void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
>                         paddr_t vbase, uint32_t aliased_offset);
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 643e69a..277f99f 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -664,8 +664,6 @@ unsigned long pv_guest_cr4_to_real_cr4(const struct vcpu *v);
>                X86_CR4_OSXSAVE | X86_CR4_SMEP |               \
>                X86_CR4_FSGSBASE | X86_CR4_SMAP | X86_CR4_PCIDE))
>   
> -#define domain_max_vcpus(d) (is_hvm_domain(d) ? HVM_MAX_VCPUS : MAX_VIRT_CPUS)
> -
>   static inline struct vcpu_guest_context *alloc_vcpu_guest_context(void)
>   {
>       return vmalloc(sizeof(struct vcpu_guest_context));
> 

-- 
Julien Grall

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

Re: [PATCH v3 3/5] xen/domain: Move guest type checks into the arch_sanitise_domain_config() path

[Jan Beulich]

>>> On 14.11.18 at 18:44, <andrew.cooper3@citrix.com> wrote:
> This is a more appropriate location for the checks to happen, and cleans up
> the common code substantially.
> 
> Take the opportunity to make ARM strictly require HVM|HAP for guests, which is
> how the toolstack behaves, and leave a dprintk() behind for auditing failures.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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