[PATCH v8 0/8] Support more than 8 vcpus on arm64 with GICv3

[PATCH v8 0/8] 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 V7:
* Updates some commit logs
* Drop aff2 and aff3 in struct sgi_target for it is unused.
* Minor changes according to previous reviews.

Changes from V6:
* Use the new 'struct sgi_target' instead of cpumask_t in vgic_to_sgi.
* Make the domain_max_vcpus to return MAX_VIRT_CPUS and avoid to split
  arch_domain_init.

Changes form V5:
* Rework gicv3_sgir_to_cpumask in #5
* Rework #8 to split arch_domain_create into two parts:
  - arch_domain_preinit to initialise vgic_ops before evtchn_init is
    called
  - the rest of logic remains in arch_domain_create
* Use a field value in struct vgic_ops instead of the function point
  for max_vcpus.
* Minor changes according to previous reviews.

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 (8):
  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 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/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             | 20 ++++++++++++-----
 xen/arch/arm/domain_build.c       | 14 +++++++++---
 xen/arch/arm/vgic-v2.c            |  8 ++++---
 xen/arch/arm/vgic-v3.c            | 18 ++++++++++------
 xen/arch/arm/vgic.c               | 45 +++++++++++++++++----------------------
 xen/arch/arm/vpsci.c              |  5 +----
 xen/include/asm-arm/config.h      |  4 ++++
 xen/include/asm-arm/domain.h      | 39 +++++++++++++++++++++++++++++++--
 xen/include/asm-arm/gic_v3_defs.h |  3 +++
 xen/include/asm-arm/vgic.h        |  9 +++++++-
 xen/include/public/arch-arm.h     |  4 ++--
 12 files changed, 129 insertions(+), 54 deletions(-)

-- 
2.1.4

[PATCH v8 1/8] 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>
Acked-by: Ian Campbell <ian.campbell@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 v8 2/8] 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. (See the note of 'Bits[15:0]' in '5.7.29 ICC_SGI0R_EL1
ICC_SGI1R_EL1 and ICC_ASGI1R_EL1, GICv3 Architecture Specification')
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>
---
 xen/include/asm-arm/domain.h | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 75b17af..35b9a6d 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -266,6 +266,44 @@ 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 the 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.
+ */
+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 v8 3/8] 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
  - Both PSCI 0.1 and PSCI 0.2 are modified to respect the MPIDR like.

Signed-off-by: Chen Baozi <baozich@gmail.com>
Reviewed-by: Julien Grall <julien.grall@citrix.com>
---
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.

Thus, we consider CPUID is equal to the 'reg' register in DT (which
is an MPIDR-like value).

 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 v8 4/8] xen/arm: Use AFF1 when translating ICC_SGI1R_EL1 to cpumask

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

The old unsigned long type of vcpu_mask can only express 64 cpus at the
most, which might not be enough for the guest which used vGICv3. We
introduce a new struct sgi_target for the target cpu list of SGI, which
holds the affinity path information (only level 1 at the moment). For
GICv2 that has no affinity level, we can just set the corresponding
fields to be 0.

Signed-off-by: Chen Baozi <baozich@gmail.com>
---
 xen/arch/arm/vgic-v2.c            |  7 +++---
 xen/arch/arm/vgic-v3.c            | 10 +++++----
 xen/arch/arm/vgic.c               | 45 +++++++++++++++++----------------------
 xen/include/asm-arm/gic_v3_defs.h |  3 +++
 xen/include/asm-arm/vgic.h        |  7 +++++-
 5 files changed, 38 insertions(+), 34 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 3be1a51..5949cf1 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -201,16 +201,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;
+    struct sgi_target target;
 
+    memset(&target, 0, sizeof(struct sgi_target));
     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:
+        target.list = (sgir & GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT;
         sgi_mode = SGI_TARGET_LIST;
         break;
     case GICD_SGI_TARGET_OTHERS_VAL:
@@ -226,7 +227,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, &target);
 }
 
 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..93610d0 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -977,17 +977,19 @@ 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;
+    struct sgi_target target;
 
