[PATCH V3 01/10] arm/gic-v3: Use acpi_table_parse_madt() to parse MADT subtables

[PATCH V3 01/10] arm/gic-v3: Use acpi_table_parse_madt() to parse MADT subtables

[Shanker Donthineni]

The function acpi_table_parse_madt() does the same functionality as
function acpi_parse_entries() expect it takes a few arguments.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
 xen/arch/arm/gic-v3.c | 27 ++++++---------------------
 1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 8d3f149..166f1c1 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1390,28 +1390,15 @@ gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header *header,
 
 static void __init gicv3_acpi_init(void)
 {
-    struct acpi_table_header *table;
     struct rdist_region *rdist_regs;
-    acpi_status status;
     int count, i;
 
-    status = acpi_get_table(ACPI_SIG_MADT, 0, &table);
-
-    if ( ACPI_FAILURE(status) )
-    {
-        const char *msg = acpi_format_exception(status);
-
-        panic("GICv3: Failed to get MADT table, %s", msg);
-    }
-
     /*
      * Find distributor base address. We expect one distributor entry since
      * ACPI 5.0 spec neither support multi-GIC instances nor GIC cascade.
      */
-    count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt),
-                               gic_acpi_parse_madt_distributor, table,
-                               ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0);
-
+    count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
+                                  gic_acpi_parse_madt_distributor, 0);
     if ( count <= 0 )
         panic("GICv3: No valid GICD entries exists");
 
@@ -1420,9 +1407,8 @@ static void __init gicv3_acpi_init(void)
               dbase);
 
     /* Get number of redistributor */
-    count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt),
-                               gic_acpi_get_madt_redistributor_num, table,
-                               ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, 0);
+    count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
+                                  gic_acpi_get_madt_redistributor_num, 0);
     if ( count <= 0 )
         panic("GICv3: No valid GICR entries exists");
 
@@ -1458,9 +1444,8 @@ static void __init gicv3_acpi_init(void)
     gicv3.rdist_regions= rdist_regs;
 
     /* Collect CPU base addresses */
-    count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt),
-                               gic_acpi_parse_madt_cpu, table,
-                               ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0);
+    count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
+                                  gic_acpi_parse_madt_cpu, 0);
     if ( count <= 0 )
         panic("GICv3: No valid GICC entries exists");
 
-- 
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project


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

[PATCH V3 02/10] arm/gic-v3: Do early GICD ioremap and clean up

[Shanker Donthineni]

For ACPI based XEN boot, the GICD region needs to be accessed inside
the function gicv3_acpi_init() in later patch. There is a duplicate
panic() message, one in the DTS probe and second one in the ACPI probe
path. For these two reasons, move the code that validates the GICD base
address and does the region ioremap to a separate function. The
following patch accesses the GICD region inside gicv3_acpi_init() for
finding per CPU Redistributor size.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
Acked-by: Julien Grall <julien.grall@arm.com>
---
Changes sicne v1:
  Edited commit text.

 xen/arch/arm/gic-v3.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 166f1c1..1f8fbc4 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1169,6 +1169,17 @@ static void __init gicv3_init_v2(void)
     vgic_v2_setup_hw(dbase, cbase, csize, vbase, 0);
 }
 
+static void __init gicv3_ioremap_distributor(paddr_t dist_paddr)
+{
+    if ( dist_paddr & ~PAGE_MASK )
+        panic("GICv3:  Found unaligned distributor address %"PRIpaddr"",
+              dbase);
+
+    gicv3.map_dbase = ioremap_nocache(dist_paddr, SZ_64K);
+    if ( !gicv3.map_dbase )
+        panic("GICv3: Failed to ioremap for GIC distributor\n");
+}
+
 static void __init gicv3_dt_init(void)
 {
     struct rdist_region *rdist_regs;
@@ -1179,9 +1190,7 @@ static void __init gicv3_dt_init(void)
     if ( res )
         panic("GICv3: Cannot find a valid distributor address");
 
-    if ( (dbase & ~PAGE_MASK) )
-        panic("GICv3:  Found unaligned distributor address %"PRIpaddr"",
-              dbase);
+    gicv3_ioremap_distributor(dbase);
 
     if ( !dt_property_read_u32(node, "#redistributor-regions",
                 &gicv3.rdist_count) )
@@ -1402,9 +1411,7 @@ static void __init gicv3_acpi_init(void)
     if ( count <= 0 )
         panic("GICv3: No valid GICD entries exists");
 
-    if ( (dbase & ~PAGE_MASK) )
-        panic("GICv3: Found unaligned distributor address %"PRIpaddr"",
-              dbase);
+    gicv3_ioremap_distributor(dbase);
 
     /* Get number of redistributor */
     count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
@@ -1476,10 +1483,6 @@ static int __init gicv3_init(void)
     else
         gicv3_acpi_init();
 
-    gicv3.map_dbase = ioremap_nocache(dbase, SZ_64K);
-    if ( !gicv3.map_dbase )
-        panic("GICv3: Failed to ioremap for GIC distributor\n");
-
     reg = readl_relaxed(GICD + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
     if ( reg != GIC_PIDR2_ARCH_GICv3 && reg != GIC_PIDR2_ARCH_GICv4 )
          panic("GICv3: no distributor detected\n");
-- 
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project


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

[PATCH V3 03/10] arm/gic-v3: Move GICR subtable parsing into a new function

[Shanker Donthineni]

Add a new function to parse GICR subtable and move the code that
is specific to GICR table to a new function without changing the
function gicv3_acpi_init() behavior.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
Changes since v2:
  Changed function gic_acpi_add_rdist_region() protoype.
  Removed the address validation check in gic_acpi_parse_madt_redistributor().
  Edited commit text.

Changes since v1:
  Removed the unnecessary GICR ioremap operation inside GICR table parse code.


 xen/arch/arm/gic-v3.c | 56 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 1f8fbc4..efdb56b 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1282,6 +1282,14 @@ static int gicv3_iomem_deny_access(const struct domain *d)
 }
 
 #ifdef CONFIG_ACPI
+static void __init gic_acpi_add_rdist_region(paddr_t base, paddr_t size)
+{
+    unsigned int idx = gicv3.rdist_count++;
+
+    gicv3.rdist_regions[idx].base = base;
+    gicv3.rdist_regions[idx].size = size;
+}
+
 static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
 {
     struct acpi_subtable_header *header;
@@ -1387,6 +1395,22 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header,
 
     return 0;
 }
+
+static int __init
+gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header,
+                                  const unsigned long end)
+{
+    struct acpi_madt_generic_redistributor *rdist;
+
+    rdist = (struct acpi_madt_generic_redistributor *)header;
+    if ( BAD_MADT_ENTRY(rdist, end) )
+        return -EINVAL;
+
+    gic_acpi_add_rdist_region(rdist->base_address, rdist->length);
+
+    return 0;
+}
+
 static int __init
 gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header *header,
                                     const unsigned long end)
@@ -1400,7 +1424,7 @@ gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header *header,
 static void __init gicv3_acpi_init(void)
 {
     struct rdist_region *rdist_regs;
-    int count, i;
+    int count;
 
     /*
      * Find distributor base address. We expect one distributor entry since
@@ -1419,37 +1443,25 @@ static void __init gicv3_acpi_init(void)
     if ( count <= 0 )
         panic("GICv3: No valid GICR entries exists");
 
-    gicv3.rdist_count = count;
-
-    if ( gicv3.rdist_count > MAX_RDIST_COUNT )
+    if ( count > MAX_RDIST_COUNT )
         panic("GICv3: Number of redistributor regions is more than"
               "%d (Increase MAX_RDIST_COUNT!!)\n", MAX_RDIST_COUNT);
 
-    rdist_regs = xzalloc_array(struct rdist_region, gicv3.rdist_count);
+    rdist_regs = xzalloc_array(struct rdist_region, count);
     if ( !rdist_regs )
         panic("GICv3: Failed to allocate memory for rdist regions\n");
 
-    for ( i = 0; i < gicv3.rdist_count; i++ )
-    {
-        struct acpi_subtable_header *header;
-        struct acpi_madt_generic_redistributor *gic_rdist;
-
-        header = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
-                                           i);
-        if ( !header )
-            panic("GICv3: Can't get GICR entry");
-
-        gic_rdist =
-           container_of(header, struct acpi_madt_generic_redistributor, header);
-        rdist_regs[i].base = gic_rdist->base_address;
-        rdist_regs[i].size = gic_rdist->length;
-    }
+    gicv3.rdist_regions = rdist_regs;
+
+    /* Parse always-on power domain Re-distributor entries */
+    count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
+                                  gic_acpi_parse_madt_redistributor, count);
+    if ( count <= 0 )
+        panic("GICv3: Can't get Redistributor entry");
 
     /* The vGIC code requires the region to be sorted */
     sort(rdist_regs, gicv3.rdist_count, sizeof(*rdist_regs), cmp_rdist, NULL);
 
-    gicv3.rdist_regions= rdist_regs;
-
     /* Collect CPU base addresses */
     count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
                                   gic_acpi_parse_madt_cpu, 0);
-- 
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project


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

[PATCH V3 04/10] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable

[Shanker Donthineni]

The redistributor address can be specified either as part of GICC or
GICR subtable depending on the power domain. The current driver
doesn't support parsing redistributor entry that is defined in GICC
subtable. The GIC CPU subtable entry holds the associated Redistributor
base address if it is not on always-on power domain.

