[PATCH V5 00/10] Support more than 8 vcpus on arm64 with GICv3

[PATCH V5 00/10] Support more than 8 vcpus on arm64 with GICv3

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

Currently the number of vcpus on arm64 with GICv3 is limited up to 8 due
to the fixed size of redistributor mmio region. Increasing the size
makes the number expand to 16 because of AFF0 restriction on GICv3.
To create a guest up to 128 vCPUs, which is the maxium number that GIC-500
can support, this patchset uses the AFF1 information to create a mapping
relation between vCPUID and vMPIDR and deals with the related issues.

These patches are written based upon Julien's "GICv2 on GICv3" series
and the IROUTER emulation cleanup patch.

Changes from V4:
* Split the patch 4/8 of V3 into two part:
  - Use cpumask_t type for vcpu_mask in vgic_to_sgi.
  - Use AFF1 when translating ICC_SGI1R_EL1 to cpumask.
* Use a more efficient algorithm when calculate cpumask.
* Add a patch to call arch_domain_create before evtchn_init, because
  evtchn_init needs vgic info which is initialised during
  acrh_domain_create.
* Get the max vcpu info from vgic_ops.
* Minor changes according to previous reviews.
Changes from V3:
* Drop the wrong patch that altering domain_max_vcpus to a macro.
* Change the domain_max_vcpus to return value accodring to the version
  of the vGIC in used.
Changes from V2:
* Reorder the patch which increases MAX_VIRT_CPUS to the last to make
  this series bisectable.
* Drop the dynamic re-distributor region allocation patch in tools.
* Use cpumask_t type instead of unsigned long in vgic_to_sgi and do the
  translation from GICD_SGIR to vcpu_mask in both vGICv2 and vGICv3.
* Make domain_max_vcpus be alias of max_vcpus in struct domain
Changes from V1:
* Use the way that expanding the GICR address space to support up to 128
  redistributor in guest memory layout rather than use the dynamic
  allocation.
* Add support to include AFF1 information in vMPIDR/logical CPUID.


Chen Baozi (10):
  xen/arm: gic-v3: Increase the size of GICR in address space for guest
  xen/arm: Add functions of mapping between vCPUID and virtual affinity
  xen/arm: Use the new functions for vCPUID/vaffinity transformation
  xen/arm: Use cpumask_t type for vcpu_mask in vgic_to_sgi
  xen/arm64: gicv3: Use AFF1 when translating ICC_SGI1R_EL1 to cpumask
  tools/libxl: Set 'reg' of cpu node equal to MPIDR affinity for domU
  xen/arm: Set 'reg' of cpu node for dom0 to match MPIDR's affinity
  xen: Call arch_domain_create() before evtchn_init()
  xen/arm: make domain_max_vcpus return value from vgic_ops
  xen/arm64: increase MAX_VIRT_CPUS to 128 on arm64

 tools/libxl/libxl_arm.c           | 14 +++++++++++--
 xen/arch/arm/domain.c             | 11 +++++-----
 xen/arch/arm/domain_build.c       | 14 ++++++++++---
 xen/arch/arm/vgic-v2.c            | 22 +++++++++++++++++---
 xen/arch/arm/vgic-v3.c            | 35 ++++++++++++++++++++++++++------
 xen/arch/arm/vgic.c               | 24 ++++++++++------------
 xen/arch/arm/vpsci.c              |  5 +----
 xen/common/domain.c               |  8 ++++----
 xen/include/asm-arm/config.h      |  4 ++++
 xen/include/asm-arm/domain.h      | 42 +++++++++++++++++++++++++++++++++++++--
 xen/include/asm-arm/gic.h         |  4 ++++
 xen/include/asm-arm/gic_v3_defs.h |  5 +++++
 xen/include/asm-arm/vgic.h        |  4 +++-
 xen/include/public/arch-arm.h     |  4 ++--
 14 files changed, 151 insertions(+), 45 deletions(-)

-- 
2.1.4

[PATCH V5 01/10] xen/arm: gic-v3: Increase the size of GICR in address space for guest

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

Currently it only supports up to 8 vCPUs. Increase the region to hold
up to 128 vCPUs, which is the maximum number that GIC-500 supports.

Signed-off-by: Chen Baozi <baozich@gmail.com>
Reviewed-by: Julien Grall <julien.grall@citrix.com>
---
 xen/include/public/arch-arm.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index c029e0f..ec0c261 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -388,8 +388,8 @@ struct xen_arch_domainconfig {
 #define GUEST_GICV3_RDIST_STRIDE   0x20000ULL
 #define GUEST_GICV3_RDIST_REGIONS  1
 
-#define GUEST_GICV3_GICR0_BASE     0x03020000ULL    /* vCPU0 - vCPU7 */
-#define GUEST_GICV3_GICR0_SIZE     0x00100000ULL
+#define GUEST_GICV3_GICR0_BASE     0x03020000ULL    /* vCPU0 - vCPU127 */
+#define GUEST_GICV3_GICR0_SIZE     0x01000000ULL
 
 /*
  * 16MB == 4096 pages reserved for guest to use as a region to map its
-- 
2.1.4

[PATCH V5 02/10] xen/arm: Add functions of mapping between vCPUID and virtual affinity

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

GICv3 restricts that the maximum number of CPUs in affinity 0 (one
cluster) is 16. That is to say the upper 4 bits of affinity 0 is unused.
Current implementation considers that AFF0 is equal to vCPUID, which
makes all vCPUs in one cluster, limiting its number to 16. If we would
like to support more than 16 number of vCPU in one guest, we need to
make use of AFF1. Considering the unused upper 4 bits, we need to create
a pair of functions mapping the vCPUID and virtual affinity.

Signed-off-by: Chen Baozi <baozich@gmail.com>
---
 xen/include/asm-arm/domain.h | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 75b17af..b7b5cd2 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -266,6 +266,47 @@ static inline unsigned int domain_max_vcpus(const struct domain *d)
     return MAX_VIRT_CPUS;
 }
 
+/*
+ * 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
+ * use the first 2 affinity levels here, expanding the number of vCPU up
+ * to 4096 (16*256), which is more than 128 PEs that GIC-500 supports.
+ *
+ * Since we don't save information of vCPU's topology (affinity) in
+ * vMPIDR at the moment, we map the vcpuid to the vMPIDR linearly.
+ *
+ * XXX: We may have multi-threading or virtual cluster information in
+ * the future.
+ */
+static inline unsigned int vaffinity_to_vcpuid(register_t vaff)
+{
+    unsigned int vcpuid;
+
+    vaff &= MPIDR_HWID_MASK;
+
+    vcpuid = MPIDR_AFFINITY_LEVEL(vaff, 0);
+    vcpuid |= MPIDR_AFFINITY_LEVEL(vaff, 1) << 4;
+
+    return vcpuid;
+}
+
+static inline register_t vcpuid_to_vaffinity(unsigned int vcpuid)
+{
+    register_t vaff;
+
+    /*
+     * Right now only AFF0 and AFF1 are supported in virtual affinity.
+     * Since only the first 4 bits in AFF0 are used in GICv3, the
+     * available bits are 12 (4+8).
+     */
+    BUILD_BUG_ON(!(MAX_VIRT_CPUS < ((1 << 12))));
+
+    vaff = (vcpuid & 0x0f) << MPIDR_LEVEL_SHIFT(0);
+    vaff |= ((vcpuid >> 4) & MPIDR_LEVEL_MASK) << MPIDR_LEVEL_SHIFT(1);
+
+    return vaff;
+}
+
 #endif /* __ASM_DOMAIN_H__ */
 
 /*
-- 
2.1.4

[PATCH V5 03/10] xen/arm: Use the new functions for vCPUID/vaffinity transformation

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

There are 3 places to change:

* Initialise vMPIDR value in vcpu_initialise()
* Find the vCPU from vMPIDR affinity information when accessing GICD
  registers in vGIC
* Find the vCPU from vMPIDR affinity information when booting with vPSCI
  in vGIC
  - Also make the code for PSCI 0.1 use MPIDR-like value as the cpuid.

Signed-off-by: Chen Baozi <baozich@gmail.com>
---
Comments from Julien in the previous review:

I read more closely the spec of PSCI 0.1 Section 6.3 (ARM DEN 0022A):

"Ideally platform discovery mechanism such as firmware tables would be
used by secure firmware to describe the set of valid CPUIDs to the
hypervisor or Rich OS, if the former is not present. The hypervisor in
turn can create and supply virtual discovery mechanisms to its guests."

It looks like to me that the CPUID is equal to the "reg" field in the
CPU node which is an MPIDR-like value. So I think the affinity should be
called in both case.

 xen/arch/arm/domain.c  | 6 +-----
 xen/arch/arm/vgic-v3.c | 2 +-
 xen/arch/arm/vpsci.c   | 5 +----
 3 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 2bde26e..0cf147c 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -501,11 +501,7 @@ int vcpu_initialise(struct vcpu *v)
 
     v->arch.sctlr = SCTLR_GUEST_INIT;
 
-    /*
-     * By default exposes an SMP system with AFF0 set to the VCPU ID
-     * TODO: Handle multi-threading processor and cluster
-     */
-    v->arch.vmpidr = MPIDR_SMP | (v->vcpu_id << MPIDR_AFF0_SHIFT);
+    v->arch.vmpidr = MPIDR_SMP | vcpuid_to_vaffinity(v->vcpu_id);
 
     v->arch.actlr = READ_SYSREG32(ACTLR_EL1);
 
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 540f85f..ef9a71a 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -61,7 +61,7 @@ static struct vcpu *vgic_v3_irouter_to_vcpu(struct domain *d, uint64_t irouter)
     if ( irouter & GICD_IROUTER_SPI_MODE_ANY )
         return d->vcpu[0];
 
-    vcpu_id = irouter & MPIDR_AFF0_MASK;
+    vcpu_id = vaffinity_to_vcpuid(irouter);
     if ( vcpu_id >= d->max_vcpus )
         return NULL;
 
diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
index 5d899be..aebe1e2 100644
--- a/xen/arch/arm/vpsci.c
+++ b/xen/arch/arm/vpsci.c
@@ -32,10 +32,7 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
     int is_thumb = entry_point & 1;
     register_t vcpuid;
 
-    if( ver == XEN_PSCI_V_0_2 )
-        vcpuid = (target_cpu & MPIDR_HWID_MASK);
-    else
-        vcpuid = target_cpu;
+    vcpuid = vaffinity_to_vcpuid(target_cpu);
 
     if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
         return PSCI_INVALID_PARAMETERS;
-- 
2.1.4

[PATCH V5 04/10] xen/arm: Use cpumask_t type for vcpu_mask in vgic_to_sgi

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

Use cpumask_t instead of unsigned long which can only express 64 cpus at
the most. Add the {gicv2|gicv3}_sgir_to_cpumask in corresponding vGICs
to translate GICD_SGIR/ICC_SGI1R_EL1 to vcpu_mask for vgic_to_sgi.

Signed-off-by: Chen Baozi <baozich@gmail.com>
---
 xen/arch/arm/vgic-v2.c            | 16 +++++++++++++---
 xen/arch/arm/vgic-v3.c            | 19 +++++++++++++++----
 xen/arch/arm/vgic.c               | 24 +++++++++++-------------
 xen/include/asm-arm/gic.h         |  1 +
 xen/include/asm-arm/gic_v3_defs.h |  2 ++
 xen/include/asm-arm/vgic.h        |  2 +-
 6 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 3be1a51..17a3c9f 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -33,6 +33,15 @@
 #include <asm/gic.h>
 #include <asm/vgic.h>
 
+static inline void gicv2_sgir_to_cpumask(cpumask_t *cpumask,
+                                         const register_t sgir)
+{
+    unsigned long target_list;
+
+    target_list = ((sgir & GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT);
+    bitmap_copy(cpumask_bits(cpumask), &target_list, GICD_SGI_TARGET_BITS);
+}
+
 static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
 {
     struct hsr_dabt dabt = info->dabt;
@@ -201,16 +210,17 @@ static int vgic_v2_to_sgi(struct vcpu *v, register_t sgir)
     int virq;
     int irqmode;
     enum gic_sgi_mode sgi_mode;
-    unsigned long vcpu_mask = 0;
+    cpumask_t vcpu_mask;
 
+    cpumask_clear(&vcpu_mask);
     irqmode = (sgir & GICD_SGI_TARGET_LIST_MASK) >> GICD_SGI_TARGET_LIST_SHIFT;
     virq = (sgir & GICD_SGI_INTID_MASK);
-    vcpu_mask = (sgir & GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT;
 
     /* Map GIC sgi value to enum value */
     switch ( irqmode )
     {
     case GICD_SGI_TARGET_LIST_VAL:
+        gicv2_sgir_to_cpumask(&vcpu_mask, sgir);
         sgi_mode = SGI_TARGET_LIST;
         break;
     case GICD_SGI_TARGET_OTHERS_VAL:
@@ -226,7 +236,7 @@ static int vgic_v2_to_sgi(struct vcpu *v, register_t sgir)
         return 0;
     }
 
-    return vgic_to_sgi(v, sgir, sgi_mode, virq, vcpu_mask);
+    return vgic_to_sgi(v, sgir, sgi_mode, virq, &vcpu_mask);
 }
 
 static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index ef9a71a..a283c8c 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -972,22 +972,33 @@ write_ignore:
     return 1;
 }
 
+static inline void gicv3_sgir_to_cpumask(cpumask_t *cpumask,
+                                         const register_t sgir)
+{
+    unsigned long target_list;
+
+    target_list = sgir & ICH_SGI_TARGETLIST_MASK;
+    bitmap_copy(cpumask_bits(cpumask), &target_list, ICH_SGI_TARGET_BITS);
+
+}
+
 static int vgic_v3_to_sgi(struct vcpu *v, register_t sgir)
 {
     int virq;
     int irqmode;
     enum gic_sgi_mode sgi_mode;
-    unsigned long vcpu_mask = 0;
+    cpumask_t vcpu_mask;
 
+    cpumask_clear(&vcpu_mask);
     irqmode = (sgir >> ICH_SGI_IRQMODE_SHIFT) & ICH_SGI_IRQMODE_MASK;
     virq = (sgir >> ICH_SGI_IRQ_SHIFT ) & ICH_SGI_IRQ_MASK;
-    /* SGI's are injected at Rdist level 0. ignoring affinity 1, 2, 3 */
-    vcpu_mask = sgir & ICH_SGI_TARGETLIST_MASK;
 
     /* Map GIC sgi value to enum value */
     switch ( irqmode )
     {
     case ICH_SGI_TARGET_LIST:
+        /* SGI's are injected at Rdist level 0. ignoring affinity 1, 2, 3 */
+        gicv3_sgir_to_cpumask(&vcpu_mask, sgir);
         sgi_mode = SGI_TARGET_LIST;
         break;
     case ICH_SGI_TARGET_OTHERS:
@@ -998,7 +1009,7 @@ static int vgic_v3_to_sgi(struct vcpu *v, register_t sgir)
         return 0;
     }
 
-    return vgic_to_sgi(v, sgir, sgi_mode, virq, vcpu_mask);
+    return vgic_to_sgi(v, sgir, sgi_mode, virq, &vcpu_mask);
 }
 
 static int vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 7b387b7..f9e43f9 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -318,16 +318,12 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
     }
 }
 
