[PATCH 0/7] ARM: vGICv3: clean up optional DT properties

[PATCH 0/7] ARM: vGICv3: clean up optional DT properties

[Andre Przywara]

The GICv3 devicetree binding describes two optional properties
(redistributor-stride and #redistributor-regions), which are meant to work
around implementations which deviate from the architecture. By the nature
of those they are only needed if the values are different from what the
architecture describes (1 region with a stride of 128K for GICv3 and 256K
for GICv4).
While we need to fully parse and obey these properties for the hypervisor
GIC driver, the visibility for guests should be very limited:
* DomUs get to see an emulated interrupt controller and a constructed
  memory map, so don't need those properties at all.
* Dom0 gets to see an emulated interrupt controller, but has to live with
  the host's memory map. That means we have to propagate multiple regions,
  but can (and should) use the architected stride.

In the moment those properties get more attention in the code than they
actually deserve: so far there seems to be only one platform using those,
and ACPI actually doesn't have any provisions at all to describe those
systems.
This patch series cleans up the handling of these properties, underlining
the workaround feature and helping to reduce a dependency between the GIC
and the VGIC (which was the actual driver of this series).
Patch 1 removes the redundant properties from DomU's device tree.
Patch 2-4 clean up the handling of the regions property, using it only
for Dom0, when needed.
Patch 5-7 rework the handling of the stride parameter, by simplifying
it for the host GICv3 driver, and removing it at all from the VGIC.

Cheers,
Andre

Andre Przywara (7):
  tools: ARM: vGICv3: avoid inserting optional DT properties
  ARM: vGICv3: drop GUEST_GICV3_RDIST_REGIONS symbol
  ARM: GICv3: emit optional DT property only when necessary
  ARM: GICv3: use hardware GICv3 redistributor regions for Dom0
  ARM: GICv3: simplify GICv3 redistributor stride handling
  ARM: vGICv3: always use architected redist stride
  ARM: vGICv3: remove rdist_stride from VGIC structure

 tools/libxl/libxl_arm.c           |  8 ------
 xen/arch/arm/gic-v3.c             | 54 +++++++++++++++++++--------------------
 xen/arch/arm/vgic-v3.c            | 45 +++++++++++++-------------------
 xen/include/asm-arm/domain.h      |  1 -
 xen/include/asm-arm/gic_v3_defs.h |  5 ++++
 xen/include/asm-arm/vgic.h        |  1 -
 xen/include/public/arch-arm.h     |  3 ---
 7 files changed, 49 insertions(+), 68 deletions(-)

-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/7] tools: ARM: vGICv3: avoid inserting optional DT properties

[Andre Przywara]

When creating a GICv3 devicetree node, we currently insert the
redistributor-stride and #redistributor-regions properties, with fixed
values which are actually the architected ones. But those properties are
optional and only needed to cover for broken platforms, where the values
differ from the architected one. This will never be the case for the
constructed DomU memory map.
So we drop those properties altogether and provide a clean and architected
GICv3 DT node for DomUs.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 tools/libxl/libxl_arm.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 3e46554301..b5bba3cd33 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -524,14 +524,6 @@ static int make_gicv3_node(libxl__gc *gc, void *fdt)
     res = fdt_property(fdt, "interrupt-controller", NULL, 0);
     if (res) return res;
 
-    res = fdt_property_cell(fdt, "redistributor-stride",
-                            GUEST_GICV3_RDIST_STRIDE);
-    if (res) return res;
-
-    res = fdt_property_cell(fdt, "#redistributor-regions",
-                            GUEST_GICV3_RDIST_REGIONS);
-    if (res) return res;
-
     res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
                             2,
                             gicd_base, gicd_size,
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/7] ARM: vGICv3: drop GUEST_GICV3_RDIST_REGIONS symbol

[Andre Przywara]

Architecturally there is only one GICv3 redistributor region.
Drop the symbol which suggested that was a delibarate choice for Xen
guests, instead hard code the "1" in the appropriate places, along with
a comment to explain the reasons.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/vgic-v3.c        | 17 ++++++++++++-----
 xen/include/public/arch-arm.h |  1 -
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index af16dfd005..7d3ea171b4 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1640,8 +1640,18 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
 
 static inline unsigned int vgic_v3_rdist_count(struct domain *d)
 {
-    return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
-               GUEST_GICV3_RDIST_REGIONS;
+    /*
+     * Architecturally there is only one GICv3 redistributor region.
+     * The GICv3 DT binding provisions for multiple regions, since there are
+     * platforms out there which break this architectural assumption.
+     * ACPI does not support this workaround at all.
+     * For Dom0 we have to live with the MMIO layout the hardware provides,
+     * so we have to copy the multiple regions - as the first region may not
+     * provide enough space to hold all redistributors we need.
+     * However DomU get a constructed memory map, so we can go with
+     * the architected single redistributor region.
+     */
+    return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions : 1;
 }
 
 static int vgic_v3_domain_init(struct domain *d)
@@ -1700,9 +1710,6 @@ static int vgic_v3_domain_init(struct domain *d)
     {
         d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE;
 
-        /* XXX: Only one Re-distributor region mapped for the guest */
-        BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1);
-
         d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE;
 
         /* The first redistributor should contain enough space for all CPUs */
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 05fd11ca38..ca79ab6284 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -402,7 +402,6 @@ typedef uint64_t xen_callback_t;
 #define GUEST_GICV3_GICD_SIZE      xen_mk_ullong(0x00010000)
 
 #define GUEST_GICV3_RDIST_STRIDE   xen_mk_ullong(0x00020000)
-#define GUEST_GICV3_RDIST_REGIONS  1
 
 #define GUEST_GICV3_GICR0_BASE     xen_mk_ullong(0x03020000) /* vCPU0..127 */
 #define GUEST_GICV3_GICR0_SIZE     xen_mk_ullong(0x01000000)
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/7] ARM: GICv3: emit optional DT property only when necessary

