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

[PATCH V4 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 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 cpumask_t type for vcpu_mask in vgic_to_sgi
  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/arm64: increase MAX_VIRT_CPUS to 128 on arm64
  xen/arm: make domain_max_vcpus return value according to vGIC version

 tools/libxl/libxl_arm.c       | 14 ++++++++++++--
 xen/arch/arm/domain.c         | 12 +++++++-----
 xen/arch/arm/domain_build.c   | 14 +++++++++++---
 xen/arch/arm/vgic-v2.c        | 16 ++++++++++++++--
 xen/arch/arm/vgic-v3.c        | 22 +++++++++++++++++-----
 xen/arch/arm/vgic.c           | 15 +++++++--------
 xen/arch/arm/vpsci.c          |  2 +-
 xen/include/asm-arm/config.h  |  4 ++++
 xen/include/asm-arm/domain.h  | 37 +++++++++++++++++++++++++++++++++++--
 xen/include/asm-arm/gic.h     |  3 +++
 xen/include/asm-arm/vgic.h    |  2 +-
 xen/include/public/arch-arm.h |  4 ++--
 12 files changed, 114 insertions(+), 31 deletions(-)

-- 
2.1.4

[PATCH V4 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>
---
 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 V4 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. 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 | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 75b17af..603a20b 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -266,6 +266,42 @@ 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;
+
+    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

Re: [PATCH V4 2/8] xen/arm: Add functions of mapping between vCPUID and virtual affinity

[Julien Grall]

Hi Chen,

On 28/05/15 11:15, 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>
> ---
>  xen/include/asm-arm/domain.h | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 75b17af..603a20b 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -266,6 +266,42 @@ 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;
> +
> +    BUILD_BUG_ON(!(MAX_VIRT_CPUS < ((1 << 12))));

Can you add a small comment to explain the 1 << 12? I.e we only support
AFF0 (4 bits) and AFF1 (8 bits).

Other than that this patch looks good to me.

Regards,



-- 
Julien Grall

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

Signed-off-by: Chen Baozi <baozich@gmail.com>
---
 xen/arch/arm/domain.c  | 6 +-----
 xen/arch/arm/vgic-v3.c | 2 +-
 xen/arch/arm/vpsci.c   | 2 +-
 3 files changed, 3 insertions(+), 7 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..1c1e7de 100644
--- a/xen/arch/arm/vpsci.c
+++ b/xen/arch/arm/vpsci.c
@@ -33,7 +33,7 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
     register_t vcpuid;
 
     if( ver == XEN_PSCI_V_0_2 )
-        vcpuid = (target_cpu & MPIDR_HWID_MASK);
+        vcpuid = vaffinity_to_vcpuid(target_cpu);
     else
         vcpuid = target_cpu;
 
-- 
2.1.4

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

[Julien Grall]

Hi Chen,

On 28/05/15 11:15, 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
> 
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>  xen/arch/arm/domain.c  | 6 +-----
>  xen/arch/arm/vgic-v3.c | 2 +-
>  xen/arch/arm/vpsci.c   | 2 +-
>  3 files changed, 3 insertions(+), 7 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..1c1e7de 100644
> --- a/xen/arch/arm/vpsci.c
> +++ b/xen/arch/arm/vpsci.c
> @@ -33,7 +33,7 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
>      register_t vcpuid;
>  
>      if( ver == XEN_PSCI_V_0_2 )
> -        vcpuid = (target_cpu & MPIDR_HWID_MASK);
> +        vcpuid = vaffinity_to_vcpuid(target_cpu);
>      else
>          vcpuid = target_cpu;

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.

Please retain my comment in the next version (maybe after ---) in order
to avoid pushing the code without any double confirmation...

Regards,

-- 
Julien Grall

[PATCH V4 4/8] 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        | 15 +++++++--------
 xen/include/asm-arm/vgic.h |  2 +-
 4 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 3be1a51..2dbe371 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -33,6 +33,17 @@
 #include <asm/gic.h>
 #include <asm/vgic.h>
 
+static void gicv2_sgir_to_cpumask(cpumask_t *cpumask, const register_t sgir)
+{
+    unsigned long target_list;
+    int cpuid;
+
+    target_list = ((sgir & GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT);
+    for_each_set_bit( cpuid, &target_list, 8 )
+        cpumask_set_cpu(cpuid, cpumask);
+
+}
+
 static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
 {
     struct hsr_dabt dabt = info->dabt;
@@ -201,11 +212,12 @@ 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;
+    gicv2_sgir_to_cpumask(&vcpu_mask, sgir);
 
     /* Map GIC sgi value to enum value */
     switch ( irqmode )
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index ef9a71a..0da031c 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -972,17 +972,30 @@ write_ignore:
     return 1;
 }
 
+static void gicv3_sgir_to_cpumask(cpumask_t *cpumask, const register_t sgir)
+{
+    int target, cpuid;
+    unsigned long target_mask = sgir & ICH_SGI_TARGETLIST_MASK;
+
+    for_each_set_bit( target, &target_mask, 16 )
+    {
+        /* XXX: We assume that only AFF1 is used in ICC_SGI1R_EL1. */
+        cpuid = target + ((sgir >> 16) & 0xff) * 16;
+        cpumask_set_cpu(cpuid, cpumask);
+    }
+}
+
 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;
+    gicv3_sgir_to_cpumask(&vcpu_mask, sgir);
 
     /* Map GIC sgi value to enum value */
     switch ( irqmode )
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 7b387b7..4bf8486 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -318,9 +318,8 @@ 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;
@@ -341,12 +340,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 +354,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,12 +364,12 @@ 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_each_cpu( vcpuid, &vcpu_mask )
     {
         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);
+                    , wrong CPUTargetList\n", sgir);
             continue;
         }
         vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq);
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 6dcdf9f..c27117e 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

Re: [PATCH V4 4/8] xen/arm: Use cpumask_t type for vcpu_mask in vgic_to_sgi

[Julien Grall]

Hi Chen,

On 28/05/15 11:15, 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.

This patch does more than using cpumask_t. It's also add AFF1 support
for GICv3. This should at least be mentioned in the patch (and commit
message). But I would prefer a separate patch to add this feature.

Furthermore, cpumask_t is based on NR_CPUS. There is not relation
between NR_CPUS and MAX_VIRT_CPUS so you need to add a BUILD_BUG_ON in
order to catch wrong configuration.

> 
> 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        | 15 +++++++--------
>  xen/include/asm-arm/vgic.h |  2 +-
>  4 files changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 3be1a51..2dbe371 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -33,6 +33,17 @@
>  #include <asm/gic.h>
>  #include <asm/vgic.h>
>  
> +static void gicv2_sgir_to_cpumask(cpumask_t *cpumask, const register_t sgir)

static inline

> +{
> +    unsigned long target_list;
> +    int cpuid;
> +
> +    target_list = ((sgir & GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT);
> +    for_each_set_bit( cpuid, &target_list, 8 )
> +        cpumask_set_cpu(cpuid, cpumask);

The SGI code is called very often by the guest so we need an optimized code.
As there is already a loop later I'd like to avoid another here.

I think the loop can be replaced with:

bitmap_copy(cpumask_bits(cpumask), &target_list, 8);

The operation will be in O(1).

Also please use a define rather than the hardcoded value 8.

> +
> +}
> +
>  static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
>  {
>      struct hsr_dabt dabt = info->dabt;
> @@ -201,11 +212,12 @@ 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;
> +    gicv2_sgir_to_cpumask(&vcpu_mask, sgir);

This is only necessary to call when the SGI target a list of VCPUs
(GICD_SGI_TARGET_LIST_VAL).

>  
>      /* Map GIC sgi value to enum value */
>      switch ( irqmode )
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index ef9a71a..0da031c 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -972,17 +972,30 @@ write_ignore:
>      return 1;
>  }
>  
> +static void gicv3_sgir_to_cpumask(cpumask_t *cpumask, const register_t sgir)
> +{
> +    int target, cpuid;
> +    unsigned long target_mask = sgir & ICH_SGI_TARGETLIST_MASK;
> +
> +    for_each_set_bit( target, &target_mask, 16 )
> +    {
> +        /* XXX: We assume that only AFF1 is used in ICC_SGI1R_EL1. */
> +        cpuid = target + ((sgir >> 16) & 0xff) * 16;

Please use define rather than constant.

> +        cpumask_set_cpu(cpuid, cpumask);
> +    }

Same remark as above, the loop can be avoided.

Furthermore you need to validate the AFF1. A malicious guest can decide
to pass AFF1 >= 16 which will give a CPU higher than 128 and potentially
allow the guest to do a stack overflow in Xen.

> +}
> +
>  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;
> +    gicv3_sgir_to_cpumask(&vcpu_mask, sgir);

It's only necessary when the SGI targets a list of VCPU
(ICH_SGI_TARGET_LIST).

>  
>      /* Map GIC sgi value to enum value */
>      switch ( irqmode )
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 7b387b7..4bf8486 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -318,9 +318,8 @@ 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)

You need to pass a pointer here, otherwise you will copy all the
cpumask_t everytime which is very expensive.

>  {
>      struct domain *d = v->domain;
>      int vcpuid;

The BUG_ON below can be removed. Keeping here is a call to a potential
unwanted crash of Xen by the guest.

> @@ -341,12 +340,12 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int

>          break;
>      case SGI_TARGET_SELF:
> @@ -355,8 +354,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,12 +364,12 @@ 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_each_cpu( vcpuid, &vcpu_mask )

Is there any potential performance impact? You are moving from a loop of
d->max_vcpus to 128 (and potentially more).

>      {
>          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);
> +                    , wrong CPUTargetList\n", sgir);
>              continue;
>          }

Regards,

-- 
Julien Grall

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

Re: [PATCH V4 5/8] tools/libxl: Set 'reg' of cpu node equal to MPIDR affinity for domU

[Julien Grall]

Hi Chen,

On 28/05/15 11:15, 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 (uint64_t) is not necessary.

> +        name = GCSPRINTF("cpu@%lx", mpidr_aff);

It's not necessary to change the cpu@.

Regards,

-- 
Julien Grall

Re: [PATCH V4 5/8] tools/libxl: Set 'reg' of cpu node equal to MPIDR affinity for domU

[Ian Campbell]

On Fri, 2015-05-29 at 16:44 +0100, Julien Grall wrote:

> > +        name = GCSPRINTF("cpu@%lx", mpidr_aff);
> 
> It's not necessary to change the cpu@.

AIUI it is conventional in DT for this to match the first reg entry.

Ian.

> 
> Regards,
> 

Re: [PATCH V4 5/8] tools/libxl: Set 'reg' of cpu node equal to MPIDR affinity for domU

[Julien Grall]

On 29/05/15 16:55, Ian Campbell wrote:
> On Fri, 2015-05-29 at 16:44 +0100, Julien Grall wrote:
> 
>>> +        name = GCSPRINTF("cpu@%lx", mpidr_aff);
>>
>> It's not necessary to change the cpu@.
> 
> AIUI it is conventional in DT for this to match the first reg entry.

Well, it's conventional when the reg field is containing an address.

It's not the case of the cpu node as "reg" doesn't contain an address.
There is a mix of both in the different DTS.

Although, increment an ID make easier to read the dumped DTS for debugging.

Regards,

-- 
Julien Grall

Re: [PATCH V4 5/8] tools/libxl: Set 'reg' of cpu node equal to MPIDR affinity for domU

[Chen Baozi]

Hi Julien,

On Fri, May 29, 2015 at 05:08:08PM +0100, Julien Grall wrote:
> On 29/05/15 16:55, Ian Campbell wrote:
> > On Fri, 2015-05-29 at 16:44 +0100, Julien Grall wrote:
> > 
> >>> +        name = GCSPRINTF("cpu@%lx", mpidr_aff);
> >>
> >> It's not necessary to change the cpu@.
> > 
> > AIUI it is conventional in DT for this to match the first reg entry.
> 
> Well, it's conventional when the reg field is containing an address.
> 
> It's not the case of the cpu node as "reg" doesn't contain an address.
> There is a mix of both in the different DTS.
> 
> Although, increment an ID make easier to read the dumped DTS for debugging.

I did the change because it would be more friendly for user to read the
cpu node in /proc/device-tree/. Because the number is no longer continuous
when we have a guest more than 16 cpus. In that case, the cpu nodes will be
looked like as the following:

cpu@0, cpu@1, ..., cpu@15, cpu@256, cpu@257, ..., cpu@271, cpu@512, ...

Considering changing to the new form doesn't introduce new risk (and follows
the convention to a certain degree), I prefer the new way.

