[PATCH 0/8] Add support for parsing per CPU Redistributor entry

[PATCH 0/8] 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.

https://patchwork.kernel.org/patch/9163435/

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

Patches #4, #5, #6, #7 and #8 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.

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 (8):
  arm/gic-v3: Add a separate function for mapping GICD region
  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/mmio: Use separate memory allocation for mmio handlers
  arm/vgic: Change fixed number of mmio handlers to variable number

 xen/arch/arm/domain.c             |  12 ++-
 xen/arch/arm/gic-v3.c             | 162 ++++++++++++++++++++++++++++----------
 xen/arch/arm/io.c                 |  14 +++-
 xen/arch/arm/vgic-v2.c            |  16 +++-
 xen/arch/arm/vgic-v3.c            |  65 ++++++++++-----
 xen/arch/arm/vgic.c               |   7 ++
 xen/include/asm-arm/domain.h      |  13 +--
 xen/include/asm-arm/gic.h         |   3 +-
 xen/include/asm-arm/gic_v3_defs.h |   1 +
 xen/include/asm-arm/mmio.h        |   6 +-
 xen/include/asm-arm/vgic.h        |   5 ++
 11 files changed, 226 insertions(+), 78 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 1/8] arm/gic-v3: Add a separate function for mapping GICD region

[Shanker Donthineni]

Move the code that validates base address and does ioremap of GIC
distributor region to a separate function. Later patches need to
access the GICD region inside function gicv3_acpi_init() for
finding per CPU Redistributor size.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
 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 8d3f149..ab1f380 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 2/8] 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>
---
 xen/arch/arm/gic-v3.c | 64 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index ab1f380..af12ebc 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1387,6 +1387,36 @@ 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;
+    struct rdist_region *region;
+
+    region = gicv3.rdist_regions + gicv3.rdist_count;
+    rdist = (struct acpi_madt_generic_redistributor *)header;
+    if ( BAD_MADT_ENTRY(rdist, end) )
+        return -EINVAL;
+
+    if ( !rdist->base_address || !rdist->length )
+        return -EINVAL;
+
+    region->base = rdist->base_address;
+    region->size = rdist->length;
+
+    region->map_base = ioremap_nocache(region->base, region->size);
+    if ( !region->map_base )
+    {
+        printk("Unable to map GICR registers\n");
+        return -ENOMEM;
+    }
+    gicv3.rdist_count++;
+
+    return 0;
+}
+
 static int __init
 gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header *header,
                                     const unsigned long end)
@@ -1402,7 +1432,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 +1463,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 3/8] 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.

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

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
 xen/arch/arm/gic-v3.c             | 97 ++++++++++++++++++++++++++++++++-------
 xen/include/asm-arm/gic.h         |  2 +
 xen/include/asm-arm/gic_v3_defs.h |  1 +
 3 files changed, 83 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index af12ebc..42cf848 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,6 +1286,11 @@ static int gicv3_iomem_deny_access(const struct domain *d)
 }
 
 #ifdef CONFIG_ACPI
+static 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;
@@ -1393,18 +1402,39 @@ gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header,
                                   const unsigned long end)
 {
     struct acpi_madt_generic_redistributor *rdist;
+    struct acpi_madt_generic_interrupt *processor;
     struct rdist_region *region;
 
     region = gicv3.rdist_regions + gicv3.rdist_count;
-    rdist = (struct acpi_madt_generic_redistributor *)header;
-    if ( BAD_MADT_ENTRY(rdist, end) )
-        return -EINVAL;
+    if ( header->type == ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR )
+    {
+        rdist = (struct acpi_madt_generic_redistributor *)header;
+        if ( BAD_MADT_ENTRY(rdist, end) )
+            return -EINVAL;
 
-    if ( !rdist->base_address || !rdist->length )
-        return -EINVAL;
+        if ( !rdist->base_address || !rdist->length )
+            return -EINVAL;
+
+        region->base = rdist->base_address;
+        region->size = rdist->length;
+    }
+    else if ( header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT )
+    {
+        processor = (struct acpi_madt_generic_interrupt *)header;
+        if ( BAD_MADT_ENTRY(processor, end) )
+            return -EINVAL;
+
+        if ( !(processor->flags & ACPI_MADT_ENABLED) )
+            return 0;
+
+        if ( !processor->gicr_base_address )
+            return -EINVAL;
+
+        region->base = processor->gicr_base_address;
+        region->size = gic_dist_supports_dvis() ? SZ_256K : SZ_128K;
+	region->single_rdist = true;
+   }
 
-    region->base = rdist->base_address;
-    region->size = rdist->length;
 
     region->map_base = ioremap_nocache(region->base, region->size);
     if ( !region->map_base )
@@ -1412,6 +1442,7 @@ gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header,
         printk("Unable to map GICR registers\n");
         return -ENOMEM;
     }
+
     gicv3.rdist_count++;
 
     return 0;