[Andre Przywara]

The ARM GICv3 DT property "#redistributor-regions" is optional and only
useful if it has any other values than the architected "1".
Keep our generated DT node clean by emitting this property only if we
actually need more than one region.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/gic-v3.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index a0d290b55c..9ad0cd19ef 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1168,10 +1168,13 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
     if ( res )
         return res;
 
-    res = fdt_property_cell(fdt, "#redistributor-regions",
-                            d->arch.vgic.nr_regions);
-    if ( res )
-        return res;
+    if ( d->arch.vgic.nr_regions > 1 )
+    {
+        res = fdt_property_cell(fdt, "#redistributor-regions",
+                                d->arch.vgic.nr_regions);
+        if ( res )
+            return res;
+    }
 
     len = dt_cells_to_size(dt_n_addr_cells(gic) + dt_n_size_cells(gic));
     /*
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 4/7] ARM: GICv3: use hardware GICv3 redistributor regions for Dom0

[Andre Przywara]

The code to generate the DT node or MADT table for Dom0 reaches into the
domain's VGIC structure to learn the number of redistributor regions and
their base addresses.
Since those values are copied from the hardware, we can as well use
those hardware values directly when setting up the hardware domain.

This avoids the hardware GIC code to reference vGIC data structures,
making this variable VGIC internal.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/gic-v3.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 9ad0cd19ef..1c24ecbba7 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1168,10 +1168,10 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
     if ( res )
         return res;
 
-    if ( d->arch.vgic.nr_regions > 1 )
+    if ( gicv3.rdist_count > 1 )
     {
         res = fdt_property_cell(fdt, "#redistributor-regions",
-                                d->arch.vgic.nr_regions);
+                                gicv3.rdist_count);
         if ( res )
             return res;
     }
@@ -1182,7 +1182,7 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
      * CPU interface and virtual cpu interfaces accessesed as System registers
      * So cells are created only for Distributor and rdist regions
      */
-    len = len * (d->arch.vgic.nr_regions + 1);
+    len = len * (gicv3.rdist_count + 1);
     new_cells = xzalloc_bytes(len);
     if ( new_cells == NULL )
         return -FDT_ERR_XEN(ENOMEM);
@@ -1191,9 +1191,9 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
 
     dt_set_range(&tmp, gic, d->arch.vgic.dbase, SZ_64K);
 
-    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
-        dt_set_range(&tmp, gic, d->arch.vgic.rdist_regions[i].base,
-                     d->arch.vgic.rdist_regions[i].size);
+    for ( i = 0; i < gicv3.rdist_count; i++ )
+        dt_set_range(&tmp, gic, gicv3.rdist_regions[i].base,
+                     gicv3.rdist_regions[i].size);
 
     res = fdt_property(fdt, "reg", new_cells, len);
     xfree(new_cells);
@@ -1419,13 +1419,13 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
 
     /* Add Generic Redistributor */
     size = sizeof(struct acpi_madt_generic_redistributor);
-    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
+    for ( i = 0; i < gicv3.rdist_count; i++ )
     {
         gicr = (struct acpi_madt_generic_redistributor *)(base_ptr + table_len);
         gicr->header.type = ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR;
         gicr->header.length = size;
-        gicr->base_address = d->arch.vgic.rdist_regions[i].base;
-        gicr->length = d->arch.vgic.rdist_regions[i].size;
+        gicr->base_address = gicv3.rdist_regions[i].base;
+        gicr->length = gicv3.rdist_regions[i].size;
         table_len += size;
     }
 
@@ -1438,8 +1438,7 @@ static unsigned long gicv3_get_hwdom_extra_madt_size(const struct domain *d)
 {
     unsigned long size;
 
-    size = sizeof(struct acpi_madt_generic_redistributor)
-           * d->arch.vgic.nr_regions;
+    size = sizeof(struct acpi_madt_generic_redistributor) * gicv3.rdist_count;
 
     size += sizeof(struct acpi_madt_generic_translator)
             * vgic_v3_its_count(d);
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 5/7] ARM: GICv3: simplify GICv3 redistributor stride handling

[Andre Przywara]

Instead of hard coding the architected redistributor stride into the
code, lets use a clear #define to the two values for GICv3 and GICv4 and
clarify the algorithm to determine the needed stride value.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/gic-v3.c             | 18 ++++++++++--------
 xen/include/asm-arm/gic_v3_defs.h |  5 +++++
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 1c24ecbba7..2893c66492 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -690,6 +690,15 @@ static int __init gicv3_populate_rdist(void)
         do {
             typer = readq_relaxed(ptr + GICR_TYPER);
 
+            /* Set the architectural redist size if not overridden by DT. */
+            if ( !gicv3.rdist_stride )
+            {
+                if ( typer & GICR_TYPER_VLPIS )
+                    gicv3.rdist_stride = GICV4_GICR_SIZE;
+                else
+                    gicv3.rdist_stride = GICV3_GICR_SIZE;
+            }
+
             if ( (typer >> 32) == aff )
             {
                 this_cpu(rbase) = ptr;
@@ -732,14 +741,7 @@ static int __init gicv3_populate_rdist(void)
             if ( gicv3.rdist_regions[i].single_rdist )
                 break;
 
-            if ( gicv3.rdist_stride )
-                ptr += gicv3.rdist_stride;
-            else
-            {
-                ptr += SZ_64K * 2; /* Skip RD_base + SGI_base */
-                if ( typer & GICR_TYPER_VLPIS )
-                    ptr += SZ_64K * 2; /* Skip VLPI_base + reserved page */
-            }
+            ptr += gicv3.rdist_stride;
 
         } while ( !(typer & GICR_TYPER_LAST) );
     }
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index 65c9dc47cf..412e41afed 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -18,6 +18,8 @@
 #ifndef __ASM_ARM_GIC_V3_DEFS_H__
 #define __ASM_ARM_GIC_V3_DEFS_H__
 