Cheers,

Baozi.

Re: [PATCH V4 5/8] tools/libxl: Set 'reg' of cpu node equal to MPIDR affinity for domU

[Chen Baozi]

On Sat, May 30, 2015 at 10:08:21AM +0800, Chen Baozi wrote:
> Hi Julien,
> 
> On Fri, May 29, 2015 at 05:08:08PM +0100, Julien Grall wrote:
> > On 29/05/15 16:55, Ian Campbell wrote:
> > > On Fri, 2015-05-29 at 16:44 +0100, Julien Grall wrote:
> > > 
> > >>> +        name = GCSPRINTF("cpu@%lx", mpidr_aff);
> > >>
> > >> It's not necessary to change the cpu@.
> > > 
> > > AIUI it is conventional in DT for this to match the first reg entry.
> > 
> > Well, it's conventional when the reg field is containing an address.
> > 
> > It's not the case of the cpu node as "reg" doesn't contain an address.
> > There is a mix of both in the different DTS.
> > 
> > Although, increment an ID make easier to read the dumped DTS for debugging.
> 
> I did the change because it would be more friendly for user to read the
> cpu node in /proc/device-tree/. Because the number is no longer continuous
> when we have a guest more than 16 cpus. In that case, the cpu nodes will be
> looked like as the following:
> 
> cpu@0, cpu@1, ..., cpu@15, cpu@256, cpu@257, ..., cpu@271, cpu@512, ...