-/* TODO: unsigned long is used to fit vcpu_mask.*/
 int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int virq,
-                unsigned long vcpu_mask)
+                cpumask_t *vcpu_mask)
 {
     struct domain *d = v->domain;
-    int vcpuid;
     int i;
 
-    ASSERT(d->max_vcpus < 8*sizeof(vcpu_mask));
-
     ASSERT( virq < 16 );
 
     switch ( irqmode )
@@ -341,12 +337,12 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
          * SGI_TARGET_SELF mode. So Force vcpu_mask to 0
          */
         perfc_incr(vgic_sgi_others);
-        vcpu_mask = 0;
+        cpumask_clear(vcpu_mask);
         for ( i = 0; i < d->max_vcpus; i++ )
         {
             if ( i != current->vcpu_id && d->vcpu[i] != NULL &&
                  is_vcpu_online(d->vcpu[i]) )
-                set_bit(i, &vcpu_mask);
+                cpumask_set_cpu(i, vcpu_mask);
         }
         break;
     case SGI_TARGET_SELF:
@@ -355,8 +351,8 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
          * SGI_TARGET_SELF mode. So Force vcpu_mask to 0
          */
         perfc_incr(vgic_sgi_self);
-        vcpu_mask = 0;
-        set_bit(current->vcpu_id, &vcpu_mask);
+        cpumask_clear(vcpu_mask);
+        cpumask_set_cpu(current->vcpu_id, vcpu_mask);
         break;
     default:
         gprintk(XENLOG_WARNING,
@@ -365,15 +361,17 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
         return 0;
     }
 
-    for_each_set_bit( vcpuid, &vcpu_mask, d->max_vcpus )
+	for ( i = cpumask_first(vcpu_mask);
+	      i < d->max_vcpus;
+	      i = cpumask_next(i, vcpu_mask))
     {
-        if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) )
+        if ( d->vcpu[i] != NULL && !is_vcpu_online(d->vcpu[i]) )
         {
             gprintk(XENLOG_WARNING, "VGIC: write r=%"PRIregister" \
-                    vcpu_mask=%lx, wrong CPUTargetList\n", sgir, vcpu_mask);
+                    , wrong CPUTargetList\n", sgir);
             continue;
         }
-        vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq);
+        vgic_vcpu_inject_irq(d->vcpu[i], virq);
     }
     return 1;
 }
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 463fffb..c6ef4bf 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -64,6 +64,7 @@
 #define GICD_SGI_TARGET_SELF_VAL     (2)
 #define GICD_SGI_TARGET_SHIFT        (16)
 #define GICD_SGI_TARGET_MASK         (0xFFUL<<GICD_SGI_TARGET_SHIFT)
+#define GICD_SGI_TARGET_BITS         (8)
 #define GICD_SGI_GROUP1              (1UL<<15)
 #define GICD_SGI_INTID_MASK          (0xFUL)
 
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index 556f114..e106e67 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -152,6 +152,8 @@
 #define ICH_SGI_IRQ_SHIFT            24
 #define ICH_SGI_IRQ_MASK             0xf
 #define ICH_SGI_TARGETLIST_MASK      0xffff
+#define ICH_SGI_TARGET_BITS          16
+
 #endif /* __ASM_ARM_GIC_V3_DEFS_H__ */
 
 /*
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 6dcdf9f..2f413e1 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -201,7 +201,7 @@ DEFINE_VGIC_OPS(3)
 extern int vcpu_vgic_free(struct vcpu *v);
 extern int vgic_to_sgi(struct vcpu *v, register_t sgir,
                        enum gic_sgi_mode irqmode, int virq,
-                       unsigned long vcpu_mask);
+                       cpumask_t *vcpu_mask);
 extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq);
 
 /* Reserve a specific guest vIRQ */
-- 
2.1.4

[PATCH V5 05/10] xen/arm64: gicv3: Use AFF1 when translating ICC_SGI1R_EL1 to cpumask

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

To support more than 16 vCPUs, we have to calculate cpumask with AFF1
field value in ICC_SGI1R_EL1.

Signed-off-by: Chen Baozi <baozich@gmail.com>
---
 xen/arch/arm/vgic-v3.c            | 9 ++++++++-
 xen/include/asm-arm/gic_v3_defs.h | 3 +++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index a283c8c..21d8d3f 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -976,10 +976,17 @@ static inline void gicv3_sgir_to_cpumask(cpumask_t *cpumask,
                                          const register_t sgir)
 {
     unsigned long target_list;
+    int aff1;
 
     target_list = sgir & ICH_SGI_TARGETLIST_MASK;
-    bitmap_copy(cpumask_bits(cpumask), &target_list, ICH_SGI_TARGET_BITS);
+    /* We assume that only AFF1 is used in ICC_SGI1R_EL1. */
+    aff1 = (sgir >> ICH_SGI_AFFINITY_LEVEL(1)) & ICH_SGI_AFFx_MASK;
 
+    BUILD_BUG_ON(sizeof(cpumask_t)*8 < MAX_VIRT_CPUS);
+    BUG_ON(((aff1+1) * ICH_SGI_TARGET_BITS) > NR_CPUS);
+
+    memcpy((uint16_t *)cpumask + aff1, &target_list,
+           (ICH_SGI_TARGET_BITS/8));
 }
 
 static int vgic_v3_to_sgi(struct vcpu *v, register_t sgir)
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index e106e67..3743e66 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -153,6 +153,9 @@
 #define ICH_SGI_IRQ_MASK             0xf
 #define ICH_SGI_TARGETLIST_MASK      0xffff
 #define ICH_SGI_TARGET_BITS          16