+#include <xen/sizes.h>
+
 /*
  * Additional registers defined in GIC v3.
  * Common GICD registers are defined in gic.h
@@ -68,6 +70,9 @@
 #define GICV3_GICD_IIDR_VAL          0x34c
 #define GICV3_GICR_IIDR_VAL          GICV3_GICD_IIDR_VAL
 
+#define GICV3_GICR_SIZE              (2 * SZ_64K)
+#define GICV4_GICR_SIZE              (4 * SZ_64K)
+
 #define GICR_CTLR                    (0x0000)
 #define GICR_IIDR                    (0x0004)
 #define GICR_TYPER                   (0x0008)
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 6/7] ARM: vGICv3: always use architected redist stride

[Andre Przywara]

The redistributor-stride property in a GICv3 DT node is only there to
cover broken platforms where this value deviates from the architected one.
Since we emulate the GICv3 distributor even for Dom0, we don't need to
copy the broken behaviour. All the special handling for Dom0s using
GICv3 is just for using the hardware's memory map, which is unaffected
by the redistributor stride - it can never be smaller than the
architected two pages.
Remove the redistributor-stride property from Dom0's DT node and also
remove the code that tried to reuse the hardware value for Dom0's GICv3
emulation.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/gic-v3.c  |  5 -----
 xen/arch/arm/vgic-v3.c | 14 ++++++--------
 2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 2893c66492..1684d9cf1b 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1165,11 +1165,6 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
     if ( res )
         return res;
 
-    res = fdt_property_cell(fdt, "redistributor-stride",
-                            d->arch.vgic.rdist_stride);
-    if ( res )
-        return res;
-
     if ( gicv3.rdist_count > 1 )
     {
         res = fdt_property_cell(fdt, "#redistributor-regions",
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 7d3ea171b4..b4159317dd 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1030,10 +1030,9 @@ static struct vcpu *get_vcpu_from_rdist(struct domain *d,
     paddr_t gpa, uint32_t *offset)
 {
     struct vcpu *v;
-    uint32_t stride = d->arch.vgic.rdist_stride;
     unsigned int vcpu_id;
 
-    vcpu_id = region->first_cpu + ((gpa - region->base) / stride);
+    vcpu_id = region->first_cpu + ((gpa - region->base) / GICV3_GICR_SIZE);
     if ( unlikely(vcpu_id >= d->max_vcpus) )
         return NULL;
 
@@ -1594,7 +1593,6 @@ 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;
 
     /*
      * Find the region where the re-distributor lives. For this purpose,
@@ -1610,11 +1608,11 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
 
     /* Get the base address of the redistributor */
     rdist_base = region->base;
-    rdist_base += (v->vcpu_id - region->first_cpu) * rdist_stride;
+    rdist_base += (v->vcpu_id - region->first_cpu) * GICV3_GICR_SIZE;
 
     /* Check if a valid region was found for the re-distributor */
     if ( (rdist_base < region->base) ||
-         ((rdist_base + rdist_stride) > (region->base + region->size)) )
+         ((rdist_base + GICV3_GICR_SIZE) > (region->base + region->size)) )
     {
         dprintk(XENLOG_ERR,
                 "d%u: Unable to find a re-distributor for VCPU %u\n",
@@ -1630,7 +1628,7 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
      * VGIC_V3_RDIST_LAST flags.
      * Note that we are assuming max_vcpus will never change.
      */
-    last_cpu = (region->size / rdist_stride) + region->first_cpu - 1;
+    last_cpu = (region->size / GICV3_GICR_SIZE) + region->first_cpu - 1;
 
     if ( v->vcpu_id == last_cpu || (v->vcpu_id == (d->max_vcpus - 1)) )
         v->arch.vgic.flags |= VGIC_V3_RDIST_LAST;
@@ -1701,7 +1699,7 @@ static int vgic_v3_domain_init(struct domain *d)
             /* Set the first CPU handled by this region */
             d->arch.vgic.rdist_regions[i].first_cpu = first_cpu;
 
-            first_cpu += size / d->arch.vgic.rdist_stride;
+            first_cpu += size / GICV3_GICR_SIZE;
         }
 
         d->arch.vgic.intid_bits = vgic_v3_hw.intid_bits;
@@ -1713,7 +1711,7 @@ static int vgic_v3_domain_init(struct domain *d)
         d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE;
 
         /* The first redistributor should contain enough space for all CPUs */
-        BUILD_BUG_ON((GUEST_GICV3_GICR0_SIZE / GUEST_GICV3_RDIST_STRIDE) < MAX_VIRT_CPUS);
+        BUILD_BUG_ON((GUEST_GICV3_GICR0_SIZE / GICV3_GICR_SIZE) < MAX_VIRT_CPUS);
         d->arch.vgic.rdist_regions[0].base = GUEST_GICV3_GICR0_BASE;
         d->arch.vgic.rdist_regions[0].size = GUEST_GICV3_GICR0_SIZE;
         d->arch.vgic.rdist_regions[0].first_cpu = 0;
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 7/7] ARM: vGICv3: remove rdist_stride from VGIC structure

[Andre Przywara]

The last patch removed the usage of the hardware's redistributor-stride
value from our (Dom0) GICv3 emulation. This means we no longer need to
store this value in the VGIC data structure.
Remove that variable and every code snippet that handled that, instead
simply always use the architected value.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/gic-v3.c         |  3 +--
 xen/arch/arm/vgic-v3.c        | 14 --------------
 xen/include/asm-arm/domain.h  |  1 -
 xen/include/asm-arm/vgic.h    |  1 -
 xen/include/public/arch-arm.h |  2 --
 5 files changed, 1 insertion(+), 20 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 1684d9cf1b..588fe94cab 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1693,8 +1693,7 @@ static int __init gicv3_init(void)
     reg = readl_relaxed(GICD + GICD_TYPER);
     intid_bits = GICD_TYPE_ID_BITS(reg);
 
