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

[PATCH RFC 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 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

Andrew Cooper (5):
  xen/domain: Introduce a new check_domain_config() helper
  xen/domain: Introduce a new arch_check_domain_config() helper
  xen/domain: Audit config->max_vcpus during {,arch_}check_domain_config()
  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         | 62 +++++++++++++++++++++++++++++--------------
 xen/arch/arm/vgic-v2.c        |  1 -
 xen/arch/arm/vgic-v3.c        | 34 ++----------------------
 xen/arch/arm/vgic.c           |  5 ----
 xen/arch/arm/vgic/vgic-init.c |  3 ---
 xen/arch/arm/vgic/vgic.c      | 16 -----------
 xen/arch/x86/domain.c         | 11 ++++++++
 xen/common/domain.c           | 45 ++++++++++++++++++++++---------
 xen/common/domctl.c           |  9 -------
 xen/include/asm-arm/domain.h  |  6 -----
 xen/include/asm-arm/vgic.h    |  4 ---
 xen/include/asm-x86/domain.h  |  2 --
 xen/include/xen/sched.h       |  6 +++++
 13 files changed, 93 insertions(+), 111 deletions(-)

-- 
2.1.4


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

[PATCH 1/5] xen/domain: Introduce a new check_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.
This means that the corner case of the toolstack providing bad configuration
will burn a domid, but production setups shouldn't ever get into this
situation.

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/common/domain.c | 15 +++++++++++++++
 xen/common/domctl.c |  9 ---------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 65151e2..3f09a57 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -288,6 +288,18 @@ static void _domain_destroy(struct domain *d)
     free_domain_struct(d);
 }
 
+static int check_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) )
+        return -EINVAL;
+
+    return 0;
+}
+
 struct domain *domain_create(domid_t domid,
                              struct xen_domctl_createdomain *config,
                              bool is_priv)
@@ -297,6 +309,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 = check_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 2/5] xen/domain: Introduce a new arch_check_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>
---
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   | 44 ++++++++++++++++++++++++--------------------
 xen/arch/x86/domain.c   |  5 +++++
 xen/common/domain.c     |  2 +-
 xen/include/xen/sched.h |  6 ++++++
 4 files changed, 36 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index feebbf5..43593a4 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -599,6 +599,29 @@ void vcpu_switch_to_aarch64_mode(struct vcpu *v)
     v->arch.hcr_el2 |= HCR_RW;
 }
 
+int arch_check_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:
+            BUG();
+        }
+    }
+
+    return 0;
+}
+
 int arch_domain_create(struct domain *d,
                        struct xen_domctl_createdomain *config)
 {
@@ -629,24 +652,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 +661,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 d67a047..26cab7c 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -401,6 +401,11 @@ void arch_vcpu_destroy(struct vcpu *v)
         pv_vcpu_destroy(v);
 }
 
+int arch_check_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 3f09a57..236c2ad 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -297,7 +297,7 @@ static int check_domain_config(struct xen_domctl_createdomain *config)
                            XEN_DOMCTL_CDF_xs_domain) )
         return -EINVAL;
 
-    return 0;
+    return arch_check_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 0ba80cb..7a35e53 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -542,6 +542,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_check_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 3/5] xen/domain: Audit config->max_vcpus during {, arch_}check_domain_config()

[Andrew Cooper]

The purpose of this is to move the auduting to be earlier than
arch_domain_create().

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>

The max_vcpus setting for GIC_V3 is somewhat confusing.  The current GIC_V3
driver claims to support 4096 cpus, while the newer GIC_V3 driver uses 255.
In both cases, this gets clipped to 128 or 8 by the MAX_VIRT_CPUS setting.
---
 xen/arch/arm/domain.c | 18 ++++++++++++++++++
 xen/arch/x86/domain.c |  6 ++++++
 xen/common/domain.c   |  3 +++
 3 files changed, 27 insertions(+)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 43593a4..9676893 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_check_domain_config(struct xen_domctl_createdomain *config)
 {
+    unsigned int max_vcpus = 0;
+
     /* Fill in the native GIC version, passed back to the toolstack. */
     if ( config->arch.gic_version == XEN_DOMCTL_CONFIG_GIC_NATIVE )
     {
@@ -619,6 +621,22 @@ int arch_check_domain_config(struct xen_domctl_createdomain *config)
         }
     }
 
