[PATCH 0/3] xen/arm: vgic-v3: Correctly retrieved the vCPU associated to a re-distributor

[PATCH 0/3] xen/arm: vgic-v3: Correctly retrieved the vCPU associated to a re-distributor

[Julien Grall]

Hi,

This small series aims to fix the issue discovered by Shameerali when booting
Xen on his platform.

Note the last patch is just a clean-up because I was bothered with the really
long name in the I/O emulation. It has been placed at the end so we can
backport easily the first 2 patches in Xen 4.6 if necessary.

I pushed a branch will this patch series based on staging:

git://xenbits.xen.org/people/julieng/xen-unstable.git branch gicv3-redist

Sincerely yours,

Cc: shameerali.kolothum.thodi@huawei.com

Julien Grall (3):
  xen/arm: io: Extend write/read handler to pass private data
  xen/arm: vgic-v3: Correctly retrieve the vCPU associated to a
    re-distributor
  xen/arm: io: Shorten the name of the fields and clean up

 xen/arch/arm/io.c            | 57 ++++++++++++++++++++------------
 xen/arch/arm/vgic-v2.c       | 12 ++++---
 xen/arch/arm/vgic-v3.c       | 79 ++++++++++++++++++--------------------------
 xen/arch/arm/vuart.c         | 15 +++++----
 xen/include/asm-arm/domain.h |  3 +-
 xen/include/asm-arm/mmio.h   | 19 ++++++-----
 6 files changed, 96 insertions(+), 89 deletions(-)

-- 
2.1.4

[PATCH 1/3] xen/arm: io: Extend write/read handler to pass private data

[Julien Grall]

Some handlers may require to use private data in order to get quickly
information related to the region emulated.

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

---

Cc: shameerali.kolothum.thodi@huawei.com

    This will be necessary in the follow-up in order to fix bug in the
    GICR emulation.
---
 xen/arch/arm/io.c          | 15 +++++++++++----
 xen/arch/arm/vgic-v2.c     |  8 +++++---
 xen/arch/arm/vgic-v3.c     | 17 +++++++++++------
 xen/arch/arm/vuart.c       | 11 ++++++-----
 xen/include/asm-arm/mmio.h |  7 ++++---
 5 files changed, 37 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 8e55d49..85797f1 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -37,18 +37,24 @@ int handle_mmio(mmio_info_t *info)
         if ( (info->gpa >= mmio_handler->addr) &&
              (info->gpa < (mmio_handler->addr + mmio_handler->size)) )
         {
-            return info->dabt.write ?
-                mmio_handler->mmio_handler_ops->write_handler(v, info) :
-                mmio_handler->mmio_handler_ops->read_handler(v, info);
+            goto found;
         }
     }
 
     return 0;
+
+found:
+    if ( info->dabt.write )
+        return mmio_handler->mmio_handler_ops->write_handler(v, info,
+                                                             mmio_handler->priv);
+    else
+        return mmio_handler->mmio_handler_ops->read_handler(v, info,
+                                                            mmio_handler->priv);
 }
 
 void register_mmio_handler(struct domain *d,
                            const struct mmio_handler_ops *handle,
-                           paddr_t addr, paddr_t size)
+                           paddr_t addr, paddr_t size, void *priv)
 {
     struct io_handler *handler = &d->arch.io_handlers;
 
@@ -59,6 +65,7 @@ void register_mmio_handler(struct domain *d,
     handler->mmio_handlers[handler->num_entries].mmio_handler_ops = handle;
     handler->mmio_handlers[handler->num_entries].addr = addr;
     handler->mmio_handlers[handler->num_entries].size = size;
+    handler->mmio_handlers[handler->num_entries].priv = priv;
     dsb(ish);
     handler->num_entries++;
 
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index fa71598..8e50f22 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -50,7 +50,8 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t vbase)
     vgic_v2_hw.vbase = vbase;
 }
 
-static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
+static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
+                                   void *priv)
 {
     struct hsr_dabt dabt = info->dabt;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
@@ -247,7 +248,8 @@ static int vgic_v2_to_sgi(struct vcpu *v, register_t sgir)
     return vgic_to_sgi(v, sgir, sgi_mode, virq, &target);
 }
 
-static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
+static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
+                                    void *priv)
 {
     struct hsr_dabt dabt = info->dabt;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
@@ -590,7 +592,7 @@ static int vgic_v2_domain_init(struct domain *d)
                sizeof(d->arch.vgic.shared_irqs[i].v2.itargets));
 
     register_mmio_handler(d, &vgic_v2_distr_mmio_handler, d->arch.vgic.dbase,
-                          PAGE_SIZE);
+                          PAGE_SIZE, NULL);
 
     return 0;
 }
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index f1c482d..6a4feb2 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -634,7 +634,8 @@ static inline struct vcpu *get_vcpu_from_rdist(paddr_t gpa,
     return d->vcpu[vcpu_id];
 }
 
-static int vgic_v3_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info)
+static int vgic_v3_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info,
+                                    void *priv)
 {
     uint32_t offset;
 
@@ -656,7 +657,8 @@ static int vgic_v3_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info)
     return 0;
 }
 