@@ -1421,9 +1452,22 @@ static int __init
 gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header *header,
                                     const unsigned long end)
 {
-    /* Nothing to do here since it only wants to get the number of GIC
-     * redistributors.
-     */
+    struct acpi_madt_generic_redistributor *rdist;
+    struct acpi_madt_generic_interrupt *cpuif;
+
+    if ( header->type == ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR )
+    {
+	 rdist = (struct acpi_madt_generic_redistributor *)header;
+	 if ( BAD_MADT_ENTRY(rdist, end) || !rdist->base_address )
+	     return -EINVAL;
+    }
+    else if ( header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT )
+    {
+	 cpuif = (struct acpi_madt_generic_interrupt *)header;
+	 if ( BAD_MADT_ENTRY(cpuif, end) || !cpuif->gicr_base_address )
+	     return -EINVAL;
+    }
+
     return 0;
 }
 
@@ -1431,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;
 
@@ -1460,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_redistributor_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"
@@ -1473,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_madt_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..7f9ad86 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -101,6 +101,8 @@
 #define GICD_TYPE_CPUS_SHIFT 5
 #define GICD_TYPE_CPUS  0x0e0
 #define GICD_TYPE_SEC   0x400
+#define GICD_TYPER_LPIS (1U << 17)
+#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 4/8] 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>
---
 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 | 12 +++++++-----
 xen/include/asm-arm/vgic.h   |  2 ++
 5 files changed, 35 insertions(+), 8 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..9492727 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -40,6 +40,12 @@ struct vtimer {
         uint64_t cval;
 };
 
+struct vgic_rdist_region {
+    paddr_t base;		    /* Base address */
+    paddr_t size;		    /* Size */
+    unsigned int first_cpu;	    /* First CPU handled */
+};
+
 struct arch_domain
 {
 #ifdef CONFIG_ARM_64
@@ -103,11 +109,7 @@ struct arch_domain
 #ifdef CONFIG_HAS_GICV3
         /* GIC V3 addressing */
         /* List of contiguous occupied by the redistributors */
-        struct vgic_rdist_region {
-            paddr_t base;                   /* Base address */
-            paddr_t size;                   /* Size */
-            unsigned int first_cpu;         /* First CPU handled */
-        } rdist_regions[MAX_RDIST_COUNT];
+        struct vgic_rdist_region *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 5/8] 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 42cf848..28c00cf 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 7f9ad86..3694e07 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 6/8] arm: vgic: Split vgic_domain_init() functionality into two functions

[Shanker Donthineni]

Split code that installs mmio handlers for GICD and Re-distributor
regions to a new function. The intension of this separation is to defer
steps that registers vgic_v2/v3 mmio handlers.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
 xen/arch/arm/vgic-v2.c     | 10 +++++++---
 xen/arch/arm/vgic-v3.c     | 40 +++++++++++++++++++++++-----------------
 xen/arch/arm/vgic.c        |  3 +++
 xen/include/asm-arm/vgic.h |  2 ++
 4 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index f5778e6..d42b408 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -645,6 +645,12 @@ static int vgic_v2_vcpu_init(struct vcpu *v)
     return 0;
 }
 
+static void vgic_v2_domain_register_mmio(struct domain *d)
+{
+    register_mmio_handler(d, &vgic_v2_distr_mmio_handler, d->arch.vgic.dbase,
+			  PAGE_SIZE, NULL);
+}
+
 static int vgic_v2_domain_init(struct domain *d)
 {
     int ret;
@@ -693,9 +699,6 @@ static int vgic_v2_domain_init(struct domain *d)
     if ( ret )
         return ret;
 
-    register_mmio_handler(d, &vgic_v2_distr_mmio_handler, d->arch.vgic.dbase,
-                          PAGE_SIZE, NULL);
-
     return 0;
 }
 
@@ -708,6 +711,7 @@ 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,
+    .domain_register_mmio = vgic_v2_domain_register_mmio,
     .max_vcpus = 8,
 };
 
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index e877e9e..3a5aeb6 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1391,6 +1391,28 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
     return 0;
 }
 
+static void vgic_v3_domain_register_mmio(struct domain *d)
+{
+    int i;
+
+    /* Register mmio handle for the Distributor */
+    register_mmio_handler(d, &vgic_distr_mmio_handler, d->arch.vgic.dbase,
+		          SZ_64K, NULL);
+
+    /*
+     * Register mmio handler per contiguous region occupied by the
+     * redistributors. The handler will take care to choose which
+     * redistributor is targeted.
+     */
+    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
+    {
+        struct vgic_rdist_region *region = &d->arch.vgic.rdist_regions[i];
+
+        register_mmio_handler(d, &vgic_rdistr_mmio_handler,
+			  region->base, region->size, region);
+    }
+}
+
 static int vgic_v3_domain_init(struct domain *d)
 {
     struct vgic_rdist_region *rdist_regions;
@@ -1455,23 +1477,6 @@ static int vgic_v3_domain_init(struct domain *d)
         d->arch.vgic.rdist_regions[0].first_cpu = 0;
     }
 
-    /* Register mmio handle for the Distributor */
-    register_mmio_handler(d, &vgic_distr_mmio_handler, d->arch.vgic.dbase,
-                          SZ_64K, NULL);
-
-    /*
-     * Register mmio handler per contiguous region occupied by the
-     * redistributors. The handler will take care to choose which
-     * redistributor is targeted.
-     */
-    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
-    {
-        struct vgic_rdist_region *region = &d->arch.vgic.rdist_regions[i];
-
-        register_mmio_handler(d, &vgic_rdistr_mmio_handler,
-                              region->base, region->size, region);
-    }
-
     d->arch.vgic.ctlr = VGICD_CTLR_DEFAULT;
 
     return 0;