+    /* Calculate the maximum number of vcpus from the selected GIC version... */
+    switch ( config->arch.gic_version )
+    {
+    case GIC_V2: max_vcpus = 8;   break;
+    case GIC_V3: max_vcpus = 255; break;
+
+    default:
+        return -EOPNOTSUPP;
+    }
+
+    /* ... clipped at the maximum value Xen has been configured for. */
+    max_vcpus = min(max_vcpus, MAX_VIRT_CPUS + 0u);
+
+    if ( config->max_vcpus > max_vcpus )
+        return -EINVAL;
+
     return 0;
 }
 
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 26cab7c..19023d4 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -403,6 +403,12 @@ void arch_vcpu_destroy(struct vcpu *v)
 
 int arch_check_domain_config(struct xen_domctl_createdomain *config)
 {
+    unsigned int max_vcpus = ((config->flags & XEN_DOMCTL_CDF_hvm_guest)
+                              ? HVM_MAX_VCPUS : MAX_VIRT_CPUS);
+
+    if ( config->max_vcpus > max_vcpus )
+        return -EINVAL;
+
     return 0;
 }
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 236c2ad..9882550 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -297,6 +297,9 @@ static int check_domain_config(struct xen_domctl_createdomain *config)
                            XEN_DOMCTL_CDF_xs_domain) )
         return -EINVAL;
 
+    if ( config->max_vcpus < 1 )
+        return -EINVAL;
+
     return arch_check_domain_config(config);
 }
 
-- 
2.1.4


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

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

[Andrew Cooper]

With config->max_vcpus now being audited by the check_domain_config() path, we
can alloate d->vcpu[] before calling arch_domain_create().

Doing do allows for the removal of domain_max_vcpus(), which on the ARM side
removes vgic_max_vcpus() and the .max_vcpus field from vgic_ops.

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/vgic-v2.c        |  1 -
 xen/arch/arm/vgic-v3.c        |  5 -----
 xen/arch/arm/vgic.c           |  5 -----
 xen/arch/arm/vgic/vgic-init.c |  3 ---
 xen/arch/arm/vgic/vgic.c      | 16 ----------------
 xen/common/domain.c           | 27 ++++++++++++++-------------
 xen/include/asm-arm/domain.h  |  6 ------
 xen/include/asm-arm/vgic.h    |  4 ----
 xen/include/asm-x86/domain.h  |  2 --
 9 files changed, 14 insertions(+), 55 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index f6c11f1..055a30a 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -724,7 +724,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 efe824c..b4476f3 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..664aa0b 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -667,11 +667,6 @@ 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)
-{
-    return min_t(unsigned int, MAX_VIRT_CPUS, d->arch.vgic.handler->max_vcpus);
-}
-
 /*
  * Local variables:
  * mode: C
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..e1ee216 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -949,22 +949,6 @@ 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_vcpu_limit;
-
-    switch ( d->arch.vgic.version )
-    {
-    case GIC_V2:
-        vgic_vcpu_limit = VGIC_V2_MAX_CPUS;
-        break;
-    default:
-        BUG();
-    }
-
-    return min_t(unsigned int, MAX_VIRT_CPUS, vgic_vcpu_limit);
-}
-
 #ifdef CONFIG_GICV3
 /* Dummy implementation to allow building without actual vGICv3 support. */
 void vgic_v3_setup_hw(paddr_t dbase,
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 9882550..ee7b889 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -354,6 +354,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 )
@@ -405,19 +419,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 374fdaa..5cf2858 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -238,8 +238,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 */
@@ -354,8 +352,6 @@ 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);
-
 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 cb0721e..9854a74 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -659,8 +659,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 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>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: 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 b4476f3..e909502 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 3/5] xen/domain: Audit config->max_vcpus during {, arch_}check_domain_config()