-static int vgic_v3_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info)
+static int vgic_v3_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info,
+                                     void *priv)
 {
     uint32_t offset;
 
@@ -678,7 +680,8 @@ static int vgic_v3_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info)
     return 0;
 }
 
-static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
+static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
+                                   void *priv)
 {
     struct hsr_dabt dabt = info->dabt;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
@@ -835,7 +838,8 @@ read_as_zero:
     return 1;
 }
 
-static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
+static int vgic_v3_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
+                                    void *priv)
 {
     struct hsr_dabt dabt = info->dabt;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
@@ -1200,7 +1204,7 @@ static int vgic_v3_domain_init(struct domain *d)
 
     /* Register mmio handle for the Distributor */
     register_mmio_handler(d, &vgic_distr_mmio_handler, d->arch.vgic.dbase,
-                          SZ_64K);
+                          SZ_64K, NULL);
 
     /*
      * Register mmio handler per contiguous region occupied by the
@@ -1210,7 +1214,8 @@ static int vgic_v3_domain_init(struct domain *d)
     for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
         register_mmio_handler(d, &vgic_rdistr_mmio_handler,
             d->arch.vgic.rdist_regions[i].base,
-            d->arch.vgic.rdist_regions[i].size);
+            d->arch.vgic.rdist_regions[i].size,
+            NULL);
 
     d->arch.vgic.ctlr = VGICD_CTLR_DEFAULT;
 
diff --git a/xen/arch/arm/vuart.c b/xen/arch/arm/vuart.c
index d9f4249..51d0557 100644
--- a/xen/arch/arm/vuart.c
+++ b/xen/arch/arm/vuart.c
@@ -45,8 +45,8 @@
 
 #define domain_has_vuart(d) ((d)->arch.vuart.info != NULL)
 
-static int vuart_mmio_read(struct vcpu *v, mmio_info_t *info);
-static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info);
+static int vuart_mmio_read(struct vcpu *v, mmio_info_t *info, void *priv);
+static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info, void *priv);
 
 static const struct mmio_handler_ops vuart_mmio_handler = {
     .read_handler  = vuart_mmio_read,
@@ -70,7 +70,8 @@ int domain_vuart_init(struct domain *d)
 
     register_mmio_handler(d, &vuart_mmio_handler,
                           d->arch.vuart.info->base_addr,
-                          d->arch.vuart.info->size);
+                          d->arch.vuart.info->size,
+                          NULL);
 
     return 0;
 }
@@ -105,7 +106,7 @@ static void vuart_print_char(struct vcpu *v, char c)
     spin_unlock(&uart->lock);
 }
 
-static int vuart_mmio_read(struct vcpu *v, mmio_info_t *info)
+static int vuart_mmio_read(struct vcpu *v, mmio_info_t *info, void *priv)
 {
     struct domain *d = v->domain;
     struct hsr_dabt dabt = info->dabt;
@@ -125,7 +126,7 @@ static int vuart_mmio_read(struct vcpu *v, mmio_info_t *info)
     return 1;
 }
 
-static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info)
+static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info, void *priv)
 {
     struct domain *d = v->domain;
     struct hsr_dabt dabt = info->dabt;
diff --git a/xen/include/asm-arm/mmio.h b/xen/include/asm-arm/mmio.h
index 0160f09..294c18b 100644
--- a/xen/include/asm-arm/mmio.h
+++ b/xen/include/asm-arm/mmio.h
@@ -32,8 +32,8 @@ typedef struct
     paddr_t gpa;
 } mmio_info_t;
 
-typedef int (*mmio_read_t)(struct vcpu *v, mmio_info_t *info);
-typedef int (*mmio_write_t)(struct vcpu *v, mmio_info_t *info);
+typedef int (*mmio_read_t)(struct vcpu *v, mmio_info_t *info, void *priv);
+typedef int (*mmio_write_t)(struct vcpu *v, mmio_info_t *info, void *priv);
 typedef int (*mmio_check_t)(struct vcpu *v, paddr_t addr);
 
 struct mmio_handler_ops {
@@ -45,6 +45,7 @@ struct mmio_handler {
     paddr_t addr;
     paddr_t size;
     const struct mmio_handler_ops *mmio_handler_ops;
+    void *priv;
 };
 
 struct io_handler {
@@ -56,7 +57,7 @@ struct io_handler {
 extern int handle_mmio(mmio_info_t *info);
 void register_mmio_handler(struct domain *d,
                            const struct mmio_handler_ops *handle,
-                           paddr_t addr, paddr_t size);
+                           paddr_t addr, paddr_t size, void *priv);
 int domain_io_init(struct domain *d);
 
 #endif  /* __ASM_ARM_MMIO_H__ */
-- 
2.1.4

Re: [PATCH 1/3] xen/arm: io: Extend write/read handler to pass private data

[Ian Campbell]