@@ -1487,6 +1492,7 @@ static const struct vgic_ops v3_ops = {
     .domain_init = vgic_v3_domain_init,
     .domain_free = vgic_v3_domain_free,
     .emulate_sysreg  = vgic_v3_emulate_sysreg,
+    .domain_register_mmio = vgic_v3_domain_register_mmio,
     /*
      * We use both AFF1 and AFF0 in (v)MPIDR. Thus, the max number of CPU
      * that can be supported is up to 4096(==256*16) in theory.
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 5df5f01..5b39e0d 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -151,9 +151,12 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
     for ( i = 0; i < NR_GIC_SGI; i++ )
         set_bit(i, d->arch.vgic.allocated_irqs);
 
+    d->arch.vgic.handler->domain_register_mmio(d);
+
     return 0;
 }
 
+
 void register_vgic_ops(struct domain *d, const struct vgic_ops *ops)
 {
    d->arch.vgic.handler = ops;
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index fbb763a..8fe65b4 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -132,6 +132,8 @@ struct vgic_ops {
     void (*domain_free)(struct domain *d);
     /* vGIC sysreg emulation */
     int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr hsr);
+    /* Register mmio handlers */
+    void (*domain_register_mmio)(struct domain *d);
     /* Maximum number of vCPU supported */
     const unsigned int max_vcpus;
 };
-- 
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 7/8] arm/mmio: 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 8/8] 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_domain_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_init()
   count = MAX_IO_HANDLER + d->arch.vgic.mmio_count;
   domain_io_init(count)
     domain_vgic_register_mmio()

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

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 4010ff2..ef567c8 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;
 
@@ -593,6 +589,12 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     if ( (rc = domain_vgic_init(d, config->nr_spis)) != 0 )
         goto fail;
 
+    count = MAX_IO_HANDLER + d->arch.vgic.mmio_count;
+    if ( (rc = domain_io_init(d, count)) != 0 )
+        goto fail;
+
+    domain_vgic_register_mmio(d);
+
     if ( (rc = domain_vtimer_init(d, config)) != 0 )
         goto fail;
 
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index d42b408..7ed7d32 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -699,6 +699,8 @@ static int vgic_v2_domain_init(struct domain *d)
     if ( ret )
         return ret;
 
+    d->arch.vgic.mmio_count = 1; /* Only GICD region */
+
     return 0;
 }
 
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 3a5aeb6..6a5f333 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1479,6 +1479,9 @@ static int vgic_v3_domain_init(struct domain *d)
 
     d->arch.vgic.ctlr = VGICD_CTLR_DEFAULT;
 
+    /* GICD region + number of Re-distributors */
+    d->arch.vgic.mmio_count = d->arch.vgic.nr_regions + 1;
+
     return 0;
 }
 
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 5b39e0d..7d34942 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -88,6 +88,11 @@ static void vgic_rank_init(struct vgic_irq_rank *rank, uint8_t index,
         rank->vcpu[i] = vcpu;
 }
 
+void domain_vgic_register_mmio(struct domain *d)
+{
+    d->arch.vgic.handler->domain_register_mmio(d);
+}
+
 int domain_vgic_init(struct domain *d, unsigned int nr_spis)
 {
     int i;
@@ -151,8 +156,6 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
     for ( i = 0; i < NR_GIC_SGI; i++ )
         set_bit(i, d->arch.vgic.allocated_irqs);
 
-    d->arch.vgic.handler->domain_register_mmio(d);
-
     return 0;
 }
 
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 9492727..974cf93 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -113,6 +113,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 8fe65b4..0032633 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -309,6 +309,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 void domain_vgic_register_mmio(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 0/8] Add support for parsing per CPU Redistributor entry

[Julien Grall]

Hello Shanker,

On 19/06/16 00:45, Shanker Donthineni wrote:
> 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.
>
> https://patchwork.kernel.org/patch/9163435/

You addressed only one part of my comment. This series increases the 
number of I/O handlers but the lookup is still linear (see handle_mmio 
in arch/arm/io.c).

After this series, the maximum number of I/O handlers is 160. So in the 
worst case, we have to do 160 iterations before finding an handler or 
concluding the I/O cannot be emulated.

Regards,

-- 
Julien Grall

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

Re: [PATCH 1/8] arm/gic-v3: Add a separate function for mapping GICD region

[Julien Grall]

Hello Shanker,

On 19/06/16 00:45, Shanker Donthineni wrote:
> Move the code that validates base address and does ioremap of GIC
> distributor region to a separate function. Later patches need to
> access the GICD region inside function gicv3_acpi_init() for
> finding per CPU Redistributor size.

This patch contains two things:
    - A clean up: I.e moving the validation/ioremap in a function
    - Calling ioremap map earlier

The latter is the most important point of this patch, not the clean up. 
However based on your commit message/title, only the clean up matter.

Please reword the commit message/title to make clear what matters.

>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> ---
>   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 8d3f149..ab1f380 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) )

The second pair of brackets is pointless.

> +        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");
> +}
> +

Regards,

-- 
Julien Grall

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

Re: [PATCH 3/8] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable

[Julien Grall]

Hello Shanker,

