[PATCH V2 00/10] Add support for parsing per CPU Redistributor entry

[PATCH V2 00/10] Add support for parsing per CPU Redistributor entry

[Shanker Donthineni]

The current driver doesn't support parsing Redistributor entries that
are described in the MADT GICC table. Not all the GIC implementors
places the Redistributor regions in the always-on power domain. On
systems, the UEFI firmware should describe Redistributor base address
in the associated GIC CPU Interface (GICC) instead of GIC Redistributor
(GICR) table.

The maximum number of mmio handlers and struct vgic_rdist_region
that holds Redistributor addresses are allocated through a static
array with hardcoded size. I don't think this is the right approach
and is not scalable for implementing features like this. I have
decided to convert static to dynamic allocation based on comments
from the below link.

Patches #1 fixes the bug in the current driver.

Patches #2, #3 and #4 adds support for parsing not always-on power
domain Redistributor regions.

Patches #5, #6, #7, #8 and #10 refactors the code and allocates the
memory for mmio handlers and vgic_rdist_region based on the number of
Redistributors required for dom0/domU instead of hardcoded values.

Patch #9 changes the linear to binary search to avoid lookup overhead.

This pacthset is created on tip of Julien's branch http://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=shortlog;h=refs/heads/irq-routing-acpi-rfc

Shanker Donthineni (10):
  arm/gic-v3: Fix bug in function cmp_rdist()
  arm/gic-v3: Do early GICD ioremap and clean up
  arm/gic-v3: Fold GICR subtable parsing into a new function
  arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable
  xen/arm: vgic: Use dynamic memory allocation for vgic_rdist_region
  arm/gic-v3: Remove an unused macro MAX_RDIST_COUNT
  arm: vgic: Split vgic_domain_init() functionality into two functions
  arm/io: Use separate memory allocation for mmio handlers
  xen/arm: io: Use binary search for mmio handler lookup
  arm/vgic: Change fixed number of mmio handlers to variable number

 xen/arch/arm/domain.c             |  13 +++-
 xen/arch/arm/gic-v3.c             | 158 ++++++++++++++++++++++++++++----------
 xen/arch/arm/io.c                 |  64 +++++++++++----
 xen/arch/arm/vgic-v2.c            |   7 ++
 xen/arch/arm/vgic-v3.c            |  30 +++++++-
 xen/arch/arm/vgic.c               |  30 +++++---
 xen/include/asm-arm/domain.h      |   3 +-
 xen/include/asm-arm/gic.h         |   2 +-
 xen/include/asm-arm/gic_v3_defs.h |   1 +
 xen/include/asm-arm/mmio.h        |   6 +-
 xen/include/asm-arm/vgic.h        |   3 +
 11 files changed, 241 insertions(+), 76 deletions(-)

-- 
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 V2 01/10] arm/gic-v3: Fix bug in function cmp_rdist()

[Shanker Donthineni]

The cmp_rdist() is always returning value zero irrespective of the
input Redistributor base addresses. Both the local variables 'l' and
'r' are pointing to the first argument 'a' causing the logical
expression 'l->base < r->base' always evaluated as false which is
wrong.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
 xen/arch/arm/gic-v3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 8d3f149..b89c608 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1133,7 +1133,7 @@ static const hw_irq_controller gicv3_guest_irq_type = {
 
 static int __init cmp_rdist(const void *a, const void *b)
 {
-    const struct rdist_region *l = a, *r = a;
+    const struct rdist_region *l = a, *r = b;
 
     /* We assume that re-distributor regions can never overlap */
     return ( l->base < r->base) ? -1 : 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 V2 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 pacth. 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 pacth accesses the GICD region inside gicv3_acpi_init() for
finding per CPU Redistributor size.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
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 b89c608..542c4f3 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) )
@@ -1415,9 +1424,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_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt),
@@ -1491,10 +1498,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 V2 03/10] arm/gic-v3: Fold GICR subtable parsing into a new function

[Shanker Donthineni]

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

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
Changes since v1:
  Removed the unnecessary GICR ioremap operation inside GICR table parse code.

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

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 542c4f3..0471fea 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(u64 base_addr, u32 size)
+{
+    unsigned int idx = gicv3.rdist_count++;
+
+    gicv3.rdist_regions[idx].base = base_addr;
+    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,25 @@ 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;
+
+    if ( !rdist->base_address || !rdist->length )
+        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)
@@ -1402,7 +1429,7 @@ static void __init gicv3_acpi_init(void)
     struct acpi_table_header *table;
     struct rdist_region *rdist_regs;
     acpi_status status;
-    int count, i;
+    int count;
 
     status = acpi_get_table(ACPI_SIG_MADT, 0, &table);
 
@@ -1433,37 +1460,27 @@ 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_parse_entries(ACPI_SIG_MADT,
+                               sizeof(struct acpi_table_madt),
+                               gic_acpi_parse_madt_redistributor, table,
+                               ACPI_MADT_TYPE_GENERIC_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_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt),
                                gic_acpi_parse_madt_cpu, table,