+    memset(&target, 0, sizeof(struct sgi_target));
     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:
+        /* We assume that only AFF1 is used in ICC_SGI1R_EL1. */
+        target.aff1 = (sgir >> ICH_SGI_AFFINITY_LEVEL(1)) & ICH_SGI_AFFx_MASK;
+        target.list = sgir & ICH_SGI_TARGETLIST_MASK;
         sgi_mode = SGI_TARGET_LIST;
         break;
     case ICH_SGI_TARGET_OTHERS:
@@ -998,7 +1000,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, &target);
 }
 
 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..59bd98a 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -318,15 +318,14 @@ 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)
+                const struct sgi_target *target)
 {
     struct domain *d = v->domain;
     int vcpuid;
     int i;
-
-    ASSERT(d->max_vcpus < 8*sizeof(vcpu_mask));
+    unsigned int base;
+    unsigned long int bitmap;
 
     ASSERT( virq < 16 );
 
@@ -334,29 +333,33 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
     {
     case SGI_TARGET_LIST:
         perfc_incr(vgic_sgi_list);
+        base = target->aff1 << 4;
+        bitmap = target->list;
+        for_each_set_bit( i, &bitmap, sizeof(target->list) * 8 )
+        {
+            vcpuid = base + i;
+            if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) )
+            {
+                gprintk(XENLOG_WARNING, "VGIC: write r=%"PRIregister" \
+                        target->list=%hx, wrong CPUTargetList \n",
+                        sgir, target->list);
+                continue;
+            }
+            vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq);
+        }
         break;
     case SGI_TARGET_OTHERS:
-        /*
-         * We expect vcpu_mask to be 0 for SGI_TARGET_OTHERS and
-         * SGI_TARGET_SELF mode. So Force vcpu_mask to 0
-         */
         perfc_incr(vgic_sgi_others);
-        vcpu_mask = 0;
         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);
+                vgic_vcpu_inject_irq(d->vcpu[i], virq);
         }
         break;
     case SGI_TARGET_SELF:
-        /*
-         * We expect vcpu_mask to be 0 for SGI_TARGET_OTHERS and
-         * 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);
+        vgic_vcpu_inject_irq(d->vcpu[current->vcpu_id], virq);
         break;
     default:
         gprintk(XENLOG_WARNING,
@@ -365,16 +368,6 @@ 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 )
-    {
-        if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) )
-        {
-            gprintk(XENLOG_WARNING, "VGIC: write r=%"PRIregister" \
-                    vcpu_mask=%lx, wrong CPUTargetList\n", sgir, vcpu_mask);
-            continue;
-        }
-        vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq);
-    }
     return 1;
 }
 
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index 556f114..960669d 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -152,6 +152,9 @@
 #define ICH_SGI_IRQ_SHIFT            24
 #define ICH_SGI_IRQ_MASK             0xf
 #define ICH_SGI_TARGETLIST_MASK      0xffff
+#define ICH_SGI_AFFx_MASK            0xff
+#define ICH_SGI_AFFINITY_LEVEL(x)    (16 * (x))
+
 #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..d68539a 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -98,6 +98,11 @@ struct vgic_irq_rank {
     };
 };
 
+struct sgi_target {
+    uint8_t aff1;
+    uint16_t list;
+};
+
 struct vgic_ops {
     /* Initialize vGIC */
     int (*vcpu_init)(struct vcpu *v);
@@ -201,7 +206,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);
+                       const struct sgi_target *target);
 extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq);
 
 /* Reserve a specific guest vIRQ */
-- 
2.1.4

[PATCH v8 5/8] 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>
Reviewed-by: Julien Grall <julien.grall@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.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..5f3c434 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 = (i & 0x0f) | (((i >> 4) & 0xff) << 8);
+        name = GCSPRINTF("cpu@%"PRIx64, 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 v8 6/8] 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>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@citrix.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..12b46ca 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;
+    uint64_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@%"PRIx64" (logical CPUID: %d) node\n", mpidr_aff, cpu);
 