Ah, I misundertood your comment. I think what you mean here is to keep the
name as "cpu@i" not about the form of the "cpu@%d".

However, I still prefer the new form :)

Cheers,

Baozi.

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

Re: [PATCH V4 6/8] xen/arm: Set 'reg' of cpu node for dom0 to match MPIDR's affinity

[Julien Grall]

Hi Chen,

On 28/05/15 11:15, 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;
>  
>      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);

I would print both the "logical CPUID" and the MPIDR. It will be easier
for debugging.

>  
> -        snprintf(buf, sizeof(buf), "cpu@%u", cpu);
> +        snprintf(buf, sizeof(buf), "cpu@%x", mpidr_aff);

Changing cpu@ is not necessary.

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

Regards,


-- 
Julien Grall

Re: [PATCH V4 6/8] xen/arm: Set 'reg' of cpu node for dom0 to match MPIDR's affinity

[Chen Baozi]

On Fri, May 29, 2015 at 04:49:42PM +0100, Julien Grall wrote:
> Hi Chen,
> 
> On 28/05/15 11:15, 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;
> >  
> >      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);
> 
> I would print both the "logical CPUID" and the MPIDR. It will be easier
> for debugging.

Ok.

> 
> >  
> > -        snprintf(buf, sizeof(buf), "cpu@%u", cpu);
> > +        snprintf(buf, sizeof(buf), "cpu@%x", mpidr_aff);
> 
> Changing cpu@ is not necessary.