-    vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions,
-                     gicv3.rdist_stride, intid_bits);
+    vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions, intid_bits);
     gicv3_init_v2();
 
     spin_lock_init(&gicv3.lock);
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index b4159317dd..47856a0a73 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -58,21 +58,18 @@ static struct {
     /* Re-distributor regions */
     unsigned int nr_rdist_regions;
     const struct rdist_region *regions;
-    uint32_t rdist_stride; /* Re-distributor stride */
     unsigned int intid_bits;  /* Number of interrupt ID bits */
 } 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,
                       unsigned int intid_bits)
 {
     vgic_v3_hw.enabled = true;
     vgic_v3_hw.dbase = dbase;
     vgic_v3_hw.nr_rdist_regions = nr_rdist_regions;
     vgic_v3_hw.regions = regions;
-    vgic_v3_hw.rdist_stride = rdist_stride;
     vgic_v3_hw.intid_bits = intid_bits;
 }
 
@@ -1680,15 +1677,6 @@ static int vgic_v3_domain_init(struct domain *d)
 
         d->arch.vgic.dbase = vgic_v3_hw.dbase;
 
-        d->arch.vgic.rdist_stride = vgic_v3_hw.rdist_stride;
-        /*
-         * If the stride is not set, the default stride for GICv3 is 2 * 64K:
-         *     - first 64k page for Control and Physical LPIs
-         *     - second 64k page for Control and Generation of SGIs
-         */
-        if ( !d->arch.vgic.rdist_stride )
-            d->arch.vgic.rdist_stride = 2 * SZ_64K;
-
         for ( i = 0; i < vgic_v3_hw.nr_rdist_regions; i++ )
         {
             paddr_t size = vgic_v3_hw.regions[i].size;
@@ -1708,8 +1696,6 @@ static int vgic_v3_domain_init(struct domain *d)
     {
         d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE;
 
-        d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE;
-
         /* The first redistributor should contain enough space for all CPUs */
         BUILD_BUG_ON((GUEST_GICV3_GICR0_SIZE / GICV3_GICR_SIZE) < MAX_VIRT_CPUS);
         d->arch.vgic.rdist_regions[0].base = GUEST_GICV3_GICR0_BASE;
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 4fe189b1c3..3eda7196ff 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -108,7 +108,6 @@ struct arch_domain
             unsigned int first_cpu;         /* First CPU handled */
         } *rdist_regions;
         int nr_regions;                     /* Number of rdist regions */
-        uint32_t rdist_stride;              /* Re-Distributor stride */
         unsigned long int nr_lpis;
         uint64_t rdist_propbase;
         struct rb_root its_devices;         /* Devices mapped to an ITS */
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 2a93a7bef9..7919cc53f4 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -255,7 +255,6 @@ 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,
                       unsigned int intid_bits);
 #endif
 
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index ca79ab6284..3bca165fbf 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -401,8 +401,6 @@ typedef uint64_t xen_callback_t;
 #define GUEST_GICV3_GICD_BASE      xen_mk_ullong(0x03001000)
 #define GUEST_GICV3_GICD_SIZE      xen_mk_ullong(0x00010000)
 
-#define GUEST_GICV3_RDIST_STRIDE   xen_mk_ullong(0x00020000)
-
 #define GUEST_GICV3_GICR0_BASE     xen_mk_ullong(0x03020000) /* vCPU0..127 */
 #define GUEST_GICV3_GICR0_SIZE     xen_mk_ullong(0x01000000)
 
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/7] tools: ARM: vGICv3: avoid inserting optional DT properties

[Julien Grall]

(+ Tools maintainers)

Hi Andre,

On 24/01/18 14:35, Andre Przywara wrote:
> When creating a GICv3 devicetree node, we currently insert the
> redistributor-stride and #redistributor-regions properties, with fixed
> values which are actually the architected ones. But those properties are
> optional and only needed to cover for broken platforms, where the values
> differ from the architected one. This will never be the case for the

I understand that the stride is defined by GICv3. But I don't think this 
is true for the number of regions. Looking at the spec, multiple regions 
seems to be allowed (see GICR_TYPER.Last). Did I miss anything?

The rest looks good to me.

> constructed DomU memory map.
> So we drop those properties altogether and provide a clean and architected
> GICv3 DT node for DomUs.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>   tools/libxl/libxl_arm.c | 8 --------
>   1 file changed, 8 deletions(-)
> 
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index 3e46554301..b5bba3cd33 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -524,14 +524,6 @@ static int make_gicv3_node(libxl__gc *gc, void *fdt)
>       res = fdt_property(fdt, "interrupt-controller", NULL, 0);
>       if (res) return res;
>   
> -    res = fdt_property_cell(fdt, "redistributor-stride",
> -                            GUEST_GICV3_RDIST_STRIDE);
> -    if (res) return res;
> -
> -    res = fdt_property_cell(fdt, "#redistributor-regions",
> -                            GUEST_GICV3_RDIST_REGIONS);
> -    if (res) return res;
> -
>       res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS,
>                               2,
>                               gicd_base, gicd_size,
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/7] ARM: vGICv3: drop GUEST_GICV3_RDIST_REGIONS symbol

[Julien Grall]

Hi Andre,