The per CPU Redistributor size is not defined in ACPI specification.
Set the GICR region size to SZ_256K if the GIC hardware is capable of
Direct Virtual LPI Injection feature, SZ_128K otherwise.

This patch adds necessary code to handle both types of Redistributors
base addresses.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
Changes since v2:
  Removed the unnecessary validation checks and edited commit text.

 xen/arch/arm/gic-v3.c             | 68 +++++++++++++++++++++++++++++++++++----
 xen/include/asm-arm/gic.h         |  1 +
 xen/include/asm-arm/gic_v3_defs.h |  1 +
 3 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index efdb56b..352799e 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -659,6 +659,10 @@ static int __init gicv3_populate_rdist(void)
                         smp_processor_id(), i, ptr);
                 return 0;
             }
+
+            if ( gicv3.rdist_regions[i].single_rdist )
+                break;
+
             if ( gicv3.rdist_stride )
                 ptr += gicv3.rdist_stride;
             else
@@ -1282,14 +1286,21 @@ static int gicv3_iomem_deny_access(const struct domain *d)
 }
 
 #ifdef CONFIG_ACPI
-static void __init gic_acpi_add_rdist_region(paddr_t base, paddr_t size)
+static void __init
+gic_acpi_add_rdist_region(paddr_t base, paddr_t size, bool single_rdist)
 {
     unsigned int idx = gicv3.rdist_count++;
 
+    gicv3.rdist_regions[idx].single_rdist = single_rdist;
     gicv3.rdist_regions[idx].base = base;
     gicv3.rdist_regions[idx].size = size;
 }
 
+static inline bool gic_dist_supports_dvis(void)
+{
+    return !!(readl_relaxed(GICD + GICD_TYPER) & GICD_TYPER_DVIS);
+}
+
 static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
 {
     struct acpi_subtable_header *header;
@@ -1397,6 +1408,36 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header,
 }
 
 static int __init
+gic_acpi_parse_cpu_redistributor(struct acpi_subtable_header *header,
+                                 const unsigned long end)
+{
+    struct acpi_madt_generic_interrupt *processor;
+    u32 size;
+
+    processor = (struct acpi_madt_generic_interrupt *)header;
+    if ( !(processor->flags & ACPI_MADT_ENABLED) )
+        return 0;
+
+    size = gic_dist_supports_dvis() ? 4 * SZ_64K : 2 * SZ_64K;
+    gic_acpi_add_rdist_region(processor->gicr_base_address, size, true);
+
+    return 0;
+}
+
+static int __init
+gic_acpi_get_madt_cpu_num(struct acpi_subtable_header *header,
+                          const unsigned long end)
+{
+    struct acpi_madt_generic_interrupt *cpuif;
+
+    cpuif = (struct acpi_madt_generic_interrupt *)header;
+    if ( BAD_MADT_ENTRY(cpuif, end) || !cpuif->gicr_base_address )
+        return -EINVAL;
+
+    return 0;
+}
+
+static int __init
 gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header,
                                   const unsigned long end)
 {
@@ -1406,7 +1447,7 @@ gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header,
     if ( BAD_MADT_ENTRY(rdist, end) )
         return -EINVAL;
 
-    gic_acpi_add_rdist_region(rdist->base_address, rdist->length);
+    gic_acpi_add_rdist_region(rdist->base_address, rdist->length, false);
 
     return 0;
 }
@@ -1424,6 +1465,7 @@ gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header *header,
 static void __init gicv3_acpi_init(void)
 {
     struct rdist_region *rdist_regs;
+    bool gicr_table = true;
     int count;
 
     /*
@@ -1440,8 +1482,15 @@ static void __init gicv3_acpi_init(void)
     /* Get number of redistributor */
     count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
                                   gic_acpi_get_madt_redistributor_num, 0);
-    if ( count <= 0 )
-        panic("GICv3: No valid GICR entries exists");
+    /* Count the total number of CPU interface entries */
+    if ( count <= 0 ) {
+        count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
+                                      gic_acpi_get_madt_cpu_num, 0);
+        if (count <= 0)
+            panic("GICv3: No valid GICR entries exists");
+
+        gicr_table = false;
+    }
 
     if ( count > MAX_RDIST_COUNT )
         panic("GICv3: Number of redistributor regions is more than"
@@ -1453,9 +1502,14 @@ static void __init gicv3_acpi_init(void)
 
     gicv3.rdist_regions = rdist_regs;
 
-    /* Parse always-on power domain Re-distributor entries */
-    count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
-                                  gic_acpi_parse_madt_redistributor, count);
+    if ( gicr_table )
+        /* Parse always-on power domain Re-distributor entries */
+        count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
+                                      gic_acpi_parse_madt_redistributor, count);
+    else
+        /* Parse Re-distributor entries described in CPU interface table */
+        count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
+                                      gic_acpi_parse_cpu_redistributor, count);
     if ( count <= 0 )
         panic("GICv3: Can't get Redistributor entry");
 
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 44b9ef6..fedf1fa 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -101,6 +101,7 @@
 #define GICD_TYPE_CPUS_SHIFT 5
 #define GICD_TYPE_CPUS  0x0e0
 #define GICD_TYPE_SEC   0x400
+#define GICD_TYPER_DVIS (1U << 18)
 
 #define GICC_CTL_ENABLE 0x1
 #define GICC_CTL_EOI    (0x1 << 9)
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index 6d98491..6bd25a5 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -141,6 +141,7 @@ struct rdist_region {
     paddr_t base;
     paddr_t size;
     void __iomem *map_base;
+    bool single_rdist;
 };
 
 #endif /* __ASM_ARM_GIC_V3_DEFS_H__ */
-- 
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project


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

[PATCH V3 05/10] xen/arm: vgic: Use dynamic memory allocation for vgic_rdist_region

[Shanker Donthineni]

The number of Redistributor regions allowed for dom0 is hardcoded
to a define MAX_RDIST_COUNT which is 4. Some systems, especially
latest server chips, may have more than 4 redistributors. Either we
have to increase MAX_RDIST_COUNT to a bigger number or allocate
memory based on the number of redistributors that are found in MADT
table. In the worst case scenario, the macro MAX_RDIST_COUNT should
be equal to CONFIG_NR_CPUS in order to support per CPU Redistributors.

Increasing MAX_RDIST_COUNT has a effect, it blows 'struct domain'
size and hits BUILD_BUG_ON() in domain build code path.

struct domain *alloc_domain_struct(void)
{
    struct domain *d;
    BUILD_BUG_ON(sizeof(*d) > PAGE_SIZE);
    d = alloc_xenheap_pages(0, 0);
    if ( d == NULL )
        return NULL;
...

This patch uses the second approach to fix the BUILD_BUG().

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
Changes sicne v2:
  Edited commit text.

 xen/arch/arm/vgic-v2.c       |  6 ++++++
 xen/arch/arm/vgic-v3.c       | 27 ++++++++++++++++++++++++---
 xen/arch/arm/vgic.c          |  1 +
 xen/include/asm-arm/domain.h |  2 +-
 xen/include/asm-arm/vgic.h   |  2 ++
 5 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 9adb4a9..f5778e6 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -699,9 +699,15 @@ static int vgic_v2_domain_init(struct domain *d)
     return 0;
 }
 
+static void vgic_v2_domain_free(struct domain *d)
+{
+    /* Nothing to be cleanup for this driver */
+}
+
 static const struct vgic_ops vgic_v2_ops = {
     .vcpu_init   = vgic_v2_vcpu_init,
     .domain_init = vgic_v2_domain_init,
+    .domain_free = vgic_v2_domain_free,
     .max_vcpus = 8,
 };
 
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index b37a7c0..be9a9a3 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1391,9 +1391,26 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
     return 0;
 }
 
+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;
+}
+
 static int vgic_v3_domain_init(struct domain *d)
 {
-    int i;
+    struct vgic_rdist_region *rdist_regions;
+    int rdist_count, i;
+
+    /* Allocate memory for Re-distributor regions */
+    rdist_count = vgic_v3_rdist_count(d);
+
+    rdist_regions = xzalloc_array(struct vgic_rdist_region, rdist_count);
+    if ( !rdist_regions )
+        return -ENOMEM;
+
+    d->arch.vgic.nr_regions = rdist_count;
+    d->arch.vgic.rdist_regions = rdist_regions;
 
     /*
      * Domain 0 gets the hardware address.
@@ -1426,7 +1443,6 @@ static int vgic_v3_domain_init(struct domain *d)
 
             first_cpu += size / d->arch.vgic.rdist_stride;
         }
-        d->arch.vgic.nr_regions = vgic_v3_hw.nr_rdist_regions;
     }
     else
     {
@@ -1435,7 +1451,6 @@ static int vgic_v3_domain_init(struct domain *d)
         /* XXX: Only one Re-distributor region mapped for the guest */
         BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1);
 
-        d->arch.vgic.nr_regions = GUEST_GICV3_RDIST_REGIONS;
         d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE;
 
         /* The first redistributor should contain enough space for all CPUs */
@@ -1467,9 +1482,15 @@ static int vgic_v3_domain_init(struct domain *d)
     return 0;
 }
 