[Alan Robinson]

[-- Attachment #1.1: Type: text/plain, Size: 193 bytes --]

On Fri, Oct 05, 2018 at 04:54:47PM +0200, Andrew Cooper wrote:
> The purpose of this is to move the auduting to be earlier than
> arch_domain_create().
> 

s/auduting/auditing/

Alan


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2642 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Jan Beulich]

>>> On 05.10.18 at 16:54, <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.
> This means that the corner case of the toolstack providing bad configuration
> will burn a domid, but production setups shouldn't ever get into this
> situation.

"Burn" as in "skip in the current round", not as in "leak" afaiu?

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -288,6 +288,18 @@ static void _domain_destroy(struct domain *d)
>      free_domain_struct(d);
>  }
>  
> +static int check_domain_config(struct xen_domctl_createdomain *config)

I was tempted to ask for the parameter to be constified, but since on
its own the code movement here makes no sense (and the description
also doesn't supply any hint), I've peeked into patch 2, where I found
that Arm's arch_check_domain_config() actually modifies the config.
With that I don't consider "check" the right term for the function name;
"sanitize" or "massage" perhaps?

Jan



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

Re: [PATCH 2/5] xen/domain: Introduce a new arch_check_domain_config() helper

[Jan Beulich]

>>> On 05.10.18 at 16:54, <andrew.cooper3@citrix.com> wrote:
> 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>

The minimal x86 pieces
Acked-by: Jan Beulich <jbeulich@suse.com>
(possibly subject to renaming as per patch 1's comment)

Jan



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

Re: [PATCH 3/5] xen/domain: Audit config->max_vcpus during {, arch_}check_domain_config()

[Jan Beulich]

>>> On 05.10.18 at 16:54, <andrew.cooper3@citrix.com> wrote:
> --- 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_check_domain_config(struct xen_domctl_createdomain *config)
>  {
> +    unsigned int max_vcpus = 0;

Is the initializer really needed here considering ...

> @@ -619,6 +621,22 @@ int arch_check_domain_config(struct xen_domctl_createdomain *config)
>          }
>      }
>  
> +    /* Calculate the maximum number of vcpus from the selected GIC version... */
> +    switch ( config->arch.gic_version )
> +    {
> +    case GIC_V2: max_vcpus = 8;   break;
> +    case GIC_V3: max_vcpus = 255; break;
> +
> +    default:
> +        return -EOPNOTSUPP;

... this?

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -297,6 +297,9 @@ static int check_domain_config(struct xen_domctl_createdomain *config)
>                             XEN_DOMCTL_CDF_xs_domain) )
>          return -EINVAL;
>  
> +    if ( config->max_vcpus < 1 )
> +        return -EINVAL;
> +
>      return arch_check_domain_config(config);
>  }

Any reason you don't remove the now redundant check from
domain_create(), which would allow ditching altogether x86's
domain_max_vcpus()?

Jan



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

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

[Jan Beulich]

>>> On 05.10.18 at 16:54, <andrew.cooper3@citrix.com> wrote:
> @@ -405,19 +419,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;

Ah, there it goes away. But I think it would be more logical for this to
happen in the previous patch. Anyway
Reviewed-by: Jan Beulich <jbeulich@suse.com>
for both, preferably (but not necessarily) with the removal moved
there.

Jan



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

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

[Andrew Cooper]

On 08/10/18 14:51, Jan Beulich wrote:
>>>> On 05.10.18 at 16:54, <andrew.cooper3@citrix.com> wrote:
>> @@ -405,19 +419,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;
> Ah, there it goes away. But I think it would be more logical for this to
> happen in the previous patch. Anyway
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> for both, preferably (but not necessarily) with the removal moved
> there.

The x86 side is trivial, but the ARM side is not.  I don't think patches
3 and 4 should be merged, but I'll let Julien/Stefano have the final say.

~Andrew

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

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

[Jan Beulich]

