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

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

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

[Sorry for the incorrect list address previously.]

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.

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/arm64: increase MAX_VIRT_CPUS to 128 on arm64
  xen/arm: gic-v3: Increase the size of GICR in address space for guest
  xen/arm: Add funtions of mapping between vCPUID and vMPIDR
  xen/arm: Use the new mapping relations between vCPUID and vMPIDR
  xen/arm: vGIC: Consider AFF1 when injecting SGI.
  tools/libxl: Make DT node of GICv3 according to max_vcpus
  tools/libxl: Set logical CPUID in DT node equal to MPIDR for domU
  xen/arm: Set logical CPUID in DT node for dom0 the same as MPIDR

 tools/libxl/libxl_arm.c       | 17 ++++++++++++-----
 xen/arch/arm/domain.c         |  6 +-----
 xen/arch/arm/domain_build.c   | 11 ++++++++---
 xen/arch/arm/vgic-v3.c        | 24 ++++++++----------------
 xen/arch/arm/vgic.c           | 10 ++++++++--
 xen/arch/arm/vpsci.c          |  2 +-
 xen/include/asm-arm/config.h  |  4 ++++
 xen/include/asm-arm/domain.h  | 34 ++++++++++++++++++++++++++++++++++
 xen/include/public/arch-arm.h |  4 ++--
 9 files changed, 78 insertions(+), 34 deletions(-)

-- 
2.1.4

[PATCH V2 1/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       | 2 +-
 xen/include/asm-arm/config.h | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 45d54a2..40e1892 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -906,7 +906,7 @@ 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);
+        BUG_ON(v->domain->max_vcpus > MAX_VIRT_CPUS);
         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 V2 1/8] xen/arm64: increase MAX_VIRT_CPUS to 128 on arm64

[Julien Grall]

Hi Chen,

On 23/05/2015 14:52, 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 series have to be bisectable. Although, this patch will break 
compilation on ARM64 because you increased MAX_VIRT_CPUS without the 
re-distributor region (done in the next patch).

Furthermore, GICv2 is only supporting 8 CPUs. We don't have to allow a 
guest with more than 8 vCPUs when the GICv2 is in use.

Lastly, given that the support of 128 vCPUs will only worked when the 
last patch of this series is applied, I would move this patch at the end.

> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>   xen/arch/arm/vgic-v3.c       | 2 +-
>   xen/include/asm-arm/config.h | 4 ++++
>   2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 45d54a2..40e1892 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -906,7 +906,7 @@ 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);
> +        BUG_ON(v->domain->max_vcpus > MAX_VIRT_CPUS);

This check is pointless. max_vcpus will always be inferior or equal to 
MAX_VIRT_CPUS.

>           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 V2 1/8] xen/arm64: increase MAX_VIRT_CPUS to 128 on arm64

[Chen Baozi]

On Sat, May 23, 2015 at 03:46:32PM +0100, Julien Grall wrote:
> Hi Chen,
> 
> On 23/05/2015 14:52, 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 series have to be bisectable. Although, this patch will break
> compilation on ARM64 because you increased MAX_VIRT_CPUS without the
> re-distributor region (done in the next patch).
> 
> Furthermore, GICv2 is only supporting 8 CPUs. We don't have to allow a guest
> with more than 8 vCPUs when the GICv2 is in use.

What's your suggestion? I don't think we could know whether it is built for
a GICv2 or a GICv3 machine and define different values for corresponding
target.

Cheers,

Baozi.

> 
> Lastly, given that the support of 128 vCPUs will only worked when the last
> patch of this series is applied, I would move this patch at the end.
> 
> >Signed-off-by: Chen Baozi <baozich@gmail.com>
> >---
> >  xen/arch/arm/vgic-v3.c       | 2 +-
> >  xen/include/asm-arm/config.h | 4 ++++
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> >
> >diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> >index 45d54a2..40e1892 100644
> >--- a/xen/arch/arm/vgic-v3.c
> >+++ b/xen/arch/arm/vgic-v3.c
> >@@ -906,7 +906,7 @@ 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);
> >+        BUG_ON(v->domain->max_vcpus > MAX_VIRT_CPUS);
> 
> This check is pointless. max_vcpus will always be inferior or equal to
> MAX_VIRT_CPUS.
> 
> >          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 V2 1/8] xen/arm64: increase MAX_VIRT_CPUS to 128 on arm64

[Julien Grall]

Hi Chen,