On Mon, 2015-09-28 at 21:31 +0100, Julien Grall wrote:
> Some handlers may require to use private data in order to get quickly
> information related to the region emulated.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> ---
> 
> Cc: shameerali.kolothum.thodi@huawei.com
> 
>     This will be necessary in the follow-up in order to fix bug in the
>     GICR emulation.
> ---
>  xen/arch/arm/io.c          | 15 +++++++++++----
>  xen/arch/arm/vgic-v2.c     |  8 +++++---
>  xen/arch/arm/vgic-v3.c     | 17 +++++++++++------
>  xen/arch/arm/vuart.c       | 11 ++++++-----
>  xen/include/asm-arm/mmio.h |  7 ++++---
>  5 files changed, 37 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index 8e55d49..85797f1 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -37,18 +37,24 @@ int handle_mmio(mmio_info_t *info)
>          if ( (info->gpa >= mmio_handler->addr) &&
>               (info->gpa < (mmio_handler->addr + mmio_handler->size)) )
>          {
> -            return info->dabt.write ?
> -                mmio_handler->mmio_handler_ops->write_handler(v, info) :
> -                mmio_handler->mmio_handler_ops->read_handler(v, info);
> +            goto found;

Rather than goto use break instead.

>          }
>      }
>  
>      return 0;

And make this "if ( i == io_handlers->num_entries ) return 0;"

The continue to handle the op as you have done.

Other than that looks good.

Ian.

[PATCH 2/3] xen/arm: vgic-v3: Correctly retrieve the vCPU associated to a re-distributor

[Julien Grall]

When the guest is accessing the re-distributor, Xen retrieves the base
of the re-distributor using a mask based on the stride.

Although, when the stride contains multiple set, the corresponding mask
will be computed incorrectly [1] and therefore giving invalid vCPU and
offset:

(XEN) d0v0: vGICR: unknown gpa read address 000000008d130008
(XEN) traps.c:2447:d0v1 HSR=0x93c08006 pc=0xffffffc00032362c
gva=0xffffff80000b0008 gpa=0x0000008d130008

For instance if the region of re-distributor is starting at 0x8d100000
and the stride is 0x30000, an access to the address 0x8d130008 should
be valid and use the re-distributor of vCPU1 with an offset of 0x8.
Although, Xen is returning the vCPU0 and an offset of 0x20008.

I didn't find a way to replace the current computation of the mask with
a valid. The only solution I have found is to pass the region in private
data of the handler. So we can directly get the offset from the
beginning of the region and find the corresponding vCPU/offset in the
re-distributor.

This is also make the code simpler and avoid fast/slow path.

[1] http://lists.xen.org/archives/html/xen-devel/2015-09/msg03372.html

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

---
Cc: shameerali.kolothum.thodi@huawei.com
Cc: Wei Liu <wei.liu2@citrix.com>

    This is technically a good candidate for Xen 4.6. Without it DOM0
    may not be able to bring secondary CPU on platform using a stride
    with multiple bit set [1]. Although, we don't support such platform
    right now. So I would rather defer this change to Xen 4.6.1 and
    avoid possible downside/bug in this patch.

    Shamaeerali, I've only tested quickly on the foundation model. Can
    you give a try on your platform to check if it fixes DOM0 boot?

    [1] http://lists.xen.org/archives/html/xen-devel/2015-09/msg03372.html
---
 xen/arch/arm/vgic-v3.c | 58 +++++++++++++++++---------------------------------
 1 file changed, 20 insertions(+), 38 deletions(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 6a4feb2..0a14184 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -593,55 +593,34 @@ write_ignore_32:
     return 1;
 }
 