>>> On 08.10.18 at 19:39, <andrew.cooper3@citrix.com> wrote:
> On 08/10/18 14:51, Jan Beulich wrote:
>>>>> On 05.10.18 at 16:54, <andrew.cooper3@citrix.com> wrote:
>>> @@ -405,19 +419,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;
>> Ah, there it goes away. But I think it would be more logical for this to
>> happen in the previous patch. Anyway
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> for both, preferably (but not necessarily) with the removal moved
>> there.
> 
> The x86 side is trivial, but the ARM side is not.  I don't think patches
> 3 and 4 should be merged, but I'll let Julien/Stefano have the final say.

I didn't suggest merging the patches, I'd merely like to see
the removal above to move into the earlier patch, and the
x86 variant of domain_max_vcpus() be dropped there
instead of here (because that's where the need for both
logically disappears).

Jan



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

Re: [PATCH 2/5] xen/domain: Introduce a new arch_check_domain_config() helper

[Julien Grall]

Hi Andrew,

On 05/10/2018 15:54, Andrew Cooper wrote:
> 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>
> ---
> 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>

Regardless the decision on the hook name:

Reviewed-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 3/5] xen/domain: Audit config->max_vcpus during {, arch_}check_domain_config()

[Julien Grall]

Hi Andrew,

On 05/10/2018 15:54, Andrew Cooper wrote:
> The purpose of this is to move the auduting to be earlier than
> arch_domain_create().
> 
> 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>
> 
> The max_vcpus setting for GIC_V3 is somewhat confusing.  The current GIC_V3
> driver claims to support 4096 cpus, while the newer GIC_V3 driver uses 255.

The maximum number of vCPUs supported for GICv3 depends on the number of 
affinity levels supported by the vGIC emulation.

> ---
>   xen/arch/arm/domain.c | 18 ++++++++++++++++++
>   xen/arch/x86/domain.c |  6 ++++++
>   xen/common/domain.c   |  3 +++
>   3 files changed, 27 insertions(+)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 43593a4..9676893 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_check_domain_config(struct xen_domctl_createdomain *config)
>   {
> +    unsigned int max_vcpus = 0;
> +
>       /* Fill in the native GIC version, passed back to the toolstack. */
>       if ( config->arch.gic_version == XEN_DOMCTL_CONFIG_GIC_NATIVE )
>       {
> @@ -619,6 +621,22 @@ int arch_check_domain_config(struct xen_domctl_createdomain *config)
>           }
>       }
>   
> +    /* Calculate the maximum number of vcpus from the selected GIC version... */
> +    switch ( config->arch.gic_version )
> +    {
> +    case GIC_V2: max_vcpus = 8;   break;
> +    case GIC_V3: max_vcpus = 255; break;
> +
> +    default:
> +        return -EOPNOTSUPP;
> +    }

I would prefer to keep those values in a separate helper implemented by 
each vGIC.

> +
> +    /* ... clipped at the maximum value Xen has been configured for. */
> +    max_vcpus = min(max_vcpus, MAX_VIRT_CPUS + 0u);

+ 0U feels a bit odd to read. It would be better to append u in 
MAX_VIRT_CPUS.

> +
> +    if ( config->max_vcpus > max_vcpus )
> +        return -EINVAL;
> +
>       return 0;
>   }
>   
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 26cab7c..19023d4 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -403,6 +403,12 @@ void arch_vcpu_destroy(struct vcpu *v)
>   
>   int arch_check_domain_config(struct xen_domctl_createdomain *config)
>   {
> +    unsigned int max_vcpus = ((config->flags & XEN_DOMCTL_CDF_hvm_guest)
> +                              ? HVM_MAX_VCPUS : MAX_VIRT_CPUS);
> +
> +    if ( config->max_vcpus > max_vcpus )
> +        return -EINVAL;
> +
>       return 0;
>   }
>   
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 236c2ad..9882550 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -297,6 +297,9 @@ static int check_domain_config(struct xen_domctl_createdomain *config)
>                              XEN_DOMCTL_CDF_xs_domain) )
>           return -EINVAL;
>   
> +    if ( config->max_vcpus < 1 )
> +        return -EINVAL;
> +
>       return arch_check_domain_config(config);
>   }
>   
> 

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Andrew,