+static void vgic_v3_domain_free(struct domain *d)
+{
+    xfree(d->arch.vgic.rdist_regions);
+}
+
 static const struct vgic_ops v3_ops = {
     .vcpu_init   = vgic_v3_vcpu_init,
     .domain_init = vgic_v3_domain_init,
+    .domain_free = vgic_v3_domain_free,
     .emulate_sysreg  = vgic_v3_emulate_sysreg,
     /*
      * We use both AFF1 and AFF0 in (v)MPIDR. Thus, the max number of CPU
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 3e1c572..5df5f01 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -177,6 +177,7 @@ void domain_vgic_free(struct domain *d)
         }
     }
 
+    d->arch.vgic.handler->domain_free(d);
     xfree(d->arch.vgic.shared_irqs);
     xfree(d->arch.vgic.pending_irqs);
     xfree(d->arch.vgic.allocated_irqs);
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 370cdeb..29346c6 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -107,7 +107,7 @@ struct arch_domain
             paddr_t base;                   /* Base address */
             paddr_t size;                   /* Size */
             unsigned int first_cpu;         /* First CPU handled */
-        } rdist_regions[MAX_RDIST_COUNT];
+        } *rdist_regions;
         int nr_regions;                     /* Number of rdist regions */
         uint32_t rdist_stride;              /* Re-Distributor stride */
 #endif
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index a2fccc0..c3cc4f6 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -128,6 +128,8 @@ struct vgic_ops {
     int (*vcpu_init)(struct vcpu *v);
     /* Domain specific initialization of vGIC */
     int (*domain_init)(struct domain *d);
+    /* Release resources that were allocated by domain_init */
+    void (*domain_free)(struct domain *d);
     /* vGIC sysreg emulation */
     int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr hsr);
     /* Maximum number of vCPU supported */
-- 
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project


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

[PATCH v3 06/10] arm/gic-v3: Remove an unused macro MAX_RDIST_COUNT

[Shanker Donthineni]

The macro MAX_RDIST_COUNT is not being used after converting code
to handle number of redistributor dynamically. So remove it from
header file and the two other panic() messages that are not valid
anymore.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
Acked-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/gic-v3.c     | 8 --------
 xen/include/asm-arm/gic.h | 1 -
 2 files changed, 9 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 352799e..948052b 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1200,10 +1200,6 @@ static void __init gicv3_dt_init(void)
                 &gicv3.rdist_count) )
         gicv3.rdist_count = 1;
 
-    if ( gicv3.rdist_count > MAX_RDIST_COUNT )
-        panic("GICv3: Number of redistributor regions is more than"
-              "%d (Increase MAX_RDIST_COUNT!!)\n", MAX_RDIST_COUNT);
-
     rdist_regs = xzalloc_array(struct rdist_region, gicv3.rdist_count);
     if ( !rdist_regs )
         panic("GICv3: Failed to allocate memory for rdist regions\n");
@@ -1492,10 +1488,6 @@ static void __init gicv3_acpi_init(void)
         gicr_table = false;
     }
 
-    if ( count > MAX_RDIST_COUNT )
-        panic("GICv3: Number of redistributor regions is more than"
-              "%d (Increase MAX_RDIST_COUNT!!)\n", MAX_RDIST_COUNT);
-
     rdist_regs = xzalloc_array(struct rdist_region, count);
     if ( !rdist_regs )
         panic("GICv3: Failed to allocate memory for rdist regions\n");
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index fedf1fa..db7b2d0 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -20,7 +20,6 @@
 
 #define NR_GIC_LOCAL_IRQS  NR_LOCAL_IRQS
 #define NR_GIC_SGI         16
-#define MAX_RDIST_COUNT    4
 
 #define GICD_CTLR       (0x000)
 #define GICD_TYPER      (0x004)
-- 
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project


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

[PATCH V3 07/10] arm: vgic: Split vgic_domain_init() functionality into two functions

[Shanker Donthineni]

Separate the code logic that does the registration of vgic_v3/v2 ops
to a new function domain_vgic_register(). The intention of this
separation is to record the required mmio count in vgic_v3/v2_init()
and pass it to function domain_io_init() in a follow-up patch patch.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
Changes since v2:
  Edited commit text.

 xen/arch/arm/vgic.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 5df5f01..f5e89af 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -88,19 +88,8 @@ static void vgic_rank_init(struct vgic_irq_rank *rank, uint8_t index,
         rank->vcpu[i] = vcpu;
 }
 