-- 
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 V2 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 it's size to SZ_256K if the GIC hardware is capable of Direct
Virtual LPI Injection feature otherwise SZ_128K.

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

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
Changes since v1:
  Edited commit text and fixed white spaces.
  Added a new function for parsing per CPU Redistributor entry.

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

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 0471fea..3977244 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(u64 base_addr, u32 size)
+static void __init
+gic_acpi_add_rdist_region(u64 base_addr, u32 size, bool single_rdist)
 {
     unsigned int idx = gicv3.rdist_count++;
 
+    gicv3.rdist_regions[idx].single_rdist = single_rdist;
     gicv3.rdist_regions[idx].base = base_addr;
     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,42 @@ 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 ( BAD_MADT_ENTRY(processor, end) )
+        return -EINVAL;
+
+    if ( !processor->gicr_base_address )
+        return -EINVAL;
+
+    if ( processor->flags & ACPI_MADT_ENABLED )
+    {
+        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)
 {
@@ -1409,7 +1456,7 @@ gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header,
     if ( !rdist->base_address || !rdist->length )
         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;
 }
@@ -1428,6 +1475,7 @@ static void __init gicv3_acpi_init(void)
 {
     struct acpi_table_header *table;
     struct rdist_region *rdist_regs;
+    bool gicr_table = true;
     acpi_status status;
     int count;
 
@@ -1457,8 +1505,18 @@ static void __init gicv3_acpi_init(void)
     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);
-    if ( count <= 0 )
-        panic("GICv3: No valid GICR entries exists");
+
+    /* Count the total number of CPU interface entries */
+    if (count <= 0) {
+        count = acpi_parse_entries(ACPI_SIG_MADT,
+                                   sizeof(struct acpi_table_madt),
+                                   gic_acpi_get_madt_cpu_num,
+                                   table, ACPI_MADT_TYPE_GENERIC_INTERRUPT, 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"
@@ -1470,11 +1528,19 @@ static void __init gicv3_acpi_init(void)
 
     gicv3.rdist_regions = rdist_regs;
 
-    /* Parse always-on power domain Re-distributor entries */
-    count = acpi_parse_entries(ACPI_SIG_MADT,
-                               sizeof(struct acpi_table_madt),
-                               gic_acpi_parse_madt_redistributor, table,
-                               ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, count);
+    if ( gicr_table )
+        /* Parse always-on power domain Re-distributor entries */
+        count = acpi_parse_entries(ACPI_SIG_MADT,
+                                   sizeof(struct acpi_table_madt),
+                                   gic_acpi_parse_madt_redistributor, table,
+                                   ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, count);
+    else
+        /* Parse Re-distributor entries described in CPU interface table */
+        count = acpi_parse_entries(ACPI_SIG_MADT,
+                                   sizeof(struct acpi_table_madt),
+                                   gic_acpi_parse_cpu_redistributor, table,
+                                   ACPI_MADT_TYPE_GENERIC_INTERRUPT, 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 V2 05/10] xen/arm: vgic: Use dynamic memory allocation for vgic_rdist_region

[Shanker Donthineni]

The number of Re-distributor regions allowed for dom0 is hardcoded
to a compile time macro MAX_RDIST_COUNT which is 4. On some systems,
especially latest server chips might have more than 4 redistributors.
Either we have to increase MAX_RDIST_COUNT to a bigger number or
allocate memory based on 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 side 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 since v1:
  Keep 'struct vgic_rdist_region' definition inside 'struct arch_domain'.

 xen/arch/arm/vgic-v2.c       |  6 ++++++
 xen/arch/arm/vgic-v3.c       | 22 +++++++++++++++++++---
 xen/arch/arm/vgic.c          |  1 +
 xen/include/asm-arm/domain.h |  2 +-
 xen/include/asm-arm/vgic.h   |  2 ++
 5 files changed, 29 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..e877e9e 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1393,7 +1393,19 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
 
 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 = is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
+                   GUEST_GICV3_RDIST_REGIONS;
+
+    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 +1438,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 +1446,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 +1477,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..fbb763a 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 are 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 V2 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>
---
 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 3977244..87f4ecf 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");
@@ -1518,10 +1514,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 V2 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 fucntion 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 the later patch.

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

 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..7627eff 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 V2 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>
---
 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 V2 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>
---
 xen/arch/arm/io.c | 50 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index a5b2c2d..abf49fb 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -20,6 +20,7 @@
 #include <xen/lib.h>
 #include <xen/spinlock.h>
 #include <xen/sched.h>
+#include <xen/sort.h>
 #include <asm/current.h>
 #include <asm/mmio.h>
 
@@ -70,23 +71,38 @@ static int handle_write(const struct mmio_handler *handler, struct vcpu *v,
                                handler->priv);
 }
 
-int handle_mmio(mmio_info_t *info)
+const struct mmio_handler *find_mmio_handler(struct vcpu *v, paddr_t addr)
 {
-    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 = vmmio->handlers;
+    unsigned int eidx = vmmio->num_entries;
+    unsigned int midx = eidx / 2;
+    unsigned int sidx = 0;
 
-    for ( i = 0; i < vmmio->num_entries; i++ )
+    /* Do binary search for matching mmio handler */
+    while ( sidx != midx )
     {
-        handler = &vmmio->handlers[i];
-
-        if ( (info->gpa >= handler->addr) &&
-             (info->gpa < (handler->addr + handler->size)) )
-            break;
+        if ( addr < handler[midx].addr )
+            eidx = midx;
+        else
+            sidx = midx;
+        midx = sidx + (eidx - sidx) / 2;
     }
 
-    if ( i == vmmio->num_entries )
+    if ( (addr >= handler[sidx].addr) &&
+         (addr < (handler[sidx].addr + handler[sidx].size)) )
+        return handler + sidx;
+
+    return NULL;
+}
+
+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 +111,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 +146,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 V2 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       | 12 ++++++++++--
 xen/arch/arm/vgic.c          |  6 +-----
 xen/include/asm-arm/domain.h |  1 +
 xen/include/asm-arm/vgic.h   |  1 +
 6 files changed, 21 insertions(+), 11 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 e877e9e..472deac 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1391,14 +1391,19 @@ 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)
 {
     struct vgic_rdist_region *rdist_regions;
     int rdist_count, i;
 
     /* Allocate memory for Re-distributor regions */
-    rdist_count = is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
-                   GUEST_GICV3_RDIST_REGIONS;
+    rdist_count = vgic_v3_rdist_count(d);
 
     rdist_regions = xzalloc_array(struct vgic_rdist_region, rdist_count);
     if ( !rdist_regions )
@@ -1504,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 7627eff..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 fbb763a..1ce441c 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 V2 01/10] arm/gic-v3: Fix bug in function cmp_rdist()

[Julien Grall]

Hi Shanker,

On 26/06/16 18:48, Shanker Donthineni wrote:
> The cmp_rdist() is always returning value zero irrespective of the
> input Redistributor base addresses. Both the local variables 'l' and
> 'r' are pointing to the first argument 'a' causing the logical
> expression 'l->base < r->base' always evaluated as false which is
> wrong.
>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> ---
>   xen/arch/arm/gic-v3.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 8d3f149..b89c608 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1133,7 +1133,7 @@ static const hw_irq_controller gicv3_guest_irq_type = {
>
>   static int __init cmp_rdist(const void *a, const void *b)
>   {
> -    const struct rdist_region *l = a, *r = a;
> +    const struct rdist_region *l = a, *r = b;

Thank you for spotting the error. The sorting was required because of 
the way the vGIC emulated the re-distributors. However, this code has 
been reworked and sorted array is not necessary anymore.

So I would directly drop the sorting here.

>
>       /* We assume that re-distributor regions can never overlap */
>       return ( l->base < r->base) ? -1 : 0;
>

Regards,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Shanker,

On 26/06/16 18:48, Shanker Donthineni wrote:
> For ACPI based XEN boot, the GICD region needs to be accessed inside
> the function gicv3_acpi_init() in later pacth. There is a duplicate

s/pachth/patch/

> 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 pacth accesses the GICD region inside gicv3_acpi_init() for

s/pacth/patch/

> finding per CPU Redistributor size.
>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>

Acked-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 V2 03/10] arm/gic-v3: Fold GICR subtable parsing into a new function

[Julien Grall]

Hi Shanker,

Title: I think you want to say "Move GICR..." rather than "Fold GICR...".

On 26/06/16 18:48, Shanker Donthineni wrote:
> Add a new function for parsing GICR subtable and move the code

Add a new function to parse GICR...

> that is specific to GICR table to new function without changing

to a new function

> the function gicv3_acpi_init() behavior.
>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> ---
> Changes since v1:
>    Removed the unnecessary GICR ioremap operation inside GICR table parse code.
>
>   xen/arch/arm/gic-v3.c | 61 ++++++++++++++++++++++++++++++++-------------------
>   1 file changed, 39 insertions(+), 22 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 542c4f3..0471fea 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(u64 base_addr, u32 size)

Please use paddr_t for both parameter. Also the suffix _addr is pointless.

> +{
> +    unsigned int idx = gicv3.rdist_count++;
> +
> +    gicv3.rdist_regions[idx].base = base_addr;
> +    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,25 @@ 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;
> +
> +    if ( !rdist->base_address || !rdist->length )
> +        return -EINVAL;

In the commit message you said that the behavior is unchanged, however 
this check is not part of the previous code.

Anyway, I don't think this check is necessary.

> +
> +    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)
> @@ -1402,7 +1429,7 @@ static void __init gicv3_acpi_init(void)
>       struct acpi_table_header *table;
>       struct rdist_region *rdist_regs;
>       acpi_status status;
> -    int count, i;
> +    int count;
>
>       status = acpi_get_table(ACPI_SIG_MADT, 0, &table);
>
> @@ -1433,37 +1460,27 @@ 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_parse_entries(ACPI_SIG_MADT,
> +                               sizeof(struct acpi_table_madt),
> +                               gic_acpi_parse_madt_redistributor, table,
> +                               ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, count);

Please use acpi_table_parse_madt here.

> +    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_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt),
>                                  gic_acpi_parse_madt_cpu, table,
>

Regards,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Shanker,

On 26/06/16 18:48, 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 it's size to SZ_256K if the GIC hardware is capable of Direct

s/it's/its/, although I would use "the".

> Virtual LPI Injection feature otherwise SZ_128K.

"Set the size to SZ_256K if the GIC hardware is supporting Direct 
Virtual LPI injection, 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 v1:
>    Edited commit text and fixed white spaces.
>    Added a new function for parsing per CPU Redistributor entry.
>
>   xen/arch/arm/gic-v3.c             | 84 ++++++++++++++++++++++++++++++++++-----
>   xen/include/asm-arm/gic.h         |  1 +
>   xen/include/asm-arm/gic_v3_defs.h |  1 +
>   3 files changed, 77 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 0471fea..3977244 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(u64 base_addr, u32 size)
> +static void __init
> +gic_acpi_add_rdist_region(u64 base_addr, u32 size, bool single_rdist)
>   {
>       unsigned int idx = gicv3.rdist_count++;
>
> +    gicv3.rdist_regions[idx].single_rdist = single_rdist;
>       gicv3.rdist_regions[idx].base = base_addr;
>       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,42 @@ 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 ( BAD_MADT_ENTRY(processor, end) )
> +        return -EINVAL;
> +
> +    if ( !processor->gicr_base_address )
> +        return -EINVAL;

You already check it in gic_acpi_get_madt_cpu_num, so there is no reason 
to do it again.

> +
> +    if ( processor->flags & ACPI_MADT_ENABLED )
> +    {
> +        size = gic_dist_supports_dvis() ? 4 * SZ_64K : 2 * SZ_64K;
> +        gic_acpi_add_rdist_region(processor->gicr_base_address, size, true);
> +    }

I would revert the condition to avoid one level of indentation. I.e

if ( !(processor->flags & ACPI_MADT_ENABLED) )
   return 0;

size = ....
gic_acpi_add...

return 0;

However, it looks like that the other function that parses GICC within 
gic-v3.c (see gic_acpi_parse_madt_cpu) does not check if the CPU is usable.

I think we need to have the same parsing behavior on every function.

> +
> +    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)
>   {
> @@ -1409,7 +1456,7 @@ gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header,
>       if ( !rdist->base_address || !rdist->length )
>           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;
>   }
> @@ -1428,6 +1475,7 @@ static void __init gicv3_acpi_init(void)
>   {
>       struct acpi_table_header *table;
>       struct rdist_region *rdist_regs;
> +    bool gicr_table = true;
>       acpi_status status;
>       int count;
>
> @@ -1457,8 +1505,18 @@ static void __init gicv3_acpi_init(void)
>       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);
> -    if ( count <= 0 )
> -        panic("GICv3: No valid GICR entries exists");
> +
> +    /* Count the total number of CPU interface entries */
> +    if (count <= 0) {

Coding style:

if ( count <= 0 )
{

> +        count = acpi_parse_entries(ACPI_SIG_MADT,
> +                                   sizeof(struct acpi_table_madt),
> +                                   gic_acpi_get_madt_cpu_num,
> +                                   table, ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0);

Please use acpi_table_parse_madt.

> +        if (count <= 0)

Coding style:

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"
> @@ -1470,11 +1528,19 @@ static void __init gicv3_acpi_init(void)
>
>       gicv3.rdist_regions = rdist_regs;
>
> -    /* Parse always-on power domain Re-distributor entries */
> -    count = acpi_parse_entries(ACPI_SIG_MADT,
> -                               sizeof(struct acpi_table_madt),
> -                               gic_acpi_parse_madt_redistributor, table,
> -                               ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, count);
> +    if ( gicr_table )
> +        /* Parse always-on power domain Re-distributor entries */
> +        count = acpi_parse_entries(ACPI_SIG_MADT,
> +                                   sizeof(struct acpi_table_madt),
> +                                   gic_acpi_parse_madt_redistributor, table,
> +                                   ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, count);

Please use acpi_table_parse_madt.

> +    else
> +        /* Parse Re-distributor entries described in CPU interface table */
> +        count = acpi_parse_entries(ACPI_SIG_MADT,
> +                                   sizeof(struct acpi_table_madt),
> +                                   gic_acpi_parse_cpu_redistributor, table,
> +                                   ACPI_MADT_TYPE_GENERIC_INTERRUPT, count);

Ditto.

> +
>       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__ */
>

Regards,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Shanker,

On 26/06/16 18:48, Shanker Donthineni wrote:
> The number of Re-distributor regions allowed for dom0 is hardcoded

s/Re-distributor/Redistributor/

> to a compile time macro MAX_RDIST_COUNT which is 4. On some systems,

s/a compile time macro/a define/
s/On some/Some/

> especially latest server chips might have more than 4 redistributors.

I would add a comma after 'chips'.

NIT: s/redistributors/Redistributors/

> Either we have to increase MAX_RDIST_COUNT to a bigger number or
> allocate memory based on number of redistributors that are found in

s/based on number/based on the number/

> 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 side effect, it blows 'struct domain'

s/has side/has a/

> 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 since v1:
>    Keep 'struct vgic_rdist_region' definition inside 'struct arch_domain'.
>
>   xen/arch/arm/vgic-v2.c       |  6 ++++++
>   xen/arch/arm/vgic-v3.c       | 22 +++++++++++++++++++---
>   xen/arch/arm/vgic.c          |  1 +
>   xen/include/asm-arm/domain.h |  2 +-
>   xen/include/asm-arm/vgic.h   |  2 ++
>   5 files changed, 29 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..e877e9e 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1393,7 +1393,19 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
>
>   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 = is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
> +                   GUEST_GICV3_RDIST_REGIONS;

I would directly introduce the inline helper in this patch, rather than 
in patch #10.

> +
> +    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.

[...]

> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index a2fccc0..fbb763a 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 are allocated by domain_init */

s/are/were/

> +    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 */
>

Regards,

-- 
Julien Grall

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

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

[Andrew Cooper]

>> to a compile time macro MAX_RDIST_COUNT which is 4. On some systems,
>
> s/a compile time macro/a define/
> s/On some/Some/
>
>> especially latest server chips might have more than 4 redistributors.
>
> I would add a comma after 'chips'.

In English, rules for dual commas forming a subclause like this are that
the sentence still makes grammatical sense if the subclause is omitted.

Therefore

"On some systems, especially latest server chips, might have more than 4
redistributors."

is expected be equivalent to

"On some systems might have more than 4 redistributors."


The latter however doesn't make sense.

May I recommend instead:

"Some systems, especially latest server chips, may have more than 4
redistributors."

~Andrew

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

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

[Julien Grall]

Hi Shanker,

On 26/06/16 18:48, Shanker Donthineni wrote:
> Separate the code logic that does the registration of vgic_v3/v2 ops
> to a new fucntion domain_vgic_register(). The intention of this

s/fucntion/function/

> separation is to record the required mmio count in vgic_v3/v2_init()
> and pass it to function domain_io_init() in the later patch.

s/the later/a follow-up patch/ or s/the later/the last/. Although I 
prefer the former.

>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> ---
> Changes since v1:
>    Moved registration of vgic_v3/v2 functionality to a new domain_vgic_register().
>
>   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..7627eff 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)

Coding style:

if ( ret < 0 )

> +        return ret;
> +
>       spin_lock_init(&d->arch.vgic.lock);
>
>       d->arch.vgic.shared_irqs =
>

Regards,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Shanker,

On 26/06/16 18:48, 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>
> ---
>   xen/arch/arm/io.c | 50 +++++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 39 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index a5b2c2d..abf49fb 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -20,6 +20,7 @@
>   #include <xen/lib.h>
>   #include <xen/spinlock.h>
>   #include <xen/sched.h>
> +#include <xen/sort.h>
>   #include <asm/current.h>
>   #include <asm/mmio.h>
>
> @@ -70,23 +71,38 @@ static int handle_write(const struct mmio_handler *handler, struct vcpu *v,
>                                  handler->priv);
>   }
>
> -int handle_mmio(mmio_info_t *info)
> +const struct mmio_handler *find_mmio_handler(struct vcpu *v, paddr_t addr)
>   {
> -    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 = vmmio->handlers;
> +    unsigned int eidx = vmmio->num_entries;
> +    unsigned int midx = eidx / 2;
> +    unsigned int sidx = 0;
>
> -    for ( i = 0; i < vmmio->num_entries; i++ )
> +    /* Do binary search for matching mmio handler */
> +    while ( sidx != midx )
>       {
> -        handler = &vmmio->handlers[i];
> -
> -        if ( (info->gpa >= handler->addr) &&
> -             (info->gpa < (handler->addr + handler->size)) )
> -            break;
> +        if ( addr < handler[midx].addr )
> +            eidx = midx;
> +        else
> +            sidx = midx;
> +        midx = sidx + (eidx - sidx) / 2;

This binary search can be simplified. For instance, why do you want to 
compute midx at the end rather than at the beginning. This would avoid 
to have "unsigned int midx = eidx / 2" at the beginning.

>       }
>
> -    if ( i == vmmio->num_entries )
> +    if ( (addr >= handler[sidx].addr) &&
> +         (addr < (handler[sidx].addr + handler[sidx].size)) )
> +        return handler + sidx;

Please use a temporary variable for handler[sidx]. So it will be easier 
to read the code.

> +
> +    return NULL;
> +}
> +
> +int handle_mmio(mmio_info_t *info)
> +{
> +    const struct mmio_handler *handler;
> +    struct vcpu *v = current;
> +
> +    handler = find_mmio_handler(v, info->gpa);

I would have expected some locking here. Could you explain why it is 
safe to looking find the handler with your solution?

For what is worth, there was no locking before because 
register_mmio_handler was always adding the handler at the end of the 
array. This is not true anymore because you are sorting the array.

> +    if ( !handler )
>           return 0;
>
>       if ( info->dabt.write )
> @@ -95,6 +111,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 +146,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 V2 10/10] arm/vgic: Change fixed number of mmio handlers to variable number

[Julien Grall]

Hi Shanker,

On 26/06/16 18:48, Shanker Donthineni wrote:
> 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 */

Is it necessary to have this value part of the arch_domain? I.e Do we 
need this value after the initialization? If not, then it might be 
better to add a parameter to domain_vgic_register uint32_t *pointer.

>       } vgic;
>
>       struct vuart {
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index fbb763a..1ce441c 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,
>

Regards,

-- 
Julien Grall

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

Re: [PATCH V2 01/10] arm/gic-v3: Fix bug in function cmp_rdist()

[Shanker Donthineni]

On 06/27/2016 06:03 AM, Julien Grall wrote:
> Hi Shanker,
>
> On 26/06/16 18:48, Shanker Donthineni wrote:
>> The cmp_rdist() is always returning value zero irrespective of the
>> input Redistributor base addresses. Both the local variables 'l' and
>> 'r' are pointing to the first argument 'a' causing the logical
>> expression 'l->base < r->base' always evaluated as false which is
>> wrong.
>>
>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
>> ---
>>   xen/arch/arm/gic-v3.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 8d3f149..b89c608 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -1133,7 +1133,7 @@ static const hw_irq_controller
> gicv3_guest_irq_type = {
>>
>>   static int __init cmp_rdist(const void *a, const void *b)
>>   {
>> -    const struct rdist_region *l = a, *r = a;
>> +    const struct rdist_region *l = a, *r = b;
>
> Thank you for spotting the error. The sorting was required because of 
> the way the vGIC emulated the re-distributors. However, this code has 
> been reworked and sorted array is not necessary anymore.
>
> So I would directly drop the sorting here.
>
Thanks, I'll drop this patch in patchset-v3.

>>
>>       /* We assume that re-distributor regions can never overlap */
>>       return ( l->base < r->base) ? -1 : 0;
>>
>
> 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 V2 03/10] arm/gic-v3: Fold GICR subtable parsing into a new function

[Shanker Donthineni]

On 06/27/2016 06:26 AM, Julien Grall wrote:
> Hi Shanker,
>
> Title: I think you want to say "Move GICR..." rather than "Fold GICR...".
>
> On 26/06/16 18:48, Shanker Donthineni wrote:
>> Add a new function for parsing GICR subtable and move the code
>
> Add a new function to parse GICR...
>
>> that is specific to GICR table to new function without changing
>
> to a new function
>
>> the function gicv3_acpi_init() behavior.
>>
>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
>> ---
>> Changes since v1:
>>    Removed the unnecessary GICR ioremap operation inside GICR table
> parse code.
>>
>>   xen/arch/arm/gic-v3.c | 61
> ++++++++++++++++++++++++++++++++-------------------
>>   1 file changed, 39 insertions(+), 22 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 542c4f3..0471fea 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(u64 base_addr, u32 size)
>
> Please use paddr_t for both parameter. Also the suffix _addr is 
> pointless.
>

I'll fix.
>> +{
>> +    unsigned int idx = gicv3.rdist_count++;
>> +
>> +    gicv3.rdist_regions[idx].base = base_addr;
>> +    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,25 @@ 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;
>> +
>> +    if ( !rdist->base_address || !rdist->length )
>> +        return -EINVAL;
>
> In the commit message you said that the behavior is unchanged, however 
> this check is not part of the previous code.
>
> Anyway, I don't think this check is necessary.
>

Sure, I'll remove the validation check from here.

>> +
>> +    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)
>> @@ -1402,7 +1429,7 @@ static void __init gicv3_acpi_init(void)
>>       struct acpi_table_header *table;
>>       struct rdist_region *rdist_regs;
>>       acpi_status status;
>> -    int count, i;
>> +    int count;
>>
>>       status = acpi_get_table(ACPI_SIG_MADT, 0, &table);
>>
>> @@ -1433,37 +1460,27 @@ 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_parse_entries(ACPI_SIG_MADT,
>> +                               sizeof(struct acpi_table_madt),
>> + gic_acpi_parse_madt_redistributor,
> table,
>> + ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
> count);
>
> Please use acpi_table_parse_madt here.
>

Sure.

>> +    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_parse_entries(ACPI_SIG_MADT, sizeof(struct
> acpi_table_madt),
>> gic_acpi_parse_madt_cpu, table,
>>
>
> 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 V2 06/10] arm/gic-v3: Remove an unused macro MAX_RDIST_COUNT

[Julien Grall]

On 26/06/16 18:48, Shanker Donthineni wrote:
> 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 3977244..87f4ecf 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");
> @@ -1518,10 +1514,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)
>

-- 
Julien Grall

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

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

[Julien Grall]

Hi Shanker,

On 26/06/16 18:48, Shanker Donthineni wrote:
> 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>

Regards,

-- 
Julien Grall

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

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

[Shanker Donthineni]

On 06/27/2016 06:47 AM, Julien Grall wrote:
> Hi Shanker,
>
> On 26/06/16 18:48, 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 it's size to SZ_256K if the GIC hardware is capable of Direct
>
> s/it's/its/, although I would use "the".
>
>> Virtual LPI Injection feature otherwise SZ_128K.
>
> "Set the size to SZ_256K if the GIC hardware is supporting Direct 
> Virtual LPI injection, 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 v1:
>>    Edited commit text and fixed white spaces.
>>    Added a new function for parsing per CPU Redistributor entry.
>>
>>   xen/arch/arm/gic-v3.c             | 84
> ++++++++++++++++++++++++++++++++++-----
>>   xen/include/asm-arm/gic.h         |  1 +
>>   xen/include/asm-arm/gic_v3_defs.h |  1 +
>>   3 files changed, 77 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 0471fea..3977244 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(u64 base_addr, u32 size)
>> +static void __init
>> +gic_acpi_add_rdist_region(u64 base_addr, u32 size, bool single_rdist)
>>   {
>>       unsigned int idx = gicv3.rdist_count++;
>>
>> +    gicv3.rdist_regions[idx].single_rdist = single_rdist;
>>       gicv3.rdist_regions[idx].base = base_addr;
>>       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,42 @@ 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 ( BAD_MADT_ENTRY(processor, end) )
>> +        return -EINVAL;
>> +
>> +    if ( !processor->gicr_base_address )
>> +        return -EINVAL;
>
> You already check it in gic_acpi_get_madt_cpu_num, so there is no 
> reason to do it again.
>
Other function just finds the number of valid cpu interfaces. I would  
prefer to keep the validation check here.

>> +
>> +    if ( processor->flags & ACPI_MADT_ENABLED )
>> +    {
>> +        size = gic_dist_supports_dvis() ? 4 * SZ_64K : 2 * SZ_64K;
>> + gic_acpi_add_rdist_region(processor->gicr_base_address, size,
> true);
>> +    }
>
> I would revert the condition to avoid one level of indentation. I.e
>

I'll do.

> if ( !(processor->flags & ACPI_MADT_ENABLED) )
>   return 0;
>
> size = ....
> gic_acpi_add...
>
> return 0;
>
> However, it looks like that the other function that parses GICC within 
> gic-v3.c (see gic_acpi_parse_madt_cpu) does not check if the CPU is
> usable.
>

Disabled GICC entries should be skipped because its Redistributor region 
is not always-on power domain. Please look at my review comment to your 
KVM-ACPI patch http://www.gossamer-threads.com/lists/linux/kernel/2413670.

> I think we need to have the same parsing behavior on every function.
>
>> +
>> +    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)
>>   {
>> @@ -1409,7 +1456,7 @@ gic_acpi_parse_madt_redistributor(struct
> acpi_subtable_header *header,
>>       if ( !rdist->base_address || !rdist->length )
>>           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;
>>   }
>> @@ -1428,6 +1475,7 @@ static void __init gicv3_acpi_init(void)
>>   {
>>       struct acpi_table_header *table;
>>       struct rdist_region *rdist_regs;
>> +    bool gicr_table = true;
>>       acpi_status status;
>>       int count;
>>
>> @@ -1457,8 +1505,18 @@ static void __init gicv3_acpi_init(void)
>>       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);
>> -    if ( count <= 0 )
>> -        panic("GICv3: No valid GICR entries exists");
>> +
>> +    /* Count the total number of CPU interface entries */
>> +    if (count <= 0) {
>
> Coding style:
>
> if ( count <= 0 )
> {
>
>> +        count = acpi_parse_entries(ACPI_SIG_MADT,
>> +                                   sizeof(struct acpi_table_madt),
>> +                                   gic_acpi_get_madt_cpu_num,
>> +                                   table,
> ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0);
>
> Please use acpi_table_parse_madt.
>
>> +        if (count <= 0)
>
> Coding style:
>
> 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"
>> @@ -1470,11 +1528,19 @@ static void __init gicv3_acpi_init(void)
>>
>>       gicv3.rdist_regions = rdist_regs;
>>
>> -    /* Parse always-on power domain Re-distributor entries */
>> -    count = acpi_parse_entries(ACPI_SIG_MADT,
>> -                               sizeof(struct acpi_table_madt),
>> - gic_acpi_parse_madt_redistributor,
> table,
>> - ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
> count);
>> +    if ( gicr_table )
>> +        /* Parse always-on power domain Re-distributor entries */
>> +        count = acpi_parse_entries(ACPI_SIG_MADT,
>> +                                   sizeof(struct acpi_table_madt),
>> + gic_acpi_parse_madt_redistributor,
> table,
>> +
> ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, count);
>
> Please use acpi_table_parse_madt.
>
>> +    else
>> +        /* Parse Re-distributor entries described in CPU interface
> table */
>> +        count = acpi_parse_entries(ACPI_SIG_MADT,
>> +                                   sizeof(struct acpi_table_madt),
>> + gic_acpi_parse_cpu_redistributor,
> table,
>> + ACPI_MADT_TYPE_GENERIC_INTERRUPT,
> count);
>
> Ditto.
>
>> +
>>       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__ */
>>
>
> 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 V2 05/10] xen/arm: vgic: Use dynamic memory allocation for vgic_rdist_region

[Shanker Donthineni]

On 06/27/2016 07:38 AM, Julien Grall wrote:
> Hi Shanker,
>
> On 26/06/16 18:48, Shanker Donthineni wrote:
>> The number of Re-distributor regions allowed for dom0 is hardcoded
>
> s/Re-distributor/Redistributor/
>
>> to a compile time macro MAX_RDIST_COUNT which is 4. On some systems,
>
> s/a compile time macro/a define/
> s/On some/Some/
>
>> especially latest server chips might have more than 4 redistributors.
>
> I would add a comma after 'chips'.
>
> NIT: s/redistributors/Redistributors/
>
>> Either we have to increase MAX_RDIST_COUNT to a bigger number or
>> allocate memory based on number of redistributors that are found in
>
> s/based on number/based on the number/
>
>> 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 side effect, it blows 'struct domain'
>
> s/has side/has a/
>
>> 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 since v1:
>>    Keep 'struct vgic_rdist_region' definition inside 'struct
> arch_domain'.
>>
>>   xen/arch/arm/vgic-v2.c       |  6 ++++++
>>   xen/arch/arm/vgic-v3.c       | 22 +++++++++++++++++++---
>>   xen/arch/arm/vgic.c          |  1 +
>>   xen/include/asm-arm/domain.h |  2 +-
>>   xen/include/asm-arm/vgic.h   |  2 ++
>>   5 files changed, 29 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..e877e9e 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -1393,7 +1393,19 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
>>
>>   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 = is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
>> +                   GUEST_GICV3_RDIST_REGIONS;
>
> I would directly introduce the inline helper in this patch, rather 
> than in patch #10.
>
Okay, I'll add a helper function as part of this patch.

>> +
>> +    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.
>
> [...]
>
>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
>> index a2fccc0..fbb763a 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 are allocated by domain_init */
>
> s/are/were/
>
>> +    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 */
>>
>
> 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 V2 09/10] xen/arm: io: Use binary search for mmio handler lookup

[Shanker Donthineni]

On 06/27/2016 08:31 AM, Julien Grall wrote:
> Hi Shanker,
>
> On 26/06/16 18:48, 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>
>> ---
>>   xen/arch/arm/io.c | 50
> +++++++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 39 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>> index a5b2c2d..abf49fb 100644
>> --- a/xen/arch/arm/io.c
>> +++ b/xen/arch/arm/io.c
>> @@ -20,6 +20,7 @@
>>   #include <xen/lib.h>
>>   #include <xen/spinlock.h>
>>   #include <xen/sched.h>
>> +#include <xen/sort.h>
>>   #include <asm/current.h>
>>   #include <asm/mmio.h>
>>
>> @@ -70,23 +71,38 @@ static int handle_write(const struct mmio_handler
> *handler, struct vcpu *v,
>> handler->priv);
>>   }
>>
>> -int handle_mmio(mmio_info_t *info)
>> +const struct mmio_handler *find_mmio_handler(struct vcpu *v, paddr_t
> addr)
>>   {
>> -    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 = vmmio->handlers;
>> +    unsigned int eidx = vmmio->num_entries;
>> +    unsigned int midx = eidx / 2;
>> +    unsigned int sidx = 0;
>>
>> -    for ( i = 0; i < vmmio->num_entries; i++ )
>> +    /* Do binary search for matching mmio handler */
>> +    while ( sidx != midx )
>>       {
>> -        handler = &vmmio->handlers[i];
>> -
>> -        if ( (info->gpa >= handler->addr) &&
>> -             (info->gpa < (handler->addr + handler->size)) )
>> -            break;
>> +        if ( addr < handler[midx].addr )
>> +            eidx = midx;
>> +        else
>> +            sidx = midx;
>> +        midx = sidx + (eidx - sidx) / 2;
>
> This binary search can be simplified. For instance, why do you want to 
> compute midx at the end rather than at the beginning. This would avoid 
> to have "unsigned int midx = eidx / 2" at the beginning.
>

Let me try to use "do while()" loop logic to simplify the above code logic.

>>       }
>>
>> -    if ( i == vmmio->num_entries )
>> +    if ( (addr >= handler[sidx].addr) &&
>> +         (addr < (handler[sidx].addr + handler[sidx].size)) )
>> +        return handler + sidx;
>
> Please use a temporary variable for handler[sidx]. So it will be 
> easier to read the code.
>
>> +
>> +    return NULL;
>> +}
>> +
>> +int handle_mmio(mmio_info_t *info)
>> +{
>> +    const struct mmio_handler *handler;
>> +    struct vcpu *v = current;
>> +
>> +    handler = find_mmio_handler(v, info->gpa);
>
> I would have expected some locking here. Could you explain why it is 
> safe to looking find the handler with your solution?
>
> For what is worth, there was no locking before because 
> register_mmio_handler was always adding the handler at the end of the 
> array. This is not true anymore because you are sorting the array.
>

The function register_mmio_handler() is called only during dom0/domU 
domain build code path. We don't need locking here until unless some 
code inserting mmio handlers at runtime.



>> +    if ( !handler )
>>           return 0;
>>
>>       if ( info->dabt.write )
>> @@ -95,6 +111,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 +146,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 V2 10/10] arm/vgic: Change fixed number of mmio handlers to variable number

[Shanker Donthineni]

On 06/27/2016 08:35 AM, Julien Grall wrote:
> Hi Shanker,
>
> On 26/06/16 18:48, Shanker Donthineni wrote:
>> 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
> */
>
> Is it necessary to have this value part of the arch_domain? I.e Do we 
> need this value after the initialization? If not, then it might be 
> better to add a parameter to domain_vgic_register uint32_t *pointer.
>
Absolutely, we don't need this variable after the domain build process. 
I have taken this approach to avoid too many code changes. Your 
suggestion requires changes to functions vgic_v2/v3_init() prototype for 
adding a new parameter.

>>       } vgic;
>>
>>       struct vuart {
>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
>> index fbb763a..1ce441c 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,
>>
>
> 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 V2 04/10] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable

[Julien Grall]

On 27/06/16 15:17, Shanker Donthineni wrote:
>>>   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 ( BAD_MADT_ENTRY(processor, end) )
>>> +        return -EINVAL;
>>> +
>>> +    if ( !processor->gicr_base_address )
>>> +        return -EINVAL;
>>
>> You already check it in gic_acpi_get_madt_cpu_num, so there is no
>> reason to do it again.
>>
> Other function just finds the number of valid cpu interfaces. I would
> prefer to keep the validation check here.

The function acpi_parse_entries (& co) does not do what you think. The 
function will return an error as soon as one call to the handler (here 
gic_acpi_get_madt_cpu_num) return a non-zero value (see the 
implementation of acpi_parse_entries_array).

So if the CPU interface is not valid (i.e gicr_base_address it a 
non-zero value), then it will return an error. Therefore this check is 
pointless.

>
>>> +
>>> +    if ( processor->flags & ACPI_MADT_ENABLED )
>>> +    {
>>> +        size = gic_dist_supports_dvis() ? 4 * SZ_64K : 2 * SZ_64K;
>>> + gic_acpi_add_rdist_region(processor->gicr_base_address, size,
>> true);
>>> +    }
>>
>> I would revert the condition to avoid one level of indentation. I.e
>>
>
> I'll do.
>
>> if ( !(processor->flags & ACPI_MADT_ENABLED) )
>>   return 0;
>>
>> size = ....
>> gic_acpi_add...
>>
>> return 0;
>>
>> However, it looks like that the other function that parses GICC within
>> gic-v3.c (see gic_acpi_parse_madt_cpu) does not check if the CPU is
>> usable.
>>
>
> Disabled GICC entries should be skipped because its Redistributor region
> is not always-on power domain.