On 24/01/18 14:35, Andre Przywara wrote:
> Architecturally there is only one GICv3 redistributor region.
> Drop the symbol which suggested that was a delibarate choice for Xen
> guests, instead hard code the "1" in the appropriate places, along with
> a comment to explain the reasons.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>   xen/arch/arm/vgic-v3.c        | 17 ++++++++++++-----
>   xen/include/public/arch-arm.h |  1 -
>   2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index af16dfd005..7d3ea171b4 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1640,8 +1640,18 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
>   
>   static inline unsigned int vgic_v3_rdist_count(struct domain *d)
>   {
> -    return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
> -               GUEST_GICV3_RDIST_REGIONS;
> +    /*
> +     * Architecturally there is only one GICv3 redistributor region.
> +     * The GICv3 DT binding provisions for multiple regions, since there are
> +     * platforms out there which break this architectural assumption.
> +     * ACPI does not support this workaround at all.

This is not true. The ACPI spec supports multiple regions of 
redistributors. What ACPI does not support is a different stride.

> +     * For Dom0 we have to live with the MMIO layout the hardware provides,
> +     * so we have to copy the multiple regions - as the first region may not
> +     * provide enough space to hold all redistributors we need.
> +     * However DomU get a constructed memory map, so we can go with
> +     * the architected single redistributor region.
> +     */
> +    return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions : 1; >   }
>   
>   static int vgic_v3_domain_init(struct domain *d)
> @@ -1700,9 +1710,6 @@ static int vgic_v3_domain_init(struct domain *d)
>       {
>           d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE;
>   
> -        /* XXX: Only one Re-distributor region mapped for the guest */
> -        BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1);
> -
>           d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE;
>   
>           /* The first redistributor should contain enough space for all CPUs */
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 05fd11ca38..ca79ab6284 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -402,7 +402,6 @@ typedef uint64_t xen_callback_t;
>   #define GUEST_GICV3_GICD_SIZE      xen_mk_ullong(0x00010000)
>   
>   #define GUEST_GICV3_RDIST_STRIDE   xen_mk_ullong(0x00020000)
> -#define GUEST_GICV3_RDIST_REGIONS  1
>   
>   #define GUEST_GICV3_GICR0_BASE     xen_mk_ullong(0x03020000) /* vCPU0..127 */
>   #define GUEST_GICV3_GICR0_SIZE     xen_mk_ullong(0x01000000)
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/7] ARM: GICv3: emit optional DT property only when necessary

[Julien Grall]

Hi Andre,

On 24/01/18 14:35, Andre Przywara wrote:
> The ARM GICv3 DT property "#redistributor-regions" is optional and only
> useful if it has any other values than the architected "1".
> Keep our generated DT node clean by emitting this property only if we
> actually need more than one region.

I really don't see the benefits of this patch. It is fine to have 
#redistributor-regions property in the DT. You might "clean" the DT but 
make the code a bit more complex.

Cheers,

> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>   xen/arch/arm/gic-v3.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index a0d290b55c..9ad0cd19ef 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1168,10 +1168,13 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>       if ( res )
>           return res;
>   
> -    res = fdt_property_cell(fdt, "#redistributor-regions",
> -                            d->arch.vgic.nr_regions);
> -    if ( res )
> -        return res;
> +    if ( d->arch.vgic.nr_regions > 1 )
> +    {
> +        res = fdt_property_cell(fdt, "#redistributor-regions",
> +                                d->arch.vgic.nr_regions);
> +        if ( res )
> +            return res;
> +    }
>   
>       len = dt_cells_to_size(dt_n_addr_cells(gic) + dt_n_size_cells(gic));
>       /*
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/7] tools: ARM: vGICv3: avoid inserting optional DT properties

[Andre Przywara]

Hi,

On 24/01/18 16:08, Julien Grall wrote:
> (+ Tools maintainers)
> 
> Hi Andre,
> 
> On 24/01/18 14:35, Andre Przywara wrote:
>> When creating a GICv3 devicetree node, we currently insert the
>> redistributor-stride and #redistributor-regions properties, with fixed
>> values which are actually the architected ones. But those properties are
>> optional and only needed to cover for broken platforms, where the values
>> differ from the architected one. This will never be the case for the
> 
> I understand that the stride is defined by GICv3. But I don't think this
> is true for the number of regions. Looking at the spec, multiple regions
> seems to be allowed (see GICR_TYPER.Last). Did I miss anything?

Well, the spec does indeed not say anything about it, but the DT binding
description does:
==============
Optional:
...
- #redistributor-regions: The number of independent contiguous regions
  occupied by the redistributors. Required if more than one such
  region is present.
==============

So we don't need it in our case, and in fact we don't implement
*anything* to actually give the toolstack a choice. So we should
consequently remove these lines, as they are pointless right now. Should
we ever need to implement support for multiple regions, bringing this
back is really our least concern.

Cheers,
Andre.

> 
> The rest looks good to me.
> 
>> constructed DomU memory map.
>> So we drop those properties altogether and provide a clean and
>> architected
>> GICv3 DT node for DomUs.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>   tools/libxl/libxl_arm.c | 8 --------
>>   1 file changed, 8 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>> index 3e46554301..b5bba3cd33 100644
>> --- a/tools/libxl/libxl_arm.c
>> +++ b/tools/libxl/libxl_arm.c
>> @@ -524,14 +524,6 @@ static int make_gicv3_node(libxl__gc *gc, void *fdt)
>>       res = fdt_property(fdt, "interrupt-controller", NULL, 0);
>>       if (res) return res;
>>   -    res = fdt_property_cell(fdt, "redistributor-stride",
>> -                            GUEST_GICV3_RDIST_STRIDE);
>> -    if (res) return res;
>> -
>> -    res = fdt_property_cell(fdt, "#redistributor-regions",
>> -                            GUEST_GICV3_RDIST_REGIONS);
>> -    if (res) return res;
>> -
>>       res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS,
>> ROOT_SIZE_CELLS,
>>                               2,
>>                               gicd_base, gicd_size,
>>
> 
> Cheers,
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/7] tools: ARM: vGICv3: avoid inserting optional DT properties

[Julien Grall]

Hi Andre,

On 24/01/18 16:35, Andre Przywara wrote:
> On 24/01/18 16:08, Julien Grall wrote:
>> (+ Tools maintainers)
>>
>> Hi Andre,
>>
>> On 24/01/18 14:35, Andre Przywara wrote:
>>> When creating a GICv3 devicetree node, we currently insert the
>>> redistributor-stride and #redistributor-regions properties, with fixed
>>> values which are actually the architected ones. But those properties are
>>> optional and only needed to cover for broken platforms, where the values
>>> differ from the architected one. This will never be the case for the
>>
>> I understand that the stride is defined by GICv3. But I don't think this
>> is true for the number of regions. Looking at the spec, multiple regions
>> seems to be allowed (see GICR_TYPER.Last). Did I miss anything?
> 
> Well, the spec does indeed not say anything about it, but the DT binding
> description does:
> ==============
> Optional:
> ...
> - #redistributor-regions: The number of independent contiguous regions
>    occupied by the redistributors. Required if more than one such
>    region is present.
> ==============
> 
> So we don't need it in our case, and in fact we don't implement
> *anything* to actually give the toolstack a choice. So we should
> consequently remove these lines, as they are pointless right now. Should
> we ever need to implement support for multiple regions, bringing this
> back is really our least concern.

I am not against this patch. However, the commit message should not 
induce that platform with more than 1 re-distributor regions are broken.

So I don't see anything which would prevent us to provide a guest memory 
map with multiple re-distributor regions.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/7] ARM: GICv3: use hardware GICv3 redistributor regions for Dom0

[Julien Grall]

Hi Andre,

On 24/01/18 14:35, Andre Przywara wrote:
> The code to generate the DT node or MADT table for Dom0 reaches into the
> domain's VGIC structure to learn the number of redistributor regions and
> their base addresses.
> Since those values are copied from the hardware, we can as well use
> those hardware values directly when setting up the hardware domain.

Now the function is using a mix of fields (see dbase) from vgic and 
gicv3. Please either remove all or none.

> 
> This avoids the hardware GIC code to reference vGIC data structures,
> making this variable VGIC internal.

I don't understand the bits after the comma.

> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>   xen/arch/arm/gic-v3.c | 21 ++++++++++-----------
>   1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 9ad0cd19ef..1c24ecbba7 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1168,10 +1168,10 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>       if ( res )
>           return res;
>   
> -    if ( d->arch.vgic.nr_regions > 1 )
> +    if ( gicv3.rdist_count > 1 )
>       {
>           res = fdt_property_cell(fdt, "#redistributor-regions",
> -                                d->arch.vgic.nr_regions);
> +                                gicv3.rdist_count);
>           if ( res )
>               return res;
>       }
> @@ -1182,7 +1182,7 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>        * CPU interface and virtual cpu interfaces accessesed as System registers
>        * So cells are created only for Distributor and rdist regions
>        */
> -    len = len * (d->arch.vgic.nr_regions + 1);
> +    len = len * (gicv3.rdist_count + 1);
>       new_cells = xzalloc_bytes(len);
>       if ( new_cells == NULL )
>           return -FDT_ERR_XEN(ENOMEM);
> @@ -1191,9 +1191,9 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>   
>       dt_set_range(&tmp, gic, d->arch.vgic.dbase, SZ_64K);
>   
> -    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
> -        dt_set_range(&tmp, gic, d->arch.vgic.rdist_regions[i].base,
> -                     d->arch.vgic.rdist_regions[i].size);
> +    for ( i = 0; i < gicv3.rdist_count; i++ )
> +        dt_set_range(&tmp, gic, gicv3.rdist_regions[i].base,
> +                     gicv3.rdist_regions[i].size);
>   
>       res = fdt_property(fdt, "reg", new_cells, len);
>       xfree(new_cells);
> @@ -1419,13 +1419,13 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>   
>       /* Add Generic Redistributor */
>       size = sizeof(struct acpi_madt_generic_redistributor);
> -    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
> +    for ( i = 0; i < gicv3.rdist_count; i++ )
>       {
>           gicr = (struct acpi_madt_generic_redistributor *)(base_ptr + table_len);
>           gicr->header.type = ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR;
>           gicr->header.length = size;
> -        gicr->base_address = d->arch.vgic.rdist_regions[i].base;
> -        gicr->length = d->arch.vgic.rdist_regions[i].size;
> +        gicr->base_address = gicv3.rdist_regions[i].base;
> +        gicr->length = gicv3.rdist_regions[i].size;
>           table_len += size;
>       }
>   
> @@ -1438,8 +1438,7 @@ static unsigned long gicv3_get_hwdom_extra_madt_size(const struct domain *d)
>   {
>       unsigned long size;
>   
> -    size = sizeof(struct acpi_madt_generic_redistributor)
> -           * d->arch.vgic.nr_regions;
> +    size = sizeof(struct acpi_madt_generic_redistributor) * gicv3.rdist_count;
>   
>       size += sizeof(struct acpi_madt_generic_translator)
>               * vgic_v3_its_count(d);
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/7] ARM: vGICv3: drop GUEST_GICV3_RDIST_REGIONS symbol

[Andre Przywara]

Hi,

