[PATCH] xen/arm: Correctly read the GICv3 Re-Distributor stride

[PATCH] xen/arm: Correctly read the GICv3 Re-Distributor stride

[Shameerali Kolothum Thodi]

The GICv3 driver read a 32 bit value for the re-distributor stride, but
the dts binding is a two-cell property.

All instances of rdist stride access and reference has been modified to
accommodate 64 bit value. The changes are compiled and verified on HiSilicon
Hip05 platform.

Signed-off-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
---
 xen/arch/arm/gic-v3.c        | 9 +++++----
 xen/arch/arm/vgic-v3.c       | 8 ++++----
 xen/include/asm-arm/domain.h | 2 +-
 xen/include/asm-arm/vgic.h   | 2 +-
 4 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index d1db1ce..8003cd6 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -46,7 +46,7 @@
 static struct {
     void __iomem *map_dbase;  /* Mapped address of distributor registers */
     struct rdist_region *rdist_regions;
-    uint32_t  rdist_stride;
+    uint64_t rdist_stride;
     unsigned int rdist_count; /* Number of rdist regions count */
     unsigned int nr_priorities;
     spinlock_t lock;
@@ -1074,7 +1074,7 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
     if ( res )
         return res;

-    res = fdt_property_cell(fdt, "redistributor-stride",
+    res = fdt_property_u64(fdt, "redistributor-stride",
                             d->arch.vgic.rdist_stride);
     if ( res )
         return res;
@@ -1224,7 +1224,8 @@ static int __init gicv3_init(void)
     /* The vGIC code requires the region to be sorted */
     sort(rdist_regs, gicv3.rdist_count, sizeof(*rdist_regs), cmp_rdist, NULL);

-    if ( !dt_property_read_u32(node, "redistributor-stride", &gicv3.rdist_stride) )
+    if ( !dt_property_read_u64(node, "redistributor-stride",
+                &gicv3.rdist_stride) )
         gicv3.rdist_stride = 0;

     gicv3.rdist_regions= rdist_regs;
@@ -1248,7 +1249,7 @@ static int __init gicv3_init(void)
     printk("GICv3 initialization:\n"
            "      gic_dist_addr=%#"PRIpaddr"\n"
            "      gic_maintenance_irq=%u\n"
-           "      gic_rdist_stride=%#x\n"
+           "      gic_rdist_stride=%#lx\n"
            "      gic_rdist_regions=%d\n",
            dbase, gicv3_info.maintenance_irq,
            gicv3.rdist_stride, gicv3.rdist_count);
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index f1c482d..91442c7 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -56,13 +56,13 @@ static struct {
     /* Re-distributor regions */
     unsigned int nr_rdist_regions;
     const struct rdist_region *regions;
-    uint32_t rdist_stride; /* Re-distributor stride */
+    uint64_t rdist_stride; /* Re-distributor stride */
 } vgic_v3_hw;

 void vgic_v3_setup_hw(paddr_t dbase,
                       unsigned int nr_rdist_regions,
                       const struct rdist_region *regions,
-                      uint32_t rdist_stride)
+                      uint64_t rdist_stride)
 {
     vgic_v3_hw.enabled = 1;
     vgic_v3_hw.dbase = dbase;
@@ -598,7 +598,7 @@ static inline struct vcpu *get_vcpu_from_rdist(paddr_t gpa,
                                                uint32_t *offset)
 {
     struct domain *d = v->domain;
-    uint32_t stride = d->arch.vgic.rdist_stride;
+    uint64_t stride = d->arch.vgic.rdist_stride;
     paddr_t base;
     int i, vcpu_id;
     struct vgic_rdist_region *region;
@@ -1084,7 +1084,7 @@ static int vgic_v3_vcpu_init(struct vcpu *v)

     /* Convenient alias */
     struct domain *d = v->domain;
-    uint32_t rdist_stride = d->arch.vgic.rdist_stride;
+    uint64_t rdist_stride = d->arch.vgic.rdist_stride;

     /* For SGI and PPI the target is always this CPU */
     affinity = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 32 |
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index c3f5a95..6d5acff 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -111,7 +111,7 @@ struct arch_domain
             unsigned int first_cpu;         /* First CPU handled */
         } rdist_regions[MAX_RDIST_COUNT];
         int nr_regions;                     /* Number of rdist regions */
-        uint32_t rdist_stride;              /* Re-Distributor stride */
+        uint64_t rdist_stride;              /* Re-Distributor stride */
 #endif
     } vgic;

diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 41cadb1..dcaeeab 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -243,7 +243,7 @@ struct rdist_region;
 void vgic_v3_setup_hw(paddr_t dbase,
                       unsigned int nr_rdist_regions,
                       const struct rdist_region *regions,
-                      uint32_t rdist_stride);
+                      uint64_t rdist_stride);
 #endif

 #endif /* __ASM_ARM_VGIC_H__ */
-- 
1.9.1

Re: [PATCH] xen/arm: Correctly read the GICv3 Re-Distributor stride

[Ian Campbell]

On Wed, 2015-09-30 at 11:54 +0100, Shameerali Kolothum Thodi wrote:
> The GICv3 driver read a 32 bit value for the re-distributor stride, but
> the dts binding is a two-cell property.

The binding doc I have says:

- redistributor-stride : If using padding pages, specifies the stride
  of consecutive redistributors. Must be a multiple of 64kB.

i.e. it doesn't say anything about the size. The _example_ is 2 cells, but
I don't think that is normative.