I am not sure to follow here. A usable CPU may have his Redistributor in 
the not always-on power domain. So the issue would be the same, correct?

> Please look at my review comment to your
> KVM-ACPI patch http://www.gossamer-threads.com/lists/linux/kernel/2413670.

Well in this case the check needs to be done in the other function too 
(gic_acpi_parse_madt_cpu).

Regards,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Shanker,

On 27/06/16 15:50, Shanker Donthineni wrote:
> On 06/27/2016 08:31 AM, Julien Grall wrote:
>> On 26/06/16 18:48, 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>
>>> ---
>>>   xen/arch/arm/io.c | 50
>> +++++++++++++++++++++++++++++++++++++++-----------
>>>   1 file changed, 39 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>>> index a5b2c2d..abf49fb 100644
>>> --- a/xen/arch/arm/io.c
>>> +++ b/xen/arch/arm/io.c
>>> @@ -20,6 +20,7 @@
>>>   #include <xen/lib.h>
>>>   #include <xen/spinlock.h>
>>>   #include <xen/sched.h>
>>> +#include <xen/sort.h>
>>>   #include <asm/current.h>
>>>   #include <asm/mmio.h>
>>>
>>> @@ -70,23 +71,38 @@ static int handle_write(const struct mmio_handler
>> *handler, struct vcpu *v,
>>> handler->priv);
>>>   }
>>>
>>> -int handle_mmio(mmio_info_t *info)
>>> +const struct mmio_handler *find_mmio_handler(struct vcpu *v, paddr_t
>> addr)
>>>   {
>>> -    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 = vmmio->handlers;
>>> +    unsigned int eidx = vmmio->num_entries;
>>> +    unsigned int midx = eidx / 2;
>>> +    unsigned int sidx = 0;
>>>
>>> -    for ( i = 0; i < vmmio->num_entries; i++ )
>>> +    /* Do binary search for matching mmio handler */
>>> +    while ( sidx != midx )
>>>       {
>>> -        handler = &vmmio->handlers[i];
>>> -
>>> -        if ( (info->gpa >= handler->addr) &&
>>> -             (info->gpa < (handler->addr + handler->size)) )
>>> -            break;
>>> +        if ( addr < handler[midx].addr )
>>> +            eidx = midx;
>>> +        else
>>> +            sidx = midx;
>>> +        midx = sidx + (eidx - sidx) / 2;
>>
>> This binary search can be simplified. For instance, why do you want to
>> compute midx at the end rather than at the beginning. This would avoid
>> to have "unsigned int midx = eidx / 2" at the beginning.
>>
>
> Let me try to use "do while()" loop logic to simplify the above code logic.

Please don't try to re-invent your own binary search implementation and 
use a known one such as the one implemented by Linux (see bsearch).

>
>>>       }
>>>
>>> -    if ( i == vmmio->num_entries )
>>> +    if ( (addr >= handler[sidx].addr) &&
>>> +         (addr < (handler[sidx].addr + handler[sidx].size)) )
>>> +        return handler + sidx;
>>
>> Please use a temporary variable for handler[sidx]. So it will be
>> easier to read the code.
>>
>>> +
>>> +    return NULL;
>>> +}
>>> +
>>> +int handle_mmio(mmio_info_t *info)
>>> +{
>>> +    const struct mmio_handler *handler;
>>> +    struct vcpu *v = current;
>>> +
>>> +    handler = find_mmio_handler(v, info->gpa);
>>
>> I would have expected some locking here. Could you explain why it is
>> safe to looking find the handler with your solution?
>>
>> For what is worth, there was no locking before because
>> register_mmio_handler was always adding the handler at the end of the
>> array. This is not true anymore because you are sorting the array.
>>
>
> The function register_mmio_handler() is called only during dom0/domU
> domain build code path. We don't need locking here until unless some
> code inserting mmio handlers at runtime.

The current implementation of I/O handler is thread-safe because of the 
spin_lock and lock-less. We may have people building product on top of 
Xen using register_mmio_handler when the domain is running. So I will 
nack any patch that cause a regression on the behavior.

Regards,

-- 
Julien Grall

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

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

[Shanker Donthineni]

On 06/27/2016 06:26 AM, Julien Grall wrote:
> Hi Shanker,
>
> Title: I think you want to say "Move GICR..." rather than "Fold GICR...".
>
> On 26/06/16 18:48, Shanker Donthineni wrote:
>> Add a new function for parsing GICR subtable and move the code
>
> Add a new function to parse GICR...
>
>> that is specific to GICR table to new function without changing
>
> to a new function
>
>> the function gicv3_acpi_init() behavior.
>>
>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
>> ---
>> Changes since v1:
>>    Removed the unnecessary GICR ioremap operation inside GICR table
> parse code.
>>
>>   xen/arch/arm/gic-v3.c | 61
> ++++++++++++++++++++++++++++++++-------------------
>>   1 file changed, 39 insertions(+), 22 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 542c4f3..0471fea 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(u64 base_addr, u32 size)
>
> Please use paddr_t for both parameter. Also the suffix _addr is 
> pointless.
>
>> +{
>> +    unsigned int idx = gicv3.rdist_count++;
>> +
>> +    gicv3.rdist_regions[idx].base = base_addr;
>> +    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,25 @@ 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;
>> +
>> +    if ( !rdist->base_address || !rdist->length )
>> +        return -EINVAL;
>
> In the commit message you said that the behavior is unchanged, however 
> this check is not part of the previous code.
>
> Anyway, I don't think this check is necessary.
>
>> +
>> +    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)
>> @@ -1402,7 +1429,7 @@ static void __init gicv3_acpi_init(void)
>>       struct acpi_table_header *table;
>>       struct rdist_region *rdist_regs;
>>       acpi_status status;
>> -    int count, i;
>> +    int count;
>>
>>       status = acpi_get_table(ACPI_SIG_MADT, 0, &table);
>>
>> @@ -1433,37 +1460,27 @@ 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_parse_entries(ACPI_SIG_MADT,
>> +                               sizeof(struct acpi_table_madt),
>> + gic_acpi_parse_madt_redistributor,
> table,
>> + ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
> count);
>
> Please use acpi_table_parse_madt here.
>

How do we pass MADT table pointer that we are using to a function 
acpi_table_parse_madt()?  We have already obtained  the MADT table 
address by calling  acpi_get_table() at the beginning of the function 
gicv3_acpi_init().


>> +    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_parse_entries(ACPI_SIG_MADT, sizeof(struct
> acpi_table_madt),
>> gic_acpi_parse_madt_cpu, table,
>>
>
> 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 V2 03/10] arm/gic-v3: Fold GICR subtable parsing into a new function

[Julien Grall]

On 27/06/16 16:40, Shanker Donthineni wrote:
>>> +    gicv3.rdist_regions = rdist_regs;
>>> +
>>> +    /* Parse always-on power domain Re-distributor entries */
>>> +    count = acpi_parse_entries(ACPI_SIG_MADT,
>>> +                               sizeof(struct acpi_table_madt),
>>> + gic_acpi_parse_madt_redistributor,
>> table,
>>> + ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
>> count);
>>
>> Please use acpi_table_parse_madt here.
>>
>
> How do we pass MADT table pointer that we are using to a function
> acpi_table_parse_madt()?  We have already obtained  the MADT table
> address by calling  acpi_get_table() at the beginning of the function
> gicv3_acpi_init().

You don't need to pass it. The function will take care to get the MADT 
table for you.

Regards,

-- 
Julien Grall

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

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

[Shanker Donthineni]

  be

On 06/27/2016 10:41 AM, Julien Grall wrote:
>
>
> On 27/06/16 16:40, Shanker Donthineni wrote:
>>>> +    gicv3.rdist_regions = rdist_regs;
>>>> +
>>>> +    /* Parse always-on power domain Re-distributor entries */
>>>> +    count = acpi_parse_entries(ACPI_SIG_MADT,
>>>> +                               sizeof(struct acpi_table_madt),
>>>> + gic_acpi_parse_madt_redistributor,
>>> table,
>>>> + ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
>>> count);
>>>
>>> Please use acpi_table_parse_madt here.
>>>
>>
>> How do we pass MADT table pointer that we are using to a function
>> acpi_table_parse_madt()?  We have already obtained  the MADT table
>> address by calling  acpi_get_table() at the beginning of the function
>> gicv3_acpi_init().
>
> You don't need to pass it. The function will take care to get the MADT 
> table for you.
>

Are you expecting me to modify current driver to replace 
acpi_parse_entries() calls with acpi_table_parse_madt() before my changes?



> 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 V2 03/10] arm/gic-v3: Fold GICR subtable parsing into a new function