-int domain_vgic_init(struct domain *d, unsigned int nr_spis)
+static int domain_vgic_register(struct domain *d)
 {
-    int i;
-    int ret;
-
-    d->arch.vgic.ctlr = 0;
-
-    /* Limit the number of virtual SPIs supported to (1020 - 32) = 988  */
-    if ( nr_spis > (1020 - NR_LOCAL_IRQS) )
-        return -EINVAL;
-
-    d->arch.vgic.nr_spis = nr_spis;
-
     switch ( d->arch.vgic.version )
     {
 #ifdef CONFIG_HAS_GICV3
@@ -119,6 +108,26 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
         return -ENODEV;
     }
 
+    return 0;
+}
+
+int domain_vgic_init(struct domain *d, unsigned int nr_spis)
+{
+    int i;
+    int ret;
+
+    d->arch.vgic.ctlr = 0;
+
+    /* Limit the number of virtual SPIs supported to (1020 - 32) = 988  */
+    if ( nr_spis > (1020 - NR_LOCAL_IRQS) )
+        return -EINVAL;
+
+    d->arch.vgic.nr_spis = nr_spis;
+
+    ret = domain_vgic_register(d);
+    if ( ret < 0 )
+        return ret;
+
     spin_lock_init(&d->arch.vgic.lock);
 
     d->arch.vgic.shared_irqs =
-- 
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project


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

[PATCH V3 08/10] arm/io: Use separate memory allocation for mmio handlers

[Shanker Donthineni]

The number of mmio handlers are limited to a compile time macro
MAX_IO_HANDLER which is 16. This number is not at all sufficient
to support per CPU distributor regions. Either it needs to be
increased to a bigger number, at least CONFIG_NR_CPUS+16, or
allocate a separate memory for mmio handlers dynamically during
domain build.

This patch uses the dynamic allocation strategy to reduce memory
footprint for 'struct domain' instead of static allocation.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
Acked-by: Julien Grall <julien.grall@arm.com>
---
Changes since v1:
  Moved registration of vgic_v3/v2 functionality to a new domain_vgic_register().

 xen/arch/arm/domain.c      |  6 ++++--
 xen/arch/arm/io.c          | 14 ++++++++++++--
 xen/include/asm-arm/mmio.h |  6 ++++--
 3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 1365b4a..4010ff2 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -527,7 +527,7 @@ void vcpu_destroy(struct vcpu *v)
 int arch_domain_create(struct domain *d, unsigned int domcr_flags,
                        struct xen_arch_domainconfig *config)
 {
-    int rc;
+    int rc, count;
 
     d->arch.relmem = RELMEM_not_started;
 
@@ -550,7 +550,8 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     share_xen_page_with_guest(
         virt_to_page(d->shared_info), d, XENSHARE_writable);
 
-    if ( (rc = domain_io_init(d)) != 0 )
+    count = MAX_IO_HANDLER;
+    if ( (rc = domain_io_init(d, count)) != 0 )
         goto fail;
 
     if ( (rc = p2m_alloc_table(d)) != 0 )
@@ -644,6 +645,7 @@ void arch_domain_destroy(struct domain *d)
     free_xenheap_pages(d->arch.efi_acpi_table,
                        get_order_from_bytes(d->arch.efi_acpi_len));
 #endif
+    domain_io_free(d);
 }
 
 void arch_domain_shutdown(struct domain *d)
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 0156755..a5b2c2d 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -102,7 +102,7 @@ void register_mmio_handler(struct domain *d,
     struct vmmio *vmmio = &d->arch.vmmio;
     struct mmio_handler *handler;
 
-    BUG_ON(vmmio->num_entries >= MAX_IO_HANDLER);
+    BUG_ON(vmmio->num_entries >= vmmio->max_num_entries);
 
     spin_lock(&vmmio->lock);
 
@@ -125,14 +125,24 @@ void register_mmio_handler(struct domain *d,
     spin_unlock(&vmmio->lock);
 }
 
-int domain_io_init(struct domain *d)
+int domain_io_init(struct domain *d, int max_count)
 {
    spin_lock_init(&d->arch.vmmio.lock);
    d->arch.vmmio.num_entries = 0;
 
+   d->arch.vmmio.max_num_entries = max_count;
+   d->arch.vmmio.handlers = xzalloc_array(struct mmio_handler, max_count);
+   if ( !d->arch.vmmio.handlers )
+       return -ENOMEM;
+
    return 0;
 }
 
+void domain_io_free(struct domain *d)
+{
+    xfree(d->arch.vmmio.handlers);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/mmio.h b/xen/include/asm-arm/mmio.h
index da1cc2e..276b263 100644
--- a/xen/include/asm-arm/mmio.h
+++ b/xen/include/asm-arm/mmio.h
@@ -51,15 +51,17 @@ struct mmio_handler {
 
 struct vmmio {
     int num_entries;
+    int max_num_entries;
     spinlock_t lock;
-    struct mmio_handler handlers[MAX_IO_HANDLER];
+    struct mmio_handler *handlers;
 };
 
 extern int handle_mmio(mmio_info_t *info);
 void register_mmio_handler(struct domain *d,
                            const struct mmio_handler_ops *ops,
                            paddr_t addr, paddr_t size, void *priv);
-int domain_io_init(struct domain *d);
+int domain_io_init(struct domain *d, int max_count);
+void domain_io_free(struct domain *d);
 
 #endif  /* __ASM_ARM_MMIO_H__ */
 
-- 
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project


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

[PATCH V3 09/10] xen/arm: io: Use binary search for mmio handler lookup

[Shanker Donthineni]

As the number of I/O handlers increase, the overhead associated with
linear lookup also increases. The system might have maximum of 144
(assuming CONFIG_NR_CPUS=128) mmio handlers. In worst case scenario,
it would require 144 iterations for finding a matching handler. Now
it is time for us to change from linear (complexity O(n)) to a binary
search (complexity O(log n) for reducing mmio handler lookup overhead.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
Changes since v2:
  Converted mmio lookup code to a critical section.
  Copied the function bsreach() from Linux kernel.

 xen/arch/arm/io.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 84 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index a5b2c2d..c31fdf3 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -20,9 +20,50 @@
 #include <xen/lib.h>
 #include <xen/spinlock.h>
 #include <xen/sched.h>
+#include <xen/sort.h>
 #include <asm/current.h>
 #include <asm/mmio.h>
 
+/*
+ * bsearch - binary search an array of elements
+ * @key: pointer to item being searched for
+ * @base: pointer to first element to search
+ * @num: number of elements
+ * @size: size of each element
+ * @cmp: pointer to comparison function
+ *
+ * This function does a binary search on the given array.  The
+ * contents of the array should already be in ascending sorted order
+ * under the provided comparison function.
+ *
+ * Note that the key need not have the same type as the elements in
+ * the array, e.g. key could be a string and the comparison function
+ * could compare the string with the struct's name field.  However, if
+ * the key and elements in the array are of the same type, you can use
+ * the same comparison function for both sort() and bsearch().
+ */
+static void *bsearch(const void *key, const void *base, size_t num, size_t size,
+                     int (*cmp)(const void *key, const void *elt))
+{
+    size_t start = 0, end = num;
+    int result;
+
+    while ( start < end )
+    {
+        size_t mid = start + (end - start) / 2;
+
+        result = cmp(key, base + mid * size);
+        if ( result < 0 )
+            end = mid;
+        else if ( result > 0 )
+            start = mid + 1;
+        else
+            return (void *)base + mid * size;
+    }
+
+    return NULL;
+}
+
 static int handle_read(const struct mmio_handler *handler, struct vcpu *v,
                        mmio_info_t *info)
 {
@@ -70,23 +111,41 @@ static int handle_write(const struct mmio_handler *handler, struct vcpu *v,
                                handler->priv);
 }
 
-int handle_mmio(mmio_info_t *info)
+static int match_mmio_handler(const void *key, const void *elem)
 {
-    struct vcpu *v = current;
-    int i;
-    const struct mmio_handler *handler = NULL;
-    const struct vmmio *vmmio = &v->domain->arch.vmmio;
+    const struct mmio_handler *handler = elem;
+    paddr_t addr = (paddr_t)key;
 
-    for ( i = 0; i < vmmio->num_entries; i++ )
-    {
-        handler = &vmmio->handlers[i];
+    if ( addr < handler->addr )
+        return -1;
 
-        if ( (info->gpa >= handler->addr) &&
-             (info->gpa < (handler->addr + handler->size)) )
-            break;
-    }
+    if ( addr > (handler->addr + handler->size) )
+        return 1;
+
+    return 0;
+}
 
-    if ( i == vmmio->num_entries )
+static const struct mmio_handler *
+find_mmio_handler(struct vcpu *v, paddr_t addr)
+{
+    struct vmmio *vmmio = &v->domain->arch.vmmio;
+    const struct mmio_handler *handler;
+
+    spin_lock(&vmmio->lock);
+    handler = bsearch((const void *)addr, vmmio->handlers, vmmio->num_entries,
+                      sizeof(*handler), match_mmio_handler);
+    spin_unlock(&vmmio->lock);
+
+    return handler;
+}
+
+int handle_mmio(mmio_info_t *info)
+{
+    const struct mmio_handler *handler;
+    struct vcpu *v = current;
+
+    handler = find_mmio_handler(v, info->gpa);
+    if ( !handler )
         return 0;
 
     if ( info->dabt.write )
@@ -95,6 +154,14 @@ int handle_mmio(mmio_info_t *info)
         return handle_read(handler, v, info);
 }
 
+static int cmp_mmio_handler(const void *key, const void *elem)
+{
+    const struct mmio_handler *handler0 = key;
+    const struct mmio_handler *handler1 = elem;
+
+    return (handler0->addr < handler1->addr) ? -1 : 0;
+}
+
 void register_mmio_handler(struct domain *d,
                            const struct mmio_handler_ops *ops,
                            paddr_t addr, paddr_t size, void *priv)
@@ -122,6 +189,10 @@ void register_mmio_handler(struct domain *d,
 
     vmmio->num_entries++;
 
+    /* Sort mmio handlers in ascending order based on base address */
+    sort(vmmio->handlers, vmmio->num_entries, sizeof(struct mmio_handler),
+         cmp_mmio_handler, NULL);
+
     spin_unlock(&vmmio->lock);
 }
 
-- 
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project


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

[PATCH V3 10/10] arm/vgic: Change fixed number of mmio handlers to variable number

[Shanker Donthineni]

Record the number of mmio handlers that are required for vGICv3/2
in variable 'arch_domain.vgic.mmio_count' in vgic_v3/v2_init().
Augment this variable number to a fixed number MAX_IO_HANDLER and
pass it to domain_io_init() to allocate enough memory for handlers.

New code path:
 domain_vgic_register()
   count = MAX_IO_HANDLER + d->arch.vgic.mmio_count;
   domain_io_init(count)
     domain_vgic_init()

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
 xen/arch/arm/domain.c        | 11 +++++++----
 xen/arch/arm/vgic-v2.c       |  1 +
 xen/arch/arm/vgic-v3.c       |  3 +++
 xen/arch/arm/vgic.c          |  6 +-----
 xen/include/asm-arm/domain.h |  1 +
 xen/include/asm-arm/vgic.h   |  1 +
 6 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 4010ff2..ebc12ac 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -550,10 +550,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     share_xen_page_with_guest(
         virt_to_page(d->shared_info), d, XENSHARE_writable);
 
-    count = MAX_IO_HANDLER;
-    if ( (rc = domain_io_init(d, count)) != 0 )
-        goto fail;
-
     if ( (rc = p2m_alloc_table(d)) != 0 )
         goto fail;
 
@@ -590,6 +586,13 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
         goto fail;
     }
 
+    if ( (rc = domain_vgic_register(d)) != 0 )
+        goto fail;
+
+    count = MAX_IO_HANDLER + d->arch.vgic.mmio_count;
+    if ( (rc = domain_io_init(d, count)) != 0 )
+        goto fail;
+
     if ( (rc = domain_vgic_init(d, config->nr_spis)) != 0 )
         goto fail;
 
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index f5778e6..d5367b3 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -721,6 +721,7 @@ int vgic_v2_init(struct domain *d)
         return -ENODEV;
     }
 
+    d->arch.vgic.mmio_count = 1; /* Only GICD region */
     register_vgic_ops(d, &vgic_v2_ops);
 
     return 0;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index be9a9a3..e527f3f 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1509,6 +1509,9 @@ int vgic_v3_init(struct domain *d)
         return -ENODEV;
     }
 
+    /* GICD region + number of Re-distributors */
+    d->arch.vgic.mmio_count = vgic_v3_rdist_count(d) + 1;
+
     register_vgic_ops(d, &v3_ops);
 
     return 0;
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index f5e89af..0658bfc 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -88,7 +88,7 @@ static void vgic_rank_init(struct vgic_irq_rank *rank, uint8_t index,
         rank->vcpu[i] = vcpu;
 }
 
-static int domain_vgic_register(struct domain *d)
+int domain_vgic_register(struct domain *d)
 {
     switch ( d->arch.vgic.version )
     {
@@ -124,10 +124,6 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
 
     d->arch.vgic.nr_spis = nr_spis;
 
-    ret = domain_vgic_register(d);
-    if ( ret < 0 )
-        return ret;
-
     spin_lock_init(&d->arch.vgic.lock);
 
     d->arch.vgic.shared_irqs =
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 29346c6..b205461 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -111,6 +111,7 @@ struct arch_domain
         int nr_regions;                     /* Number of rdist regions */
         uint32_t rdist_stride;              /* Re-Distributor stride */
 #endif
+        uint32_t mmio_count;                /* Number of mmio handlers */
     } vgic;
 
     struct vuart {
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index c3cc4f6..a693f95 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -307,6 +307,7 @@ extern void register_vgic_ops(struct domain *d, const struct vgic_ops *ops);
 int vgic_v2_init(struct domain *d);
 int vgic_v3_init(struct domain *d);
 
+extern int domain_vgic_register(struct domain *d);
 extern int vcpu_vgic_free(struct vcpu *v);
 extern int vgic_to_sgi(struct vcpu *v, register_t sgir,
                        enum gic_sgi_mode irqmode, int virq,
-- 
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project


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

Re: [PATCH V3 09/10] xen/arm: io: Use binary search for mmio handler lookup

[Julien Grall]

Hi Shanker,

On 27/06/16 21:33, Shanker Donthineni wrote:
> As the number of I/O handlers increase, the overhead associated with
> linear lookup also increases. The system might have maximum of 144
> (assuming CONFIG_NR_CPUS=128) mmio handlers. In worst case scenario,
> it would require 144 iterations for finding a matching handler. Now
> it is time for us to change from linear (complexity O(n)) to a binary
> search (complexity O(log n) for reducing mmio handler lookup overhead.

However, you will add contention because the code is using a spinlock.
I am planning to send the following patch as a prerequisite of this series
to switch from spinlock to read-write lock:

commit b69e975ce25b2c94f7205b0b8329f351327fbcf7
Author: Julien Grall <julien.grall@arm.com>
Date:   Tue Jun 28 11:04:11 2016 +0100

    xen/arm: io: Protect the handlers with a read-write lock
    
    Currently, accessing the I/O handlers does not require to take a lock
    because new handlers are always added at the end of the array. In a
    follow-up patch, this array will be sort to optimize the look up.
    
    Given that most of the time the I/O handlers will not be modify,
    using a spinlock will add contention when multiple vCPU are accessing
    the emulated MMIOs. So use a read-write lock to protected the handlers.
    
    Finally, take the opportunity to re-indent correctly domain_io_init.
    
    Signed-off-by: Julien Grall <julien.grall@arm.com>

diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 0156755..5a96836 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -70,23 +70,39 @@ static int handle_write(const struct mmio_handler *handler, struct vcpu *v,
                                handler->priv);
 }
 
-int handle_mmio(mmio_info_t *info)
+static const struct mmio_handler *find_mmio_handler(struct domain *d,
+                                                    paddr_t gpa)
 {
-    struct vcpu *v = current;
-    int i;
-    const struct mmio_handler *handler = NULL;
-    const struct vmmio *vmmio = &v->domain->arch.vmmio;
+    const struct mmio_handler *handler;
+    unsigned int i;
+    struct vmmio *vmmio = &d->arch.vmmio;
+
+    read_lock(&vmmio->lock);
 
     for ( i = 0; i < vmmio->num_entries; i++ )
     {
         handler = &vmmio->handlers[i];
 
-        if ( (info->gpa >= handler->addr) &&
-             (info->gpa < (handler->addr + handler->size)) )
+        if ( (gpa >= handler->addr) &&
+             (gpa < (handler->addr + handler->size)) )
             break;
     }
 
     if ( i == vmmio->num_entries )
+        handler = NULL;
+
+    read_unlock(&vmmio->lock);
+
+    return handler;
+}
+
+int handle_mmio(mmio_info_t *info)
+{
+    struct vcpu *v = current;
+    const struct mmio_handler *handler = NULL;
+
+    handler = find_mmio_handler(v->domain, info->gpa);
+    if ( !handler )
         return 0;
 
     if ( info->dabt.write )
@@ -104,7 +120,7 @@ void register_mmio_handler(struct domain *d,
 
     BUG_ON(vmmio->num_entries >= MAX_IO_HANDLER);
 
-    spin_lock(&vmmio->lock);
+    write_lock(&vmmio->lock);
 
     handler = &vmmio->handlers[vmmio->num_entries];
 
@@ -113,24 +129,17 @@ void register_mmio_handler(struct domain *d,
     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);
-
     vmmio->num_entries++;
 
-    spin_unlock(&vmmio->lock);
+    write_unlock(&vmmio->lock);
 }
 
 int domain_io_init(struct domain *d)
 {
-   spin_lock_init(&d->arch.vmmio.lock);
-   d->arch.vmmio.num_entries = 0;
+    rwlock_init(&d->arch.vmmio.lock);
+    d->arch.vmmio.num_entries = 0;
 
-   return 0;
+    return 0;
 }
 
 /*
diff --git a/xen/include/asm-arm/mmio.h b/xen/include/asm-arm/mmio.h
index da1cc2e..32f10f2 100644
--- a/xen/include/asm-arm/mmio.h
+++ b/xen/include/asm-arm/mmio.h
@@ -20,6 +20,7 @@
 #define __ASM_ARM_MMIO_H__
 
 #include <xen/lib.h>
+#include <xen/rwlock.h>
 #include <asm/processor.h>
 #include <asm/regs.h>
 
@@ -51,7 +52,7 @@ struct mmio_handler {
 
 struct vmmio {
     int num_entries;
-    spinlock_t lock;
+    rwlock_t lock;
     struct mmio_handler handlers[MAX_IO_HANDLER];
 };

-- 
Julien Grall

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

Re: [PATCH V3 01/10] arm/gic-v3: Use acpi_table_parse_madt() to parse MADT subtables

[Julien Grall]

Hi Shanker,

On 27/06/16 21:33, Shanker Donthineni wrote:
> The function acpi_table_parse_madt() does the same functionality as
> function acpi_parse_entries() expect it takes a few arguments.
>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>

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

Regards,

> ---
>   xen/arch/arm/gic-v3.c | 27 ++++++---------------------
>   1 file changed, 6 insertions(+), 21 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 8d3f149..166f1c1 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1390,28 +1390,15 @@ gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header *header,
>
>   static void __init gicv3_acpi_init(void)
>   {
> -    struct acpi_table_header *table;
>       struct rdist_region *rdist_regs;
> -    acpi_status status;
>       int count, i;
>
> -    status = acpi_get_table(ACPI_SIG_MADT, 0, &table);
> -
> -    if ( ACPI_FAILURE(status) )
> -    {
> -        const char *msg = acpi_format_exception(status);
> -
> -        panic("GICv3: Failed to get MADT table, %s", msg);
> -    }
> -
>       /*
>        * Find distributor base address. We expect one distributor entry since
>        * ACPI 5.0 spec neither support multi-GIC instances nor GIC cascade.
>        */
> -    count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt),
> -                               gic_acpi_parse_madt_distributor, table,
> -                               ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0);
> -
> +    count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
> +                                  gic_acpi_parse_madt_distributor, 0);
>       if ( count <= 0 )
>           panic("GICv3: No valid GICD entries exists");
>
> @@ -1420,9 +1407,8 @@ static void __init gicv3_acpi_init(void)
>                 dbase);
>
>       /* Get number of redistributor */
> -    count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt),
> -                               gic_acpi_get_madt_redistributor_num, table,
> -                               ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, 0);
> +    count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
> +                                  gic_acpi_get_madt_redistributor_num, 0);
>       if ( count <= 0 )
>           panic("GICv3: No valid GICR entries exists");
>
> @@ -1458,9 +1444,8 @@ static void __init gicv3_acpi_init(void)
>       gicv3.rdist_regions= rdist_regs;
>
>       /* Collect CPU base addresses */
> -    count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt),
> -                               gic_acpi_parse_madt_cpu, table,
> -                               ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0);
> +    count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
> +                                  gic_acpi_parse_madt_cpu, 0);
>       if ( count <= 0 )
>           panic("GICv3: No valid GICC entries exists");
>
>