-static inline struct vcpu *get_vcpu_from_rdist(paddr_t gpa,
-                                               struct vcpu *v,
-                                               uint32_t *offset)
+static struct vcpu *get_vcpu_from_rdist(struct domain *d,
+    const struct vgic_rdist_region *region,
+    paddr_t gpa, uint32_t *offset)
 {
-    struct domain *d = v->domain;
+    struct vcpu *v;
     uint32_t stride = d->arch.vgic.rdist_stride;
-    paddr_t base;
-    int i, vcpu_id;
-    struct vgic_rdist_region *region;
-
-    *offset = gpa & (stride - 1);
-    base = gpa & ~((paddr_t)stride - 1);
-
-    /* Fast path: the VCPU is trying to access its re-distributor */
-    if ( likely(v->arch.vgic.rdist_base == base) )
-        return v;
-
-    /* Slow path: the VCPU is trying to access another re-distributor */
-
-    /*
-     * Find the region where the re-distributor lives. For this purpose,
-     * we look one region ahead as only MMIO range for redistributors
-     * traps here.
-     * Note: The region has been ordered during the GIC initialization
-     */
-    for ( i = 1; i < d->arch.vgic.nr_regions; i++ )
-    {
-        if ( base < d->arch.vgic.rdist_regions[i].base )
-            break;
-    }
-
-    region = &d->arch.vgic.rdist_regions[i - 1];
-
-    vcpu_id = region->first_cpu + ((base - region->base) / stride);
+    unsigned int vcpu_id;
 
+    vcpu_id = region->first_cpu + ((gpa - region->base) / stride);
     if ( unlikely(vcpu_id >= d->max_vcpus) )
         return NULL;
 
-    return d->vcpu[vcpu_id];
+    v = d->vcpu[vcpu_id];
+
+    *offset = gpa - v->arch.vgic.rdist_base;
+
+    return v;
 }
 
 static int vgic_v3_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info,
                                     void *priv)
 {
     uint32_t offset;
+    const struct vgic_rdist_region *region = priv;
 
     perfc_incr(vgicr_reads);
 
-    v = get_vcpu_from_rdist(info->gpa, v, &offset);
+    v = get_vcpu_from_rdist(v->domain, region, info->gpa, &offset);
     if ( unlikely(!v) )
         return 0;
 
@@ -661,10 +640,11 @@ static int vgic_v3_rdistr_mmio_write(struct vcpu *v, mmio_info_t *info,
                                      void *priv)
 {
     uint32_t offset;
+    const struct vgic_rdist_region *region = priv;
 
     perfc_incr(vgicr_writes);
 
-    v = get_vcpu_from_rdist(info->gpa, v, &offset);
+    v = get_vcpu_from_rdist(v->domain, region, info->gpa, &offset);
     if ( unlikely(!v) )
         return 0;
 
@@ -1212,10 +1192,12 @@ static int vgic_v3_domain_init(struct domain *d)
      * redistributor is targeted.
      */
     for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
+    {
+        struct vgic_rdist_region *region = &d->arch.vgic.rdist_regions[i];
+
         register_mmio_handler(d, &vgic_rdistr_mmio_handler,
-            d->arch.vgic.rdist_regions[i].base,
-            d->arch.vgic.rdist_regions[i].size,
-            NULL);
+                              region->base, region->size, region);
+    }
 
     d->arch.vgic.ctlr = VGICD_CTLR_DEFAULT;
 
-- 
2.1.4

Re: [PATCH 2/3] xen/arm: vgic-v3: Correctly retrieve the vCPU associated to a re-distributor

[Wei Liu]

On Mon, Sep 28, 2015 at 09:31:35PM +0100, Julien Grall wrote:
> When the guest is accessing the re-distributor, Xen retrieves the base
> of the re-distributor using a mask based on the stride.
> 
> Although, when the stride contains multiple set, the corresponding mask
> will be computed incorrectly [1] and therefore giving invalid vCPU and
> offset:
> 
> (XEN) d0v0: vGICR: unknown gpa read address 000000008d130008
> (XEN) traps.c:2447:d0v1 HSR=0x93c08006 pc=0xffffffc00032362c
> gva=0xffffff80000b0008 gpa=0x0000008d130008
> 
> For instance if the region of re-distributor is starting at 0x8d100000
> and the stride is 0x30000, an access to the address 0x8d130008 should
> be valid and use the re-distributor of vCPU1 with an offset of 0x8.
> Although, Xen is returning the vCPU0 and an offset of 0x20008.
> 
> I didn't find a way to replace the current computation of the mask with
> a valid. The only solution I have found is to pass the region in private
> data of the handler. So we can directly get the offset from the
> beginning of the region and find the corresponding vCPU/offset in the
> re-distributor.
> 
> This is also make the code simpler and avoid fast/slow path.
> 
> [1] http://lists.xen.org/archives/html/xen-devel/2015-09/msg03372.html
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> ---
> Cc: shameerali.kolothum.thodi@huawei.com
> Cc: Wei Liu <wei.liu2@citrix.com>
> 
>     This is technically a good candidate for Xen 4.6. Without it DOM0
>     may not be able to bring secondary CPU on platform using a stride
>     with multiple bit set [1]. Although, we don't support such platform
>     right now. So I would rather defer this change to Xen 4.6.1 and
>     avoid possible downside/bug in this patch.
> 

This sounds reasonable to me.

Re: [PATCH 2/3] xen/arm: vgic-v3: Correctly retrieve the vCPU associated to a re-distributor

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@citrix.com]
> Sent: 28 September 2015 21:32
> To: xen-devel@lists.xenproject.org
> Cc: ian.campbell@citrix.com; stefano.stabellini@eu.citrix.com; Julien
> Grall; Shameerali Kolothum Thodi; Wei Liu
> Subject: [PATCH 2/3] xen/arm: vgic-v3: Correctly retrieve the vCPU
> associated to a re-distributor
> 
> When the guest is accessing the re-distributor, Xen retrieves the base
> of the re-distributor using a mask based on the stride.
>
....
 
>     with multiple bit set [1]. Although, we don't support such platform
>     right now. So I would rather defer this change to Xen 4.6.1 and
>     avoid possible downside/bug in this patch.
> 
>     Shamaeerali, I've only tested quickly on the foundation model. Can
>     you give a try on your platform to check if it fixes DOM0 boot?


I tried and it fixes the issue on our platform :). 
(I applied only the first two patches in the series).

>     [1] http://lists.xen.org/archives/html/xen-devel/2015-
> 09/msg03372.html

Re: [PATCH 2/3] xen/arm: vgic-v3: Correctly retrieve the vCPU associated to a re-distributor

[Julien Grall]

On 29/09/15 13:24, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Julien Grall [mailto:julien.grall@citrix.com]
>> Sent: 28 September 2015 21:32
>> To: xen-devel@lists.xenproject.org
>> Cc: ian.campbell@citrix.com; stefano.stabellini@eu.citrix.com; Julien
>> Grall; Shameerali Kolothum Thodi; Wei Liu
>> Subject: [PATCH 2/3] xen/arm: vgic-v3: Correctly retrieve the vCPU
>> associated to a re-distributor
>>
>> When the guest is accessing the re-distributor, Xen retrieves the base
>> of the re-distributor using a mask based on the stride.
>>
> ....
>  
>>     with multiple bit set [1]. Although, we don't support such platform
>>     right now. So I would rather defer this change to Xen 4.6.1 and
>>     avoid possible downside/bug in this patch.
>>
>>     Shamaeerali, I've only tested quickly on the foundation model. Can
>>     you give a try on your platform to check if it fixes DOM0 boot?
> 
> 
> I tried and it fixes the issue on our platform :). 