On 24/01/18 16:13, Julien Grall wrote:
> Hi Andre,
> 
> On 24/01/18 14:35, Andre Przywara wrote:
>> Architecturally there is only one GICv3 redistributor region.
>> Drop the symbol which suggested that was a delibarate choice for Xen
>> guests, instead hard code the "1" in the appropriate places, along with
>> a comment to explain the reasons.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>   xen/arch/arm/vgic-v3.c        | 17 ++++++++++++-----
>>   xen/include/public/arch-arm.h |  1 -
>>   2 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index af16dfd005..7d3ea171b4 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -1640,8 +1640,18 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
>>     static inline unsigned int vgic_v3_rdist_count(struct domain *d)
>>   {
>> -    return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
>> -               GUEST_GICV3_RDIST_REGIONS;
>> +    /*
>> +     * Architecturally there is only one GICv3 redistributor region.
>> +     * The GICv3 DT binding provisions for multiple regions, since
>> there are
>> +     * platforms out there which break this architectural assumption.
>> +     * ACPI does not support this workaround at all.
> 
> This is not true. The ACPI spec supports multiple regions of
> redistributors. What ACPI does not support is a different stride.

Ah, that's true. Sorry for that, I was a bit too enthusiastic here ;-)
Will change the comment, definitely.
However I would still be interested in dropping the
GUEST_GICV3_RDIST_REGIONS symbol, possibly replacing it with
DEFAULT_GICV3_RDIST_REGIONS, avoiding the misleading GUEST_ prefix.

Cheers,
Andre.

>> +     * For Dom0 we have to live with the MMIO layout the hardware
>> provides,
>> +     * so we have to copy the multiple regions - as the first region
>> may not
>> +     * provide enough space to hold all redistributors we need.
>> +     * However DomU get a constructed memory map, so we can go with
>> +     * the architected single redistributor region.
>> +     */
>> +    return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions : 1;
>> >   }
>>     static int vgic_v3_domain_init(struct domain *d)
>> @@ -1700,9 +1710,6 @@ static int vgic_v3_domain_init(struct domain *d)
>>       {
>>           d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE;
>>   -        /* XXX: Only one Re-distributor region mapped for the guest */
>> -        BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1);
>> -
>>           d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE;
>>             /* The first redistributor should contain enough space for
>> all CPUs */
>> diff --git a/xen/include/public/arch-arm.h
>> b/xen/include/public/arch-arm.h
>> index 05fd11ca38..ca79ab6284 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -402,7 +402,6 @@ typedef uint64_t xen_callback_t;
>>   #define GUEST_GICV3_GICD_SIZE      xen_mk_ullong(0x00010000)
>>     #define GUEST_GICV3_RDIST_STRIDE   xen_mk_ullong(0x00020000)
>> -#define GUEST_GICV3_RDIST_REGIONS  1
>>     #define GUEST_GICV3_GICR0_BASE     xen_mk_ullong(0x03020000) /*
>> vCPU0..127 */
>>   #define GUEST_GICV3_GICR0_SIZE     xen_mk_ullong(0x01000000)
>>
> 
> Cheers,
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/7] ARM: GICv3: emit optional DT property only when necessary

[Andre Przywara]

Hi,

On 24/01/18 16:32, Julien Grall wrote:
> Hi Andre,
> 
> On 24/01/18 14:35, Andre Przywara wrote:
>> The ARM GICv3 DT property "#redistributor-regions" is optional and only
>> useful if it has any other values than the architected "1".
>> Keep our generated DT node clean by emitting this property only if we
>> actually need more than one region.
> 
> I really don't see the benefits of this patch. It is fine to have
> #redistributor-regions property in the DT. You might "clean" the DT but
> make the code a bit more complex.

Fair enough, I can easily drop this patch.
Was a victim patch anyway to give reviewers some satisfaction :-D

Cheers,
Andre.