On 25/05/2015 03:01, Chen Baozi wrote:
> On Sat, May 23, 2015 at 03:46:32PM +0100, Julien Grall wrote:
>> On 23/05/2015 14:52, 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 series have to be bisectable. Although, this patch will break
>> compilation on ARM64 because you increased MAX_VIRT_CPUS without the
>> re-distributor region (done in the next patch).
>>
>> Furthermore, GICv2 is only supporting 8 CPUs. We don't have to allow a guest
>> with more than 8 vCPUs when the GICv2 is in use.
>
> What's your suggestion? I don't think we could know whether it is built for
> a GICv2 or a GICv3 machine and define different values for corresponding
> target.

I'm not sure to understand your problem here. You know which vGIC 
version is emulated for a specific domain at domain creation time. The 
function domain_max_vcpus returns the maximum vCPUs for a give domain.

You could create a new field in vgic_v2_ops to store the maximum number 
of vCPU handled.

domain_max_vcpus would look like:

return (min(d->arch->vgic.handler.max_vcpus, MAX_VIRT_CPUS));

Regards,

-- 
-- 
Julien Grall

[PATCH V2 2/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 maxium number that GIC-500 supports.

Signed-off-by: Chen Baozi <baozich@gmail.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

Re: [PATCH V2 2/8] xen/arm: gic-v3: Increase the size of GICR in address space for guest

[Julien Grall]

Hi Chen,

On 23/05/2015 14:52, Chen Baozi wrote:
> 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 maxium number that GIC-500 supports.
>
> Signed-off-by: Chen Baozi <baozich@gmail.com>

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

Regards,

-- 
Julien Grall

[PATCH V2 3/8] xen/arm: Add funtions of mapping between vCPUID and vMPIDR

[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 vMPIDR.

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

diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 75b17af..9d3e406 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -266,6 +266,40 @@ 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: 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 = (vaff >> MPIDR_LEVEL_SHIFT(0)) & 0x0f;
+    vcpuid |= ((vaff >> MPIDR_LEVEL_SHIFT(1)) & 0xff) << 4;
+
+    return vcpuid;
+}
+
+static inline register_t vcpuid_to_vaffinity(unsigned int vcpuid)
+{
+    register_t vaff;
+
+    vaff = (vcpuid & 0x0f) << MPIDR_LEVEL_SHIFT(0);
+    vaff |= ((vcpuid >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
+
+    return vaff;
+}
+
 #endif /* __ASM_DOMAIN_H__ */
 
 /*
-- 
2.1.4

Re: [PATCH V2 3/8] xen/arm: Add funtions of mapping between vCPUID and vMPIDR

[Julien Grall]

Hi Chen,

Title: s/funtions/functions/

On 23/05/2015 14:52, 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 vMPIDR.

The functions you are adding don't deal with the vMPIDR but only a part 
of it used for the affinity.

I would rename the title and modify this last sentence to reflect it.

> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>   xen/include/asm-arm/domain.h | 34 ++++++++++++++++++++++++++++++++++
>   1 file changed, 34 insertions(+)
>
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 75b17af..9d3e406 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -266,6 +266,40 @@ 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: May have multi-threading or virtual cluster information in the

We may have ...

> + * future.
> + */
> +static inline unsigned int vaffinity_to_vcpuid(register_t vaff)
> +{
> +    unsigned int vcpuid;
> +
> +    vaff &= MPIDR_HWID_MASK;
> +
> +    vcpuid = (vaff >> MPIDR_LEVEL_SHIFT(0)) & 0x0f;

You can use MPIDR_AFFINITY_LEVEL(0)

> +    vcpuid |= ((vaff >> MPIDR_LEVEL_SHIFT(1)) & 0xff) << 4;

Same here with 1.

> +    return vcpuid;
> +}
> +
> +static inline register_t vcpuid_to_vaffinity(unsigned int vcpuid)
> +{
> +    register_t vaff;

I would add a BUILD_BUG_ON(MAX_VIRT_CPUS < ((1 << 12))) in order to 
catch MAX_VIRT_CPUS increasing without changing the mapping between the 
VCPU ID and the affinity.

> +    vaff = (vcpuid & 0x0f) << MPIDR_LEVEL_SHIFT(0);
> +    vaff |= ((vcpuid >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);

s/0xff/MPIDR_LEVEL_MASK/

> +
> +    return vaff;
> +}
> +
>   #endif /* __ASM_DOMAIN_H__ */
>
>   /*
>

Regards,

-- 
Julien Grall

[PATCH V2 4/8] xen/arm: Use the new mapping relations between vCPUID and vMPIDR

[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 vMPIRR 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 | 22 +++++++---------------
 xen/arch/arm/vpsci.c   |  2 +-
 3 files changed, 9 insertions(+), 21 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 40e1892..12007d8 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -50,14 +50,6 @@
  */
 #define VGICD_CTLR_DEFAULT  (GICD_CTLR_ARE_NS)
 
-static struct vcpu *vgic_v3_irouter_to_vcpu(struct vcpu *v, uint64_t irouter)
-{
-    irouter &= ~(GICD_IROUTER_SPI_MODE_ANY);
-    irouter = irouter & MPIDR_AFF0_MASK;
-
-    return v->domain->vcpu[irouter];
-}
-
 static uint64_t vgic_v3_vcpu_to_irouter(struct vcpu *v,
                                         unsigned int vcpu_id)
 {
@@ -80,9 +72,7 @@ static struct vcpu *vgic_v3_get_target_vcpu(struct vcpu *v, unsigned int irq)
 
     ASSERT(spin_is_locked(&rank->lock));
 
-    target = rank->v3.irouter[irq % 32];
-    target &= ~(GICD_IROUTER_SPI_MODE_ANY);
-    target &= MPIDR_AFF0_MASK;
+    target = vaffinity_to_vcpuid(rank->v3.irouter[irq % 32]);
     ASSERT(target >= 0 && target < v->domain->max_vcpus);
 
     return v->domain->vcpu[target];
@@ -751,7 +741,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
             vgic_unlock_rank(v, rank, flags);
             return 1;
         }
-        vcpu_id = irouter;
+        vcpu_id = vaffinity_to_vcpuid(irouter);
         *r = vgic_v3_vcpu_to_irouter(v, vcpu_id);
         vgic_unlock_rank(v, rank, flags);
         return 1;
@@ -841,6 +831,7 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
     uint64_t new_irouter, new_target, old_target;
     struct vcpu *old_vcpu, *new_vcpu;
     int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
+    uint32_t vcpu_id;
 
     perfc_incr(vgicd_writes);
 
@@ -925,8 +916,9 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         }
         else
         {
-            new_target = new_irouter & MPIDR_AFF0_MASK;
-            if ( new_target >= v->domain->max_vcpus )
+            new_target = new_irouter & MPIDR_HWID_MASK;
+            vcpu_id = vaffinity_to_vcpuid(new_irouter);
+            if ( vcpu_id >= v->domain->max_vcpus )
             {
                 printk(XENLOG_G_DEBUG
                        "%pv: vGICD: wrong irouter at offset %#08x\n val 0x%lx vcpu %x",
@@ -934,7 +926,7 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
                 vgic_unlock_rank(v, rank, flags);
                 return 0;
             }
-            new_vcpu = vgic_v3_irouter_to_vcpu(v, new_irouter);
+            new_vcpu = v->domain->vcpu[vcpu_id];
         }
 
         rank->v3.irouter[REG_RANK_INDEX(64, (gicd_reg - GICD_IROUTER),
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 V2 4/8] xen/arm: Use the new mapping relations between vCPUID and vMPIDR

[Julien Grall]

Hi Chen,

On 23/05/2015 14:52, 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 vMPIRR affinity information when booting with vPSCI

s/VMPIRR/vMPIDR/

>    in vGIC
>
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>   xen/arch/arm/domain.c  |  6 +-----
>   xen/arch/arm/vgic-v3.c | 22 +++++++---------------
>   xen/arch/arm/vpsci.c   |  2 +-
>   3 files changed, 9 insertions(+), 21 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 40e1892..12007d8 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -50,14 +50,6 @@
>    */
>   #define VGICD_CTLR_DEFAULT  (GICD_CTLR_ARE_NS)
>
> -static struct vcpu *vgic_v3_irouter_to_vcpu(struct vcpu *v, uint64_t irouter)
> -{
> -    irouter &= ~(GICD_IROUTER_SPI_MODE_ANY);
> -    irouter = irouter & MPIDR_AFF0_MASK;
> -
> -    return v->domain->vcpu[irouter];
> -}
> -
>   static uint64_t vgic_v3_vcpu_to_irouter(struct vcpu *v,
>                                           unsigned int vcpu_id)
>   {
> @@ -80,9 +72,7 @@ static struct vcpu *vgic_v3_get_target_vcpu(struct vcpu *v, unsigned int irq)
>
>       ASSERT(spin_is_locked(&rank->lock));
>
> -    target = rank->v3.irouter[irq % 32];
> -    target &= ~(GICD_IROUTER_SPI_MODE_ANY);
> -    target &= MPIDR_AFF0_MASK;
> +    target = vaffinity_to_vcpuid(rank->v3.irouter[irq % 32]);

When irouter.IRM  = 1 (i.e any processor can be used for SPIs), the 
affinity may be unknown.

Although, when this register is saved we make sure to have AFF0 and AFF1 
set to 0.

This change, as the current wasn't clear about it. I would be tempt to 
add a specific case for irouter.IRM = 1. But I don't mind if you only 
add a comment.

>       ASSERT(target >= 0 && target < v->domain->max_vcpus);
>
>       return v->domain->vcpu[target];
> @@ -751,7 +741,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
>               vgic_unlock_rank(v, rank, flags);
>               return 1;
>           }
> -        vcpu_id = irouter;
> +        vcpu_id = vaffinity_to_vcpuid(irouter);
>           *r = vgic_v3_vcpu_to_irouter(v, vcpu_id);

The current code is very pointless, irouter contains the value to 
return. vgic_v3_vcpu_to_irouter is just an identity function.

The read emulation for IROUTER can be simplify a lot to only returns the 
value irouter which is already valid.

I can send a patch to apply before your series to clean up this IROUTER 
code. I would make unnecessary some of your changes.

>           vgic_unlock_rank(v, rank, flags);
>           return 1;
> @@ -841,6 +831,7 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>       uint64_t new_irouter, new_target, old_target;
>       struct vcpu *old_vcpu, *new_vcpu;
>       int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
> +    uint32_t vcpu_id;
>
>       perfc_incr(vgicd_writes);
>
> @@ -925,8 +916,9 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>           }
>           else
>           {
> -            new_target = new_irouter & MPIDR_AFF0_MASK;
> -            if ( new_target >= v->domain->max_vcpus )
> +            new_target = new_irouter & MPIDR_HWID_MASK;
> +            vcpu_id = vaffinity_to_vcpuid(new_irouter);
> +            if ( vcpu_id >= v->domain->max_vcpus )
>               {
>                   printk(XENLOG_G_DEBUG
>                          "%pv: vGICD: wrong irouter at offset %#08x\n val 0x%lx vcpu %x",
> @@ -934,7 +926,7 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>                   vgic_unlock_rank(v, rank, flags);
>                   return 0;
>               }
> -            new_vcpu = vgic_v3_irouter_to_vcpu(v, new_irouter);

I would prefer to keep vgic_v3_irouter_to_vcpu and return NULL if the 
VCPU ID is too high.

The emulation code would be:

      new_vcpu = vgic_v3_irouter_to_vcpu(v, new_irouter);
      if ( !new_vcpu )
      {
         printk(.....);
      }

Although the current emulation is wrong, if the guest is passing a wrong 
MPIDR, we should just ignore the setting and let the interrupt going 
pending. Anyway, I think it would require more work in Xen so I'm okay 
with the current behavior.

> +            new_vcpu = v->domain->vcpu[vcpu_id];
>           }
>
>           rank->v3.irouter[REG_RANK_INDEX(64, (gicd_reg - GICD_IROUTER),
> 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;

AFAICT in PSCI 0.1, target_cpu is a CPUID which is a MPIDR-like value. 
If so, I think we may need to call vaffinity_to_vcpuid.

But, I wasn't able to confirm with the spec. I guessed it from the Linux 
usage. Maybe there is limit of number of CPU used with PSCI 0.1?

Regards,

-- 
Julien Grall

Re: [PATCH V2 4/8] xen/arm: Use the new mapping relations between vCPUID and vMPIDR

[Chen Baozi]

On Sun, May 24, 2015 at 01:51:21PM +0100, Julien Grall wrote:
> Hi Chen,
> 
> On 23/05/2015 14:52, 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 vMPIRR affinity information when booting with vPSCI
> 
> s/VMPIRR/vMPIDR/
> 
> >   in vGIC
> >
> >Signed-off-by: Chen Baozi <baozich@gmail.com>
> >---
> >  xen/arch/arm/domain.c  |  6 +-----
> >  xen/arch/arm/vgic-v3.c | 22 +++++++---------------
> >  xen/arch/arm/vpsci.c   |  2 +-
> >  3 files changed, 9 insertions(+), 21 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 40e1892..12007d8 100644
> >--- a/xen/arch/arm/vgic-v3.c
> >+++ b/xen/arch/arm/vgic-v3.c
> >@@ -50,14 +50,6 @@
> >   */
> >  #define VGICD_CTLR_DEFAULT  (GICD_CTLR_ARE_NS)
> >
> >-static struct vcpu *vgic_v3_irouter_to_vcpu(struct vcpu *v, uint64_t irouter)
> >-{
> >-    irouter &= ~(GICD_IROUTER_SPI_MODE_ANY);
> >-    irouter = irouter & MPIDR_AFF0_MASK;
> >-
> >-    return v->domain->vcpu[irouter];
> >-}
> >-
> >  static uint64_t vgic_v3_vcpu_to_irouter(struct vcpu *v,
> >                                          unsigned int vcpu_id)
> >  {
> >@@ -80,9 +72,7 @@ static struct vcpu *vgic_v3_get_target_vcpu(struct vcpu *v, unsigned int irq)
> >
> >      ASSERT(spin_is_locked(&rank->lock));
> >
> >-    target = rank->v3.irouter[irq % 32];
> >-    target &= ~(GICD_IROUTER_SPI_MODE_ANY);
> >-    target &= MPIDR_AFF0_MASK;
> >+    target = vaffinity_to_vcpuid(rank->v3.irouter[irq % 32]);
> 
> When irouter.IRM  = 1 (i.e any processor can be used for SPIs), the affinity
> may be unknown.
> 
> Although, when this register is saved we make sure to have AFF0 and AFF1 set
> to 0.
> 
> This change, as the current wasn't clear about it. I would be tempt to add a
> specific case for irouter.IRM = 1. But I don't mind if you only add a
> comment.

If we add a specific case for iroute.IRM == 1, then which vcpu it will return?
Will it better to check this bit before calling this function?

> 
> >      ASSERT(target >= 0 && target < v->domain->max_vcpus);
> >
> >      return v->domain->vcpu[target];
> >@@ -751,7 +741,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
> >              vgic_unlock_rank(v, rank, flags);
> >              return 1;
> >          }
> >-        vcpu_id = irouter;
> >+        vcpu_id = vaffinity_to_vcpuid(irouter);
> >          *r = vgic_v3_vcpu_to_irouter(v, vcpu_id);
> 
> The current code is very pointless, irouter contains the value to return.
> vgic_v3_vcpu_to_irouter is just an identity function.
> 
> The read emulation for IROUTER can be simplify a lot to only returns the
> value irouter which is already valid.
> 
> I can send a patch to apply before your series to clean up this IROUTER
> code. I would make unnecessary some of your changes.

That will be fine.

> 
> >          vgic_unlock_rank(v, rank, flags);
> >          return 1;
> >@@ -841,6 +831,7 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
> >      uint64_t new_irouter, new_target, old_target;
> >      struct vcpu *old_vcpu, *new_vcpu;
> >      int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
> >+    uint32_t vcpu_id;
> >
> >      perfc_incr(vgicd_writes);
> >
> >@@ -925,8 +916,9 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
> >          }
> >          else
> >          {
> >-            new_target = new_irouter & MPIDR_AFF0_MASK;
> >-            if ( new_target >= v->domain->max_vcpus )
> >+            new_target = new_irouter & MPIDR_HWID_MASK;
> >+            vcpu_id = vaffinity_to_vcpuid(new_irouter);
> >+            if ( vcpu_id >= v->domain->max_vcpus )
> >              {
> >                  printk(XENLOG_G_DEBUG
> >                         "%pv: vGICD: wrong irouter at offset %#08x\n val 0x%lx vcpu %x",
> >@@ -934,7 +926,7 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
> >                  vgic_unlock_rank(v, rank, flags);
> >                  return 0;
> >              }
> >-            new_vcpu = vgic_v3_irouter_to_vcpu(v, new_irouter);
> 
> I would prefer to keep vgic_v3_irouter_to_vcpu and return NULL if the VCPU
> ID is too high.
> 
> The emulation code would be:
> 
>      new_vcpu = vgic_v3_irouter_to_vcpu(v, new_irouter);
>      if ( !new_vcpu )
>      {
>         printk(.....);
>      }
> 
> Although the current emulation is wrong, if the guest is passing a wrong
> MPIDR, we should just ignore the setting and let the interrupt going
> pending. Anyway, I think it would require more work in Xen so I'm okay with
> the current behavior.
> 
> >+            new_vcpu = v->domain->vcpu[vcpu_id];
> >          }
> >
> >          rank->v3.irouter[REG_RANK_INDEX(64, (gicd_reg - GICD_IROUTER),
> >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;
> 
> AFAICT in PSCI 0.1, target_cpu is a CPUID which is a MPIDR-like value. If
> so, I think we may need to call vaffinity_to_vcpuid.
> 
> But, I wasn't able to confirm with the spec. I guessed it from the Linux
> usage. Maybe there is limit of number of CPU used with PSCI 0.1?

I didn't read the spec, just change the code according the current
'& MPIDR_HWID_MASK' code.

Cheers,

Baozi.

Re: [PATCH V2 4/8] xen/arm: Use the new mapping relations between vCPUID and vMPIDR

[Julien Grall]

Hi Chen,

On 25/05/2015 04:34, Chen Baozi wrote:
> On Sun, May 24, 2015 at 01:51:21PM +0100, Julien Grall wrote:
>>> @@ -80,9 +72,7 @@ static struct vcpu *vgic_v3_get_target_vcpu(struct vcpu *v, unsigned int irq)
>>>
>>>       ASSERT(spin_is_locked(&rank->lock));
>>>
>>> -    target = rank->v3.irouter[irq % 32];
>>> -    target &= ~(GICD_IROUTER_SPI_MODE_ANY);
>>> -    target &= MPIDR_AFF0_MASK;
>>> +    target = vaffinity_to_vcpuid(rank->v3.irouter[irq % 32]);
>>
>> When irouter.IRM  = 1 (i.e any processor can be used for SPIs), the affinity
>> may be unknown.
>>
>> Although, when this register is saved we make sure to have AFF0 and AFF1 set
>> to 0.
>>
>> This change, as the current wasn't clear about it. I would be tempt to add a
>> specific case for irouter.IRM = 1. But I don't mind if you only add a
>> comment.
>
> If we add a specific case for iroute.IRM == 1, then which vcpu it will return?

You can choose any vCPU. Although, vCPU0 is the best one because it is 
always allocated and running.

> Will it better to check this bit before calling this function?

No, this function is called in generic code to get the VCPU target for a 
given interrupt.

>>
>>>       ASSERT(target >= 0 && target < v->domain->max_vcpus);
>>>
>>>       return v->domain->vcpu[target];
>>> @@ -751,7 +741,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
>>>               vgic_unlock_rank(v, rank, flags);
>>>               return 1;
>>>           }
>>> -        vcpu_id = irouter;
>>> +        vcpu_id = vaffinity_to_vcpuid(irouter);
>>>           *r = vgic_v3_vcpu_to_irouter(v, vcpu_id);
>>
>> The current code is very pointless, irouter contains the value to return.
>> vgic_v3_vcpu_to_irouter is just an identity function.
>>
>> The read emulation for IROUTER can be simplify a lot to only returns the
>> value irouter which is already valid.
>>
>> I can send a patch to apply before your series to clean up this IROUTER
>> code. I would make unnecessary some of your changes.
>
> That will be fine.

I will give a try to send one by tomorrow.

>>
>>>           vgic_unlock_rank(v, rank, flags);
>>>           return 1;
>>> @@ -841,6 +831,7 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>>>       uint64_t new_irouter, new_target, old_target;
>>>       struct vcpu *old_vcpu, *new_vcpu;
>>>       int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
>>> +    uint32_t vcpu_id;
>>>
>>>       perfc_incr(vgicd_writes);
>>>
>>> @@ -925,8 +916,9 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>>>           }
>>>           else
>>>           {
>>> -            new_target = new_irouter & MPIDR_AFF0_MASK;
>>> -            if ( new_target >= v->domain->max_vcpus )
>>> +            new_target = new_irouter & MPIDR_HWID_MASK;
>>> +            vcpu_id = vaffinity_to_vcpuid(new_irouter);
>>> +            if ( vcpu_id >= v->domain->max_vcpus )
>>>               {
>>>                   printk(XENLOG_G_DEBUG
>>>                          "%pv: vGICD: wrong irouter at offset %#08x\n val 0x%lx vcpu %x",
>>> @@ -934,7 +926,7 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>>>                   vgic_unlock_rank(v, rank, flags);
>>>                   return 0;
>>>               }
>>> -            new_vcpu = vgic_v3_irouter_to_vcpu(v, new_irouter);
>>
>> I would prefer to keep vgic_v3_irouter_to_vcpu and return NULL if the VCPU
>> ID is too high.
>>
>> The emulation code would be:
>>
>>       new_vcpu = vgic_v3_irouter_to_vcpu(v, new_irouter);
>>       if ( !new_vcpu )
>>       {
>>          printk(.....);
>>       }
>>
>> Although the current emulation is wrong, if the guest is passing a wrong
>> MPIDR, we should just ignore the setting and let the interrupt going
>> pending. Anyway, I think it would require more work in Xen so I'm okay with
>> the current behavior.
>>
>>> +            new_vcpu = v->domain->vcpu[vcpu_id];
>>>           }
>>>
>>>           rank->v3.irouter[REG_RANK_INDEX(64, (gicd_reg - GICD_IROUTER),
>>> 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;
>>
>> AFAICT in PSCI 0.1, target_cpu is a CPUID which is a MPIDR-like value. If
>> so, I think we may need to call vaffinity_to_vcpuid.
>>
>> But, I wasn't able to confirm with the spec. I guessed it from the Linux
>> usage. Maybe there is limit of number of CPU used with PSCI 0.1?
>
> I didn't read the spec, just change the code according the current
> '& MPIDR_HWID_MASK' code.

I wasn't able to find any difference between MPDIR vs CPUID in the spec.

(CC Parth the author of the code).

Regards,


-- 
Julien Grall

[PATCH V2 5/8] xen/arm: vGIC: Consider AFF1 when injecting SGI.

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

Use the AFF1 value of ICC_SGI1R_EL1 when injecting SGI in vGIC,
which expands the number of supported vCPU more than 16 that
target list bitmap can hold independently.

Signed-off-by: Chen Baozi <baozich@gmail.com>
---
 xen/arch/arm/vgic.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 7b387b7..27bd137 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -367,13 +367,19 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
 
     for_each_set_bit( vcpuid, &vcpu_mask, d->max_vcpus )
     {
-        if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) )
+        /*
+         * XXX: We assumes that only AFF1 and target list are used in
+         * ICC_SGI1R_EL1.
+         */
+        int real_id = vcpuid + ((sgir >> 16) & 0xff) * 16;
+
+        if ( d->vcpu[real_id] != NULL && !is_vcpu_online(d->vcpu[real_id]) )
         {
             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);
+        vgic_vcpu_inject_irq(d->vcpu[real_id], virq);
     }
     return 1;
 }
-- 
2.1.4

Re: [PATCH V2 5/8] xen/arm: vGIC: Consider AFF1 when injecting SGI.

[Julien Grall]

Hi Chen,

On 23/05/2015 15:52, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
>
> Use the AFF1 value of ICC_SGI1R_EL1 when injecting SGI in vGIC,
> which expands the number of supported vCPU more than 16 that
> target list bitmap can hold independently.
>
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>   xen/arch/arm/vgic.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 7b387b7..27bd137 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -367,13 +367,19 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
>
>       for_each_set_bit( vcpuid, &vcpu_mask, d->max_vcpus )
>       {
> -        if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) )
> +        /*
> +         * XXX: We assumes that only AFF1 and target list are used in
> +         * ICC_SGI1R_EL1.
> +         */
> +        int real_id = vcpuid + ((sgir >> 16) & 0xff) * 16;
> +

This is not the right way to do it. vgic_to_sgi is common with GICv2 and 
GICv3 and should stay like that.

Even if we take aside the GICv2 problem, the code is buggy as it only 
works when the SGI is sent to a list of VCPUs (i.e SGI_TARGET_LIST).

Overall this should be done in vgic_v3_to_sgi by setting the correct bit 
in vcpu_mask. Although this will require to find another type for 
vcpu_mask as it's only able to store 64 cpus.

Regards,

-- 
Julien Grall

[PATCH V2 6/8] tools/libxl: Make DT node of GICv3 according to max_vcpus

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

Since the size of GICR is determined by the number of CPU
cores, add 'nr_cpus' parameter when creating its DT node
and set gicr0_size dynamically.

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

diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index c5088c4..75d2aed 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -407,13 +407,13 @@ static int make_gicv2_node(libxl__gc *gc, void *fdt,
     return 0;
 }
 
-static int make_gicv3_node(libxl__gc *gc, void *fdt)
+static int make_gicv3_node(libxl__gc *gc, void *fdt, const int nr_cpus)
 {
     int res;
     const uint64_t gicd_base = GUEST_GICV3_GICD_BASE;
     const uint64_t gicd_size = GUEST_GICV3_GICD_SIZE;
     const uint64_t gicr0_base = GUEST_GICV3_GICR0_BASE;
-    const uint64_t gicr0_size = GUEST_GICV3_GICR0_SIZE;
+    const uint64_t gicr0_size = GUEST_GICV3_RDIST_STRIDE * nr_cpus;
     const char *name = GCSPRINTF("interrupt-controller@%"PRIx64, gicd_base);
 
     res = fdt_begin_node(fdt, name);
@@ -640,7 +640,7 @@ next_resize:
                                  GUEST_GICC_BASE, GUEST_GICC_SIZE) );
             break;
         case XEN_DOMCTL_CONFIG_GIC_V3:
-            FDT( make_gicv3_node(gc, fdt) );
+            FDT( make_gicv3_node(gc, fdt, info->max_vcpus) );
             break;
         default:
             LOG(ERROR, "Unknown GIC version %s",
-- 
2.1.4

Re: [PATCH V2 6/8] tools/libxl: Make DT node of GICv3 according to max_vcpus

[Julien Grall]

Hi Chen,

On 23/05/2015 15:52, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
>
> Since the size of GICR is determined by the number of CPU
> cores, add 'nr_cpus' parameter when creating its DT node
> and set gicr0_size dynamically.

This patch is not necessary, the re-distributor region can be bigger 
without any issue. The OS will know the end of the region with 
GICR_TYPER.Last.

Regards,

-- 
Julien Grall

[PATCH V2 7/8] tools/libxl: Set logical CPUID in DT node equal to MPIDR for domU

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

Linux kernel sometimes uses the 'hwid' which is fetched from DT node
of CPU as the MPIDR. We set the logical CPUID in the corresponding DT
node to MPIDR to keep consistency.

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

diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 75d2aed..6026cab 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 cpu_id;
 
     res = fdt_begin_node(fdt, "cpus");
     if (res) return res;
@@ -283,7 +284,13 @@ 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;
+
+        /*
+         * Linux kernel assumes that MPIDR is equal to logical CPUID
+         */
+        cpu_id = (uint64_t)((i & 0x0f) | (((i >> 4) & 0xff) << 8));
+        name = GCSPRINTF("cpu@%lx", cpu_id);
 
         res = fdt_begin_node(fdt, name);
         if (res) return res;
@@ -297,7 +304,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, cpu_id);
         if (res) return res;
 
         res = fdt_end_node(fdt);
-- 
2.1.4

Re: [PATCH V2 7/8] tools/libxl: Set logical CPUID in DT node equal to MPIDR for domU

[Julien Grall]

Hi Chen,

On 23/05/2015 15:52, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
>
> Linux kernel sometimes uses the 'hwid' which is fetched from DT node
> of CPU as the MPIDR. We set the logical CPUID in the corresponding DT
> node to MPIDR to keep consistency.

Hmmm... this is wrong. The field "reg" in the DT contains the affinity 
part of the MPIDR.

The logical CPUID is only an internal representation in Linux which may 
not be equal to the "reg" in the DT.

>
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>   tools/libxl/libxl_arm.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index 75d2aed..6026cab 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 cpu_id;
>
>       res = fdt_begin_node(fdt, "cpus");
>       if (res) return res;
> @@ -283,7 +284,13 @@ 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;
> +
> +        /*
> +         * Linux kernel assumes that MPIDR is equal to logical CPUID

Wrong. See my comment above.

> +         */
> +        cpu_id = (uint64_t)((i & 0x0f) | (((i >> 4) & 0xff) << 8));

You need to add a comment explaining why we handle only AFF0 and AFF1.

> +        name = GCSPRINTF("cpu@%lx", cpu_id);
>
>           res = fdt_begin_node(fdt, name);
>           if (res) return res;
> @@ -297,7 +304,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, cpu_id);
>           if (res) return res;
>
>           res = fdt_end_node(fdt);
>

Regards,

-- 
Julien Grall

[PATCH V2 8/8] xen/arm: Set logical CPUID in DT node for dom0 the same as MPIDR

[Chen Baozi]

From: Chen Baozi <baozich@gmail.com>

This patch does the same thing as the previous one but for dom0 kernel.

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

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index a156de9..28b4f75 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 cpu_id;
 
     DPRINT("Create cpus node\n");
 
@@ -761,9 +762,13 @@ 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);
+        /*
+         * Linux kernel assumes that MPIDR is equal to logical CPUID
+         */
+        cpu_id = vcpuid_to_vaffinity(cpu);
+        DPRINT("Create cpu@%x node\n", cpu_id);
 
-        snprintf(buf, sizeof(buf), "cpu@%u", cpu);
+        snprintf(buf, sizeof(buf), "cpu@%x", cpu_id);
         res = fdt_begin_node(fdt, buf);
         if ( res )
             return res;
@@ -776,7 +781,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", cpu_id);
         if ( res )
             return res;
 
-- 
2.1.4

Re: [PATCH V2 8/8] xen/arm: Set logical CPUID in DT node for dom0 the same as MPIDR

[Julien Grall]

Hi Chen,

On 23/05/2015 15:52, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
>
> This patch does the same thing as the previous one but for dom0 kernel.

Please be explicit, the 2 patches may not be contiguous in Xen upstream.

>
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>   xen/arch/arm/domain_build.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index a156de9..28b4f75 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 cpu_id;
>
>       DPRINT("Create cpus node\n");
>
> @@ -761,9 +762,13 @@ 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);
> +        /*
> +         * Linux kernel assumes that MPIDR is equal to logical CPUID
> +         */

Same remark as patch #7.

> +        cpu_id = vcpuid_to_vaffinity(cpu);
> +        DPRINT("Create cpu@%x node\n", cpu_id);
>
> -        snprintf(buf, sizeof(buf), "cpu@%u", cpu);
> +        snprintf(buf, sizeof(buf), "cpu@%x", cpu_id);
>           res = fdt_begin_node(fdt, buf);
>           if ( res )
>               return res;
> @@ -776,7 +781,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", cpu_id);
>           if ( res )
>               return res;
>
>

Regards,

-- 
Julien Grall