On 19/06/16 00:45, 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.
>
> This patch adds necessary code to handle both types of Redistributors
> base addresses.
>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> ---
>   xen/arch/arm/gic-v3.c             | 97 ++++++++++++++++++++++++++++++++-------
>   xen/include/asm-arm/gic.h         |  2 +
>   xen/include/asm-arm/gic_v3_defs.h |  1 +
>   3 files changed, 83 insertions(+), 17 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index af12ebc..42cf848 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,6 +1286,11 @@ static int gicv3_iomem_deny_access(const struct domain *d)
>   }
>
>   #ifdef CONFIG_ACPI
> +static bool gic_dist_supports_dvis(void)

static inline and please use bool_t here.

> +{
> +    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;
> @@ -1393,18 +1402,39 @@ gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header,
>                                     const unsigned long end)
>   {
>       struct acpi_madt_generic_redistributor *rdist;
> +    struct acpi_madt_generic_interrupt *processor;
>       struct rdist_region *region;
>
>       region = gicv3.rdist_regions + gicv3.rdist_count;
> -    rdist = (struct acpi_madt_generic_redistributor *)header;
> -    if ( BAD_MADT_ENTRY(rdist, end) )
> -        return -EINVAL;
> +    if ( header->type == ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR )
> +    {
> +        rdist = (struct acpi_madt_generic_redistributor *)header;
> +        if ( BAD_MADT_ENTRY(rdist, end) )
> +            return -EINVAL;
>
> -    if ( !rdist->base_address || !rdist->length )
> -        return -EINVAL;
> +        if ( !rdist->base_address || !rdist->length )
> +            return -EINVAL;
> +
> +        region->base = rdist->base_address;
> +        region->size = rdist->length;
> +    }
> +    else if ( header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT )
> +    {

Parsing the GICC and the redistributor is quite different. I would much 
prefer a function for parsing each table and an helper to add a new 
redistributor.

> +        processor = (struct acpi_madt_generic_interrupt *)header;
> +        if ( BAD_MADT_ENTRY(processor, end) )
> +            return -EINVAL;
> +
> +        if ( !(processor->flags & ACPI_MADT_ENABLED) )
> +            return 0;
> +
> +        if ( !processor->gicr_base_address )
> +            return -EINVAL;
> +
> +        region->base = processor->gicr_base_address;
> +        region->size = gic_dist_supports_dvis() ? SZ_256K : SZ_128K;

Please explain in the commit message how you find the size. I would also 
prefer if you use (4 x SZ_64K) and (2 * SZ_64K) as we do in populate_rdist.

> +	region->single_rdist = true;

The indentation looks wrong.

> +   }


>
> -    region->base = rdist->base_address;
> -    region->size = rdist->length;
>
>       region->map_base = ioremap_nocache(region->base, region->size);
>       if ( !region->map_base )
> @@ -1412,6 +1442,7 @@ gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header,
>           printk("Unable to map GICR registers\n");
>           return -ENOMEM;
>       }
> +

Spurious change.

>       gicv3.rdist_count++;
>
>       return 0;
> @@ -1421,9 +1452,22 @@ static int __init
>   gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header *header,
>                                       const unsigned long end)
>   {
> -    /* Nothing to do here since it only wants to get the number of GIC
> -     * redistributors.
> -     */
> +    struct acpi_madt_generic_redistributor *rdist;
> +    struct acpi_madt_generic_interrupt *cpuif;
> +
> +    if ( header->type == ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR )
> +    {
> +	 rdist = (struct acpi_madt_generic_redistributor *)header;
> +	 if ( BAD_MADT_ENTRY(rdist, end) || !rdist->base_address )
> +	     return -EINVAL;
> +    }
> +    else if ( header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT )
> +    {
> +	 cpuif = (struct acpi_madt_generic_interrupt *)header;
> +	 if ( BAD_MADT_ENTRY(cpuif, end) || !cpuif->gicr_base_address )
> +	     return -EINVAL;
> +    }
> +

Ditto for the parsing.

>       return 0;
>   }
>

[...]

> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 44b9ef6..7f9ad86 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -101,6 +101,8 @@
>   #define GICD_TYPE_CPUS_SHIFT 5
>   #define GICD_TYPE_CPUS  0x0e0
>   #define GICD_TYPE_SEC   0x400
> +#define GICD_TYPER_LPIS (1U << 17)

This is not used by this patch. So please drop it.

> +#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 2/8] arm/gic-v3: Fold GICR subtable parsing into a new function

[Julien Grall]

Hello Shanker,

On 19/06/16 00:45, Shanker Donthineni wrote:
> 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>
> ---
>   xen/arch/arm/gic-v3.c | 64 +++++++++++++++++++++++++++++++++------------------
>   1 file changed, 42 insertions(+), 22 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index ab1f380..af12ebc 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1387,6 +1387,36 @@ 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;
> +    struct rdist_region *region;
> +
> +    region = gicv3.rdist_regions + gicv3.rdist_count;
> +    rdist = (struct acpi_madt_generic_redistributor *)header;
> +    if ( BAD_MADT_ENTRY(rdist, end) )
> +        return -EINVAL;
> +
> +    if ( !rdist->base_address || !rdist->length )
> +        return -EINVAL;
> +
> +    region->base = rdist->base_address;
> +    region->size = rdist->length;
> +
> +    region->map_base = ioremap_nocache(region->base, region->size);