-        snprintf(buf, sizeof(buf), "cpu@%u", cpu);
+        snprintf(buf, sizeof(buf), "cpu@%lx", 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 v8 7/8] xen/arm: make domain_max_vcpus return value from vgic_ops

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

Each vGIC driver supports different maximum numbers of vCPU. For
example, GICv2 is limited to 8 vCPUs, while GICv3 can support up
to 4096 vCPUs if we use both AFF0 and AFF1. Thus, domain_max_vcpus
should depend on not only MAX_VIRT_CPUS but also the version
of vGIC that the guest uses.

Since evtchn_init would call domain_max_vcpus to allocate poll_mask
when the vgic_ops haven't been initialised yet, we make it return
MAX_VIRT_CPUS at that time. On ARM32, event channel doesn't need
to allocate the poll_mask because MAX_VIRT_CPUS < BITS_PER_LONG,
while allocating more memory (2 unsigned long rather than 1) only
for poll_mask on arm64 with GICv2 looks not so expensive.

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        | 14 ++++++++++++++
 xen/arch/arm/vgic-v2.c       |  1 +
 xen/arch/arm/vgic-v3.c       |  5 +++++
 xen/include/asm-arm/domain.h |  5 +----
 xen/include/asm-arm/vgic.h   |  2 ++
 5 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 0cf147c..01d8ca89 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -890,6 +890,20 @@ void vcpu_block_unless_event_pending(struct vcpu *v)
         vcpu_unblock(current);
 }
 