Unless you can point to a bindings update which specifies strictly two
cells then I think the right answer is the use dt_read_number.

Ian.

Re: [PATCH] xen/arm: Correctly read the GICv3 Re-Distributor stride

[Julien Grall]

On 30/09/15 12:04, Ian Campbell wrote:
> On Wed, 2015-09-30 at 11:54 +0100, Shameerali Kolothum Thodi wrote:
>> The GICv3 driver read a 32 bit value for the re-distributor stride, but
>> the dts binding is a two-cell property.
> 
> The binding doc I have says:
> 
> - redistributor-stride : If using padding pages, specifies the stride
>   of consecutive redistributors. Must be a multiple of 64kB.
> 
> i.e. it doesn't say anything about the size. The _example_ is 2 cells, but
> I don't think that is normative.
> 
> Unless you can point to a bindings update which specifies strictly two
> cells then I think the right answer is the use dt_read_number.

FWIW Linux is using of_property_read_u64 to get the stride. It has
always been the case and I don't understand how we end up to use
dt_property_u32 in Xen as we port the driver from Linux...

So before using dt_read_number in Xen, I would rather check if the DT
binding is not clear enough or the Linux drivers doesn't respect the
binding.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: Correctly read the GICv3 Re-Distributor stride

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: 30 September 2015 12:04
> To: Shameerali Kolothum Thodi; xen-devel@lists.xenproject.org
> Cc: julien.grall@citrix.com; vijaya.kumar@caviumnetworks.com
> Subject: Re: [PATCH] xen/arm: Correctly read the GICv3 Re-Distributor
> stride
> 
> On Wed, 2015-09-30 at 11:54 +0100, Shameerali Kolothum Thodi wrote:
> > The GICv3 driver read a 32 bit value for the re-distributor stride,
> > but the dts binding is a two-cell property.
> 
> The binding doc I have says:
> 
> - redistributor-stride : If using padding pages, specifies the stride
>   of consecutive redistributors. Must be a multiple of 64kB.
> 
> i.e. it doesn't say anything about the size. The _example_ is 2 cells,
> but I don't think that is normative.
> 
> Unless you can point to a bindings update which specifies strictly two
> cells then I think the right answer is the use dt_read_number.

You are right. The binding text is not clear. But Linux gic driver from the
beginning is using the 64 bit value.

This is what I could find from the forums(https://patches.linaro.org/24165/)
----------------------------
> +	gic: interrupt-controller@2c010000 {
> +		compatible = "arm,gic-v3";
> +		#interrupt-cells = <3>;
> +		interrupt-controller;
> +		redistributor-stride = 0x40000;		// 256kB stride

I raised the point in patch 1 about the stide value being read as a
64-bit value, which would imply that the example should be the
following, I think:
------------------------

Actually linux sets the stride to zero, if it's not 64 bit.

irq-gic-v3.c - gic_of_init()

if (of_property_read_u64(node, "redistributor-stride", &redist_stride))
                redist_stride = 0;


> Ian.

Re: [PATCH] xen/arm: Correctly read the GICv3 Re-Distributor stride

[Ian Campbell]

On Wed, 2015-09-30 at 11:29 +0000, Shameerali Kolothum Thodi wrote:
> 
> > -----Original Message-----
> > From: Ian Campbell [mailto:ian.campbell@citrix.com]
> > Sent: 30 September 2015 12:04
> > To: Shameerali Kolothum Thodi; xen-devel@lists.xenproject.org
> > Cc: julien.grall@citrix.com; vijaya.kumar@caviumnetworks.com
> > Subject: Re: [PATCH] xen/arm: Correctly read the GICv3 Re-Distributor
> > stride
> > 
> > On Wed, 2015-09-30 at 11:54 +0100, Shameerali Kolothum Thodi wrote:
> > > The GICv3 driver read a 32 bit value for the re-distributor stride,
> > > but the dts binding is a two-cell property.
> > 
> > The binding doc I have says:
> > 
> > - redistributor-stride : If using padding pages, specifies the stride
> >   of consecutive redistributors. Must be a multiple of 64kB.
> > 
> > i.e. it doesn't say anything about the size. The _example_ is 2 cells,
> > but I don't think that is normative.
> > 
> > Unless you can point to a bindings update which specifies strictly two
> > cells then I think the right answer is the use dt_read_number.
> 
> You are right. The binding text is not clear. But Linux gic driver from the
> beginning is using the 64 bit value.

The Linux gic driver is also not a normative declaration of the spec.

I think the spec needs to be clarified and then Linux and/or Xen need to be
fixed to follow the spec.

Ian.

Re: [PATCH] xen/arm: Correctly read the GICv3 Re-Distributor stride

[Julien Grall]

Hi Shameerali,


On 30/09/15 11:54, Shameerali Kolothum Thodi wrote:
> The GICv3 driver read a 32 bit value for the re-distributor stride, but

s/read/reads/

> the dts binding is a two-cell property.

I would mention that two-cell = 64 bit to make clear that the issue is
we are reading only one-cell (i.e 32 bit).

> All instances of rdist stride access and reference has been modified to
> accommodate 64 bit value. The changes are compiled and verified on HiSilicon
> Hip05 platform.
> 
> Signed-off-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>

FIY, this patch will clash with the series [1].

Regards,

[1] http://www.gossamer-threads.com/lists/xen/devel/400241

-- 
Julien Grall