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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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@xxxxxxxxxx>

---
Cc: shameerali.kolothum.thodi@xxxxxxxxxx
Cc: Wei Liu <wei.liu2@xxxxxxxxxx>

    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


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