-- 
Julien Grall

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

Re: [PATCH V3 03/10] arm/gic-v3: Move GICR subtable parsing into a new function

[Julien Grall]

Hi Shanker,

On 27/06/16 21:33, Shanker Donthineni wrote:
> Add a new function to parse GICR subtable and move the code that
> is specific to GICR table to a new function without changing the
> function gicv3_acpi_init() behavior.
>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>

Acked-by: Julien Grall <julien.grall@arm.com>

Regards,

> ---
> Changes since v2:
>    Changed function gic_acpi_add_rdist_region() protoype.
>    Removed the address validation check in gic_acpi_parse_madt_redistributor().
>    Edited commit text.
>
> Changes since v1:
>    Removed the unnecessary GICR ioremap operation inside GICR table parse code.
>
>
>   xen/arch/arm/gic-v3.c | 56 +++++++++++++++++++++++++++++++--------------------
>   1 file changed, 34 insertions(+), 22 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 1f8fbc4..efdb56b 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1282,6 +1282,14 @@ static int gicv3_iomem_deny_access(const struct domain *d)
>   }
>
>   #ifdef CONFIG_ACPI
> +static void __init gic_acpi_add_rdist_region(paddr_t base, paddr_t size)
> +{
> +    unsigned int idx = gicv3.rdist_count++;
> +
> +    gicv3.rdist_regions[idx].base = base;
> +    gicv3.rdist_regions[idx].size = size;
> +}
> +
>   static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>   {
>       struct acpi_subtable_header *header;
> @@ -1387,6 +1395,22 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header,
>
>       return 0;
>   }
> +
> +static int __init
> +gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header,
> +                                  const unsigned long end)
> +{
> +    struct acpi_madt_generic_redistributor *rdist;
> +
> +    rdist = (struct acpi_madt_generic_redistributor *)header;
> +    if ( BAD_MADT_ENTRY(rdist, end) )
> +        return -EINVAL;
> +
> +    gic_acpi_add_rdist_region(rdist->base_address, rdist->length);
> +
> +    return 0;
> +}
> +
>   static int __init
>   gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header *header,
>                                       const unsigned long end)
> @@ -1400,7 +1424,7 @@ gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header *header,
>   static void __init gicv3_acpi_init(void)
>   {
>       struct rdist_region *rdist_regs;
> -    int count, i;
> +    int count;
>
>       /*
>        * Find distributor base address. We expect one distributor entry since
> @@ -1419,37 +1443,25 @@ static void __init gicv3_acpi_init(void)
>       if ( count <= 0 )
>           panic("GICv3: No valid GICR entries exists");
>
> -    gicv3.rdist_count = count;
> -
> -    if ( gicv3.rdist_count > MAX_RDIST_COUNT )
> +    if ( count > MAX_RDIST_COUNT )
>           panic("GICv3: Number of redistributor regions is more than"
>                 "%d (Increase MAX_RDIST_COUNT!!)\n", MAX_RDIST_COUNT);
>
> -    rdist_regs = xzalloc_array(struct rdist_region, gicv3.rdist_count);
> +    rdist_regs = xzalloc_array(struct rdist_region, count);
>       if ( !rdist_regs )
>           panic("GICv3: Failed to allocate memory for rdist regions\n");
>
> -    for ( i = 0; i < gicv3.rdist_count; i++ )
> -    {
> -        struct acpi_subtable_header *header;
> -        struct acpi_madt_generic_redistributor *gic_rdist;
> -
> -        header = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
> -                                           i);
> -        if ( !header )
> -            panic("GICv3: Can't get GICR entry");
> -
> -        gic_rdist =
> -           container_of(header, struct acpi_madt_generic_redistributor, header);
> -        rdist_regs[i].base = gic_rdist->base_address;
> -        rdist_regs[i].size = gic_rdist->length;
> -    }
> +    gicv3.rdist_regions = rdist_regs;
> +
> +    /* Parse always-on power domain Re-distributor entries */
> +    count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
> +                                  gic_acpi_parse_madt_redistributor, count);
> +    if ( count <= 0 )
> +        panic("GICv3: Can't get Redistributor entry");
>
>       /* The vGIC code requires the region to be sorted */
>       sort(rdist_regs, gicv3.rdist_count, sizeof(*rdist_regs), cmp_rdist, NULL);
>
> -    gicv3.rdist_regions= rdist_regs;
> -
>       /* Collect CPU base addresses */
>       count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
>                                     gic_acpi_parse_madt_cpu, 0);
>