+unsigned int domain_max_vcpus(const struct domain *d)
+{
+    /*
+     * Since evtchn_init would call domain_max_vcpus for poll_mask
+     * allocation when the vgic_ops haven't been initialised yet,
+     * we return MAX_VIRT_CPUS if d->arch.vgic.handler is null.
+     */
+    if ( !d->arch.vgic.handler )
+        return MAX_VIRT_CPUS;
+    else
+        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-v2.c b/xen/arch/arm/vgic-v2.c
index 5949cf1..bbeb740 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -585,6 +585,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,
+    .max_vcpus = 8,
 };
 
 /*
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 93610d0..93af6c8 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1204,6 +1204,11 @@ 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,
+    /*
+     * 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,
 };
 
 /*
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 35b9a6d..23598dd 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/vgic.h b/xen/include/asm-arm/vgic.h
index d68539a..4477962 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -115,6 +115,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);
+    /* Maximum number of vCPU supported */
+    const unsigned int max_vcpus;
 };
 
 /* 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 v8 8/8] xen/arm64: increase MAX_VIRT_CPUS to 128 on arm64

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

After we have increased the size of GICR in address space for guest
and made use of both AFF0 and AFF1 in (v)MPIDR, we are now able to
support up to 4096 vCPUs in theory. However, it will cost 512M
address space for GICR region, which is unnecessary big at the
moment. Considering the max CPU number that GIC-500 can support and
the old value of MAX_VIRT_CPUS before commit aa25a61, we increase
its value to 128.

Signed-off-by: Chen Baozi <baozich@gmail.com>
---
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
separate cleanup patch series.

 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 93af6c8..2548b62 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 v8 2/8] xen/arm: Add functions of mapping between vCPUID and virtual affinity

[Ian Campbell]

On Fri, 2015-06-12 at 16:32 +0800, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
> 
> GICv3 restricts that the maximum number of CPUs in affinity 0 (one
> cluster) is 16. (See the note of 'Bits[15:0]' in '5.7.29 ICC_SGI0R_EL1
> ICC_SGI1R_EL1 and ICC_ASGI1R_EL1, GICv3 Architecture Specification')
> 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>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v8 3/8] xen/arm: Use the new functions for vCPUID/vaffinity transformation

[Ian Campbell]

On Fri, 2015-06-12 at 16:32 +0800, 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
>   - Both PSCI 0.1 and PSCI 0.2 are modified to respect the MPIDR like.
> 
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> Reviewed-by: Julien Grall <julien.grall@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v8 4/8] xen/arm: Use AFF1 when translating ICC_SGI1R_EL1 to cpumask

[Ian Campbell]

On Fri, 2015-06-12 at 16:32 +0800, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
> 
> The old unsigned long type of vcpu_mask can only express 64 cpus at the
> most, which might not be enough for the guest which used vGICv3. We
> introduce a new struct sgi_target for the target cpu list of SGI, which
> holds the affinity path information (only level 1 at the moment). For
> GICv2 that has no affinity level, we can just set the corresponding
> fields to be 0.
> 
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>  xen/arch/arm/vgic-v2.c            |  7 +++---
>  xen/arch/arm/vgic-v3.c            | 10 +++++----
>  xen/arch/arm/vgic.c               | 45 +++++++++++++++++----------------------
>  xen/include/asm-arm/gic_v3_defs.h |  3 +++
>  xen/include/asm-arm/vgic.h        |  7 +++++-
>  5 files changed, 38 insertions(+), 34 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 3be1a51..5949cf1 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -201,16 +201,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;
> +    struct sgi_target target;
>  
> +    memset(&target, 0, sizeof(struct sgi_target));

I'd prefer explicit initialisation of the relevant fields please. Which
may mean setting aff1 to 0 somewhere at the top, with a suitable comment
as to why, and might involve setting target.list to zero in some other
cases below or via an explicit initialiser here.

Or maybe you prefer a struct initialiser on the declaration with the
defaults.

> -    unsigned long vcpu_mask = 0;
> +    struct sgi_target target;
>  
> +    memset(&target, 0, sizeof(struct sgi_target));

Likewise.

> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 7b387b7..59bd98a 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -318,15 +318,14 @@ 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)
> +                const struct sgi_target *target)

For a 3 byte struct perhaps we can pass by value instead of reference?

I suppose it might eventually be 5 bytes, but even so...

> @@ -334,29 +333,33 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
>      {
>      case SGI_TARGET_LIST:
>          perfc_incr(vgic_sgi_list);
> +        base = target->aff1 << 4;
> +        bitmap = target->list;
> +        for_each_set_bit( i, &bitmap, sizeof(target->list) * 8 )
> +        {
> +            vcpuid = base + i;
> +            if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) )

What if d->vcpu[vcpuid] is NULL? (Was this a latent bug before, or am I
missing something?)

Note that at least the use of d->max_vcpus from the old code has now
gone.

> @@ -365,16 +368,6 @@ 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 )
> -    {
> -        if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) )
> -        {
> -            gprintk(XENLOG_WARNING, "VGIC: write r=%"PRIregister" \
> -                    vcpu_mask=%lx, wrong CPUTargetList\n", sgir, vcpu_mask);
> -            continue;
> -        }
> -        vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq);
> -    }
>      return 1;
>  }
>  

Ian.

Re: [PATCH v8 7/8] xen/arm: make domain_max_vcpus return value from vgic_ops

[Ian Campbell]

On Fri, 2015-06-12 at 16:32 +0800, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
> 
> Each vGIC driver supports different maximum numbers of vCPU. For
> example, GICv2 is limited to 8 vCPUs, while GICv3 can support up
> to 4096 vCPUs if we use both AFF0 and AFF1. Thus, domain_max_vcpus
> should depend on not only MAX_VIRT_CPUS but also the version
> of vGIC that the guest uses.
> 
> Since evtchn_init would call domain_max_vcpus to allocate poll_mask
> when the vgic_ops haven't been initialised yet, we make it return
> MAX_VIRT_CPUS at that time. On ARM32, event channel doesn't need
> to allocate the poll_mask because MAX_VIRT_CPUS < BITS_PER_LONG,
> while allocating more memory (2 unsigned long rather than 1) only
> for poll_mask on arm64 with GICv2 looks not so expensive.
> 
> 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>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[...]
> +     * 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.

Please stick an " == " ......................^ here.




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

Re: [PATCH v8 8/8] xen/arm64: increase MAX_VIRT_CPUS to 128 on arm64

[Ian Campbell]

On Fri, 2015-06-12 at 16:32 +0800, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
> 
> After we have increased the size of GICR in address space for guest
> and made use of both AFF0 and AFF1 in (v)MPIDR, we are now able to
> support up to 4096 vCPUs in theory. However, it will cost 512M
> address space for GICR region, which is unnecessary big at the

"unnecessarily"

> moment. Considering the max CPU number that GIC-500 can support and
> the old value of MAX_VIRT_CPUS before commit aa25a61, we increase
> its value to 128.
> 
> Signed-off-by: Chen Baozi <baozich@gmail.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v8 0/8] Support more than 8 vcpus on arm64 with GICv3

[Ian Campbell]

On Fri, 2015-06-12 at 16:32 +0800, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>

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

Apart from a few comments this series is now acked and I anticipate v9
being fine, but it wouldn't apply without this prerequisite. Does it
depends on the entire thing or just a subset which might already be
ready to go (i.e. is already acked too)?

Ian.

Re: [PATCH v8 4/8] xen/arm: Use AFF1 when translating ICC_SGI1R_EL1 to cpumask

[Julien Grall]

On 17/06/15 14:00, Ian Campbell wrote:
> On Fri, 2015-06-12 at 16:32 +0800, Chen Baozi wrote:
>> From: Chen Baozi <baozich@gmail.com>
>>
>> The old unsigned long type of vcpu_mask can only express 64 cpus at the
>> most, which might not be enough for the guest which used vGICv3. We
>> introduce a new struct sgi_target for the target cpu list of SGI, which
>> holds the affinity path information (only level 1 at the moment). For
>> GICv2 that has no affinity level, we can just set the corresponding
>> fields to be 0.
>>
>> Signed-off-by: Chen Baozi <baozich@gmail.com>
>> ---
>>  xen/arch/arm/vgic-v2.c            |  7 +++---
>>  xen/arch/arm/vgic-v3.c            | 10 +++++----
>>  xen/arch/arm/vgic.c               | 45 +++++++++++++++++----------------------
>>  xen/include/asm-arm/gic_v3_defs.h |  3 +++
>>  xen/include/asm-arm/vgic.h        |  7 +++++-
>>  5 files changed, 38 insertions(+), 34 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
>> index 3be1a51..5949cf1 100644
>> --- a/xen/arch/arm/vgic-v2.c
>> +++ b/xen/arch/arm/vgic-v2.c
>> @@ -201,16 +201,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;
>> +    struct sgi_target target;
>>  
>> +    memset(&target, 0, sizeof(struct sgi_target));
> 
> I'd prefer explicit initialisation of the relevant fields please. Which
> may mean setting aff1 to 0 somewhere at the top, with a suitable comment
> as to why, and might involve setting target.list to zero in some other
> cases below or via an explicit initialiser here.

Well, only SGI_TARGET_LIST is caring about struct sgi_target (see
vgic_to_sgi). I would only initialize it when it's required.

> Or maybe you prefer a struct initialiser on the declaration with the
> defaults.

It would be nice to have something initializing all the fields (even a
new one is added). It would avoid problem later.

>> -    unsigned long vcpu_mask = 0;
>> +    struct sgi_target target;
>>  
>> +    memset(&target, 0, sizeof(struct sgi_target));
> 
> Likewise.
> 
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 7b387b7..59bd98a 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -318,15 +318,14 @@ 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)
>> +                const struct sgi_target *target)
> 
> For a 3 byte struct perhaps we can pass by value instead of reference?
> 
> I suppose it might eventually be 5 bytes, but even so...
> 
>> @@ -334,29 +333,33 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
>>      {
>>      case SGI_TARGET_LIST:
>>          perfc_incr(vgic_sgi_list);
>> +        base = target->aff1 << 4;
>> +        bitmap = target->list;
>> +        for_each_set_bit( i, &bitmap, sizeof(target->list) * 8 )
>> +        {
>> +            vcpuid = base + i;
>> +            if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) )
> 
> What if d->vcpu[vcpuid] is NULL? (Was this a latent bug before, or am I
> missing something?)

I don't see any problem, if d->vcpu[vcpuid] is NULL there is no need to
send an SGI as the VCPU is not present.

Although d->vcpu[vcpuid] will unlikely be NULL.

> Note that at least the use of d->max_vcpus from the old code has now
> gone.

The array d->vcpu is based on d->max_vcpus, if by any chance vcpuid is
greater, you will access to a wrong address which will potentially crash
the host.

Regards,

-- 
Julien Grall

Re: [PATCH v8 4/8] xen/arm: Use AFF1 when translating ICC_SGI1R_EL1 to cpumask

[Ian Campbell]

On Wed, 2015-06-17 at 14:13 +0100, Julien Grall wrote:
> On 17/06/15 14:00, Ian Campbell wrote:
> > On Fri, 2015-06-12 at 16:32 +0800, Chen Baozi wrote:
> >> From: Chen Baozi <baozich@gmail.com>
> >>
> >> The old unsigned long type of vcpu_mask can only express 64 cpus at the
> >> most, which might not be enough for the guest which used vGICv3. We
> >> introduce a new struct sgi_target for the target cpu list of SGI, which
> >> holds the affinity path information (only level 1 at the moment). For
> >> GICv2 that has no affinity level, we can just set the corresponding
> >> fields to be 0.
> >>
> >> Signed-off-by: Chen Baozi <baozich@gmail.com>
> >> ---
> >>  xen/arch/arm/vgic-v2.c            |  7 +++---
> >>  xen/arch/arm/vgic-v3.c            | 10 +++++----
> >>  xen/arch/arm/vgic.c               | 45 +++++++++++++++++----------------------
> >>  xen/include/asm-arm/gic_v3_defs.h |  3 +++
> >>  xen/include/asm-arm/vgic.h        |  7 +++++-
> >>  5 files changed, 38 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> >> index 3be1a51..5949cf1 100644
> >> --- a/xen/arch/arm/vgic-v2.c
> >> +++ b/xen/arch/arm/vgic-v2.c
> >> @@ -201,16 +201,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;
> >> +    struct sgi_target target;
> >>  
> >> +    memset(&target, 0, sizeof(struct sgi_target));
> > 
> > I'd prefer explicit initialisation of the relevant fields please. Which
> > may mean setting aff1 to 0 somewhere at the top, with a suitable comment
> > as to why, and might involve setting target.list to zero in some other
> > cases below or via an explicit initialiser here.
> 
> Well, only SGI_TARGET_LIST is caring about struct sgi_target (see
> vgic_to_sgi). I would only initialize it when it's required.

Good point, and by keeping it a pointer you could even pass NULL in the
other cases, making this more obvious still.

> > 
> >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> >> index 7b387b7..59bd98a 100644
> >> --- a/xen/arch/arm/vgic.c
> >> +++ b/xen/arch/arm/vgic.c
> >> @@ -318,15 +318,14 @@ 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)
> >> +                const struct sgi_target *target)
> > 
> > For a 3 byte struct perhaps we can pass by value instead of reference?
> > 
> > I suppose it might eventually be 5 bytes, but even so...
> > 
> >> @@ -334,29 +333,33 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
> >>      {
> >>      case SGI_TARGET_LIST:
> >>          perfc_incr(vgic_sgi_list);
> >> +        base = target->aff1 << 4;
> >> +        bitmap = target->list;
> >> +        for_each_set_bit( i, &bitmap, sizeof(target->list) * 8 )
> >> +        {
> >> +            vcpuid = base + i;
> >> +            if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) )
> > 
> > What if d->vcpu[vcpuid] is NULL? (Was this a latent bug before, or am I
> > missing something?)
> 
> I don't see any problem, if d->vcpu[vcpuid] is NULL there is no need to
> send an SGI as the VCPU is not present.

But the code will, I think. I should have quoted a bit more, briefly it
is :

+            if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) )
                    continue
+            vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq);

So if d->vcpu[vcpuid] == NULL it will try and send an SGI to it, won't
it?

Ian.

Re: [PATCH v8 4/8] xen/arm: Use AFF1 when translating ICC_SGI1R_EL1 to cpumask

[Julien Grall]

On 17/06/15 14:19, Ian Campbell wrote:
> On Wed, 2015-06-17 at 14:13 +0100, Julien Grall wrote:
>> On 17/06/15 14:00, Ian Campbell wrote:
>>> On Fri, 2015-06-12 at 16:32 +0800, Chen Baozi wrote:
>>>> From: Chen Baozi <baozich@gmail.com>
>>>> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
>>>> index 3be1a51..5949cf1 100644
>>>> --- a/xen/arch/arm/vgic-v2.c
>>>> +++ b/xen/arch/arm/vgic-v2.c
>>>> @@ -201,16 +201,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;
>>>> +    struct sgi_target target;
>>>>  
>>>> +    memset(&target, 0, sizeof(struct sgi_target));
>>>
>>> I'd prefer explicit initialisation of the relevant fields please. Which
>>> may mean setting aff1 to 0 somewhere at the top, with a suitable comment
>>> as to why, and might involve setting target.list to zero in some other
>>> cases below or via an explicit initialiser here.
>>
>> Well, only SGI_TARGET_LIST is caring about struct sgi_target (see
>> vgic_to_sgi). I would only initialize it when it's required.
> 
> Good point, and by keeping it a pointer you could even pass NULL in the
> other cases, making this more obvious still.

Good idea.

>>>
>>>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>>>> index 7b387b7..59bd98a 100644
>>>> --- a/xen/arch/arm/vgic.c
>>>> +++ b/xen/arch/arm/vgic.c
>>>> @@ -318,15 +318,14 @@ 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)
>>>> +                const struct sgi_target *target)
>>>
>>> For a 3 byte struct perhaps we can pass by value instead of reference?
>>>
>>> I suppose it might eventually be 5 bytes, but even so...
>>>
>>>> @@ -334,29 +333,33 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
>>>>      {
>>>>      case SGI_TARGET_LIST:
>>>>          perfc_incr(vgic_sgi_list);
>>>> +        base = target->aff1 << 4;
>>>> +        bitmap = target->list;
>>>> +        for_each_set_bit( i, &bitmap, sizeof(target->list) * 8 )
>>>> +        {
>>>> +            vcpuid = base + i;
>>>> +            if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) )
>>>
>>> What if d->vcpu[vcpuid] is NULL? (Was this a latent bug before, or am I
>>> missing something?)
>>
>> I don't see any problem, if d->vcpu[vcpuid] is NULL there is no need to
>> send an SGI as the VCPU is not present.
> 
> But the code will, I think. I should have quoted a bit more, briefly it
> is :
> 
> +            if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) )
>                     continue
> +            vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq);
> 
> So if d->vcpu[vcpuid] == NULL it will try and send an SGI to it, won't
> it?

Hmmm, correct. I didn't read carefully the if, sorry. It should be it
"d->vcpu[vcpuid] == NULL || !is_vcpu_online(d->vcpu[vcpuid])".

And yes, this is a latent bug. Although, XEN_DOMCTL_max_vcpus will
return -ENOMEM if it fail to allocate a VCPU and libxl will continue to
create the domain. So no possibility for the guest to crash Xen.

Regards,

-- 
Julien Grall

Re: [PATCH v8 0/8] Support more than 8 vcpus on arm64 with GICv3

[Julien Grall]

Hi Ian,

On 17/06/15 14:04, Ian Campbell wrote:
> On Fri, 2015-06-12 at 16:32 +0800, Chen Baozi wrote:
>> From: Chen Baozi <baozich@gmail.com>
> 
>> These patches are written based upon Julien's "GICv2 on GICv3" series
>> and the IROUTER emulation cleanup patch.
> 
> Apart from a few comments this series is now acked and I anticipate v9
> being fine, but it wouldn't apply without this prerequisite. Does it
> depends on the entire thing or just a subset which might already be
> ready to go (i.e. is already acked too)?

AFAICT, only the patch #7 depends on my "GICv2 on GICv3 series" to get
the vGIC version.

I don't expect to see my series upstream within the next few weeks. I
don't want to delay this series, so I can try to extract the vGIC
version and send a separate patch.

Regards,

-- 
Julien Grall