[Julien Grall]

On 27/06/16 17:07, Shanker Donthineni wrote:
> On 06/27/2016 10:41 AM, Julien Grall wrote:
>>
>>
>> On 27/06/16 16:40, Shanker Donthineni wrote:
>>>>> +    gicv3.rdist_regions = rdist_regs;
>>>>> +
>>>>> +    /* Parse always-on power domain Re-distributor entries */
>>>>> +    count = acpi_parse_entries(ACPI_SIG_MADT,
>>>>> +                               sizeof(struct acpi_table_madt),
>>>>> + gic_acpi_parse_madt_redistributor,
>>>> table,
>>>>> + ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
>>>> count);
>>>>
>>>> Please use acpi_table_parse_madt here.
>>>>
>>>
>>> How do we pass MADT table pointer that we are using to a function
>>> acpi_table_parse_madt()?  We have already obtained  the MADT table
>>> address by calling  acpi_get_table() at the beginning of the function
>>> gicv3_acpi_init().
>>
>> You don't need to pass it. The function will take care to get the MADT
>> table for you.
>>
>
> Are you expecting me to modify current driver to replace
> acpi_parse_entries() calls with acpi_table_parse_madt() before my changes?

No, I was just suggesting to modify the caller you changed/added within 
the different patch.

Regards,