Great, thank you for testing! Can I add your Tested-by?

> (I applied only the first two patches in the series).

The last one is only a clean up so it's fine.

>>     [1] http://lists.xen.org/archives/html/xen-devel/2015-
>> 09/msg03372.html
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 


-- 
Julien Grall

Re: [PATCH 2/3] xen/arm: vgic-v3: Correctly retrieve the vCPU associated to a re-distributor

[Ian Campbell]

On Mon, 2015-09-28 at 21:31 +0100, Julien Grall wrote:
> When the guest is accessing the re-distributor, Xen retrieves the base
> of the re-distributor using a mask based on the stride.
> 
> Although, when the stride contains multiple set, the corresponding mask

                                                 ^bits?

(Also, drop the "Although, ").

> will be computed incorrectly [1] and therefore giving invalid vCPU and
> offset:
> 
> (XEN) d0v0: vGICR: unknown gpa read address 000000008d130008
> (XEN) traps.c:2447:d0v1 HSR=0x93c08006 pc=0xffffffc00032362c
> gva=0xffffff80000b0008 gpa=0x0000008d130008
> 
> For instance if the region of re-distributor is starting at 0x8d100000
> and the stride is 0x30000, an access to the address 0x8d130008 should
> be valid and use the re-distributor of vCPU1 with an offset of 0x8.
> Although, Xen is returning the vCPU0 and an offset of 0x20008.
> 
> I didn't find a way to replace the current computation of the mask with
> a valid. The only solution I have found is to pass the region in private

        ^one

> data of the handler. So we can directly get the offset from the
> beginning of the region and find the corresponding vCPU/offset in the
> re-distributor.
> 
> This is also make the code simpler and avoid fast/slow path.
> 
> [1] http://lists.xen.org/archives/html/xen-devel/2015-09/msg03372.html
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> ---
> Cc: shameerali.kolothum.thodi@huawei.com
> Cc: Wei Liu <wei.liu2@citrix.com>
> 
>     This is technically a good candidate for Xen 4.6. Without it DOM0
>     may not be able to bring secondary CPU on platform using a stride
>     with multiple bit set [1]. Although, we don't support such platform
>     right now. So I would rather defer this change to Xen 4.6.1 and
>     avoid possible downside/bug in this patch.

Agreed.