In the commit message you said there is no functional change, however 
the remapping is not part of gicv3_acpi_init. So why did you add this 
line here?

> +    if ( !region->map_base )
> +    {
> +        printk("Unable to map GICR registers\n");
> +        return -ENOMEM;
> +    }
> +    gicv3.rdist_count++;
> +
> +    return 0;
> +}
> +

[...]

Regards,

-- 
Julien Grall

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

Re: [PATCH 4/8] xen/arm: vgic: Use dynamic memory allocation for vgic_rdist_region

[Julien Grall]

Hello Shanker,

On 19/06/16 00:45, Shanker Donthineni wrote:
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 370cdeb..9492727 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -40,6 +40,12 @@ struct vtimer {
>           uint64_t cval;
>   };
>
> +struct vgic_rdist_region {
> +    paddr_t base;		    /* Base address */
> +    paddr_t size;		    /* Size */
> +    unsigned int first_cpu;	    /* First CPU handled */
> +};
> +
>   struct arch_domain
>   {
>   #ifdef CONFIG_ARM_64
> @@ -103,11 +109,7 @@ struct arch_domain
>   #ifdef CONFIG_HAS_GICV3
>           /* GIC V3 addressing */
>           /* List of contiguous occupied by the redistributors */
> -        struct vgic_rdist_region {
> -            paddr_t base;                   /* Base address */
> -            paddr_t size;                   /* Size */
> -            unsigned int first_cpu;         /* First CPU handled */
> -        } rdist_regions[MAX_RDIST_COUNT];
> +        struct vgic_rdist_region *rdist_regions;

I don't think it is necessary to move the definition of 
vgic_rdist_region outside.

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

Regards,

-- 
Julien Grall

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

Re: [PATCH 6/8] arm: vgic: Split vgic_domain_init() functionality into two functions

[Julien Grall]

Hello Shanker,

On 19/06/16 00:45, Shanker Donthineni wrote:
> Split code that installs mmio handlers for GICD and Re-distributor
> regions to a new function. The intension of this separation is to defer
> steps that registers vgic_v2/v3 mmio handlers.

Looking at this patch and the follow-up ones, I don't think this is the 
right way to go. You differ the registration of the IO handlers just 
because you are unable to find the size of the handlers array.

I am wondering if the array for the handlers is the best solution here. 
On another side, it would be possible to find the maximum of handlers 
before hand.

> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 5df5f01..5b39e0d 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -151,9 +151,12 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
>       for ( i = 0; i < NR_GIC_SGI; i++ )
>           set_bit(i, d->arch.vgic.allocated_irqs);
>
> +    d->arch.vgic.handler->domain_register_mmio(d);
> +
>       return 0;
>   }
>
> +

Spurious change.

>   void register_vgic_ops(struct domain *d, const struct vgic_ops *ops)
>   {
>      d->arch.vgic.handler = ops;
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index fbb763a..8fe65b4 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -132,6 +132,8 @@ struct vgic_ops {
>       void (*domain_free)(struct domain *d);
>       /* vGIC sysreg emulation */
>       int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr hsr);
> +    /* Register mmio handlers */
> +    void (*domain_register_mmio)(struct domain *d);
>       /* Maximum number of vCPU supported */
>       const unsigned int max_vcpus;
>   };
>

Regards,

-- 
Julien Grall

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

Re: [PATCH 0/8] Add support for parsing per CPU Redistributor entry

[Shanker Donthineni]

On 06/21/2016 04:28 AM, Julien Grall wrote:
> Hello Shanker,
>
> On 19/06/16 00:45, Shanker Donthineni wrote:
>> 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.
>>
>> https://patchwork.kernel.org/patch/9163435/
>
> You addressed only one part of my comment. This series increases the 
> number of I/O handlers but the lookup is still linear (see handle_mmio 
> in arch/arm/io.c).
>
I agree with you, we need to bring binary search algorithm similar to 
Linux KVM code. I want to keep it this change outside of this patchset.
> After this series, the maximum number of I/O handlers is 160. So in 
> the worst case, we have to do 160 iterations before finding an handler 
> or concluding the I/O cannot be emulated.
>
> 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 0/8] Add support for parsing per CPU Redistributor entry

[Julien Grall]

On 21/06/16 14:37, Shanker Donthineni wrote:
> On 06/21/2016 04:28 AM, Julien Grall wrote:
>> On 19/06/16 00:45, Shanker Donthineni wrote:
>>> 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.
>>>
>>> https://patchwork.kernel.org/patch/9163435/
>>
>> You addressed only one part of my comment. This series increases the
>> number of I/O handlers but the lookup is still linear (see handle_mmio
>> in arch/arm/io.c).
>>
> I agree with you, we need to bring binary search algorithm similar to
> Linux KVM code. I want to keep it this change outside of this patchset.

This should be a prerequisite of this series then, not a follow-up.

Regards,

-- 
Julien Grall

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

Re: [PATCH 3/8] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable

[Shanker Donthineni]