> 
> Cheers,
> 
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>   xen/arch/arm/gic-v3.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index a0d290b55c..9ad0cd19ef 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -1168,10 +1168,13 @@ static int gicv3_make_hwdom_dt_node(const
>> struct domain *d,
>>       if ( res )
>>           return res;
>>   -    res = fdt_property_cell(fdt, "#redistributor-regions",
>> -                            d->arch.vgic.nr_regions);
>> -    if ( res )
>> -        return res;
>> +    if ( d->arch.vgic.nr_regions > 1 )
>> +    {
>> +        res = fdt_property_cell(fdt, "#redistributor-regions",
>> +                                d->arch.vgic.nr_regions);
>> +        if ( res )
>> +            return res;
>> +    }
>>         len = dt_cells_to_size(dt_n_addr_cells(gic) +
>> dt_n_size_cells(gic));
>>       /*
>>
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/7] ARM: vGICv3: drop GUEST_GICV3_RDIST_REGIONS symbol

[Julien Grall]

Hi Andre,

On 24/01/18 16:58, Andre Przywara wrote:
> On 24/01/18 16:13, Julien Grall wrote:
>> Hi Andre,
>>
>> On 24/01/18 14:35, Andre Przywara wrote:
>>> Architecturally there is only one GICv3 redistributor region.
>>> Drop the symbol which suggested that was a delibarate choice for Xen
>>> guests, instead hard code the "1" in the appropriate places, along with
>>> a comment to explain the reasons.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>>> ---
>>>    xen/arch/arm/vgic-v3.c        | 17 ++++++++++++-----
>>>    xen/include/public/arch-arm.h |  1 -
>>>    2 files changed, 12 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>>> index af16dfd005..7d3ea171b4 100644
>>> --- a/xen/arch/arm/vgic-v3.c
>>> +++ b/xen/arch/arm/vgic-v3.c
>>> @@ -1640,8 +1640,18 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
>>>      static inline unsigned int vgic_v3_rdist_count(struct domain *d)
>>>    {
>>> -    return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
>>> -               GUEST_GICV3_RDIST_REGIONS;
>>> +    /*
>>> +     * Architecturally there is only one GICv3 redistributor region.
>>> +     * The GICv3 DT binding provisions for multiple regions, since
>>> there are
>>> +     * platforms out there which break this architectural assumption.
>>> +     * ACPI does not support this workaround at all.
>>
>> This is not true. The ACPI spec supports multiple regions of
>> redistributors. What ACPI does not support is a different stride.
> 
> Ah, that's true. Sorry for that, I was a bit too enthusiastic here ;-)
> Will change the comment, definitely.
> However I would still be interested in dropping the
> GUEST_GICV3_RDIST_REGIONS symbol, possibly replacing it with
> DEFAULT_GICV3_RDIST_REGIONS, avoiding the misleading GUEST_ prefix.

Why GUEST_ is misleading?

Bear in mind that the toolstack is in charge of the memory map. Not the 
hypervisor.

Today, the memory map is static and so rather than implementing a bunch 
of hypercalls the hypervisor is directly using the value.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/7] ARM: vGICv3: always use architected redist stride

[Julien Grall]

Hi Andre,

On 24/01/18 14:35, Andre Przywara wrote:
> The redistributor-stride property in a GICv3 DT node is only there to
> cover broken platforms where this value deviates from the architected one.
> Since we emulate the GICv3 distributor even for Dom0, we don't need to
> copy the broken behaviour. All the special handling for Dom0s using
> GICv3 is just for using the hardware's memory map, which is unaffected
> by the redistributor stride - it can never be smaller than the
> architected two pages.

Do we want a BUG_ON() just in case?

> Remove the redistributor-stride property from Dom0's DT node and also
> remove the code that tried to reuse the hardware value for Dom0's GICv3
> emulation.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>

In any case:

Acked-by: Julien Grall <julien.grall@linaro.org>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 7/7] ARM: vGICv3: remove rdist_stride from VGIC structure

[Julien Grall]

Hi Andre,

On 24/01/18 14:35, Andre Przywara wrote:
> The last patch removed the usage of the hardware's redistributor-stride
> value from our (Dom0) GICv3 emulation. This means we no longer need to
> store this value in the VGIC data structure.
> Remove that variable and every code snippet that handled that, instead
> simply always use the architected value.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>

Acked-by: Julien Grall <julien.grall@linaro.org>

Cheers,

> ---
>   xen/arch/arm/gic-v3.c         |  3 +--
>   xen/arch/arm/vgic-v3.c        | 14 --------------
>   xen/include/asm-arm/domain.h  |  1 -
>   xen/include/asm-arm/vgic.h    |  1 -
>   xen/include/public/arch-arm.h |  2 --
>   5 files changed, 1 insertion(+), 20 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 1684d9cf1b..588fe94cab 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1693,8 +1693,7 @@ static int __init gicv3_init(void)
>       reg = readl_relaxed(GICD + GICD_TYPER);
>       intid_bits = GICD_TYPE_ID_BITS(reg);
>   
> -    vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions,
> -                     gicv3.rdist_stride, intid_bits);
> +    vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions, intid_bits);
>       gicv3_init_v2();
>   
>       spin_lock_init(&gicv3.lock);
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index b4159317dd..47856a0a73 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -58,21 +58,18 @@ static struct {
>       /* Re-distributor regions */
>       unsigned int nr_rdist_regions;
>       const struct rdist_region *regions;
> -    uint32_t rdist_stride; /* Re-distributor stride */
>       unsigned int intid_bits;  /* Number of interrupt ID bits */
>   } 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,
>                         unsigned int intid_bits)
>   {
>       vgic_v3_hw.enabled = true;
>       vgic_v3_hw.dbase = dbase;
>       vgic_v3_hw.nr_rdist_regions = nr_rdist_regions;
>       vgic_v3_hw.regions = regions;
> -    vgic_v3_hw.rdist_stride = rdist_stride;
>       vgic_v3_hw.intid_bits = intid_bits;
>   }
>   
> @@ -1680,15 +1677,6 @@ static int vgic_v3_domain_init(struct domain *d)
>   
>           d->arch.vgic.dbase = vgic_v3_hw.dbase;
>   
> -        d->arch.vgic.rdist_stride = vgic_v3_hw.rdist_stride;
> -        /*
> -         * If the stride is not set, the default stride for GICv3 is 2 * 64K:
> -         *     - first 64k page for Control and Physical LPIs
> -         *     - second 64k page for Control and Generation of SGIs
> -         */
> -        if ( !d->arch.vgic.rdist_stride )
> -            d->arch.vgic.rdist_stride = 2 * SZ_64K;
> -
>           for ( i = 0; i < vgic_v3_hw.nr_rdist_regions; i++ )
>           {
>               paddr_t size = vgic_v3_hw.regions[i].size;
> @@ -1708,8 +1696,6 @@ static int vgic_v3_domain_init(struct domain *d)
>       {
>           d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE;
>   
> -        d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE;
> -
>           /* The first redistributor should contain enough space for all CPUs */
>           BUILD_BUG_ON((GUEST_GICV3_GICR0_SIZE / GICV3_GICR_SIZE) < MAX_VIRT_CPUS);
>           d->arch.vgic.rdist_regions[0].base = GUEST_GICV3_GICR0_BASE;
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 4fe189b1c3..3eda7196ff 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -108,7 +108,6 @@ struct arch_domain
>               unsigned int first_cpu;         /* First CPU handled */
>           } *rdist_regions;
>           int nr_regions;                     /* Number of rdist regions */
> -        uint32_t rdist_stride;              /* Re-Distributor stride */
>           unsigned long int nr_lpis;
>           uint64_t rdist_propbase;
>           struct rb_root its_devices;         /* Devices mapped to an ITS */
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 2a93a7bef9..7919cc53f4 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -255,7 +255,6 @@ 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,
>                         unsigned int intid_bits);
>   #endif
>   
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index ca79ab6284..3bca165fbf 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -401,8 +401,6 @@ typedef uint64_t xen_callback_t;
>   #define GUEST_GICV3_GICD_BASE      xen_mk_ullong(0x03001000)
>   #define GUEST_GICV3_GICD_SIZE      xen_mk_ullong(0x00010000)
>   
> -#define GUEST_GICV3_RDIST_STRIDE   xen_mk_ullong(0x00020000)
> -
>   #define GUEST_GICV3_GICR0_BASE     xen_mk_ullong(0x03020000) /* vCPU0..127 */
>   #define GUEST_GICV3_GICR0_SIZE     xen_mk_ullong(0x01000000)
>   
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel