* [PATCH 1/8] arm/gic-v3: Add a separate function for mapping GICD region
2016-06-18 23:45 [PATCH 0/8] Add support for parsing per CPU Redistributor entry Shanker Donthineni
@ 2016-06-18 23:45 ` Shanker Donthineni
2016-06-21 9:42 ` Julien Grall
2016-06-18 23:45 ` [PATCH 2/8] arm/gic-v3: Fold GICR subtable parsing into a new function Shanker Donthineni
` (7 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Shanker Donthineni @ 2016-06-18 23:45 UTC (permalink / raw)
To: xen-devel, Julien Grall, Stefano Stabellini
Cc: Philip Elcan, Shanker Donthineni, Vikram Sethi
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 1/8] arm/gic-v3: Add a separate function for mapping GICD region
2016-06-18 23:45 ` [PATCH 1/8] arm/gic-v3: Add a separate function for mapping GICD region Shanker Donthineni
@ 2016-06-21 9:42 ` Julien Grall
0 siblings, 0 replies; 25+ messages in thread
From: Julien Grall @ 2016-06-21 9:42 UTC (permalink / raw)
To: Shanker Donthineni, xen-devel, Stefano Stabellini
Cc: Philip Elcan, Vikram Sethi
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 2/8] arm/gic-v3: Fold GICR subtable parsing into a new function
2016-06-18 23:45 [PATCH 0/8] Add support for parsing per CPU Redistributor entry Shanker Donthineni
2016-06-18 23:45 ` [PATCH 1/8] arm/gic-v3: Add a separate function for mapping GICD region Shanker Donthineni
@ 2016-06-18 23:45 ` Shanker Donthineni
2016-06-21 10:17 ` Julien Grall
2016-06-18 23:45 ` [PATCH 3/8] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable Shanker Donthineni
` (6 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Shanker Donthineni @ 2016-06-18 23:45 UTC (permalink / raw)
To: xen-devel, Julien Grall, Stefano Stabellini
Cc: Philip Elcan, Shanker Donthineni, Vikram Sethi
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 2/8] arm/gic-v3: Fold GICR subtable parsing into a new function
2016-06-18 23:45 ` [PATCH 2/8] arm/gic-v3: Fold GICR subtable parsing into a new function Shanker Donthineni
@ 2016-06-21 10:17 ` Julien Grall
2016-06-21 14:02 ` Shanker Donthineni
0 siblings, 1 reply; 25+ messages in thread
From: Julien Grall @ 2016-06-21 10:17 UTC (permalink / raw)
To: Shanker Donthineni, xen-devel, Stefano Stabellini
Cc: Philip Elcan, Vikram Sethi
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 2/8] arm/gic-v3: Fold GICR subtable parsing into a new function
2016-06-21 10:17 ` Julien Grall
@ 2016-06-21 14:02 ` Shanker Donthineni
0 siblings, 0 replies; 25+ messages in thread
From: Shanker Donthineni @ 2016-06-21 14:02 UTC (permalink / raw)
To: Julien Grall, xen-devel, Stefano Stabellini; +Cc: Philip Elcan, Vikram Sethi
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 3/8] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable
2016-06-18 23:45 [PATCH 0/8] Add support for parsing per CPU Redistributor entry Shanker Donthineni
2016-06-18 23:45 ` [PATCH 1/8] arm/gic-v3: Add a separate function for mapping GICD region Shanker Donthineni
2016-06-18 23:45 ` [PATCH 2/8] arm/gic-v3: Fold GICR subtable parsing into a new function Shanker Donthineni
@ 2016-06-18 23:45 ` Shanker Donthineni
2016-06-21 10:16 ` Julien Grall
2016-06-18 23:45 ` [PATCH 4/8] xen/arm: vgic: Use dynamic memory allocation for vgic_rdist_region Shanker Donthineni
` (5 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Shanker Donthineni @ 2016-06-18 23:45 UTC (permalink / raw)
To: xen-devel, Julien Grall, Stefano Stabellini
Cc: Philip Elcan, Shanker Donthineni, Vikram Sethi
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 3/8] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable
2016-06-18 23:45 ` [PATCH 3/8] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable Shanker Donthineni
@ 2016-06-21 10:16 ` Julien Grall
2016-06-21 13:52 ` Shanker Donthineni
0 siblings, 1 reply; 25+ messages in thread
From: Julien Grall @ 2016-06-21 10:16 UTC (permalink / raw)
To: Shanker Donthineni, xen-devel, Stefano Stabellini
Cc: Philip Elcan, Vikram Sethi
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 3/8] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable
2016-06-21 10:16 ` Julien Grall
@ 2016-06-21 13:52 ` Shanker Donthineni
2016-06-22 13:06 ` Julien Grall
0 siblings, 1 reply; 25+ messages in thread
From: Shanker Donthineni @ 2016-06-21 13:52 UTC (permalink / raw)
To: Julien Grall, xen-devel, Stefano Stabellini; +Cc: Philip Elcan, Vikram Sethi
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 3/8] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable
2016-06-21 13:52 ` Shanker Donthineni
@ 2016-06-22 13:06 ` Julien Grall
0 siblings, 0 replies; 25+ messages in thread
From: Julien Grall @ 2016-06-22 13:06 UTC (permalink / raw)
To: shankerd, xen-devel, Stefano Stabellini; +Cc: Philip Elcan, Vikram Sethi
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 4/8] xen/arm: vgic: Use dynamic memory allocation for vgic_rdist_region
2016-06-18 23:45 [PATCH 0/8] Add support for parsing per CPU Redistributor entry Shanker Donthineni
` (2 preceding siblings ...)
2016-06-18 23:45 ` [PATCH 3/8] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable Shanker Donthineni
@ 2016-06-18 23:45 ` Shanker Donthineni
2016-06-21 10:26 ` Julien Grall
2016-06-18 23:45 ` [PATCH 5/8] arm/gic-v3: Remove an unused macro MAX_RDIST_COUNT Shanker Donthineni
` (4 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Shanker Donthineni @ 2016-06-18 23:45 UTC (permalink / raw)
To: xen-devel, Julien Grall, Stefano Stabellini
Cc: Philip Elcan, Shanker Donthineni, Vikram Sethi
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 4/8] xen/arm: vgic: Use dynamic memory allocation for vgic_rdist_region
2016-06-18 23:45 ` [PATCH 4/8] xen/arm: vgic: Use dynamic memory allocation for vgic_rdist_region Shanker Donthineni
@ 2016-06-21 10:26 ` Julien Grall
0 siblings, 0 replies; 25+ messages in thread
From: Julien Grall @ 2016-06-21 10:26 UTC (permalink / raw)
To: Shanker Donthineni, xen-devel, Stefano Stabellini
Cc: Philip Elcan, Vikram Sethi
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 5/8] arm/gic-v3: Remove an unused macro MAX_RDIST_COUNT
2016-06-18 23:45 [PATCH 0/8] Add support for parsing per CPU Redistributor entry Shanker Donthineni
` (3 preceding siblings ...)
2016-06-18 23:45 ` [PATCH 4/8] xen/arm: vgic: Use dynamic memory allocation for vgic_rdist_region Shanker Donthineni
@ 2016-06-18 23:45 ` Shanker Donthineni
2016-06-18 23:45 ` [PATCH 6/8] arm: vgic: Split vgic_domain_init() functionality into two functions Shanker Donthineni
` (3 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Shanker Donthineni @ 2016-06-18 23:45 UTC (permalink / raw)
To: xen-devel, Julien Grall, Stefano Stabellini
Cc: Philip Elcan, Shanker Donthineni, Vikram Sethi
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 6/8] arm: vgic: Split vgic_domain_init() functionality into two functions
2016-06-18 23:45 [PATCH 0/8] Add support for parsing per CPU Redistributor entry Shanker Donthineni
` (4 preceding siblings ...)
2016-06-18 23:45 ` [PATCH 5/8] arm/gic-v3: Remove an unused macro MAX_RDIST_COUNT Shanker Donthineni
@ 2016-06-18 23:45 ` Shanker Donthineni
2016-06-21 10:49 ` Julien Grall
2016-06-18 23:45 ` [PATCH 7/8] arm/mmio: Use separate memory allocation for mmio handlers Shanker Donthineni
` (2 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Shanker Donthineni @ 2016-06-18 23:45 UTC (permalink / raw)
To: xen-devel, Julien Grall, Stefano Stabellini
Cc: Philip Elcan, Shanker Donthineni, Vikram Sethi
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 6/8] arm: vgic: Split vgic_domain_init() functionality into two functions
2016-06-18 23:45 ` [PATCH 6/8] arm: vgic: Split vgic_domain_init() functionality into two functions Shanker Donthineni
@ 2016-06-21 10:49 ` Julien Grall
2016-06-21 14:36 ` Shanker Donthineni
0 siblings, 1 reply; 25+ messages in thread
From: Julien Grall @ 2016-06-21 10:49 UTC (permalink / raw)
To: Shanker Donthineni, xen-devel, Stefano Stabellini
Cc: Philip Elcan, Vikram Sethi
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 6/8] arm: vgic: Split vgic_domain_init() functionality into two functions
2016-06-21 10:49 ` Julien Grall
@ 2016-06-21 14:36 ` Shanker Donthineni
2016-06-21 14:48 ` Julien Grall
0 siblings, 1 reply; 25+ messages in thread
From: Shanker Donthineni @ 2016-06-21 14:36 UTC (permalink / raw)
To: Julien Grall, xen-devel, Stefano Stabellini; +Cc: Philip Elcan, Vikram Sethi
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 6/8] arm: vgic: Split vgic_domain_init() functionality into two functions
2016-06-21 14:36 ` Shanker Donthineni
@ 2016-06-21 14:48 ` Julien Grall
2016-06-21 15:09 ` Shanker Donthineni
0 siblings, 1 reply; 25+ messages in thread
From: Julien Grall @ 2016-06-21 14:48 UTC (permalink / raw)
To: shankerd, xen-devel, Stefano Stabellini; +Cc: Philip Elcan, Vikram Sethi
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 6/8] arm: vgic: Split vgic_domain_init() functionality into two functions
2016-06-21 14:48 ` Julien Grall
@ 2016-06-21 15:09 ` Shanker Donthineni
0 siblings, 0 replies; 25+ messages in thread
From: Shanker Donthineni @ 2016-06-21 15:09 UTC (permalink / raw)
To: Julien Grall, xen-devel, Stefano Stabellini; +Cc: Philip Elcan, Vikram Sethi
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 7/8] arm/mmio: Use separate memory allocation for mmio handlers
2016-06-18 23:45 [PATCH 0/8] Add support for parsing per CPU Redistributor entry Shanker Donthineni
` (5 preceding siblings ...)
2016-06-18 23:45 ` [PATCH 6/8] arm: vgic: Split vgic_domain_init() functionality into two functions Shanker Donthineni
@ 2016-06-18 23:45 ` Shanker Donthineni
2016-06-18 23:45 ` [PATCH 8/8] arm/vgic: Change fixed number of mmio handlers to variable number Shanker Donthineni
2016-06-21 9:28 ` [PATCH 0/8] Add support for parsing per CPU Redistributor entry Julien Grall
8 siblings, 0 replies; 25+ messages in thread
From: Shanker Donthineni @ 2016-06-18 23:45 UTC (permalink / raw)
To: xen-devel, Julien Grall, Stefano Stabellini
Cc: Philip Elcan, Shanker Donthineni, Vikram Sethi
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 8/8] arm/vgic: Change fixed number of mmio handlers to variable number
2016-06-18 23:45 [PATCH 0/8] Add support for parsing per CPU Redistributor entry Shanker Donthineni
` (6 preceding siblings ...)
2016-06-18 23:45 ` [PATCH 7/8] arm/mmio: Use separate memory allocation for mmio handlers Shanker Donthineni
@ 2016-06-18 23:45 ` Shanker Donthineni
2016-06-21 9:28 ` [PATCH 0/8] Add support for parsing per CPU Redistributor entry Julien Grall
8 siblings, 0 replies; 25+ messages in thread
From: Shanker Donthineni @ 2016-06-18 23:45 UTC (permalink / raw)
To: xen-devel, Julien Grall, Stefano Stabellini
Cc: Philip Elcan, Shanker Donthineni, Vikram Sethi
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 0/8] Add support for parsing per CPU Redistributor entry
2016-06-18 23:45 [PATCH 0/8] Add support for parsing per CPU Redistributor entry Shanker Donthineni
` (7 preceding siblings ...)
2016-06-18 23:45 ` [PATCH 8/8] arm/vgic: Change fixed number of mmio handlers to variable number Shanker Donthineni
@ 2016-06-21 9:28 ` Julien Grall
2016-06-21 13:37 ` Shanker Donthineni
8 siblings, 1 reply; 25+ messages in thread
From: Julien Grall @ 2016-06-21 9:28 UTC (permalink / raw)
To: Shanker Donthineni, xen-devel, Stefano Stabellini
Cc: Philip Elcan, Vikram Sethi
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 0/8] Add support for parsing per CPU Redistributor entry
2016-06-21 9:28 ` [PATCH 0/8] Add support for parsing per CPU Redistributor entry Julien Grall
@ 2016-06-21 13:37 ` Shanker Donthineni
2016-06-21 13:50 ` Julien Grall
0 siblings, 1 reply; 25+ messages in thread
From: Shanker Donthineni @ 2016-06-21 13:37 UTC (permalink / raw)
To: Julien Grall, xen-devel, Stefano Stabellini; +Cc: Philip Elcan, Vikram Sethi
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/8] Add support for parsing per CPU Redistributor entry
2016-06-21 13:37 ` Shanker Donthineni
@ 2016-06-21 13:50 ` Julien Grall
2016-06-21 14:16 ` Shanker Donthineni
0 siblings, 1 reply; 25+ messages in thread
From: Julien Grall @ 2016-06-21 13:50 UTC (permalink / raw)
To: shankerd, xen-devel, Stefano Stabellini; +Cc: Philip Elcan, Vikram Sethi
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/8] Add support for parsing per CPU Redistributor entry
2016-06-21 13:50 ` Julien Grall
@ 2016-06-21 14:16 ` Shanker Donthineni
2016-06-21 14:44 ` Julien Grall
0 siblings, 1 reply; 25+ messages in thread
From: Shanker Donthineni @ 2016-06-21 14:16 UTC (permalink / raw)
To: Julien Grall, xen-devel, Stefano Stabellini; +Cc: Philip Elcan, Vikram Sethi
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/8] Add support for parsing per CPU Redistributor entry
2016-06-21 14:16 ` Shanker Donthineni
@ 2016-06-21 14:44 ` Julien Grall
0 siblings, 0 replies; 25+ messages in thread
From: Julien Grall @ 2016-06-21 14:44 UTC (permalink / raw)
To: shankerd, xen-devel, Stefano Stabellini; +Cc: Philip Elcan, Vikram Sethi
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
^ permalink raw reply [flat|nested] 25+ messages in thread