On 06/21/2016 05:16 AM, Julien Grall wrote:
> Hello Shanker,
>
> On 19/06/16 00:45, 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.
>>
>> This patch adds necessary code to handle both types of Redistributors
>> base addresses.
>>
>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
>> ---
>>   xen/arch/arm/gic-v3.c             | 97
> ++++++++++++++++++++++++++++++++-------
>>   xen/include/asm-arm/gic.h         |  2 +
>>   xen/include/asm-arm/gic_v3_defs.h |  1 +
>>   3 files changed, 83 insertions(+), 17 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index af12ebc..42cf848 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,6 +1286,11 @@ static int gicv3_iomem_deny_access(const struct
> domain *d)
>>   }
>>
>>   #ifdef CONFIG_ACPI
>> +static bool gic_dist_supports_dvis(void)
>
> static inline and please use bool_t here.
>
Still learning XEN coding style, I'll fix it.
>> +{
>> +    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;
>> @@ -1393,18 +1402,39 @@ gic_acpi_parse_madt_redistributor(struct
> acpi_subtable_header *header,
>>                                     const unsigned long end)
>>   {
>>       struct acpi_madt_generic_redistributor *rdist;
>> +    struct acpi_madt_generic_interrupt *processor;
>>       struct rdist_region *region;
>>
>>       region = gicv3.rdist_regions + gicv3.rdist_count;
>> -    rdist = (struct acpi_madt_generic_redistributor *)header;
>> -    if ( BAD_MADT_ENTRY(rdist, end) )
>> -        return -EINVAL;
>> +    if ( header->type == ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR )
>> +    {
>> +        rdist = (struct acpi_madt_generic_redistributor *)header;
>> +        if ( BAD_MADT_ENTRY(rdist, end) )
>> +            return -EINVAL;
>>
>> -    if ( !rdist->base_address || !rdist->length )
>> -        return -EINVAL;
>> +        if ( !rdist->base_address || !rdist->length )
>> +            return -EINVAL;
>> +
>> +        region->base = rdist->base_address;
>> +        region->size = rdist->length;
>> +    }
>> +    else if ( header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT )
>> +    {
>
> Parsing the GICC and the redistributor is quite different. I would 
> much prefer a function for parsing each table and an helper to add a 
> new redistributor.
>
  I'll do.
>> +        processor = (struct acpi_madt_generic_interrupt *)header;
>> +        if ( BAD_MADT_ENTRY(processor, end) )
>> +            return -EINVAL;
>> +
>> +        if ( !(processor->flags & ACPI_MADT_ENABLED) )
>> +            return 0;
>> +
>> +        if ( !processor->gicr_base_address )
>> +            return -EINVAL;
>> +
>> +        region->base = processor->gicr_base_address;
>> +        region->size = gic_dist_supports_dvis() ? SZ_256K : SZ_128K;
>
> Please explain in the commit message how you find the size. I would 
> also prefer if you use (4 x SZ_64K) and (2 * SZ_64K) as we do in
> populate_rdist.
>
>> +    region->single_rdist = true;
>
> The indentation looks wrong.
>
>> +   }
>
>
>>
>> -    region->base = rdist->base_address;
>> -    region->size = rdist->length;
>>
>>       region->map_base = ioremap_nocache(region->base, region->size);
>>       if ( !region->map_base )
>> @@ -1412,6 +1442,7 @@ gic_acpi_parse_madt_redistributor(struct
> acpi_subtable_header *header,
>>           printk("Unable to map GICR registers\n");
>>           return -ENOMEM;
>>       }
>> +
>
> Spurious change.
>
>>       gicv3.rdist_count++;
>>
>>       return 0;
>> @@ -1421,9 +1452,22 @@ static int __init
>>   gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header
> *header,
>> const unsigned long end)
>>   {
>> -    /* Nothing to do here since it only wants to get the number of GIC
>> -     * redistributors.
>> -     */
>> +    struct acpi_madt_generic_redistributor *rdist;
>> +    struct acpi_madt_generic_interrupt *cpuif;
>> +
>> +    if ( header->type == ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR )
>> +    {
>> +     rdist = (struct acpi_madt_generic_redistributor *)header;
>> +     if ( BAD_MADT_ENTRY(rdist, end) || !rdist->base_address )
>> +         return -EINVAL;
>> +    }
>> +    else if ( header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT )
>> +    {
>> +     cpuif = (struct acpi_madt_generic_interrupt *)header;
>> +     if ( BAD_MADT_ENTRY(cpuif, end) || !cpuif->gicr_base_address )
>> +         return -EINVAL;
>> +    }
>> +
>
> Ditto for the parsing.
>
>>       return 0;
>>   }
>>
>
> [...]
>
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index 44b9ef6..7f9ad86 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -101,6 +101,8 @@
>>   #define GICD_TYPE_CPUS_SHIFT 5
>>   #define GICD_TYPE_CPUS  0x0e0
>>   #define GICD_TYPE_SEC   0x400
>> +#define GICD_TYPER_LPIS (1U << 17)
>
> This is not used by this patch. So please drop it.
>
I'll drop it.
>> +#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 2/8] arm/gic-v3: Fold GICR subtable parsing into a new function

[Shanker Donthineni]

On 06/21/2016 05:17 AM, Julien Grall wrote:
> Hello Shanker,
>
> On 19/06/16 00:45, Shanker Donthineni wrote:
>> 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>
>> ---
>>   xen/arch/arm/gic-v3.c | 64
> +++++++++++++++++++++++++++++++++------------------
>>   1 file changed, 42 insertions(+), 22 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index ab1f380..af12ebc 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -1387,6 +1387,36 @@ 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;
>> +    struct rdist_region *region;
>> +
>> +    region = gicv3.rdist_regions + gicv3.rdist_count;
>> +    rdist = (struct acpi_madt_generic_redistributor *)header;
>> +    if ( BAD_MADT_ENTRY(rdist, end) )
>> +        return -EINVAL;
>> +
>> +    if ( !rdist->base_address || !rdist->length )
>> +        return -EINVAL;
>> +
>> +    region->base = rdist->base_address;
>> +    region->size = rdist->length;
>> +
>> +    region->map_base = ioremap_nocache(region->base, region->size);
>
> In the commit message you said there is no functional change, however 
> the remapping is not part of gicv3_acpi_init. So why did you add this 
> line here?
>
Thanks for catching coding bug, it was my mistake and this code should 
not be here.
>> +    if ( !region->map_base )
>> +    {
>> +        printk("Unable to map GICR registers\n");
>> +        return -ENOMEM;
>> +    }
>> +    gicv3.rdist_count++;
>> +
>> +    return 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 0/8] Add support for parsing per CPU Redistributor entry

[Shanker Donthineni]

On 06/21/2016 08:50 AM, Julien Grall wrote:
>
>
> On 21/06/16 14:37, Shanker Donthineni wrote:
>> On 06/21/2016 04:28 AM, Julien Grall wrote:
>>> On 19/06/16 00:45, Shanker Donthineni wrote:
>>>> 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.
>>>>
>>>> https://patchwork.kernel.org/patch/9163435/
>>>
>>> You addressed only one part of my comment. This series increases the
>>> number of I/O handlers but the lookup is still linear (see handle_mmio
>>> in arch/arm/io.c).
>>>
>> I agree with you, we need to bring binary search algorithm similar to
>> Linux KVM code. I want to keep it this change outside of this patchset.
>
> This should be a prerequisite of this series then, not a follow-up.
>
For the  functionality and correctness purpose we don't need this change 
immediately.
We are not able to boot XEN on Qualcomm Technologies because of not 
supporting
GICC table parsing for GICR address.

I am okay to wait for my patchset if someone adding bseach look ups for 
mmio handlers.

> 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 6/8] arm: vgic: Split vgic_domain_init() functionality into two functions

[Shanker Donthineni]

On 06/21/2016 05:49 AM, Julien Grall wrote:
> Hello Shanker,
>
> On 19/06/16 00:45, Shanker Donthineni wrote:
>> Split code that installs mmio handlers for GICD and Re-distributor
>> regions to a new function. The intension of this separation is to defer
>> steps that registers vgic_v2/v3 mmio handlers.
>
> Looking at this patch and the follow-up ones, I don't think this is 
> the right way to go. You differ the registration of the IO handlers 
> just because you are unable to find the size of the handlers array.
>
Is there any better approach?

> I am wondering if the array for the handlers is the best solution 
> here. On another side, it would be possible to find the maximum of 
> handlers before hand.
>
The purpose of this change is to limit size of 'struct domain' less than 
PAGE_SIZE. I can think of second approach split vgic_init() into two 
stages, one for vgic registration and the second one for vgic_init(). 
This also requires a few lines of code changes to vgic_v2/v3_init() and 
vgic_init().

int arch_domain_create(struct domain *d, unsigned int domcr_flags,
                        struct xen_arch_domainconfig *config)
    ...
    domain_vgic_register(d));

    domain_io_init(d, mmio_count);

    domain_vgic_init(d, config->nr_spis));

>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 5df5f01..5b39e0d 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -151,9 +151,12 @@ int domain_vgic_init(struct domain *d, unsigned int
> nr_spis)
>>       for ( i = 0; i < NR_GIC_SGI; i++ )
>>           set_bit(i, d->arch.vgic.allocated_irqs);
>>
>> +    d->arch.vgic.handler->domain_register_mmio(d);
>> +
>>       return 0;
>>   }
>>
>> +
>
> Spurious change.
>
>>   void register_vgic_ops(struct domain *d, const struct vgic_ops *ops)
>>   {
>>      d->arch.vgic.handler = ops;
>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
>> index fbb763a..8fe65b4 100644
>> --- a/xen/include/asm-arm/vgic.h
>> +++ b/xen/include/asm-arm/vgic.h
>> @@ -132,6 +132,8 @@ struct vgic_ops {
>>       void (*domain_free)(struct domain *d);
>>       /* vGIC sysreg emulation */
>>       int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr hsr);
>> +    /* Register mmio handlers */
>> +    void (*domain_register_mmio)(struct domain *d);
>>       /* Maximum number of vCPU supported */
>>       const unsigned int max_vcpus;
>>   };
>>
>
> 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 0/8] Add support for parsing per CPU Redistributor entry

[Julien Grall]