Other than the text:

Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
>     Shamaeerali, I've only tested quickly on the foundation model. Can
>     you give a try on your platform to check if it fixes DOM0 boot?
> 
>     [1] 
> http://lists.xen.org/archives/html/xen-devel/2015-09/msg03372.html
> ---
>  xen/arch/arm/vgic-v3.c | 58 +++++++++++++++++---------------------------
> ------
>  1 file changed, 20 insertions(+), 38 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 6a4feb2..0a14184 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -593,55 +593,34 @@ write_ignore_32:
>      return 1;
>  }
>  
> -static inline struct vcpu *get_vcpu_from_rdist(paddr_t gpa,
> -                                               struct vcpu *v,
> -                                               uint32_t *offset)
> +static struct vcpu *get_vcpu_from_rdist(struct domain *d,
> +    const struct vgic_rdist_region *region,
> +    paddr_t gpa, uint32_t *offset)
>  {
> -    struct domain *d = v->domain;
> +    struct vcpu *v;
>      uint32_t stride = d->arch.vgic.rdist_stride;
> -    paddr_t base;
> -    int i, vcpu_id;
> -    struct vgic_rdist_region *region;
> -
> -    *offset = gpa & (stride - 1);
> -    base = gpa & ~((paddr_t)stride - 1);
> -
> -    /* Fast path: the VCPU is trying to access its re-distributor */
> -    if ( likely(v->arch.vgic.rdist_base == base) )
> -        return v;
> -
> -    /* Slow path: the VCPU is trying to access another re-distributor */
> -
> -    /*
> -     * Find the region where the re-distributor lives. For this purpose,
> -     * we look one region ahead as only MMIO range for redistributors
> -     * traps here.
> -     * Note: The region has been ordered during the GIC initialization
> -     */
> -    for ( i = 1; i < d->arch.vgic.nr_regions; i++ )
> -    {
> -        if ( base < d->arch.vgic.rdist_regions[i].base )
> -            break;
> -    }
> -
> -    region = &d->arch.vgic.rdist_regions[i - 1];
> -
> -    vcpu_id = region->first_cpu + ((base - region->base) / stride);
> +    unsigned int vcpu_id;
>  
> +    vcpu_id = region->first_cpu + ((gpa - region->base) / stride);
>      if ( unlikely(vcpu_id >= d->max_vcpus) )
>          return NULL;
>  
> -    return d->vcpu[vcpu_id];
> +    v = d->vcpu[vcpu_id];
> +
> +    *offset = gpa - v->arch.vgic.rdist_base;
> +
> +    return v;
>  }
>  
>  static int vgic_v3_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info,
>                                      void *priv)
>  {
>      uint32_t offset;
> +    const struct vgic_rdist_region *region = priv;
>  
>      perfc_incr(vgicr_reads);
>  
> -    v = get_vcpu_from_rdist(info->gpa, v, &offset);
> +    v = get_vcpu_from_rdist(v->domain, region, info->gpa, &offset);
>      if ( unlikely(!v) )
>          return 0;
>  
> @@ -661,10 +640,11 @@ static int vgic_v3_rdistr_mmio_write(struct vcpu
> *v, mmio_info_t *info,
>                                       void *priv)
>  {
>      uint32_t offset;
> +    const struct vgic_rdist_region *region = priv;
>  
>      perfc_incr(vgicr_writes);
>  
> -    v = get_vcpu_from_rdist(info->gpa, v, &offset);
> +    v = get_vcpu_from_rdist(v->domain, region, info->gpa, &offset);
>      if ( unlikely(!v) )
>          return 0;
>  
> @@ -1212,10 +1192,12 @@ static int vgic_v3_domain_init(struct domain *d)
>       * redistributor is targeted.
>       */
>      for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
> +    {
> +        struct vgic_rdist_region *region = &d
> ->arch.vgic.rdist_regions[i];
> +
>          register_mmio_handler(d, &vgic_rdistr_mmio_handler,
> -            d->arch.vgic.rdist_regions[i].base,
> -            d->arch.vgic.rdist_regions[i].size,
> -            NULL);
> +                              region->base, region->size, region);
> +    }
>  
>      d->arch.vgic.ctlr = VGICD_CTLR_DEFAULT;
>  

Re: [PATCH 2/3] xen/arm: vgic-v3: Correctly retrieve the vCPU associated to a re-distributor

[Julien Grall]

On 29/09/15 14:56, Ian Campbell wrote:
> On Mon, 2015-09-28 at 21:31 +0100, Julien Grall wrote:
>> When the guest is accessing the re-distributor, Xen retrieves the base
>> of the re-distributor using a mask based on the stride.
>>
>> Although, when the stride contains multiple set, the corresponding mask
> 
>                                                  ^bits?

I guess you mean "multiple bits set" here?

> (Also, drop the "Although, ").
> 
>> will be computed incorrectly [1] and therefore giving invalid vCPU and
>> offset:
>>
>> (XEN) d0v0: vGICR: unknown gpa read address 000000008d130008
>> (XEN) traps.c:2447:d0v1 HSR=0x93c08006 pc=0xffffffc00032362c
>> gva=0xffffff80000b0008 gpa=0x0000008d130008
>>
>> For instance if the region of re-distributor is starting at 0x8d100000
>> and the stride is 0x30000, an access to the address 0x8d130008 should
>> be valid and use the re-distributor of vCPU1 with an offset of 0x8.
>> Although, Xen is returning the vCPU0 and an offset of 0x20008.
>>
>> I didn't find a way to replace the current computation of the mask with
>> a valid. The only solution I have found is to pass the region in private
> 
>         ^one
> 
>> data of the handler. So we can directly get the offset from the
>> beginning of the region and find the corresponding vCPU/offset in the
>> re-distributor.
>>
>> This is also make the code simpler and avoid fast/slow path.
>>
>> [1] http://lists.xen.org/archives/html/xen-devel/2015-09/msg03372.html
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>>
>> ---
>> Cc: shameerali.kolothum.thodi@huawei.com
>> Cc: Wei Liu <wei.liu2@citrix.com>
>>
>>     This is technically a good candidate for Xen 4.6. Without it DOM0
>>     may not be able to bring secondary CPU on platform using a stride
>>     with multiple bit set [1]. Although, we don't support such platform
>>     right now. So I would rather defer this change to Xen 4.6.1 and
>>     avoid possible downside/bug in this patch.
> 
> Agreed.
> 
> Other than the text:
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thank you.

Regards,

-- 
Julien Grall

Re: [PATCH 2/3] xen/arm: vgic-v3: Correctly retrieve the vCPU associated to a re-distributor

[Ian Campbell]

On Tue, 2015-09-29 at 15:15 +0100, Julien Grall wrote:
> On 29/09/15 14:56, Ian Campbell wrote:
> > On Mon, 2015-09-28 at 21:31 +0100, Julien Grall wrote:
> > > When the guest is accessing the re-distributor, Xen retrieves the
> > > base
> > > of the re-distributor using a mask based on the stride.
> > > 
> > > Although, when the stride contains multiple set, the corresponding
> > > mask
> > 
> >                                                  ^bits?
> 
> I guess you mean "multiple bits set" here?

I meant "multiple set bits" but that works too.

Ian.

[PATCH 3/3] xen/arm: io: Shorten the name of the fields and clean up

[Julien Grall]

The field names in the IO emulation are really long and use repeatedly
the term handler which make some line cumbersome to read:

mmio_handler->mmio_handler_ops->write_handler

Also take the opportunity to do some clean up:
    - Avoid "handler" vs "handle" in register_mmio_handler
    - Use a local variable to initialize handler in
    register_mmio_handler
    - Add a comment explaining the dsb(ish) in register_mmio_handler
    - Rename the structure io_handler into vmmio because the io_handler
    is in fine handling multiple handlers and the name a the fields was
    io_handlers. Also rename the field io_handlers to vmmmio
    - Rename the field mmio_handler_ops to ops because we are in the
    structure mmio_handler to not need to repeat it
    - Rename the field mmio_handlers to handlers because we are in the
    vmmio structure
    - Make it clear that register_mmio_ops is taking an ops and not an
    handle
    - Clean up local variable to helpe to understand the code

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

---
    This is the last patch of the series so to make easy to backport the
    previous patches to Xen 4.6 if necessary.
---
 xen/arch/arm/io.c            | 52 +++++++++++++++++++++++++-------------------
 xen/arch/arm/vgic-v2.c       |  4 ++--
 xen/arch/arm/vgic-v3.c       |  8 +++----
 xen/arch/arm/vuart.c         |  4 ++--
 xen/include/asm-arm/domain.h |  3 ++-
 xen/include/asm-arm/mmio.h   | 12 +++++-----
 6 files changed, 46 insertions(+), 37 deletions(-)

diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 85797f1..f7443d2 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -27,15 +27,15 @@ int handle_mmio(mmio_info_t *info)
 {
     struct vcpu *v = current;
     int i;
-    const struct mmio_handler *mmio_handler;
-    const struct io_handler *io_handlers = &v->domain->arch.io_handlers;
+    const struct mmio_handler *handler;
+    const struct vmmio *vmmio = &v->domain->arch.vmmio;
 
-    for ( i = 0; i < io_handlers->num_entries; i++ )
+    for ( i = 0; i < vmmio->num_entries; i++ )
     {
-        mmio_handler = &io_handlers->mmio_handlers[i];
+        handler = &vmmio->handlers[i];
 
-        if ( (info->gpa >= mmio_handler->addr) &&
-             (info->gpa < (mmio_handler->addr + mmio_handler->size)) )
+        if ( (info->gpa >= handler->addr) &&
+             (info->gpa < (handler->addr + handler->size)) )
         {
             goto found;
         }
@@ -45,37 +45,45 @@ int handle_mmio(mmio_info_t *info)
 
 found:
     if ( info->dabt.write )
-        return mmio_handler->mmio_handler_ops->write_handler(v, info,
-                                                             mmio_handler->priv);
+        return handler->ops->write(v, info, handler->priv);
     else
-        return mmio_handler->mmio_handler_ops->read_handler(v, info,
-                                                            mmio_handler->priv);
+        return handler->ops->read(v, info, handler->priv);
 }
 
 void register_mmio_handler(struct domain *d,
-                           const struct mmio_handler_ops *handle,
+                           const struct mmio_handler_ops *ops,
                            paddr_t addr, paddr_t size, void *priv)
 {
-    struct io_handler *handler = &d->arch.io_handlers;
+    struct vmmio *vmmio = &d->arch.vmmio;
+    struct mmio_handler *handler;
 
-    BUG_ON(handler->num_entries >= MAX_IO_HANDLER);
+    BUG_ON(vmmio->num_entries >= MAX_IO_HANDLER);
 
-    spin_lock(&handler->lock);
+    spin_lock(&vmmio->lock);
 
-    handler->mmio_handlers[handler->num_entries].mmio_handler_ops = handle;
-    handler->mmio_handlers[handler->num_entries].addr = addr;
-    handler->mmio_handlers[handler->num_entries].size = size;
-    handler->mmio_handlers[handler->num_entries].priv = priv;
+    handler = &vmmio->handlers[vmmio->num_entries];
+
+    handler->ops = ops;
+    handler->addr = addr;
+    handler->size = size;
+    handler->priv = priv;
+
+    /*
+     * handle_mmio is not using the lock to avoid contention.
+     * Make sure the other processors see the new handler before
+     * updating the number of entries
+     */
     dsb(ish);
-    handler->num_entries++;
 
-    spin_unlock(&handler->lock);
+    vmmio->num_entries++;
+
+    spin_unlock(&vmmio->lock);
 }
 
 int domain_io_init(struct domain *d)
 {
-   spin_lock_init(&d->arch.io_handlers.lock);
-   d->arch.io_handlers.num_entries = 0;
+   spin_lock_init(&d->arch.vmmio.lock);
+   d->arch.vmmio.num_entries = 0;
 
    return 0;
 }
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 8e50f22..f886724 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -495,8 +495,8 @@ write_ignore:
 }
 
 static const struct mmio_handler_ops vgic_v2_distr_mmio_handler = {
-    .read_handler  = vgic_v2_distr_mmio_read,
-    .write_handler = vgic_v2_distr_mmio_write,
+    .read  = vgic_v2_distr_mmio_read,
+    .write = vgic_v2_distr_mmio_write,
 };
 
 static struct vcpu *vgic_v2_get_target_vcpu(struct vcpu *v, unsigned int irq)
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 0a14184..beb3621 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1037,13 +1037,13 @@ static int vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
 }
 
 static const struct mmio_handler_ops vgic_rdistr_mmio_handler = {
-    .read_handler  = vgic_v3_rdistr_mmio_read,
-    .write_handler = vgic_v3_rdistr_mmio_write,
+    .read  = vgic_v3_rdistr_mmio_read,
+    .write = vgic_v3_rdistr_mmio_write,
 };
 
 static const struct mmio_handler_ops vgic_distr_mmio_handler = {
-    .read_handler  = vgic_v3_distr_mmio_read,
-    .write_handler = vgic_v3_distr_mmio_write,
+    .read  = vgic_v3_distr_mmio_read,
+    .write = vgic_v3_distr_mmio_write,
 };
 
 static int vgic_v3_get_irq_priority(struct vcpu *v, unsigned int irq)