-- 
Julien Grall

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

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

[Shanker Donthineni]

On 06/27/2016 11:09 AM, Julien Grall wrote:
> On 27/06/16 17:07, Shanker Donthineni wrote:
>> On 06/27/2016 10:41 AM, Julien Grall wrote:
>>>
>>>
>>> On 27/06/16 16:40, Shanker Donthineni wrote:
>>>>>> +    gicv3.rdist_regions = rdist_regs;
>>>>>> +
>>>>>> +    /* Parse always-on power domain Re-distributor entries */
>>>>>> +    count = acpi_parse_entries(ACPI_SIG_MADT,
>>>>>> +                               sizeof(struct acpi_table_madt),
>>>>>> + gic_acpi_parse_madt_redistributor,
>>>>> table,
>>>>>> + ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
>>>>> count);
>>>>>
>>>>> Please use acpi_table_parse_madt here.
>>>>>
>>>>
>>>> How do we pass MADT table pointer that we are using to a function
>>>> acpi_table_parse_madt()?  We have already obtained  the MADT table
>>>> address by calling  acpi_get_table() at the beginning of the function
>>>> gicv3_acpi_init().
>>>
>>> You don't need to pass it. The function will take care to get the MADT
>>> table for you.
>>>
>>
>> Are you expecting me to modify current driver to replace
>> acpi_parse_entries() calls with acpi_table_parse_madt() before my
> changes?
>
> No, I was just suggesting to modify the caller you changed/added 
> within the different patch.
>