See my previous comment about this.

Cheers,

Baozi.

[PATCH V4 7/8] 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.

Signed-off-by: Chen Baozi <baozich@gmail.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 0da031c..be5fff1 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 V4 7/8] xen/arm64: increase MAX_VIRT_CPUS to 128 on arm64

[Julien Grall]

Hi Chen,

On 28/05/15 11:15, 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.

This should be the last patch. You still have to modify domain_max_vcpus
before being able to increase the number of vCPUS handled.

> Signed-off-by: Chen Baozi <baozich@gmail.com>

with the patch at the end of the series:

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 0da031c..be5fff1 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 */
>  
> 

Regards,

-- 
Julien Grall

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

[Julien Grall]

On 29/05/15 16:51, Julien Grall wrote:
>> 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

Looking to the last patch, the usage of MAX_VIRT_CPUS is now minimal.
Can't finish to replace MAX_VIRT_CPUS to another corresponding value and
drop the define?

Regards,

-- 
Julien Grall

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

[Andrew Cooper]

On 29/05/15 17:20, Julien Grall wrote:
> On 29/05/15 16:51, Julien Grall wrote:
>>> 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
> Looking to the last patch, the usage of MAX_VIRT_CPUS is now minimal.
> Can't finish to replace MAX_VIRT_CPUS to another corresponding value and
> drop the define?