+#define ICH_SGI_AFFx_MASK            0xff
+#define ICH_SGI_AFFINITY_LEVEL(x)    (16 * (x))
+
 
 #endif /* __ASM_ARM_GIC_V3_DEFS_H__ */
 
-- 
2.1.4

[PATCH V5 06/10] tools/libxl: Set 'reg' of cpu node equal to MPIDR affinity for domU

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

According to ARM CPUs bindings, the reg field should match the MPIDR's
affinity bits. We will use AFF0 and AFF1 when constructing the reg value
of the guest at the moment, for it is enough for the current max vcpu
number.

Signed-off-by: Chen Baozi <baozich@gmail.com>
---
 tools/libxl/libxl_arm.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index c5088c4..8aa4815 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -272,6 +272,7 @@ static int make_cpus_node(libxl__gc *gc, void *fdt, int nr_cpus,
                           const struct arch_info *ainfo)
 {
     int res, i;
+    uint64_t mpidr_aff;
 
     res = fdt_begin_node(fdt, "cpus");
     if (res) return res;
@@ -283,7 +284,16 @@ static int make_cpus_node(libxl__gc *gc, void *fdt, int nr_cpus,
     if (res) return res;
 
     for (i = 0; i < nr_cpus; i++) {
-        const char *name = GCSPRINTF("cpu@%d", i);
+        const char *name;
+
+        /*
+         * According to ARM CPUs bindings, the reg field should match
+         * the MPIDR's affinity bits. We will use AFF0 and AFF1 when
+         * constructing the reg value of the guest at the moment, for it
+         * is enough for the current max vcpu number.
+         */
+        mpidr_aff = (uint64_t)((i & 0x0f) | (((i >> 4) & 0xff) << 8));
+        name = GCSPRINTF("cpu@%lx", mpidr_aff);
 
         res = fdt_begin_node(fdt, name);
         if (res) return res;
@@ -297,7 +307,7 @@ static int make_cpus_node(libxl__gc *gc, void *fdt, int nr_cpus,
         res = fdt_property_string(fdt, "enable-method", "psci");
         if (res) return res;
 
-        res = fdt_property_regs(gc, fdt, 1, 0, 1, (uint64_t)i);
+        res = fdt_property_regs(gc, fdt, 1, 0, 1, mpidr_aff);
         if (res) return res;
 
         res = fdt_end_node(fdt);
-- 
2.1.4

[PATCH V5 07/10] xen/arm: Set 'reg' of cpu node for dom0 to match MPIDR's affinity

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

According to ARM CPUs bindings, the reg field should match the MPIDR's
affinity bits. We will use AFF0 and AFF1 when constructing the reg value
of the guest at the moment, for it is enough for the current max vcpu
number.

Signed-off-by: Chen Baozi <baozich@gmail.com>
---
 xen/arch/arm/domain_build.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index a156de9..5591d82 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -712,6 +712,7 @@ static int make_cpus_node(const struct domain *d, void *fdt,
     char buf[15];
     u32 clock_frequency;
     bool_t clock_valid;
+    uint32_t mpidr_aff;
 
     DPRINT("Create cpus node\n");
 
@@ -761,9 +762,16 @@ static int make_cpus_node(const struct domain *d, void *fdt,
 
     for ( cpu = 0; cpu < d->max_vcpus; cpu++ )
     {
-        DPRINT("Create cpu@%u node\n", cpu);
+        /*
+         * According to ARM CPUs bindings, the reg field should match
+         * the MPIDR's affinity bits. We will use AFF0 and AFF1 when
+         * constructing the reg value of the guest at the moment, for it
+         * is enough for the current max vcpu number.
+         */
+        mpidr_aff = vcpuid_to_vaffinity(cpu);
+        DPRINT("Create cpu@%x node\n", mpidr_aff);
 
-        snprintf(buf, sizeof(buf), "cpu@%u", cpu);
+        snprintf(buf, sizeof(buf), "cpu@%x", mpidr_aff);
         res = fdt_begin_node(fdt, buf);
         if ( res )
             return res;
@@ -776,7 +784,7 @@ static int make_cpus_node(const struct domain *d, void *fdt,
         if ( res )
             return res;
 
-        res = fdt_property_cell(fdt, "reg", cpu);
+        res = fdt_property_cell(fdt, "reg", mpidr_aff);
         if ( res )
             return res;
 
-- 
2.1.4

[PATCH V5 08/10] xen: Call arch_domain_create() before evtchn_init()

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

evtchn_init() will call domain_max_vcpus() to allocate poll_mask, which
needs the max vCPU number returned by domain_max_vcpus(). On arm/arm64
platform, this number is determined by the vGIC the guest is going to
use, which won't be initialised until arch_domain_create() is finished.

Signed-off-by: Chen Baozi <baozich@gmail.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Julien Grall <julien.grall@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 xen/common/domain.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 6803c4d..5b98f69 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -316,6 +316,10 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
     if ( domcr_flags & DOMCRF_dummy )
         return d;
 
+    if ( (err = arch_domain_create(d, domcr_flags, config)) != 0 )
+        goto fail;
+    init_status |= INIT_arch;
+
     if ( !is_idle_domain(d) )
     {
         if ( (err = xsm_domain_create(XSM_HOOK, d, ssidref)) != 0 )
@@ -354,10 +358,6 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
             goto fail;
     }
 
-    if ( (err = arch_domain_create(d, domcr_flags, config)) != 0 )
-        goto fail;
-    init_status |= INIT_arch;
-
     if ( (err = cpupool_add_domain(d, poolid)) != 0 )
         goto fail;
 
-- 
2.1.4

[PATCH V5 09/10] xen/arm: make domain_max_vcpus return value from vgic_ops

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

When a guest uses vGICv2, the maximum number of vCPU it can support
should not be as many as MAX_VIRT_CPUS, which is 128 at the moment.
So the domain_max_vcpus should return the value according to the vGIC
the domain uses.

We didn't keep it as the old static inline form because it will break
compilation when access the member of struct domain:

In file included from xen/include/xen/domain.h:6:0,
                 from xen/include/xen/sched.h:10,
                 from arm64/asm-offsets.c:10:
xen/include/asm/domain.h: In function ‘domain_max_vcpus’:
xen/include/asm/domain.h:266:10: error: dereferencing pointer to incomplete type
     if (d->arch.vgic.version == GIC_V2)
          ^

Signed-off-by: Chen Baozi <baozich@gmail.com>
---
 xen/arch/arm/domain.c        | 5 +++++
 xen/arch/arm/domain_build.c  | 2 +-
 xen/arch/arm/vgic-v2.c       | 6 ++++++
 xen/arch/arm/vgic-v3.c       | 6 ++++++
 xen/include/asm-arm/domain.h | 5 +----
 xen/include/asm-arm/gic.h    | 3 +++
 xen/include/asm-arm/vgic.h   | 2 ++
 7 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 0cf147c..c638ff5 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -890,6 +890,11 @@ void vcpu_block_unless_event_pending(struct vcpu *v)
         vcpu_unblock(current);
 }
 
+unsigned int domain_max_vcpus(const struct domain *d)
+{
+    return d->arch.vgic.handler->get_max_vcpus();
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 5591d82..88cfcee 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -769,7 +769,7 @@ static int make_cpus_node(const struct domain *d, void *fdt,
          * is enough for the current max vcpu number.
          */
         mpidr_aff = vcpuid_to_vaffinity(cpu);
-        DPRINT("Create cpu@%x node\n", mpidr_aff);
+        DPRINT("Create cpu@%x (logical CPUID: %d) node\n", mpidr_aff, cpu);
 
         snprintf(buf, sizeof(buf), "cpu@%x", mpidr_aff);
         res = fdt_begin_node(fdt, buf);
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 17a3c9f..206d289 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -508,6 +508,11 @@ static struct vcpu *vgic_v2_get_target_vcpu(struct vcpu *v, unsigned int irq)
     return v_target;
 }
 
+static int vgic_v2_get_max_vcpus(void)
+{
+    return GICV2_MAX_CPUS;
+}
+
 static int vgic_v2_get_irq_priority(struct vcpu *v, unsigned int irq)
 {
     int priority;
@@ -594,6 +599,7 @@ const struct vgic_ops vgic_v2_ops = {
     .domain_init = vgic_v2_domain_init,
     .get_irq_priority = vgic_v2_get_irq_priority,
     .get_target_vcpu = vgic_v2_get_target_vcpu,
+    .get_max_vcpus = vgic_v2_get_max_vcpus,
 };
 
 /*
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 21d8d3f..e79b010 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1048,6 +1048,11 @@ static int vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
     }
 }
 
+static int vgic_v3_get_max_vcpus(void)
+{
+    return GICV3_MAX_CPUS;
+};
+
 static const struct mmio_handler_ops vgic_rdistr_mmio_handler = {
     .read_handler  = vgic_v3_rdistr_mmio_read,
     .write_handler = vgic_v3_rdistr_mmio_write,
@@ -1220,6 +1225,7 @@ const struct vgic_ops vgic_v3_ops = {
     .get_irq_priority = vgic_v3_get_irq_priority,
     .get_target_vcpu  = vgic_v3_get_target_vcpu,
     .emulate_sysreg  = vgic_v3_emulate_sysreg,
+    .get_max_vcpus = vgic_v3_get_max_vcpus,
 };
 
 /*
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index b7b5cd2..b525bec 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -261,10 +261,7 @@ struct arch_vcpu
 void vcpu_show_execution_state(struct vcpu *);
 void vcpu_show_registers(const struct vcpu *);
 
-static inline unsigned int domain_max_vcpus(const struct domain *d)
-{
-    return MAX_VIRT_CPUS;
-}
+unsigned int domain_max_vcpus(const struct domain *);
 
 /*
  * Due to the restriction of GICv3, the number of vCPUs in AFF0 is
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index c6ef4bf..55f99cd 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -18,6 +18,9 @@
 #ifndef __ASM_ARM_GIC_H__
 #define __ASM_ARM_GIC_H__
 
+#define GICV2_MAX_CPUS  8
+#define GICV3_MAX_CPUS  128
+
 #define NR_GIC_LOCAL_IRQS  NR_LOCAL_IRQS
 #define NR_GIC_SGI         16
 #define MAX_RDIST_COUNT    4
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 2f413e1..1157b04 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -110,6 +110,8 @@ struct vgic_ops {
     struct vcpu *(*get_target_vcpu)(struct vcpu *v, unsigned int irq);
     /* vGIC sysreg emulation */
     int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr hsr);
+    /* Get the maximum number of vCPU supported */
+    int (*get_max_vcpus)(void);
 };
 
 /* Number of ranks of interrupt registers for a domain */
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH V5 10/10] xen/arm64: increase MAX_VIRT_CPUS to 128 on arm64

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

GIC-500 supports up to 128 cores in a single SoC. Increase MAX_VIRT_CPUS
to 128 on arm64.

Since the domain_max_vcpus has been changed to depends on vgic_ops,
we could have done more work in order to drop the definition of
MAX_VIRT_CPUS. However, because it is still used for some conditional
compilation in common code, I think that would be better done in a
seperate cleanup patch series.

Signed-off-by: Chen Baozi <baozich@gmail.com>
Acked-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/vgic-v3.c       | 1 -
 xen/include/asm-arm/config.h | 4 ++++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index e79b010..25b6bad 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -889,7 +889,6 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER,
                                 DABT_DOUBLE_WORD);
         if ( rank == NULL ) goto write_ignore;
-        BUG_ON(v->domain->max_vcpus > 8);
         new_irouter = *r;
         vgic_lock_rank(v, rank, flags);
 
diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index 3b23e05..817c216 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -47,7 +47,11 @@
 #define NR_CPUS 128
 #endif
 
+#ifdef CONFIG_ARM_64
+#define MAX_VIRT_CPUS 128
+#else
 #define MAX_VIRT_CPUS 8
+#endif
 
 #define asmlinkage /* Nothing needed */
 
-- 
2.1.4

Re: [PATCH V5 05/10] xen/arm64: gicv3: Use AFF1 when translating ICC_SGI1R_EL1 to cpumask

[Chen Baozi]

On Sat, May 30, 2015 at 07:07:26PM +0800, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
> 
> To support more than 16 vCPUs, we have to calculate cpumask with AFF1
> field value in ICC_SGI1R_EL1.
> 
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>  xen/arch/arm/vgic-v3.c            | 9 ++++++++-
>  xen/include/asm-arm/gic_v3_defs.h | 3 +++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index a283c8c..21d8d3f 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -976,10 +976,17 @@ static inline void gicv3_sgir_to_cpumask(cpumask_t *cpumask,
>                                           const register_t sgir)
>  {
>      unsigned long target_list;
> +    int aff1;
>  
>      target_list = sgir & ICH_SGI_TARGETLIST_MASK;
> -    bitmap_copy(cpumask_bits(cpumask), &target_list, ICH_SGI_TARGET_BITS);
> +    /* We assume that only AFF1 is used in ICC_SGI1R_EL1. */
> +    aff1 = (sgir >> ICH_SGI_AFFINITY_LEVEL(1)) & ICH_SGI_AFFx_MASK;
>  
> +    BUILD_BUG_ON(sizeof(cpumask_t)*8 < MAX_VIRT_CPUS);
> +    BUG_ON(((aff1+1) * ICH_SGI_TARGET_BITS) > NR_CPUS);
> +
> +    memcpy((uint16_t *)cpumask + aff1, &target_list,
> +           (ICH_SGI_TARGET_BITS/8));
>  }
>  
>  static int vgic_v3_to_sgi(struct vcpu *v, register_t sgir)
> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index e106e67..3743e66 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -153,6 +153,9 @@
>  #define ICH_SGI_IRQ_MASK             0xf
>  #define ICH_SGI_TARGETLIST_MASK      0xffff
>  #define ICH_SGI_TARGET_BITS          16
> +#define ICH_SGI_AFFx_MASK            0xff
> +#define ICH_SGI_AFFINITY_LEVEL(x)    (16 * (x))
> +
  ^^ Sorry for the unnecessary trailing line...

Baozi.

Re: [PATCH V5 02/10] xen/arm: Add functions of mapping between vCPUID and virtual affinity

[Julien Grall]

Hi Chen,

On 30/05/2015 12:07, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
>
> GICv3 restricts that the maximum number of CPUs in affinity 0 (one
> cluster) is 16. That is to say the upper 4 bits of affinity 0 is unused.
> Current implementation considers that AFF0 is equal to vCPUID, which
> makes all vCPUs in one cluster, limiting its number to 16. If we would
> like to support more than 16 number of vCPU in one guest, we need to
> make use of AFF1. Considering the unused upper 4 bits, we need to create
> a pair of functions mapping the vCPUID and virtual affinity.
>
> Signed-off-by: Chen Baozi <baozich@gmail.com>
Reviewed-by: Julien Grall <julien.grall@citrix.com>

Regards,

-- 
Julien Grall

Re: [PATCH V5 03/10] xen/arm: Use the new functions for vCPUID/vaffinity transformation

[Julien Grall]

Hi Chen,

On 30/05/2015 12:07, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
>
> There are 3 places to change:
>
> * Initialise vMPIDR value in vcpu_initialise()
> * Find the vCPU from vMPIDR affinity information when accessing GICD
>    registers in vGIC
> * Find the vCPU from vMPIDR affinity information when booting with vPSCI
>    in vGIC
>    - Also make the code for PSCI 0.1 use MPIDR-like value as the cpuid.
>
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
> Comments from Julien in the previous review:
>
> I read more closely the spec of PSCI 0.1 Section 6.3 (ARM DEN 0022A):
>
> "Ideally platform discovery mechanism such as firmware tables would be
> used by secure firmware to describe the set of valid CPUIDs to the
> hypervisor or Rich OS, if the former is not present. The hypervisor in
> turn can create and supply virtual discovery mechanisms to its guests."
>
> It looks like to me that the CPUID is equal to the "reg" field in the
> CPU node which is an MPIDR-like value. So I think the affinity should be
> called in both case.

Assuming that I was right here:

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

Regards,

-- 
Julien Grall

Re: [PATCH V5 04/10] xen/arm: Use cpumask_t type for vcpu_mask in vgic_to_sgi

[Julien Grall]

Hi Chen,

On 30/05/2015 12:07, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
>
> Use cpumask_t instead of unsigned long which can only express 64 cpus at
> the most. Add the {gicv2|gicv3}_sgir_to_cpumask in corresponding vGICs
> to translate GICD_SGIR/ICC_SGI1R_EL1 to vcpu_mask for vgic_to_sgi.
>
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>   xen/arch/arm/vgic-v2.c            | 16 +++++++++++++---
>   xen/arch/arm/vgic-v3.c            | 19 +++++++++++++++----
>   xen/arch/arm/vgic.c               | 24 +++++++++++-------------
>   xen/include/asm-arm/gic.h         |  1 +
>   xen/include/asm-arm/gic_v3_defs.h |  2 ++
>   xen/include/asm-arm/vgic.h        |  2 +-
>   6 files changed, 43 insertions(+), 21 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 3be1a51..17a3c9f 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -33,6 +33,15 @@
>   #include <asm/gic.h>
>   #include <asm/vgic.h>
>
> +static inline void gicv2_sgir_to_cpumask(cpumask_t *cpumask,
> +                                         const register_t sgir)
> +{
> +    unsigned long target_list;
> +
> +    target_list = ((sgir & GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT);
> +    bitmap_copy(cpumask_bits(cpumask), &target_list, GICD_SGI_TARGET_BITS);
> +}
> +
>   static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
>   {
>       struct hsr_dabt dabt = info->dabt;
> @@ -201,16 +210,17 @@ static int vgic_v2_to_sgi(struct vcpu *v, register_t sgir)
>       int virq;
>       int irqmode;
>       enum gic_sgi_mode sgi_mode;
> -    unsigned long vcpu_mask = 0;
> +    cpumask_t vcpu_mask;
>
> +    cpumask_clear(&vcpu_mask);
>       irqmode = (sgir & GICD_SGI_TARGET_LIST_MASK) >> GICD_SGI_TARGET_LIST_SHIFT;
>       virq = (sgir & GICD_SGI_INTID_MASK);
> -    vcpu_mask = (sgir & GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT;
>
>       /* Map GIC sgi value to enum value */
>       switch ( irqmode )
>       {
>       case GICD_SGI_TARGET_LIST_VAL:
> +        gicv2_sgir_to_cpumask(&vcpu_mask, sgir);
>           sgi_mode = SGI_TARGET_LIST;
>           break;
>       case GICD_SGI_TARGET_OTHERS_VAL:
> @@ -226,7 +236,7 @@ static int vgic_v2_to_sgi(struct vcpu *v, register_t sgir)
>           return 0;
>       }
>
> -    return vgic_to_sgi(v, sgir, sgi_mode, virq, vcpu_mask);
> +    return vgic_to_sgi(v, sgir, sgi_mode, virq, &vcpu_mask);
>   }
>
>   static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index ef9a71a..a283c8c 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -972,22 +972,33 @@ write_ignore:
>       return 1;
>   }
>
> +static inline void gicv3_sgir_to_cpumask(cpumask_t *cpumask,
> +                                         const register_t sgir)
> +{
> +    unsigned long target_list;
> +
> +    target_list = sgir & ICH_SGI_TARGETLIST_MASK;
> +    bitmap_copy(cpumask_bits(cpumask), &target_list, ICH_SGI_TARGET_BITS);
> +

Spurious line.

> +}
> +
>   static int vgic_v3_to_sgi(struct vcpu *v, register_t sgir)
>   {
>       int virq;
>       int irqmode;
>       enum gic_sgi_mode sgi_mode;
> -    unsigned long vcpu_mask = 0;
> +    cpumask_t vcpu_mask;
>
> +    cpumask_clear(&vcpu_mask);
>       irqmode = (sgir >> ICH_SGI_IRQMODE_SHIFT) & ICH_SGI_IRQMODE_MASK;
>       virq = (sgir >> ICH_SGI_IRQ_SHIFT ) & ICH_SGI_IRQ_MASK;
> -    /* SGI's are injected at Rdist level 0. ignoring affinity 1, 2, 3 */
> -    vcpu_mask = sgir & ICH_SGI_TARGETLIST_MASK;
>
>       /* Map GIC sgi value to enum value */
>       switch ( irqmode )
>       {
>       case ICH_SGI_TARGET_LIST:
> +        /* SGI's are injected at Rdist level 0. ignoring affinity 1, 2, 3 */
> +        gicv3_sgir_to_cpumask(&vcpu_mask, sgir);
>           sgi_mode = SGI_TARGET_LIST;
>           break;
>       case ICH_SGI_TARGET_OTHERS:
> @@ -998,7 +1009,7 @@ static int vgic_v3_to_sgi(struct vcpu *v, register_t sgir)
>           return 0;
>       }
>
> -    return vgic_to_sgi(v, sgir, sgi_mode, virq, vcpu_mask);
> +    return vgic_to_sgi(v, sgir, sgi_mode, virq, &vcpu_mask);
>   }
>
>   static int vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 7b387b7..f9e43f9 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -318,16 +318,12 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>       }
>   }
>
> -/* TODO: unsigned long is used to fit vcpu_mask.*/
>   int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int virq,
> -                unsigned long vcpu_mask)
> +                cpumask_t *vcpu_mask)
>   {
>       struct domain *d = v->domain;
> -    int vcpuid;
>       int i;
>
> -    ASSERT(d->max_vcpus < 8*sizeof(vcpu_mask));
> -

As said in the previous version, cpumask_t is based on NR_CPUS and there 
is no relation between NR_CPUS and MAX_VIRT_CPUS. Furthermore, NR_CPUS 
which can be configured at build time by the user (see MAX_PHYS_CPUS). 
We need to catch at build time possible when the cpumask_t would be 
smaller in order to avoid insecure hypervisor.

>       ASSERT( virq < 16 );
>
>       switch ( irqmode )
> @@ -341,12 +337,12 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
>            * SGI_TARGET_SELF mode. So Force vcpu_mask to 0
>            */
>           perfc_incr(vgic_sgi_others);
> -        vcpu_mask = 0;
> +        cpumask_clear(vcpu_mask);

Given that you already clear the cpumask on the caller of this function, 
I think we can consider that it will be nice with us rather than wasting 
CPU time to clear twice. That would require to update the comment above.

>           for ( i = 0; i < d->max_vcpus; i++ )
>           {
>               if ( i != current->vcpu_id && d->vcpu[i] != NULL &&
>                    is_vcpu_online(d->vcpu[i]) )
> -                set_bit(i, &vcpu_mask);
> +                cpumask_set_cpu(i, vcpu_mask);
>           }
>           break;
>       case SGI_TARGET_SELF:
> @@ -355,8 +351,8 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
>            * SGI_TARGET_SELF mode. So Force vcpu_mask to 0
>            */
>           perfc_incr(vgic_sgi_self);
> -        vcpu_mask = 0;
> -        set_bit(current->vcpu_id, &vcpu_mask);
> +        cpumask_clear(vcpu_mask);

Ditto.

> +        cpumask_set_cpu(current->vcpu_id, vcpu_mask);
>           break;
>       default:
>           gprintk(XENLOG_WARNING,
> @@ -365,15 +361,17 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
>           return 0;
>       }
>
> -    for_each_set_bit( vcpuid, &vcpu_mask, d->max_vcpus )
> +	for ( i = cpumask_first(vcpu_mask);
> +	      i < d->max_vcpus;
> +	      i = cpumask_next(i, vcpu_mask))

The indentation looks wrong here. And why don't you use vcpuid? It would 
have avoid the renaming vcpuid/i below which increase the path size for 
nothing.

>       {
> -        if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) )
> +        if ( d->vcpu[i] != NULL && !is_vcpu_online(d->vcpu[i]) )
>           {
>               gprintk(XENLOG_WARNING, "VGIC: write r=%"PRIregister" \
> -                    vcpu_mask=%lx, wrong CPUTargetList\n", sgir, vcpu_mask);
> +                    , wrong CPUTargetList\n", sgir);
>               continue;
>           }
> -        vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq);
> +        vgic_vcpu_inject_irq(d->vcpu[i], virq);
>       }
>       return 1;
>   }
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 463fffb..c6ef4bf 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -64,6 +64,7 @@
>   #define GICD_SGI_TARGET_SELF_VAL     (2)
>   #define GICD_SGI_TARGET_SHIFT        (16)
>   #define GICD_SGI_TARGET_MASK         (0xFFUL<<GICD_SGI_TARGET_SHIFT)
> +#define GICD_SGI_TARGET_BITS         (8)
>   #define GICD_SGI_GROUP1              (1UL<<15)
>   #define GICD_SGI_INTID_MASK          (0xFUL)
>
> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index 556f114..e106e67 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -152,6 +152,8 @@
>   #define ICH_SGI_IRQ_SHIFT            24
>   #define ICH_SGI_IRQ_MASK             0xf
>   #define ICH_SGI_TARGETLIST_MASK      0xffff
> +#define ICH_SGI_TARGET_BITS          16
> +
>   #endif /* __ASM_ARM_GIC_V3_DEFS_H__ */
>
>   /*
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 6dcdf9f..2f413e1 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -201,7 +201,7 @@ DEFINE_VGIC_OPS(3)
>   extern int vcpu_vgic_free(struct vcpu *v);
>   extern int vgic_to_sgi(struct vcpu *v, register_t sgir,
>                          enum gic_sgi_mode irqmode, int virq,
> -                       unsigned long vcpu_mask);
> +                       cpumask_t *vcpu_mask);
>   extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq);
>
>   /* Reserve a specific guest vIRQ */
>