On 05/10/2018 15:54, Andrew Cooper wrote:
> 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>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>

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

Cheers,

> ---
>   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 b4476f3..e909502 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);
> 

-- 
Julien Grall

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

Re: [PATCH 3/5] xen/domain: Audit config->max_vcpus during {, arch_}check_domain_config()

[Andrew Cooper]

On 09/10/18 12:23, Julien Grall wrote:
> On 05/10/2018 15:54, Andrew Cooper wrote:
>> ---
>>   xen/arch/arm/domain.c | 18 ++++++++++++++++++
>>   xen/arch/x86/domain.c |  6 ++++++
>>   xen/common/domain.c   |  3 +++
>>   3 files changed, 27 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 43593a4..9676893 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_check_domain_config(struct xen_domctl_createdomain *config)
>>   {
>> +    unsigned int max_vcpus = 0;
>> +
>>       /* Fill in the native GIC version, passed back to the
>> toolstack. */
>>       if ( config->arch.gic_version == XEN_DOMCTL_CONFIG_GIC_NATIVE )
>>       {
>> @@ -619,6 +621,22 @@ int arch_check_domain_config(struct
>> xen_domctl_createdomain *config)
>>           }
>>       }
>>   +    /* Calculate the maximum number of vcpus from the selected GIC
>> version... */
>> +    switch ( config->arch.gic_version )
>> +    {
>> +    case GIC_V2: max_vcpus = 8;   break;
>> +    case GIC_V3: max_vcpus = 255; break;
>> +
>> +    default:
>> +        return -EOPNOTSUPP;
>> +    }
>
> I would prefer to keep those values in a separate helper implemented
> by each vGIC.

How do you intend that working?  The values can't be hooked off a GIC
object, because we don't have one yet.

>
>> +
>> +    /* ... clipped at the maximum value Xen has been configured for. */
>> +    max_vcpus = min(max_vcpus, MAX_VIRT_CPUS + 0u);
>
> + 0U feels a bit odd to read. It would be better to append u in
> MAX_VIRT_CPUS.

Will do.

~Andrew

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

Re: [PATCH 3/5] xen/domain: Audit config->max_vcpus during {, arch_}check_domain_config()

[Andrew Cooper]

On 08/10/18 14:45, Jan Beulich wrote:
>
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -297,6 +297,9 @@ static int check_domain_config(struct xen_domctl_createdomain *config)
>>                             XEN_DOMCTL_CDF_xs_domain) )
>>          return -EINVAL;
>>  
>> +    if ( config->max_vcpus < 1 )
>> +        return -EINVAL;
>> +
>>      return arch_check_domain_config(config);
>>  }
> Any reason you don't remove the now redundant check from
> domain_create(), which would allow ditching altogether x86's
> domain_max_vcpus()?

That's done in the next patch, to simplify this one.

~Andrew

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

Re: [PATCH 3/5] xen/domain: Audit config->max_vcpus during {, arch_}check_domain_config()

[Jan Beulich]

>>> On 09.11.18 at 19:44, <andrew.cooper3@citrix.com> wrote:
> On 08/10/18 14:45, Jan Beulich wrote:
>>
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -297,6 +297,9 @@ static int check_domain_config(struct 
> xen_domctl_createdomain *config)
>>>                             XEN_DOMCTL_CDF_xs_domain) )
>>>          return -EINVAL;
>>>  
>>> +    if ( config->max_vcpus < 1 )
>>> +        return -EINVAL;
>>> +
>>>      return arch_check_domain_config(config);
>>>  }
>> Any reason you don't remove the now redundant check from
>> domain_create(), which would allow ditching altogether x86's
>> domain_max_vcpus()?
> 
> That's done in the next patch, to simplify this one.

Yeah, I had noticed this once I got there. Half a sentence in the
description to explain the now possible cleanup is deferred
intentionally would have helped, but that's fine in any event.