You cant drop MAX_VIRT_CPUS (I tried this when introducing
max_domain_vcpus()). It is used for some conditional compilation in
common code.

~Andrew

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

[Julien Grall]

On 29/05/15 17:41, Andrew Cooper wrote:
> On 29/05/15 17:20, Julien Grall wrote:
>> On 29/05/15 16:51, Julien Grall wrote:
>>>> 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
>> Looking to the last patch, the usage of MAX_VIRT_CPUS is now minimal.
>> Can't finish to replace MAX_VIRT_CPUS to another corresponding value and
>> drop the define?
> 
> You cant drop MAX_VIRT_CPUS (I tried this when introducing
> max_domain_vcpus()). It is used for some conditional compilation in
> common code.

AFAICT only in the event channel code to avoid allocating memory when
less than 64 vCPU is used.

Anyway, if we can drop it. I would add a check in domain_max_vcpus for
safety.

Regards,

-- 
Julien Grall

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

[Andrew Cooper]

On 29/05/15 17:45, Julien Grall wrote:
> On 29/05/15 17:41, Andrew Cooper wrote:
>> On 29/05/15 17:20, Julien Grall wrote:
>>> On 29/05/15 16:51, Julien Grall wrote:
>>>>> 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
>>> Looking to the last patch, the usage of MAX_VIRT_CPUS is now minimal.
>>> Can't finish to replace MAX_VIRT_CPUS to another corresponding value and
>>> drop the define?
>> You cant drop MAX_VIRT_CPUS (I tried this when introducing
>> max_domain_vcpus()). It is used for some conditional compilation in
>> common code.
> AFAICT only in the event channel code to avoid allocating memory when
> less than 64 vCPU is used.

Correct.

>
> Anyway, if we can drop it. I would add a check in domain_max_vcpus for
> safety.

It has a second use currently for auditing the dom0_max_vcpus parameter
(for both x86 and ARM), but could easily be replaced with a
max_domain_vcpus() call.

~Andrew

[PATCH V4 8/8] xen/arm: make domain_max_vcpus return value according to vGIC version

[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
version 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        | 6 ++++++
 xen/include/asm-arm/domain.h | 5 +----
 xen/include/asm-arm/gic.h    | 3 +++
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 0cf147c..78b77b1 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -890,6 +890,12 @@ 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.version == GIC_V2) ?
+            GICV2_MAX_CPUS : GICV3_MAX_CPUS);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 603a20b..c4cb15d 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 463fffb..a7bc0d1 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
-- 
2.1.4


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

Re: [PATCH V4 8/8] xen/arm: make domain_max_vcpus return value according to vGIC version

[Andrew Cooper]

On 28/05/15 11:15, 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
> version 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>

Looks much better!

It is a shame about the compilation issue - I had a similar issue on the
x86 side.  At some point, I need to untangle the rats nest which is our
currently header file layout, so that we don't have to rely on tricks
like this to keep it compiling.

~Andrew

> ---
>  xen/arch/arm/domain.c        | 6 ++++++
>  xen/include/asm-arm/domain.h | 5 +----
>  xen/include/asm-arm/gic.h    | 3 +++
>  3 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 0cf147c..78b77b1 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -890,6 +890,12 @@ 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.version == GIC_V2) ?
> +            GICV2_MAX_CPUS : GICV3_MAX_CPUS);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 603a20b..c4cb15d 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 463fffb..a7bc0d1 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


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

Re: [PATCH V4 8/8] xen/arm: make domain_max_vcpus return value according to vGIC version

[Julien Grall]

Hi Chen,

On 28/05/15 11:15, 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
> version 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        | 6 ++++++
>  xen/include/asm-arm/domain.h | 5 +----
>  xen/include/asm-arm/gic.h    | 3 +++
>  3 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 0cf147c..78b77b1 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -890,6 +890,12 @@ 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.version == GIC_V2) ?
> +            GICV2_MAX_CPUS : GICV3_MAX_CPUS);

We try to get the common code vgic agnostic. With this solution every
time a new vGIC driver is added you have to modify domain_max_vcpus.

As suggested on a previous version, I would prefer to extend the
vgic_ops to store the maximum number of VCPU handled by the vGIC.

Regards,

-- 
Julien Grall

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