On 21/06/16 15:16, Shanker Donthineni wrote:
> On 06/21/2016 08:50 AM, Julien Grall wrote:
>> On 21/06/16 14:37, Shanker Donthineni wrote:
>>> On 06/21/2016 04:28 AM, Julien Grall wrote:
>>>> On 19/06/16 00:45, Shanker Donthineni wrote:
>>>>> 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.
>>>>>
>>>>> https://patchwork.kernel.org/patch/9163435/
>>>>
>>>> You addressed only one part of my comment. This series increases the
>>>> number of I/O handlers but the lookup is still linear (see handle_mmio
>>>> in arch/arm/io.c).
>>>>
>>> I agree with you, we need to bring binary search algorithm similar to
>>> Linux KVM code. I want to keep it this change outside of this patchset.
>>
>> This should be a prerequisite of this series then, not a follow-up.
>>
> For the  functionality and correctness purpose we don't need this change
> immediately.
> We are not able to boot XEN on Qualcomm Technologies because of not
> supporting
> GICC table parsing for GICR address.
>
> I am okay to wait for my patchset if someone adding bseach look ups for
> mmio handlers.

I am not aware of anyone planning to add bsearch.

Regards,

-- 
Julien Grall

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

Re: [PATCH 6/8] arm: vgic: Split vgic_domain_init() functionality into two functions

[Julien Grall]

On 21/06/16 15:36, Shanker Donthineni wrote:
>
>
> On 06/21/2016 05:49 AM, Julien Grall wrote:
>> Hello Shanker,
>>
>> On 19/06/16 00:45, Shanker Donthineni wrote:
>>> Split code that installs mmio handlers for GICD and Re-distributor
>>> regions to a new function. The intension of this separation is to defer
>>> steps that registers vgic_v2/v3 mmio handlers.
>>
>> Looking at this patch and the follow-up ones, I don't think this is
>> the right way to go. You differ the registration of the IO handlers
>> just because you are unable to find the size of the handlers array.
>>
> Is there any better approach?

Possibly using a different data structure.

>> I am wondering if the array for the handlers is the best solution
>> here. On another side, it would be possible to find the maximum of
>> handlers before hand.
>>
> The purpose of this change is to limit size of 'struct domain' less than
> PAGE_SIZE. I can think of second approach split vgic_init() into two
> stages, one for vgic registration and the second one for vgic_init().
> This also requires a few lines of code changes to vgic_v2/v3_init() and
> vgic_init().

I am fine as long as vgic_register_ does not do more than counting the 
number of IO handlers. You could re-use vgic_init_v{2,3} for this purpose.

Regards,

-- 
Julien Grall

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

Re: [PATCH 6/8] arm: vgic: Split vgic_domain_init() functionality into two functions

[Shanker Donthineni]

Hi Julien,

On 06/21/2016 09:48 AM, Julien Grall wrote:
>
>
> On 21/06/16 15:36, Shanker Donthineni wrote:
>>
>>
>> On 06/21/2016 05:49 AM, Julien Grall wrote:
>>> Hello Shanker,
>>>
>>> On 19/06/16 00:45, Shanker Donthineni wrote:
>>>> Split code that installs mmio handlers for GICD and Re-distributor
>>>> regions to a new function. The intension of this separation is to 
>>>> defer
>>>> steps that registers vgic_v2/v3 mmio handlers.
>>>
>>> Looking at this patch and the follow-up ones, I don't think this is
>>> the right way to go. You differ the registration of the IO handlers
>>> just because you are unable to find the size of the handlers array.
>>>
>> Is there any better approach?
>
> Possibly using a different data structure.
>
>>> I am wondering if the array for the handlers is the best solution
>>> here. On another side, it would be possible to find the maximum of
>>> handlers before hand.
>>>
>> The purpose of this change is to limit size of 'struct domain' less than
>> PAGE_SIZE. I can think of second approach split vgic_init() into two
>> stages, one for vgic registration and the second one for vgic_init().
>> This also requires a few lines of code changes to vgic_v2/v3_init() and
>> vgic_init().
>
> I am fine as long as vgic_register_ does not do more than counting the 
> number of IO handlers. You could re-use vgic_init_v{2,3} for this 
> purpose.
>
The way we are doing vgic_init() initialization has to be cleaned-up and 
rearrange a few lines of code for retrieving the number mmio handlers 
that are required dom0/domU domain.

> 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 3/8] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable

[Julien Grall]

Hello Shanker,

On 21/06/16 14:52, Shanker Donthineni wrote:
> On 06/21/2016 05:16 AM, Julien Grall wrote:
>> Hello Shanker,
>>
>> On 19/06/16 00:45, 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.
>>>
>>> This patch adds necessary code to handle both types of Redistributors
>>> base addresses.
>>>
>>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
>>> ---
>>>   xen/arch/arm/gic-v3.c             | 97
>> ++++++++++++++++++++++++++++++++-------
>>>   xen/include/asm-arm/gic.h         |  2 +
>>>   xen/include/asm-arm/gic_v3_defs.h |  1 +
>>>   3 files changed, 83 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>>> index af12ebc..42cf848 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,6 +1286,11 @@ static int gicv3_iomem_deny_access(const struct
>> domain *d)
>>>   }
>>>
>>>   #ifdef CONFIG_ACPI
>>> +static bool gic_dist_supports_dvis(void)
>>
>> static inline and please use bool_t here.
>>
> Still learning XEN coding style, I'll fix it.

It looks like Xen is moving towards bool (see [1]). So you can keep bool 
here.

Regards,

[1] 
http://lists.xenproject.org/archives/html/xen-devel/2016-06/msg02807.html

-- 
Julien Grall

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