diff --git a/xen/arch/arm/vuart.c b/xen/arch/arm/vuart.c
index 51d0557..2495e87 100644
--- a/xen/arch/arm/vuart.c
+++ b/xen/arch/arm/vuart.c
@@ -49,8 +49,8 @@ static int vuart_mmio_read(struct vcpu *v, mmio_info_t *info, void *priv);
 static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info, void *priv);
 
 static const struct mmio_handler_ops vuart_mmio_handler = {
-    .read_handler  = vuart_mmio_read,
-    .write_handler = vuart_mmio_write,
+    .read  = vuart_mmio_read,
+    .write = vuart_mmio_write,
 };
 
 int domain_vuart_init(struct domain *d)
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index c3f5a95..01859cc 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -55,7 +55,8 @@ struct arch_domain
     struct hvm_domain hvm_domain;
     xen_pfn_t *grant_table_gpfn;
 
-    struct io_handler io_handlers;
+    struct vmmio vmmio;
+
     /* Continuable domain_relinquish_resources(). */
     enum {
         RELMEM_not_started,
diff --git a/xen/include/asm-arm/mmio.h b/xen/include/asm-arm/mmio.h
index 294c18b..1cd7a7a 100644
--- a/xen/include/asm-arm/mmio.h
+++ b/xen/include/asm-arm/mmio.h
@@ -37,26 +37,26 @@ typedef int (*mmio_write_t)(struct vcpu *v, mmio_info_t *info, void *priv);
 typedef int (*mmio_check_t)(struct vcpu *v, paddr_t addr);
 
 struct mmio_handler_ops {
-    mmio_read_t read_handler;
-    mmio_write_t write_handler;
+    mmio_read_t read;
+    mmio_write_t write;
 };
 
 struct mmio_handler {
     paddr_t addr;
     paddr_t size;
-    const struct mmio_handler_ops *mmio_handler_ops;
+    const struct mmio_handler_ops *ops;
     void *priv;
 };
 
-struct io_handler {
+struct vmmio {
     int num_entries;
     spinlock_t lock;
-    struct mmio_handler mmio_handlers[MAX_IO_HANDLER];
+    struct mmio_handler handlers[MAX_IO_HANDLER];
 };
 
 extern int handle_mmio(mmio_info_t *info);
 void register_mmio_handler(struct domain *d,
-                           const struct mmio_handler_ops *handle,
+                           const struct mmio_handler_ops *ops,
                            paddr_t addr, paddr_t size, void *priv);
 int domain_io_init(struct domain *d);
 
-- 
2.1.4

Re: [PATCH 3/3] xen/arm: io: Shorten the name of the fields and clean up

[Ian Campbell]

On Mon, 2015-09-28 at 21:31 +0100, Julien Grall wrote:
> The field names in the IO emulation are really long and use repeatedly
> the term handler which make some line cumbersome to read:
> 
> mmio_handler->mmio_handler_ops->write_handler
> 
> Also take the opportunity to do some clean up:
>     - Avoid "handler" vs "handle" in register_mmio_handler
>     - Use a local variable to initialize handler in
>     register_mmio_handler
>     - Add a comment explaining the dsb(ish) in register_mmio_handler
>     - Rename the structure io_handler into vmmio because the io_handler
>     is in fine handling multiple handlers and the name a the fields was
>     io_handlers. Also rename the field io_handlers to vmmmio

Stray extra "m" in vmmmio.

>     - Rename the field mmio_handler_ops to ops because we are in the
>     structure mmio_handler to not need to repeat it
>     - Rename the field mmio_handlers to handlers because we are in the
>     vmmio structure
>     - Make it clear that register_mmio_ops is taking an ops and not an
>     handle
>     - Clean up local variable to helpe to understand the code

Stray "e" in the last line.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

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

Re: [PATCH 0/3] xen/arm: vgic-v3: Correctly retrieved the vCPU associated to a re-distributor

[Julien Grall]

On 28/09/15 21:31, Julien Grall wrote:
> This small series aims to fix the issue discovered by Shameerali when booting
> Xen on his platform.

I forgot to mention that this patch series will clash with other series
I recently sent ([1] and [2]).

I would prefer to see this patch series in staging first and rebase the
two others on top of it. Although we can figure out this once one of
these 3 series is ready to be pushed.

Regards,

[1]
http://lists.xenproject.org/archives/html/xen-devel/2015-09/msg02844.html
[2] I'm not able to find a link so:
<1443192698-16163-1-git-send-email-julien.grall@citrix.com>

-- 
Julien Grall