Regards,

-- 
Julien Grall

Re: [PATCH V5 05/10] xen/arm64: gicv3: Use AFF1 when translating ICC_SGI1R_EL1 to cpumask

[Julien Grall]

Hi Chen,

On 30/05/2015 12:07, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
>
> To support more than 16 vCPUs, we have to calculate cpumask with AFF1
> field value in ICC_SGI1R_EL1.
>
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>   xen/arch/arm/vgic-v3.c            | 9 ++++++++-
>   xen/include/asm-arm/gic_v3_defs.h | 3 +++
>   2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index a283c8c..21d8d3f 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -976,10 +976,17 @@ static inline void gicv3_sgir_to_cpumask(cpumask_t *cpumask,
>                                            const register_t sgir)
>   {
>       unsigned long target_list;
> +    int aff1;

unsigned int.

>
>       target_list = sgir & ICH_SGI_TARGETLIST_MASK;
> -    bitmap_copy(cpumask_bits(cpumask), &target_list, ICH_SGI_TARGET_BITS);
> +    /* We assume that only AFF1 is used in ICC_SGI1R_EL1. */
> +    aff1 = (sgir >> ICH_SGI_AFFINITY_LEVEL(1)) & ICH_SGI_AFFx_MASK;
>
> +    BUILD_BUG_ON(sizeof(cpumask_t)*8 < MAX_VIRT_CPUS);

Ah, here is the BUILD_BUG_ON. This is not vgic-v3 specific but generic 
to all the vgic. It would have been more logical to put it in the 
function vgic_to_sgi in the previous patch (i.e #4).

> +    BUG_ON(((aff1+1) * ICH_SGI_TARGET_BITS) > NR_CPUS);

NACK. This value is passed by the guest. With this a malicious guest 
could take down Xen.

> +
> +    memcpy((uint16_t *)cpumask + aff1, &target_list,

That's hackhish. You can't assume that the bitmap will be at the 
beginning of cpumask_t.

> +           (ICH_SGI_TARGET_BITS/8));
>   }
>
>   static int vgic_v3_to_sgi(struct vcpu *v, register_t sgir)
> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index e106e67..3743e66 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -153,6 +153,9 @@
>   #define ICH_SGI_IRQ_MASK             0xf
>   #define ICH_SGI_TARGETLIST_MASK      0xffff
>   #define ICH_SGI_TARGET_BITS          16
> +#define ICH_SGI_AFFx_MASK            0xff
> +#define ICH_SGI_AFFINITY_LEVEL(x)    (16 * (x))
> +

Spurious line.

>
>   #endif /* __ASM_ARM_GIC_V3_DEFS_H__ */

Regards,

-- 
Julien Grall

Re: [PATCH V5 06/10] tools/libxl: Set 'reg' of cpu node equal to MPIDR affinity for domU

[Julien Grall]

Hi Chen,

On 30/05/2015 12:07, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
>
> According to ARM CPUs bindings, the reg field should match the MPIDR's
> affinity bits. We will use AFF0 and AFF1 when constructing the reg value
> of the guest at the moment, for it is enough for the current max vcpu
> number.
>
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>   tools/libxl/libxl_arm.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index c5088c4..8aa4815 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -272,6 +272,7 @@ static int make_cpus_node(libxl__gc *gc, void *fdt, int nr_cpus,
>                             const struct arch_info *ainfo)
>   {
>       int res, i;
> +    uint64_t mpidr_aff;
>
>       res = fdt_begin_node(fdt, "cpus");
>       if (res) return res;
> @@ -283,7 +284,16 @@ static int make_cpus_node(libxl__gc *gc, void *fdt, int nr_cpus,
>       if (res) return res;
>
>       for (i = 0; i < nr_cpus; i++) {
> -        const char *name = GCSPRINTF("cpu@%d", i);
> +        const char *name;
> +
> +        /*
> +         * According to ARM CPUs bindings, the reg field should match
> +         * the MPIDR's affinity bits. We will use AFF0 and AFF1 when
> +         * constructing the reg value of the guest at the moment, for it
> +         * is enough for the current max vcpu number.
> +         */
> +        mpidr_aff = (uint64_t)((i & 0x0f) | (((i >> 4) & 0xff) << 8));

The cast is not necessary.

Other than that:

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

Regards,

-- 
Julien Grall

Re: [PATCH V5 07/10] xen/arm: Set 'reg' of cpu node for dom0 to match MPIDR's affinity

[Julien Grall]

Hi Chen,

On 30/05/2015 12:07, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
>
> According to ARM CPUs bindings, the reg field should match the MPIDR's
> affinity bits. We will use AFF0 and AFF1 when constructing the reg value
> of the guest at the moment, for it is enough for the current max vcpu
> number.
>
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>   xen/arch/arm/domain_build.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index a156de9..5591d82 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -712,6 +712,7 @@ static int make_cpus_node(const struct domain *d, void *fdt,
>       char buf[15];
>       u32 clock_frequency;
>       bool_t clock_valid;
> +    uint32_t mpidr_aff;

Please be consistent. Either use unsigned int (as you did in #2) or 
uint64_t (as you did in #6) or uint32_t (as you did here).

But not three different possibility as you did.

TBH, I would prefer the uint64_t as the affinity can go up to 64 bits 
(see with AFF3).

>
>       DPRINT("Create cpus node\n");
>
> @@ -761,9 +762,16 @@ static int make_cpus_node(const struct domain *d, void *fdt,
>
>       for ( cpu = 0; cpu < d->max_vcpus; cpu++ )
>       {
> -        DPRINT("Create cpu@%u node\n", cpu);
> +        /*
> +         * According to ARM CPUs bindings, the reg field should match
> +         * the MPIDR's affinity bits. We will use AFF0 and AFF1 when
> +         * constructing the reg value of the guest at the moment, for it
> +         * is enough for the current max vcpu number.
> +         */
> +        mpidr_aff = vcpuid_to_vaffinity(cpu);
> +        DPRINT("Create cpu@%x node\n", mpidr_aff);

The change I requested here is lying in the next patch (i.e #8)... 
Please move it here.

Regards,

-- 
-- 
Julien Grall

Re: [PATCH V5 08/10] xen: Call arch_domain_create() before evtchn_init()

[Julien Grall]

Hi Chen,

On 30/05/2015 12:07, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
>
> evtchn_init() will call domain_max_vcpus() to allocate poll_mask, which
> needs the max vCPU number returned by domain_max_vcpus(). On arm/arm64
> platform, this number is determined by the vGIC the guest is going to
> use, which won't be initialised until arch_domain_create() is finished.
>
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Julien Grall <julien.grall@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> ---
>   xen/common/domain.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 6803c4d..5b98f69 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -316,6 +316,10 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>       if ( domcr_flags & DOMCRF_dummy )
>           return d;
>
> +    if ( (err = arch_domain_create(d, domcr_flags, config)) != 0 )
> +        goto fail;
> +    init_status |= INIT_arch;
> +
>       if ( !is_idle_domain(d) )
>       {
>           if ( (err = xsm_domain_create(XSM_HOOK, d, ssidref)) != 0 )
> @@ -354,10 +358,6 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>               goto fail;
>       }
>
> -    if ( (err = arch_domain_create(d, domcr_flags, config)) != 0 )
> -        goto fail;
> -    init_status |= INIT_arch;
> -

NACK. Moving arch_domain_create means that we will allocate memory 
before checking the XSM policy. I don't think we want to execute an 
expensive function if the domain is not allowed to boot.

Furthermore, did you audit all the code to make sure that 
arch_domain_create (on both x86 and ARM) doesn't use the allocation done 
before (i.e evtch, grant table,...)?

Depending on this question, we may want to create a preinit domain 
creation with set the different value (such as the vGIC callback) 
without allocation memory.

Regards,

-- 
Julien Grall

Re: [PATCH V5 09/10] xen/arm: make domain_max_vcpus return value from vgic_ops

[Julien Grall]

Hi Chen,

On 30/05/2015 12:07, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
>
> When a guest uses vGICv2, the maximum number of vCPU it can support
> should not be as many as MAX_VIRT_CPUS, which is 128 at the moment.
> So the domain_max_vcpus should return the value according to the vGIC
> the domain uses.
>
> We didn't keep it as the old static inline form because it will break
> compilation when access the member of struct domain:
>
> In file included from xen/include/xen/domain.h:6:0,
>                   from xen/include/xen/sched.h:10,
>                   from arm64/asm-offsets.c:10:
> xen/include/asm/domain.h: In function ‘domain_max_vcpus’:
> xen/include/asm/domain.h:266:10: error: dereferencing pointer to incomplete type
>       if (d->arch.vgic.version == GIC_V2)
>            ^
>
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>   xen/arch/arm/domain.c        | 5 +++++
>   xen/arch/arm/domain_build.c  | 2 +-
>   xen/arch/arm/vgic-v2.c       | 6 ++++++
>   xen/arch/arm/vgic-v3.c       | 6 ++++++
>   xen/include/asm-arm/domain.h | 5 +----
>   xen/include/asm-arm/gic.h    | 3 +++
>   xen/include/asm-arm/vgic.h   | 2 ++
>   7 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 0cf147c..c638ff5 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -890,6 +890,11 @@ void vcpu_block_unless_event_pending(struct vcpu *v)
>           vcpu_unblock(current);
>   }
>
> +unsigned int domain_max_vcpus(const struct domain *d)
> +{
> +    return d->arch.vgic.handler->get_max_vcpus();

As long as we keep MAX_VIRT_VCPUS in the common vGIC drivers, we need to 
make sure that we don't return a superior value because technically, 
with your changes, the vGICv3 could support more than 128 vCPUs...

I would do: return min(MAX_VIRT_CPUS, d->arch...)

> +}
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 5591d82..88cfcee 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -769,7 +769,7 @@ static int make_cpus_node(const struct domain *d, void *fdt,
>            * is enough for the current max vcpu number.
>            */
>           mpidr_aff = vcpuid_to_vaffinity(cpu);
> -        DPRINT("Create cpu@%x node\n", mpidr_aff);
> +        DPRINT("Create cpu@%x (logical CPUID: %d) node\n", mpidr_aff, cpu);

This change belongs to patch #7...

>
>           snprintf(buf, sizeof(buf), "cpu@%x", mpidr_aff);
>           res = fdt_begin_node(fdt, buf);
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 17a3c9f..206d289 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -508,6 +508,11 @@ static struct vcpu *vgic_v2_get_target_vcpu(struct vcpu *v, unsigned int irq)
>       return v_target;
>   }
>
> +static int vgic_v2_get_max_vcpus(void)
> +{
> +    return GICV2_MAX_CPUS;
> +}
> +
>   static int vgic_v2_get_irq_priority(struct vcpu *v, unsigned int irq)
>   {
>       int priority;
> @@ -594,6 +599,7 @@ const struct vgic_ops vgic_v2_ops = {
>       .domain_init = vgic_v2_domain_init,
>       .get_irq_priority = vgic_v2_get_irq_priority,
>       .get_target_vcpu = vgic_v2_get_target_vcpu,
> +    .get_max_vcpus = vgic_v2_get_max_vcpus,
>   };
>
>   /*
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 21d8d3f..e79b010 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1048,6 +1048,11 @@ static int vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
>       }
>   }
>
> +static int vgic_v3_get_max_vcpus(void)
> +{
> +    return GICV3_MAX_CPUS;
> +};
> +
>   static const struct mmio_handler_ops vgic_rdistr_mmio_handler = {
>       .read_handler  = vgic_v3_rdistr_mmio_read,
>       .write_handler = vgic_v3_rdistr_mmio_write,
> @@ -1220,6 +1225,7 @@ const struct vgic_ops vgic_v3_ops = {
>       .get_irq_priority = vgic_v3_get_irq_priority,
>       .get_target_vcpu  = vgic_v3_get_target_vcpu,
>       .emulate_sysreg  = vgic_v3_emulate_sysreg,
> +    .get_max_vcpus = vgic_v3_get_max_vcpus,
>   };
>
>   /*
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index b7b5cd2..b525bec 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -261,10 +261,7 @@ struct arch_vcpu
>   void vcpu_show_execution_state(struct vcpu *);
>   void vcpu_show_registers(const struct vcpu *);
>
> -static inline unsigned int domain_max_vcpus(const struct domain *d)
> -{
> -    return MAX_VIRT_CPUS;
> -}
> +unsigned int domain_max_vcpus(const struct domain *);
>
>   /*
>    * Due to the restriction of GICv3, the number of vCPUs in AFF0 is
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index c6ef4bf..55f99cd 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -18,6 +18,9 @@
>   #ifndef __ASM_ARM_GIC_H__
>   #define __ASM_ARM_GIC_H__
>
> +#define GICV2_MAX_CPUS  8
> +#define GICV3_MAX_CPUS  128

This doesn't need to be exported and can go in each vGIC driver.

Furthermore, technically, the vGICv3 driver is able to support more than 
128 vCPUs...

> +
>   #define NR_GIC_LOCAL_IRQS  NR_LOCAL_IRQS
>   #define NR_GIC_SGI         16
>   #define MAX_RDIST_COUNT    4
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 2f413e1..1157b04 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -110,6 +110,8 @@ struct vgic_ops {
>       struct vcpu *(*get_target_vcpu)(struct vcpu *v, unsigned int irq);
>       /* vGIC sysreg emulation */
>       int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr hsr);
> +    /* Get the maximum number of vCPU supported */
> +    int (*get_max_vcpus)(void);

Why did you create a function? The value never change so a field value 
would have been better...

>   };
>
>   /* Number of ranks of interrupt registers for a domain */
>

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V5 10/10] xen/arm64: increase MAX_VIRT_CPUS to 128 on arm64

[Julien Grall]

Hi Chen,

On 30/05/2015 12:07, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
>
> GIC-500 supports up to 128 cores in a single SoC. Increase MAX_VIRT_CPUS
> to 128 on arm64.

Where did you find this restriction? AFAICT the changes you made in the 
vGICv3 driver allow us to use up to 4096 CPUs.

Regards,

-- 
Julien Grall

Re: [PATCH V5 05/10] xen/arm64: gicv3: Use AFF1 when translating ICC_SGI1R_EL1 to cpumask

[Chen Baozi]

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

Hi Julien,

> On May 31, 2015, at 21:14, Julien Grall <julien.grall@citrix.com> wrote:
> 
> Hi Chen,
> 
> On 30/05/2015 12:07, Chen Baozi wrote:
>> From: Chen Baozi <baozich@gmail.com>
>> 
>> To support more than 16 vCPUs, we have to calculate cpumask with AFF1
>> field value in ICC_SGI1R_EL1.
>> 
>> Signed-off-by: Chen Baozi <baozich@gmail.com>
>> ---
>>  xen/arch/arm/vgic-v3.c            | 9 ++++++++-
>>  xen/include/asm-arm/gic_v3_defs.h | 3 +++
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>> 
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index a283c8c..21d8d3f 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -976,10 +976,17 @@ static inline void gicv3_sgir_to_cpumask(cpumask_t *cpumask,
>>                                           const register_t sgir)
>>  {
>>      unsigned long target_list;
>> +    int aff1;
> 
> unsigned int.
> 
>> 
>>      target_list = sgir & ICH_SGI_TARGETLIST_MASK;
>> -    bitmap_copy(cpumask_bits(cpumask), &target_list, ICH_SGI_TARGET_BITS);
>> +    /* We assume that only AFF1 is used in ICC_SGI1R_EL1. */
>> +    aff1 = (sgir >> ICH_SGI_AFFINITY_LEVEL(1)) & ICH_SGI_AFFx_MASK;
>> 
>> +    BUILD_BUG_ON(sizeof(cpumask_t)*8 < MAX_VIRT_CPUS);
> 
> Ah, here is the BUILD_BUG_ON. This is not vgic-v3 specific but generic to all the vgic. It would have been more logical to put it in the function vgic_to_sgi in the previous patch (i.e #4).
> 
>> +    BUG_ON(((aff1+1) * ICH_SGI_TARGET_BITS) > NR_CPUS);
> 
> NACK. This value is passed by the guest. With this a malicious guest could take down Xen.
> 
>> +
>> +    memcpy((uint16_t *)cpumask + aff1, &target_list,
> 
> That's hackhish. You can't assume that the bitmap will be at the beginning of cpumask_t.

Sorry, I’m not quite understand this. Why can not?

Furthermore, considering that the size of cpumask_t might be greater than any int type, do you
get a better way to set the bitmap without a loop?

Cheers,

Baozi.

[-- Attachment #1.2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V5 09/10] xen/arm: make domain_max_vcpus return value from vgic_ops

[Chen Baozi]

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


> On May 31, 2015, at 21:35, Julien Grall <julien.grall@citrix.com> wrote:
> 
> Hi Chen,
> 
> On 30/05/2015 12:07, Chen Baozi wrote:
>> From: Chen Baozi <baozich@gmail.com>
>> 
>> When a guest uses vGICv2, the maximum number of vCPU it can support
>> should not be as many as MAX_VIRT_CPUS, which is 128 at the moment.
>> So the domain_max_vcpus should return the value according to the vGIC
>> the domain uses.
>> 
>> We didn't keep it as the old static inline form because it will break
>> compilation when access the member of struct domain:
>> 
>> In file included from xen/include/xen/domain.h:6:0,
>>                  from xen/include/xen/sched.h:10,
>>                  from arm64/asm-offsets.c:10:
>> xen/include/asm/domain.h: In function ‘domain_max_vcpus’:
>> xen/include/asm/domain.h:266:10: error: dereferencing pointer to incomplete type
>>      if (d->arch.vgic.version == GIC_V2)
>>           ^
>> 
>> Signed-off-by: Chen Baozi <baozich@gmail.com>
>> ---
>>  xen/arch/arm/domain.c        | 5 +++++
>>  xen/arch/arm/domain_build.c  | 2 +-
>>  xen/arch/arm/vgic-v2.c       | 6 ++++++
>>  xen/arch/arm/vgic-v3.c       | 6 ++++++
>>  xen/include/asm-arm/domain.h | 5 +----
>>  xen/include/asm-arm/gic.h    | 3 +++
>>  xen/include/asm-arm/vgic.h   | 2 ++
>>  7 files changed, 24 insertions(+), 5 deletions(-)
>> 
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 0cf147c..c638ff5 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -890,6 +890,11 @@ void vcpu_block_unless_event_pending(struct vcpu *v)
>>          vcpu_unblock(current);
>>  }
>> 
>> +unsigned int domain_max_vcpus(const struct domain *d)
>> +{
>> +    return d->arch.vgic.handler->get_max_vcpus();
> 
> As long as we keep MAX_VIRT_VCPUS in the common vGIC drivers, we need to make sure that we don't return a superior value because technically, with your changes, the vGICv3 could support more than 128 vCPUs...
> 
> I would do: return min(MAX_VIRT_CPUS, d->arch...)
> 
>> +}
>> +
>>  /*
>>   * Local variables:
>>   * mode: C
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 5591d82..88cfcee 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -769,7 +769,7 @@ static int make_cpus_node(const struct domain *d, void *fdt,
>>           * is enough for the current max vcpu number.
>>           */
>>          mpidr_aff = vcpuid_to_vaffinity(cpu);
>> -        DPRINT("Create cpu@%x node\n", mpidr_aff);
>> +        DPRINT("Create cpu@%x (logical CPUID: %d) node\n", mpidr_aff, cpu);
> 
> This change belongs to patch #7...
> 
>> 
>>          snprintf(buf, sizeof(buf), "cpu@%x", mpidr_aff);
>>          res = fdt_begin_node(fdt, buf);
>> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
>> index 17a3c9f..206d289 100644
>> --- a/xen/arch/arm/vgic-v2.c
>> +++ b/xen/arch/arm/vgic-v2.c
>> @@ -508,6 +508,11 @@ static struct vcpu *vgic_v2_get_target_vcpu(struct vcpu *v, unsigned int irq)
>>      return v_target;
>>  }
>> 
>> +static int vgic_v2_get_max_vcpus(void)
>> +{
>> +    return GICV2_MAX_CPUS;
>> +}
>> +
>>  static int vgic_v2_get_irq_priority(struct vcpu *v, unsigned int irq)
>>  {
>>      int priority;
>> @@ -594,6 +599,7 @@ const struct vgic_ops vgic_v2_ops = {
>>      .domain_init = vgic_v2_domain_init,
>>      .get_irq_priority = vgic_v2_get_irq_priority,
>>      .get_target_vcpu = vgic_v2_get_target_vcpu,
>> +    .get_max_vcpus = vgic_v2_get_max_vcpus,
>>  };
>> 
>>  /*
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index 21d8d3f..e79b010 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -1048,6 +1048,11 @@ static int vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
>>      }
>>  }
>> 
>> +static int vgic_v3_get_max_vcpus(void)
>> +{
>> +    return GICV3_MAX_CPUS;
>> +};
>> +
>>  static const struct mmio_handler_ops vgic_rdistr_mmio_handler = {
>>      .read_handler  = vgic_v3_rdistr_mmio_read,
>>      .write_handler = vgic_v3_rdistr_mmio_write,
>> @@ -1220,6 +1225,7 @@ const struct vgic_ops vgic_v3_ops = {
>>      .get_irq_priority = vgic_v3_get_irq_priority,
>>      .get_target_vcpu  = vgic_v3_get_target_vcpu,
>>      .emulate_sysreg  = vgic_v3_emulate_sysreg,
>> +    .get_max_vcpus = vgic_v3_get_max_vcpus,
>>  };
>> 
>>  /*
>> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>> index b7b5cd2..b525bec 100644
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -261,10 +261,7 @@ struct arch_vcpu
>>  void vcpu_show_execution_state(struct vcpu *);
>>  void vcpu_show_registers(const struct vcpu *);
>> 
>> -static inline unsigned int domain_max_vcpus(const struct domain *d)
>> -{
>> -    return MAX_VIRT_CPUS;
>> -}
>> +unsigned int domain_max_vcpus(const struct domain *);
>> 
>>  /*
>>   * Due to the restriction of GICv3, the number of vCPUs in AFF0 is
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index c6ef4bf..55f99cd 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -18,6 +18,9 @@
>>  #ifndef __ASM_ARM_GIC_H__
>>  #define __ASM_ARM_GIC_H__
>> 
>> +#define GICV2_MAX_CPUS  8
>> +#define GICV3_MAX_CPUS  128
> 
> This doesn't need to be exported and can go in each vGIC driver.
> 
> Furthermore, technically, the vGICv3 driver is able to support more than 128 vCPUs...
> 
>> +
>>  #define NR_GIC_LOCAL_IRQS  NR_LOCAL_IRQS
>>  #define NR_GIC_SGI         16
>>  #define MAX_RDIST_COUNT    4
>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
>> index 2f413e1..1157b04 100644
>> --- a/xen/include/asm-arm/vgic.h
>> +++ b/xen/include/asm-arm/vgic.h
>> @@ -110,6 +110,8 @@ struct vgic_ops {
>>      struct vcpu *(*get_target_vcpu)(struct vcpu *v, unsigned int irq);
>>      /* vGIC sysreg emulation */
>>      int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr hsr);
>> +    /* Get the maximum number of vCPU supported */
>> +    int (*get_max_vcpus)(void);
> 
> Why did you create a function? The value never change so a field value would have been better…

But is it appropriate that we define a field value in a struct called vgic_ops? I
thought ‘XXX_ops’ refers to a struct that is made up by function points. (FIXME)

Cheers,

Baozi.



[-- Attachment #1.2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V5 10/10] xen/arm64: increase MAX_VIRT_CPUS to 128 on arm64

[Chen Baozi]

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


> On May 31, 2015, at 21:40, Julien Grall <julien.grall@citrix.com> wrote:
> 
> Hi Chen,
> 
> On 30/05/2015 12:07, Chen Baozi wrote:
>> From: Chen Baozi <baozich@gmail.com>
>> 
>> GIC-500 supports up to 128 cores in a single SoC. Increase MAX_VIRT_CPUS
>> to 128 on arm64.
> 
> Where did you find this restriction? AFAICT the changes you made in the vGICv3 driver allow us to use up to 4096 CPUs.

In ARM DDI0516B (ARM CoreLinkTM GIC-500 Generic Interrupt Controller
Technical Reference Manual):

1.1 About the GIC-500

The GIC-500 is a build-time configurable interrupt controller that supports up to 128 cores.
...

And GIC-500 is the only GICv3 implementation as far as I know.

Cheers,

Baozi



[-- Attachment #1.2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V5 05/10] xen/arm64: gicv3: Use AFF1 when translating ICC_SGI1R_EL1 to cpumask

[Julien Grall]

On 31/05/2015 16:25, Chen Baozi wrote:
>> On May 31, 2015, at 21:14, Julien Grall <julien.grall@citrix.com> wrote:
>> On 30/05/2015 12:07, Chen Baozi wrote:
>>> From: Chen Baozi <baozich@gmail.com>
>>>
>>> To support more than 16 vCPUs, we have to calculate cpumask with AFF1
>>> field value in ICC_SGI1R_EL1.
>>>
>>> Signed-off-by: Chen Baozi <baozich@gmail.com>
>>> ---
>>>   xen/arch/arm/vgic-v3.c            | 9 ++++++++-
>>>   xen/include/asm-arm/gic_v3_defs.h | 3 +++
>>>   2 files changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>>> index a283c8c..21d8d3f 100644
>>> --- a/xen/arch/arm/vgic-v3.c
>>> +++ b/xen/arch/arm/vgic-v3.c
>>> @@ -976,10 +976,17 @@ static inline void gicv3_sgir_to_cpumask(cpumask_t *cpumask,
>>>                                            const register_t sgir)
>>>   {
>>>       unsigned long target_list;
>>> +    int aff1;
>>
>> unsigned int.
>>
>>>
>>>       target_list = sgir & ICH_SGI_TARGETLIST_MASK;
>>> -    bitmap_copy(cpumask_bits(cpumask), &target_list, ICH_SGI_TARGET_BITS);
>>> +    /* We assume that only AFF1 is used in ICC_SGI1R_EL1. */
>>> +    aff1 = (sgir >> ICH_SGI_AFFINITY_LEVEL(1)) & ICH_SGI_AFFx_MASK;
>>>
>>> +    BUILD_BUG_ON(sizeof(cpumask_t)*8 < MAX_VIRT_CPUS);
>>
>> Ah, here is the BUILD_BUG_ON. This is not vgic-v3 specific but generic to all the vgic. It would have been more logical to put it in the function vgic_to_sgi in the previous patch (i.e #4).
>>
>>> +    BUG_ON(((aff1+1) * ICH_SGI_TARGET_BITS) > NR_CPUS);
>>
>> NACK. This value is passed by the guest. With this a malicious guest could take down Xen.
>>
>>> +
>>> +    memcpy((uint16_t *)cpumask + aff1, &target_list,
>>
>> That's hackhish. You can't assume that the bitmap will be at the beginning of cpumask_t.
>
> Sorry, I’m not quite understand this. Why can not?

cpumask_t is a structure containing a bitmap. You are assuming that the 
address of cpumask_t is always equal to cpumask_t.bitmap.
There is helper to get the correct address (see cpumask_bits(cpumask)).

Furthermore, there is function to copy a bitmap (see bitmap_copy). 
Although it will require more than 1 line of code.

If you don't want to use it please use a direct assignation (it's only a 
16 bits data) which will always be faster than a memcpy.

Regards,

-- 
Julien Grall

Re: [PATCH V5 09/10] xen/arm: make domain_max_vcpus return value from vgic_ops

[Julien Grall]

Hi Chen,

On 31/05/2015 16:31, Chen Baozi wrote:
>> On May 31, 2015, at 21:35, Julien Grall <julien.grall@citrix.com> wrote:
>>> +
>>>   #define NR_GIC_LOCAL_IRQS  NR_LOCAL_IRQS
>>>   #define NR_GIC_SGI         16
>>>   #define MAX_RDIST_COUNT    4
>>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
>>> index 2f413e1..1157b04 100644
>>> --- a/xen/include/asm-arm/vgic.h
>>> +++ b/xen/include/asm-arm/vgic.h
>>> @@ -110,6 +110,8 @@ struct vgic_ops {
>>>       struct vcpu *(*get_target_vcpu)(struct vcpu *v, unsigned int irq);
>>>       /* vGIC sysreg emulation */
>>>       int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr hsr);
>>> +    /* Get the maximum number of vCPU supported */
>>> +    int (*get_max_vcpus)(void);
>>
>> Why did you create a function? The value never change so a field value would have been better…
>
> But is it appropriate that we define a field value in a struct called vgic_ops? I
> thought ‘XXX_ops’ refers to a struct that is made up by function points. (FIXME)

Well if the only issue if the name, please rename the structure. IHMO, 
the name is fine for me, I will let Ian & Stefano decides about it.

As the vGIC is always supporting N cpus and will never change, it's 
pointless to use a function (thinking about the "cost" vs access an field).

Regards,

-- 
Julien Grall

Re: [PATCH V5 09/10] xen/arm: make domain_max_vcpus return value from vgic_ops

[Andrew Cooper]

On 31/05/15 19:05, Julien Grall wrote:
> Hi Chen,
>
> On 31/05/2015 16:31, Chen Baozi wrote:
>>> On May 31, 2015, at 21:35, Julien Grall <julien.grall@citrix.com>
>>> wrote:
>>>> +
>>>>   #define NR_GIC_LOCAL_IRQS  NR_LOCAL_IRQS
>>>>   #define NR_GIC_SGI         16
>>>>   #define MAX_RDIST_COUNT    4
>>>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
>>>> index 2f413e1..1157b04 100644
>>>> --- a/xen/include/asm-arm/vgic.h
>>>> +++ b/xen/include/asm-arm/vgic.h
>>>> @@ -110,6 +110,8 @@ struct vgic_ops {
>>>>       struct vcpu *(*get_target_vcpu)(struct vcpu *v, unsigned int
>>>> irq);
>>>>       /* vGIC sysreg emulation */
>>>>       int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr
>>>> hsr);
>>>> +    /* Get the maximum number of vCPU supported */
>>>> +    int (*get_max_vcpus)(void);
>>>
>>> Why did you create a function? The value never change so a field
>>> value would have been better…
>>
>> But is it appropriate that we define a field value in a struct called
>> vgic_ops? I
>> thought ‘XXX_ops’ refers to a struct that is made up by function
>> points. (FIXME)
>
> Well if the only issue if the name, please rename the structure. IHMO,
> the name is fine for me, I will let Ian & Stefano decides about it.
>
> As the vGIC is always supporting N cpus and will never change, it's
> pointless to use a function (thinking about the "cost" vs access an
> field).

We have plenty of other structures with mixed function pointers and data.

A "const unsigned int max_vcpus;" should absolutely be used in a case
like this.

~Andrew

Re: [PATCH V5 10/10] xen/arm64: increase MAX_VIRT_CPUS to 128 on arm64

[Julien Grall]

Hi Chen,

On 31/05/2015 16:37, Chen Baozi wrote:
>
>> On May 31, 2015, at 21:40, Julien Grall <julien.grall@citrix.com> wrote:
>>
>> Hi Chen,
>>
>> On 30/05/2015 12:07, Chen Baozi wrote:
>>> From: Chen Baozi <baozich@gmail.com>
>>>
>>> GIC-500 supports up to 128 cores in a single SoC. Increase MAX_VIRT_CPUS
>>> to 128 on arm64.
>>
>> Where did you find this restriction? AFAICT the changes you made in the vGICv3 driver allow us to use up to 4096 CPUs.
>
> In ARM DDI0516B (ARM CoreLinkTM GIC-500 Generic Interrupt Controller
> Technical Reference Manual):
>
> 1.1 About the GIC-500
>
> The GIC-500 is a build-time configurable interrupt controller that supports up to 128 cores.
> ...
>
> And GIC-500 is the only GICv3 implementation as far as I know.

The only implementation hardware ;). The vGICv3 in both KVM and Xen are 
2 other implementations but software.

For instance, with your series a cluster can use up to 16 cores but the 
GIC-500 is only supporting up to 8 cores...

Regards,

-- 
Julien Grall

Re: [PATCH V5 10/10] xen/arm64: increase MAX_VIRT_CPUS to 128 on arm64

[Chen Baozi]

On Sun, May 31, 2015 at 07:21:22PM +0100, Julien Grall wrote:
> Hi Chen,
> 
> On 31/05/2015 16:37, Chen Baozi wrote:
> >
> >>On May 31, 2015, at 21:40, Julien Grall <julien.grall@citrix.com> wrote:
> >>
> >>Hi Chen,
> >>
> >>On 30/05/2015 12:07, Chen Baozi wrote:
> >>>From: Chen Baozi <baozich@gmail.com>
> >>>
> >>>GIC-500 supports up to 128 cores in a single SoC. Increase MAX_VIRT_CPUS
> >>>to 128 on arm64.
> >>
> >>Where did you find this restriction? AFAICT the changes you made in the vGICv3 driver allow us to use up to 4096 CPUs.
> >
> >In ARM DDI0516B (ARM CoreLinkTM GIC-500 Generic Interrupt Controller
> >Technical Reference Manual):
> >
> >1.1 About the GIC-500
> >
> >The GIC-500 is a build-time configurable interrupt controller that supports up to 128 cores.
> >...
> >
> >And GIC-500 is the only GICv3 implementation as far as I know.
> 
> The only implementation hardware ;). The vGICv3 in both KVM and Xen are 2
> other implementations but software.
> 
> For instance, with your series a cluster can use up to 16 cores but the
> GIC-500 is only supporting up to 8 cores...

Well, if 4096 is a acceptable value, it will cost 512M address space for GICR_*
(We need to chagne patch #1 too). Although we do have enough space for GICR_*
before it reaches the GNTTAB region, I don't think it is a good idea to
increase MAX_VIRT_CPUS to such a big one.

And I've also check the old value of MAX_VIRT_CPUS, it used to be 128 before
aa25a61.

Or do you have a better suggestion?

Cheers,

Baozi.

Re: [PATCH V5 10/10] xen/arm64: increase MAX_VIRT_CPUS to 128 on arm64

[Julien Grall]

Hi Chen,

On 01/06/2015 01:56, Chen Baozi wrote:
>> For instance, with your series a cluster can use up to 16 cores but the
>> GIC-500 is only supporting up to 8 cores...
>
> Well, if 4096 is a acceptable value, it will cost 512M address space for GICR_*
> (We need to chagne patch #1 too). Although we do have enough space for GICR_*
> before it reaches the GNTTAB region, I don't think it is a good idea to
> increase MAX_VIRT_CPUS to such a big one.

4096 is the limitation of using only AFF1 and AFF0. That would be 
inevitable if one day we decide to support 4096 CPUs :).

>
> And I've also check the old value of MAX_VIRT_CPUS, it used to be 128 before
> aa25a61.
>
> Or do you have a better suggestion?

I'm not against to limit to 128 vCPUs. It's just the commit message is 
not true. You are limiting the number of VCPUs for practical reason
and not because we are implementing GIC-500.

Please update the commit message according to it.

Regards,

-- 
Julien Grall