Code becomes ugly for readability purpose using two set of functions in 
a single function.

> 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 V2 03/10] arm/gic-v3: Fold GICR subtable parsing into a new function

[Julien Grall]

On 27/06/16 17:17, Shanker Donthineni wrote:
> On 06/27/2016 11:09 AM, Julien Grall wrote:
>> On 27/06/16 17:07, Shanker Donthineni wrote:
>>> On 06/27/2016 10:41 AM, Julien Grall wrote:
>>>>
>>>>
>>>> On 27/06/16 16:40, Shanker Donthineni wrote:
>>>>>>> +    gicv3.rdist_regions = rdist_regs;
>>>>>>> +
>>>>>>> +    /* Parse always-on power domain Re-distributor entries */
>>>>>>> +    count = acpi_parse_entries(ACPI_SIG_MADT,
>>>>>>> +                               sizeof(struct acpi_table_madt),
>>>>>>> + gic_acpi_parse_madt_redistributor,
>>>>>> table,
>>>>>>> + ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
>>>>>> count);
>>>>>>
>>>>>> Please use acpi_table_parse_madt here.
>>>>>>
>>>>>
>>>>> How do we pass MADT table pointer that we are using to a function
>>>>> acpi_table_parse_madt()?  We have already obtained  the MADT table
>>>>> address by calling  acpi_get_table() at the beginning of the function
>>>>> gicv3_acpi_init().
>>>>
>>>> You don't need to pass it. The function will take care to get the MADT
>>>> table for you.
>>>>
>>>
>>> Are you expecting me to modify current driver to replace
>>> acpi_parse_entries() calls with acpi_table_parse_madt() before my
>> changes?
>>
>> No, I was just suggesting to modify the caller you changed/added
>> within the different patch.
>>
>
> Code becomes ugly for readability purpose using two set of functions in
> a single function.

Feel free to modify the rest of the callers.

Regards,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Shanker,

On 27/06/16 16:02, Shanker Donthineni wrote:
>
>
> On 06/27/2016 08:35 AM, Julien Grall wrote:
>> Hi Shanker,
>>
>> On 26/06/16 18:48, Shanker Donthineni wrote:
>>> 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
>> */
>>
>> Is it necessary to have this value part of the arch_domain? I.e Do we
>> need this value after the initialization? If not, then it might be
>> better to add a parameter to domain_vgic_register uint32_t *pointer.
>>
> Absolutely, we don't need this variable after the domain build process.
> I have taken this approach to avoid too many code changes. Your
> suggestion requires changes to functions vgic_v2/v3_init() prototype for
> adding a new parameter.

The internal interface of Xen is not set in stone. We modify quite often 
to match our needs or for clean-up purpose.

I always welcome an interface changes as long as it makes sense.

Regards,

-- 
Julien Grall

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