Jan



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

Re: [PATCH 3/5] xen/domain: Audit config->max_vcpus during {, arch_}check_domain_config()

[Julien Grall]

Hi,

On 11/9/18 6:43 PM, Andrew Cooper wrote:
> On 09/10/18 12:23, Julien Grall wrote:
>> On 05/10/2018 15:54, Andrew Cooper wrote:
>>> ---
>>>    xen/arch/arm/domain.c | 18 ++++++++++++++++++
>>>    xen/arch/x86/domain.c |  6 ++++++
>>>    xen/common/domain.c   |  3 +++
>>>    3 files changed, 27 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>> index 43593a4..9676893 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_check_domain_config(struct xen_domctl_createdomain *config)
>>>    {
>>> +    unsigned int max_vcpus = 0;
>>> +
>>>        /* Fill in the native GIC version, passed back to the
>>> toolstack. */
>>>        if ( config->arch.gic_version == XEN_DOMCTL_CONFIG_GIC_NATIVE )
>>>        {
>>> @@ -619,6 +621,22 @@ int arch_check_domain_config(struct
>>> xen_domctl_createdomain *config)
>>>            }
>>>        }
>>>    +    /* Calculate the maximum number of vcpus from the selected GIC
>>> version... */
>>> +    switch ( config->arch.gic_version )
>>> +    {
>>> +    case GIC_V2: max_vcpus = 8;   break;
>>> +    case GIC_V3: max_vcpus = 255; break;
>>> +
>>> +    default:
>>> +        return -EOPNOTSUPP;
>>> +    }
>>
>> I would prefer to keep those values in a separate helper implemented
>> by each vGIC.
> 
> How do you intend that working?  The values can't be hooked off a GIC
> object, because we don't have one yet.

Sorry for the confusion. By each vGIC I meant implementation. I would 
rework vgic_max_vcpus to take the GIC version in parameter and rather 
number of vCPUs supported.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 3/5] xen/domain: Audit config->max_vcpus during {, arch_}check_domain_config()

[Andrew Cooper]

On 12/11/18 11:43, Julien Grall wrote:
> Hi,
>
> On 11/9/18 6:43 PM, Andrew Cooper wrote:
>> On 09/10/18 12:23, Julien Grall wrote:
>>> On 05/10/2018 15:54, Andrew Cooper wrote:
>>>> ---
>>>>    xen/arch/arm/domain.c | 18 ++++++++++++++++++
>>>>    xen/arch/x86/domain.c |  6 ++++++
>>>>    xen/common/domain.c   |  3 +++
>>>>    3 files changed, 27 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>>> index 43593a4..9676893 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_check_domain_config(struct xen_domctl_createdomain
>>>> *config)
>>>>    {
>>>> +    unsigned int max_vcpus = 0;
>>>> +
>>>>        /* Fill in the native GIC version, passed back to the
>>>> toolstack. */
>>>>        if ( config->arch.gic_version == XEN_DOMCTL_CONFIG_GIC_NATIVE )
>>>>        {
>>>> @@ -619,6 +621,22 @@ int arch_check_domain_config(struct
>>>> xen_domctl_createdomain *config)
>>>>            }
>>>>        }
>>>>    +    /* Calculate the maximum number of vcpus from the selected GIC
>>>> version... */
>>>> +    switch ( config->arch.gic_version )
>>>> +    {
>>>> +    case GIC_V2: max_vcpus = 8;   break;
>>>> +    case GIC_V3: max_vcpus = 255; break;
>>>> +
>>>> +    default:
>>>> +        return -EOPNOTSUPP;
>>>> +    }
>>>
>>> I would prefer to keep those values in a separate helper implemented
>>> by each vGIC.
>>
>> How do you intend that working?  The values can't be hooked off a GIC
>> object, because we don't have one yet.
>
> Sorry for the confusion. By each vGIC I meant implementation. I would
> rework vgic_max_vcpus to take the GIC version in parameter and rather
> number of vCPUs supported.

Thanks for the pointer.  I'll see what I can do.

~Andrew

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