-- 
Julien Grall

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

Re: [PATCH V3 04/10] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable

[Julien Grall]

Hello Shanker,

On 27/06/16 21:33, Shanker Donthineni wrote:
> @@ -1397,6 +1408,36 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header,
>   }
>
>   static int __init
> +gic_acpi_parse_cpu_redistributor(struct acpi_subtable_header *header,
> +                                 const unsigned long end)
> +{
> +    struct acpi_madt_generic_interrupt *processor;
> +    u32 size;
> +
> +    processor = (struct acpi_madt_generic_interrupt *)header;
> +    if ( !(processor->flags & ACPI_MADT_ENABLED) )
> +        return 0;

You did not answer to my question on previous version of this patch. You 
said that "Disabled GICC entries should be skipped because its 
Redistributor region is not always-on power domain." However from my 
understanding, an usable CPU may have his Redistributor in the not 
always-on power domain. So the issue would the same, correct?

Regards,

-- 
Julien Grall

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

Re: [PATCH V3 05/10] xen/arm: vgic: Use dynamic memory allocation for vgic_rdist_region

[Julien Grall]

Hi Shanker,

On 27/06/16 21:33, Shanker Donthineni wrote:
> The number of Redistributor regions allowed for dom0 is hardcoded
> to a define MAX_RDIST_COUNT which is 4. Some systems, especially
> latest server chips, may have more than 4 redistributors. Either we
> have to increase MAX_RDIST_COUNT to a bigger number or allocate
> memory based on the number of redistributors that are found in MADT
> table. In the worst case scenario, the macro MAX_RDIST_COUNT should
> be equal to CONFIG_NR_CPUS in order to support per CPU Redistributors.
>
> Increasing MAX_RDIST_COUNT has a effect, it blows 'struct domain'
> size and hits BUILD_BUG_ON() in domain build code path.
>
> struct domain *alloc_domain_struct(void)
> {
>      struct domain *d;
>      BUILD_BUG_ON(sizeof(*d) > PAGE_SIZE);
>      d = alloc_xenheap_pages(0, 0);
>      if ( d == NULL )
>          return NULL;
> ...
>
> This patch uses the second approach to fix the BUILD_BUG().
>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>

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

Regards,

-- 
Julien Grall

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

Re: [PATCH V3 07/10] arm: vgic: Split vgic_domain_init() functionality into two functions

[Julien Grall]

Hi Shanker,

On 27/06/16 21:33, Shanker Donthineni wrote:
> Separate the code logic that does the registration of vgic_v3/v2 ops
> to a new function domain_vgic_register(). The intention of this
> separation is to record the required mmio count in vgic_v3/v2_init()
> and pass it to function domain_io_init() in a follow-up patch patch.
>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>

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

Regards,

-- 
Julien Grall

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

Re: [PATCH V3 09/10] xen/arm: io: Use binary search for mmio handler lookup

[Julien Grall]

Hi Shanker,

On 27/06/16 21:33, Shanker Donthineni wrote:
> As the number of I/O handlers increase, the overhead associated with
> linear lookup also increases. The system might have maximum of 144
> (assuming CONFIG_NR_CPUS=128) mmio handlers. In worst case scenario,
> it would require 144 iterations for finding a matching handler. Now
> it is time for us to change from linear (complexity O(n)) to a binary
> search (complexity O(log n) for reducing mmio handler lookup overhead.
>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> ---
> Changes since v2:
>    Converted mmio lookup code to a critical section.
>    Copied the function bsreach() from Linux kernel.
>
>   xen/arch/arm/io.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 84 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index a5b2c2d..c31fdf3 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -20,9 +20,50 @@
>   #include <xen/lib.h>
>   #include <xen/spinlock.h>
>   #include <xen/sched.h>
> +#include <xen/sort.h>
>   #include <asm/current.h>
>   #include <asm/mmio.h>
>
> +/*
> + * bsearch - binary search an array of elements
> + * @key: pointer to item being searched for
> + * @base: pointer to first element to search
> + * @num: number of elements
> + * @size: size of each element
> + * @cmp: pointer to comparison function
> + *
> + * This function does a binary search on the given array.  The
> + * contents of the array should already be in ascending sorted order
> + * under the provided comparison function.
> + *
> + * Note that the key need not have the same type as the elements in
> + * the array, e.g. key could be a string and the comparison function
> + * could compare the string with the struct's name field.  However, if
> + * the key and elements in the array are of the same type, you can use
> + * the same comparison function for both sort() and bsearch().
> + */
> +static void *bsearch(const void *key, const void *base, size_t num, size_t size,
> +                     int (*cmp)(const void *key, const void *elt))

This function is not specific to I/O handlers. So this should be moved 
to common code. Also please mention in the commit message where the code 
came from.

> +{
> +    size_t start = 0, end = num;
> +    int result;
> +
> +    while ( start < end )
> +    {
> +        size_t mid = start + (end - start) / 2;
> +
> +        result = cmp(key, base + mid * size);
> +        if ( result < 0 )
> +            end = mid;
> +        else if ( result > 0 )
> +            start = mid + 1;
> +        else
> +            return (void *)base + mid * size;
> +    }
> +
> +    return NULL;
> +}
> +
>   static int handle_read(const struct mmio_handler *handler, struct vcpu *v,
>                          mmio_info_t *info)
>   {
> @@ -70,23 +111,41 @@ static int handle_write(const struct mmio_handler *handler, struct vcpu *v,
>                                  handler->priv);
>   }
>
> -int handle_mmio(mmio_info_t *info)
> +static int match_mmio_handler(const void *key, const void *elem)
>   {
> -    struct vcpu *v = current;
> -    int i;
> -    const struct mmio_handler *handler = NULL;
> -    const struct vmmio *vmmio = &v->domain->arch.vmmio;
> +    const struct mmio_handler *handler = elem;
> +    paddr_t addr = (paddr_t)key;
>
> -    for ( i = 0; i < vmmio->num_entries; i++ )
> -    {
> -        handler = &vmmio->handlers[i];
> +    if ( addr < handler->addr )
> +        return -1;
>
> -        if ( (info->gpa >= handler->addr) &&
> -             (info->gpa < (handler->addr + handler->size)) )
> -            break;
> -    }
> +    if ( addr > (handler->addr + handler->size) )
> +        return 1;
> +
> +    return 0;
> +}
>
> -    if ( i == vmmio->num_entries )
> +static const struct mmio_handler *
> +find_mmio_handler(struct vcpu *v, paddr_t addr)
> +{
> +    struct vmmio *vmmio = &v->domain->arch.vmmio;
> +    const struct mmio_handler *handler;
> +
> +    spin_lock(&vmmio->lock);
> +    handler = bsearch((const void *)addr, vmmio->handlers, vmmio->num_entries,

paddr_t is always 64-bit regardless the architecture (ARM64 vs ARM32). 
So the cast will lead to a compilation error on ARM32.

Please try to at least compile test your patch with ARM64, ARM32 and x86 
(when you touch common code).

Anyway, I would try to merge the two compare functions 
(match_mmio_handler, cmp_mmio_handler) which have very similar behavior.

> +                      sizeof(*handler), match_mmio_handler);
> +    spin_unlock(&vmmio->lock);
> +
> +    return handler;
> +}
> +
> +int handle_mmio(mmio_info_t *info)
> +{
> +    const struct mmio_handler *handler;
> +    struct vcpu *v = current;
> +
> +    handler = find_mmio_handler(v, info->gpa);
> +    if ( !handler )
>           return 0;
>
>       if ( info->dabt.write )
> @@ -95,6 +154,14 @@ int handle_mmio(mmio_info_t *info)
>           return handle_read(handler, v, info);
>   }
>
> +static int cmp_mmio_handler(const void *key, const void *elem)
> +{
> +    const struct mmio_handler *handler0 = key;
> +    const struct mmio_handler *handler1 = elem;
> +
> +    return (handler0->addr < handler1->addr) ? -1 : 0;
> +}
> +
>   void register_mmio_handler(struct domain *d,
>                              const struct mmio_handler_ops *ops,
>                              paddr_t addr, paddr_t size, void *priv)
> @@ -122,6 +189,10 @@ void register_mmio_handler(struct domain *d,
>
>       vmmio->num_entries++;
>
> +    /* Sort mmio handlers in ascending order based on base address */
> +    sort(vmmio->handlers, vmmio->num_entries, sizeof(struct mmio_handler),
> +         cmp_mmio_handler, NULL);
> +
>       spin_unlock(&vmmio->lock);
>   }
>
>

Regards,

-- 
Julien Grall

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

Re: [PATCH V3 09/10] xen/arm: io: Use binary search for mmio handler lookup

[Shanker Donthineni]

Hi Julien,

On 06/28/2016 05:49 AM, Julien Grall wrote:
> Hi Shanker,
>
> On 27/06/16 21:33, Shanker Donthineni wrote:
>> As the number of I/O handlers increase, the overhead associated with
>> linear lookup also increases. The system might have maximum of 144
>> (assuming CONFIG_NR_CPUS=128) mmio handlers. In worst case scenario,
>> it would require 144 iterations for finding a matching handler. Now
>> it is time for us to change from linear (complexity O(n)) to a binary
>> search (complexity O(log n) for reducing mmio handler lookup overhead.
>>
>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
>> ---
>> Changes since v2:
>>    Converted mmio lookup code to a critical section.
>>    Copied the function bsreach() from Linux kernel.
>>
>>   xen/arch/arm/io.c | 97
> +++++++++++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 84 insertions(+), 13 deletions(-)
>>
>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>> index a5b2c2d..c31fdf3 100644
>> --- a/xen/arch/arm/io.c
>> +++ b/xen/arch/arm/io.c
>> @@ -20,9 +20,50 @@
>>   #include <xen/lib.h>
>>   #include <xen/spinlock.h>
>>   #include <xen/sched.h>
>> +#include <xen/sort.h>
>>   #include <asm/current.h>
>>   #include <asm/mmio.h>
>>
>> +/*
>> + * bsearch - binary search an array of elements
>> + * @key: pointer to item being searched for
>> + * @base: pointer to first element to search
>> + * @num: number of elements
>> + * @size: size of each element
>> + * @cmp: pointer to comparison function
>> + *
>> + * This function does a binary search on the given array.  The
>> + * contents of the array should already be in ascending sorted order
>> + * under the provided comparison function.
>> + *
>> + * Note that the key need not have the same type as the elements in
>> + * the array, e.g. key could be a string and the comparison function
>> + * could compare the string with the struct's name field. However, if
>> + * the key and elements in the array are of the same type, you can use
>> + * the same comparison function for both sort() and bsearch().
>> + */
>> +static void *bsearch(const void *key, const void *base, size_t num,
> size_t size,
>> +                     int (*cmp)(const void *key, const void *elt))
>
> This function is not specific to I/O handlers. So this should be moved 
> to common code. Also please mention in the commit message where the 
> code came from.
>

Should I move to xen/arch/arm folder?

>> +{
>> +    size_t start = 0, end = num;
>> +    int result;
>> +
>> +    while ( start < end )
>> +    {
>> +        size_t mid = start + (end - start) / 2;
>> +
>> +        result = cmp(key, base + mid * size);
>> +        if ( result < 0 )
>> +            end = mid;
>> +        else if ( result > 0 )
>> +            start = mid + 1;
>> +        else
>> +            return (void *)base + mid * size;
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>>   static int handle_read(const struct mmio_handler *handler, struct vcpu
> *v,
>>                          mmio_info_t *info)
>>   {
>> @@ -70,23 +111,41 @@ static int handle_write(const struct mmio_handler
> *handler, struct vcpu *v,
>> handler->priv);
>>   }
>>
>> -int handle_mmio(mmio_info_t *info)
>> +static int match_mmio_handler(const void *key, const void *elem)
>>   {
>> -    struct vcpu *v = current;
>> -    int i;
>> -    const struct mmio_handler *handler = NULL;
>> -    const struct vmmio *vmmio = &v->domain->arch.vmmio;
>> +    const struct mmio_handler *handler = elem;
>> +    paddr_t addr = (paddr_t)key;
>>
>> -    for ( i = 0; i < vmmio->num_entries; i++ )
>> -    {
>> -        handler = &vmmio->handlers[i];
>> +    if ( addr < handler->addr )
>> +        return -1;
>>
>> -        if ( (info->gpa >= handler->addr) &&
>> -             (info->gpa < (handler->addr + handler->size)) )
>> -            break;
>> -    }
>> +    if ( addr > (handler->addr + handler->size) )
>> +        return 1;
>> +
>> +    return 0;
>> +}
>>
>> -    if ( i == vmmio->num_entries )
>> +static const struct mmio_handler *
>> +find_mmio_handler(struct vcpu *v, paddr_t addr)
>> +{
>> +    struct vmmio *vmmio = &v->domain->arch.vmmio;
>> +    const struct mmio_handler *handler;
>> +
>> +    spin_lock(&vmmio->lock);
>> +    handler = bsearch((const void *)addr, vmmio->handlers,
> vmmio->num_entries,
>
> paddr_t is always 64-bit regardless the architecture (ARM64 vs ARM32). 
> So the cast will lead to a compilation error on ARM32.
>

I'll fix.

> Please try to at least compile test your patch with ARM64, ARM32 and 
> x86 (when you touch common code).
>

Thanks, I'll follow next time.

> Anyway, I would try to merge the two compare functions 
> (match_mmio_handler, cmp_mmio_handler) which have very similar behavior.
>

Yes, they are not exactly same. One compares only start address and 
other one compares the range.

>> +                      sizeof(*handler), match_mmio_handler);
>> +    spin_unlock(&vmmio->lock);
>> +
>> +    return handler;
>> +}
>> +
>> +int handle_mmio(mmio_info_t *info)
>> +{
>> +    const struct mmio_handler *handler;
>> +    struct vcpu *v = current;
>> +
>> +    handler = find_mmio_handler(v, info->gpa);
>> +    if ( !handler )
>>           return 0;
>>
>>       if ( info->dabt.write )
>> @@ -95,6 +154,14 @@ int handle_mmio(mmio_info_t *info)
>>           return handle_read(handler, v, info);
>>   }
>>
>> +static int cmp_mmio_handler(const void *key, const void *elem)
>> +{
>> +    const struct mmio_handler *handler0 = key;
>> +    const struct mmio_handler *handler1 = elem;
>> +
>> +    return (handler0->addr < handler1->addr) ? -1 : 0;
>> +}
>> +
>>   void register_mmio_handler(struct domain *d,
>>                              const struct mmio_handler_ops *ops,
>>                              paddr_t addr, paddr_t size, void *priv)
>> @@ -122,6 +189,10 @@ void register_mmio_handler(struct domain *d,
>>
>>       vmmio->num_entries++;
>>
>> +    /* Sort mmio handlers in ascending order based on base address */
>> +    sort(vmmio->handlers, vmmio->num_entries, sizeof(struct
> mmio_handler),
>> +         cmp_mmio_handler, NULL);
>> +
>>       spin_unlock(&vmmio->lock);
>>   }
>>
>>
>
> Regards,
>

-- 
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project


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

Re: [PATCH V3 09/10] xen/arm: io: Use binary search for mmio handler lookup

[Julien Grall]

On 28/06/16 14:19, Shanker Donthineni wrote:
> On 06/28/2016 05:49 AM, Julien Grall wrote:
>> On 27/06/16 21:33, Shanker Donthineni wrote:
>>> As the number of I/O handlers increase, the overhead associated with
>>> linear lookup also increases. The system might have maximum of 144
>>> (assuming CONFIG_NR_CPUS=128) mmio handlers. In worst case scenario,
>>> it would require 144 iterations for finding a matching handler. Now
>>> it is time for us to change from linear (complexity O(n)) to a binary
>>> search (complexity O(log n) for reducing mmio handler lookup overhead.
>>>
>>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
>>> ---
>>> Changes since v2:
>>>    Converted mmio lookup code to a critical section.
>>>    Copied the function bsreach() from Linux kernel.
>>>
>>>   xen/arch/arm/io.c | 97
>> +++++++++++++++++++++++++++++++++++++++++++++++--------
>>>   1 file changed, 84 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>>> index a5b2c2d..c31fdf3 100644
>>> --- a/xen/arch/arm/io.c
>>> +++ b/xen/arch/arm/io.c
>>> @@ -20,9 +20,50 @@
>>>   #include <xen/lib.h>
>>>   #include <xen/spinlock.h>
>>>   #include <xen/sched.h>
>>> +#include <xen/sort.h>
>>>   #include <asm/current.h>
>>>   #include <asm/mmio.h>
>>>
>>> +/*
>>> + * bsearch - binary search an array of elements
>>> + * @key: pointer to item being searched for
>>> + * @base: pointer to first element to search
>>> + * @num: number of elements
>>> + * @size: size of each element
>>> + * @cmp: pointer to comparison function
>>> + *
>>> + * This function does a binary search on the given array.  The
>>> + * contents of the array should already be in ascending sorted order
>>> + * under the provided comparison function.
>>> + *
>>> + * Note that the key need not have the same type as the elements in
>>> + * the array, e.g. key could be a string and the comparison function
>>> + * could compare the string with the struct's name field. However, if
>>> + * the key and elements in the array are of the same type, you can use
>>> + * the same comparison function for both sort() and bsearch().
>>> + */
>>> +static void *bsearch(const void *key, const void *base, size_t num,
>> size_t size,
>>> +                     int (*cmp)(const void *key, const void *elt))
>>
>> This function is not specific to I/O handlers. So this should be moved
>> to common code. Also please mention in the commit message where the
>> code came from.
>>
>
> Should I move to xen/arch/arm folder?

To xen/common/

[...]

>> Anyway, I would try to merge the two compare functions
>> (match_mmio_handler, cmp_mmio_handler) which have very similar behavior.
>>
>
> Yes, they are not exactly same. One compares only start address and
> other one compares the range.

I don't think this will be an issue to compare the range for both and it 
will avoid to duplicate the code.

Regards,

-- 
Julien Grall

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

Re: [PATCH V3 04/10] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable

[Shanker Donthineni]

Hi Julien,


On 06/28/2016 05:40 AM, Julien Grall wrote:
> Hello Shanker,
>
> On 27/06/16 21:33, Shanker Donthineni wrote:
>> @@ -1397,6 +1408,36 @@ gic_acpi_parse_madt_distributor(struct
> acpi_subtable_header *header,
>>   }
>>
>>   static int __init
>> +gic_acpi_parse_cpu_redistributor(struct acpi_subtable_header *header,
>> +                                 const unsigned long end)
>> +{
>> +    struct acpi_madt_generic_interrupt *processor;
>> +    u32 size;
>> +
>> +    processor = (struct acpi_madt_generic_interrupt *)header;
>> +    if ( !(processor->flags & ACPI_MADT_ENABLED) )
>> +        return 0;
>
> You did not answer to my question on previous version of this patch. 
> You said that "Disabled GICC entries should be skipped because its 
> Redistributor region is not always-on power domain." However from my 
> understanding, an usable CPU may have his Redistributor in the not 
> always-on power domain. So the issue would the same, correct?
>

The gicv3_populate_rdist() is not supposed to read GICR registers if 
the  the associated hardware GICR block is in power-off state. The CPU 
accesses to disabled GICR region leads to either a system hang or an 
unexpected behavior.


> Regards,
>

-- 
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project


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

Re: [PATCH V3 04/10] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable

[Shanker Donthineni]

On 06/28/2016 08:51 AM, Shanker Donthineni wrote:
> Hi Julien,
>
>
> On 06/28/2016 05:40 AM, Julien Grall wrote:
>> Hello Shanker,
>>
>> On 27/06/16 21:33, Shanker Donthineni wrote:
>>> @@ -1397,6 +1408,36 @@ gic_acpi_parse_madt_distributor(struct
>> acpi_subtable_header *header,
>>>   }
>>>
>>>   static int __init
>>> +gic_acpi_parse_cpu_redistributor(struct acpi_subtable_header *header,
>>> +                                 const unsigned long end)
>>> +{
>>> +    struct acpi_madt_generic_interrupt *processor;
>>> +    u32 size;
>>> +
>>> +    processor = (struct acpi_madt_generic_interrupt *)header;
>>> +    if ( !(processor->flags & ACPI_MADT_ENABLED) )
>>> +        return 0;
>>
>> You did not answer to my question on previous version of this patch. 
>> You said that "Disabled GICC entries should be skipped because its 
>> Redistributor region is not always-on power domain." However from my 
>> understanding, an usable CPU may have his Redistributor in the not 
>> always-on power domain. So the issue would the same, correct?
>>
>
> The gicv3_populate_rdist() is not supposed to read GICR registers if 
> the  the associated hardware GICR block is in power-off state. The CPU 
> accesses to disabled GICR region leads to either a system hang or an 
> unexpected behavior.
>
>
The description of flag ACPI_MADT_ENABLED in ACPI-6.1 says "If zero, 
this processor in unusable, and the operating system support will not 
attempt to use it".


>> Regards,
>>
>

-- 
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project


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

Re: [PATCH V3 04/10] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable

[Julien Grall]

Hi Shanker,

On 28/06/16 15:33, Shanker Donthineni wrote:
>
>
> On 06/28/2016 08:51 AM, Shanker Donthineni wrote:
>> Hi Julien,
>>
>>
>> On 06/28/2016 05:40 AM, Julien Grall wrote:
>>> Hello Shanker,
>>>
>>> On 27/06/16 21:33, Shanker Donthineni wrote:
>>>> @@ -1397,6 +1408,36 @@ gic_acpi_parse_madt_distributor(struct
>>> acpi_subtable_header *header,
>>>>   }
>>>>
>>>>   static int __init
>>>> +gic_acpi_parse_cpu_redistributor(struct acpi_subtable_header *header,
>>>> +                                 const unsigned long end)
>>>> +{
>>>> +    struct acpi_madt_generic_interrupt *processor;
>>>> +    u32 size;
>>>> +
>>>> +    processor = (struct acpi_madt_generic_interrupt *)header;
>>>> +    if ( !(processor->flags & ACPI_MADT_ENABLED) )
>>>> +        return 0;
>>>
>>> You did not answer to my question on previous version of this patch.
>>> You said that "Disabled GICC entries should be skipped because its
>>> Redistributor region is not always-on power domain." However from my
>>> understanding, an usable CPU may have his Redistributor in the not
>>> always-on power domain. So the issue would the same, correct?
>>>
>>
>> The gicv3_populate_rdist() is not supposed to read GICR registers if
>> the  the associated hardware GICR block is in power-off state. The CPU
>> accesses to disabled GICR region leads to either a system hang or an
>> unexpected behavior.
>>
>>
> The description of flag ACPI_MADT_ENABLED in ACPI-6.1 says "If zero,
> this processor in unusable, and the operating system support will not
> attempt to use it".

Ok.  The patch looks good to me then.

Regards,

-- 
Julien Grall

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

Re: [PATCH V3 04/10] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable

[Julien Grall]

Hi Shanker,

On 27/06/16 21:33, Shanker Donthineni wrote:
> The redistributor address can be specified either as part of GICC or
> GICR subtable depending on the power domain. The current driver
> doesn't support parsing redistributor entry that is defined in GICC
> subtable. The GIC CPU subtable entry holds the associated Redistributor
> base address if it is not on always-on power domain.
>
> The per CPU Redistributor size is not defined in ACPI specification.
> Set the GICR region size to SZ_256K if the GIC hardware is capable of
> Direct Virtual LPI Injection feature, SZ_128K otherwise.
>
> This patch adds necessary code to handle both types of Redistributors
> base addresses.
>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>

Based on the discussion, this patch looks good to me:

Acked-by: Julien Grall <julien.grall@arm.com>

Regards,

-- 
Julien Grall

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

Re: [PATCH V3 01/10] arm/gic-v3: Use acpi_table_parse_madt() to parse MADT subtables

[Stefano Stabellini]

On Mon, 27 Jun 2016, Shanker Donthineni wrote:
> The function acpi_table_parse_madt() does the same functionality as
> function acpi_parse_entries() expect it takes a few arguments.
> 
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>

I committed patches 1 to 7


>  xen/arch/arm/gic-v3.c | 27 ++++++---------------------
>  1 file changed, 6 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 8d3f149..166f1c1 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1390,28 +1390,15 @@ gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header *header,
>  
>  static void __init gicv3_acpi_init(void)
>  {
> -    struct acpi_table_header *table;
>      struct rdist_region *rdist_regs;
> -    acpi_status status;
>      int count, i;
>  
> -    status = acpi_get_table(ACPI_SIG_MADT, 0, &table);
> -
> -    if ( ACPI_FAILURE(status) )
> -    {
> -        const char *msg = acpi_format_exception(status);
> -
> -        panic("GICv3: Failed to get MADT table, %s", msg);
> -    }
> -
>      /*
>       * Find distributor base address. We expect one distributor entry since
>       * ACPI 5.0 spec neither support multi-GIC instances nor GIC cascade.
>       */
> -    count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt),
> -                               gic_acpi_parse_madt_distributor, table,
> -                               ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0);
> -
> +    count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
> +                                  gic_acpi_parse_madt_distributor, 0);
>      if ( count <= 0 )
>          panic("GICv3: No valid GICD entries exists");
>  
> @@ -1420,9 +1407,8 @@ static void __init gicv3_acpi_init(void)
>                dbase);
>  
>      /* Get number of redistributor */
> -    count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt),
> -                               gic_acpi_get_madt_redistributor_num, table,
> -                               ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, 0);
> +    count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
> +                                  gic_acpi_get_madt_redistributor_num, 0);
>      if ( count <= 0 )
>          panic("GICv3: No valid GICR entries exists");
>  
> @@ -1458,9 +1444,8 @@ static void __init gicv3_acpi_init(void)
>      gicv3.rdist_regions= rdist_regs;
>  
>      /* Collect CPU base addresses */
> -    count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt),
> -                               gic_acpi_parse_madt_cpu, table,
> -                               ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0);
> +    count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
> +                                  gic_acpi_parse_madt_cpu, 0);
>      if ( count <= 0 )
>          panic("GICv3: No valid GICC entries exists");
>  
> -- 
> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
> a Linux Foundation Collaborative Project
> 

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

Re: [PATCH V3 01/10] arm/gic-v3: Use acpi_table_parse_madt() to parse MADT subtables

[Shanker Donthineni]

Hi Stefano/Juilen

On 07/14/2016 09:18 AM, Stefano Stabellini wrote:
> On Mon, 27 Jun 2016, Shanker Donthineni wrote:
>> The function acpi_table_parse_madt() does the same functionality as
>> function acpi_parse_entries() expect it takes a few arguments.
>>
>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> I committed patches 1 to 7
>

Thanks, I'll post the remaining patches for code review.


-- 
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


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