[PATCH] irqchip/gic-v3-its: Allow GIC ITS number more than MAX

Discussion:

[PATCH] irqchip/gic-v3-its: Allow GIC ITS number more than MAX_NUMNODES

Hanjun Guo

2017-07-21 09:51:55 UTC

From: Hanjun Guo <***@linaro.org>

When running 4.13-rc1 on top of D05, I got the boot log:

[ 0.000000] SRAT: PXM 0 -> ITS 0 -> Node 0
[ 0.000000] SRAT: PXM 0 -> ITS 1 -> Node 0
[ 0.000000] SRAT: PXM 0 -> ITS 2 -> Node 0
[ 0.000000] SRAT: PXM 1 -> ITS 3 -> Node 1
[ 0.000000] SRAT: ITS affinity exceeding max count[4]

This is wrong on D05 as we have 8 ITSes with 4 NUMA nodes.

So dynamically alloc the memory needed instead of using
its_srat_maps[MAX_NUMNODES], which count the number of
ITS entry(ies) in SRAT and alloc its_srat_maps as needed,
then build the mapping of numa node to ITS ID.

After doing this, I got what I wanted:

[ 0.000000] SRAT: PXM 0 -> ITS 0 -> Node 0
[ 0.000000] SRAT: PXM 0 -> ITS 1 -> Node 0
[ 0.000000] SRAT: PXM 0 -> ITS 2 -> Node 0
[ 0.000000] SRAT: PXM 1 -> ITS 3 -> Node 1
[ 0.000000] SRAT: PXM 2 -> ITS 4 -> Node 2
[ 0.000000] SRAT: PXM 2 -> ITS 5 -> Node 2
[ 0.000000] SRAT: PXM 2 -> ITS 6 -> Node 2
[ 0.000000] SRAT: PXM 3 -> ITS 7 -> Node 3

Fixes: dbd2b8267233 ("irqchip/gic-v3-its: Add ACPI NUMA node mapping")
Signed-off-by: Hanjun Guo <***@linaro.org>
Cc: Ganapatrao Kulkarni <***@cavium.com>
Cc: Lorenzo Pieralisi <***@arm.com>
Cc: Marc Zyngier <***@arm.com>
---
drivers/irqchip/irq-gic-v3-its.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 3ccdf76..fb1c090 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1847,7 +1847,7 @@ struct its_srat_map {
u32 its_id;
};

-static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata;
+static struct its_srat_map *its_srat_maps __initdata;
static int its_in_srat __initdata;

static int __init acpi_get_its_numa_node(u32 its_id)
@@ -1861,6 +1861,12 @@ static int __init acpi_get_its_numa_node(u32 its_id)
return NUMA_NO_NODE;
}

+static int __init gic_acpi_match_srat_its(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ return 0;
+}
+
static int __init gic_acpi_parse_srat_its(struct acpi_subtable_header *header,
const unsigned long end)
{
@@ -1877,12 +1883,6 @@ static int __init gic_acpi_parse_srat_its(struct acpi_subtable_header *header,
return -EINVAL;
}

- if (its_in_srat >= MAX_NUMNODES) {
- pr_err("SRAT: ITS affinity exceeding max count[%d]\n",
- MAX_NUMNODES);
- return -EINVAL;
- }
-
node = acpi_map_pxm_to_node(its_affinity->proximity_domain);

if (node == NUMA_NO_NODE || node >= MAX_NUMNODES) {
@@ -1901,6 +1901,20 @@ static int __init gic_acpi_parse_srat_its(struct acpi_subtable_header *header,

static void __init acpi_table_parse_srat_its(void)
{
+ int count;
+
+ count = acpi_table_parse_entries(ACPI_SIG_SRAT,
+ sizeof(struct acpi_table_srat),
+ ACPI_SRAT_TYPE_GIC_ITS_AFFINITY,
+ gic_acpi_match_srat_its, 0);
+ if (count <= 0)
+ return;
+
+ its_srat_maps = kmalloc(count * sizeof(struct its_srat_map),
+ GFP_KERNEL);
+ if (!its_srat_maps)
+ return;
+
acpi_table_parse_entries(ACPI_SIG_SRAT,
sizeof(struct acpi_table_srat),
ACPI_SRAT_TYPE_GIC_ITS_AFFINITY,

--
1.7.12.4

Hanjun Guo

2017-07-21 10:06:00 UTC

Permalink

Post by Hanjun Guo
[ 0.000000] SRAT: PXM 0 -> ITS 0 -> Node 0
[ 0.000000] SRAT: PXM 0 -> ITS 1 -> Node 0
[ 0.000000] SRAT: PXM 0 -> ITS 2 -> Node 0
[ 0.000000] SRAT: PXM 1 -> ITS 3 -> Node 1
[ 0.000000] SRAT: ITS affinity exceeding max count[4]
This is wrong on D05 as we have 8 ITSes with 4 NUMA nodes.
So dynamically alloc the memory needed instead of using
its_srat_maps[MAX_NUMNODES], which count the number of
ITS entry(ies) in SRAT and alloc its_srat_maps as needed,
then build the mapping of numa node to ITS ID.
[ 0.000000] SRAT: PXM 0 -> ITS 0 -> Node 0
[ 0.000000] SRAT: PXM 0 -> ITS 1 -> Node 0
[ 0.000000] SRAT: PXM 0 -> ITS 2 -> Node 0
[ 0.000000] SRAT: PXM 1 -> ITS 3 -> Node 1
[ 0.000000] SRAT: PXM 2 -> ITS 4 -> Node 2
[ 0.000000] SRAT: PXM 2 -> ITS 5 -> Node 2
[ 0.000000] SRAT: PXM 2 -> ITS 6 -> Node 2
[ 0.000000] SRAT: PXM 3 -> ITS 7 -> Node 3
Fixes: dbd2b8267233 ("irqchip/gic-v3-its: Add ACPI NUMA node mapping")
---
drivers/irqchip/irq-gic-v3-its.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 3ccdf76..fb1c090 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1847,7 +1847,7 @@ struct its_srat_map {
u32 its_id;
};
-static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata;
+static struct its_srat_map *its_srat_maps __initdata;
static int its_in_srat __initdata;
static int __init acpi_get_its_numa_node(u32 its_id)
@@ -1861,6 +1861,12 @@ static int __init acpi_get_its_numa_node(u32 its_id)
return NUMA_NO_NODE;
}

Oops, need to check if the its_srat_maps valid or not here, please
comment on what else I'm missing or wrong, then I will prepare another
version.

Thanks
Hanjun

Hanjun Guo

2017-07-21 10:50:36 UTC

Permalink

Post by Hanjun Guo

Oops, need to check if the its_srat_maps valid or not here, please
comment on what else I'm missing or wrong, then I will prepare another

^^^
And need to free the memory.

Thanks
Hanjun

Marc Zyngier

2017-07-21 11:20:08 UTC

Permalink

Post by Hanjun Guo

I'm not seeing this on the D05 I have access to. Actually, I'm not
seeing any data related to SRAT and the ITS. Is that a different firmware?

Post by Hanjun Guo

Post by Hanjun Guo
So dynamically alloc the memory needed instead of using
its_srat_maps[MAX_NUMNODES], which count the number of
ITS entry(ies) in SRAT and alloc its_srat_maps as needed,
then build the mapping of numa node to ITS ID.
[ 0.000000] SRAT: PXM 0 -> ITS 0 -> Node 0
[ 0.000000] SRAT: PXM 0 -> ITS 1 -> Node 0
[ 0.000000] SRAT: PXM 0 -> ITS 2 -> Node 0
[ 0.000000] SRAT: PXM 1 -> ITS 3 -> Node 1
[ 0.000000] SRAT: PXM 2 -> ITS 4 -> Node 2
[ 0.000000] SRAT: PXM 2 -> ITS 5 -> Node 2
[ 0.000000] SRAT: PXM 2 -> ITS 6 -> Node 2
[ 0.000000] SRAT: PXM 3 -> ITS 7 -> Node 3
Fixes: dbd2b8267233 ("irqchip/gic-v3-its: Add ACPI NUMA node mapping")
---
drivers/irqchip/irq-gic-v3-its.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 3ccdf76..fb1c090 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1847,7 +1847,7 @@ struct its_srat_map {
u32 its_id;
};
-static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata;
+static struct its_srat_map *its_srat_maps __initdata;
static int its_in_srat __initdata;
static int __init acpi_get_its_numa_node(u32 its_id)
@@ -1861,6 +1861,12 @@ static int __init acpi_get_its_numa_node(u32 its_id)
return NUMA_NO_NODE;
}

Oops, need to check if the its_srat_maps valid or not here, please
comment on what else I'm missing or wrong, then I will prepare another
version.

Please post a patch that has all of the fixes, and I'll review that.

Thanks,

M.

--
Jazz is not dead. It just smells funny...

Ganapatrao Kulkarni

2017-07-21 11:42:50 UTC

Permalink

Hi Hanjun,

Post by Marc Zyngier

Post by Hanjun Guo

Used static array to keep it simple. we can have either dynamic array
or increase static array size (which is in init data) to a larger
number.
We may have to go for dynamic array to be more sane. You can refer my
v2 patch [1], which was doing dynamic allocation and avoids
2 calls to acpi parser as done in this patch.

[1] https://patchwork.kernel.org/patch/9798659/

Post by Marc Zyngier
I'm not seeing this on the D05 I have access to. Actually, I'm not
seeing any data related to SRAT and the ITS. Is that a different firmware?

Post by Hanjun Guo

Oops, need to check if the its_srat_maps valid or not here, please
comment on what else I'm missing or wrong, then I will prepare another
version.

Please post a patch that has all of the fixes, and I'll review that.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

thanks
Ganapat

Hanjun Guo

2017-07-22 03:52:37 UTC

Permalink

Post by Ganapatrao Kulkarni
Hi Hanjun,

Hmm, although scanning the SRAT twice is bad, it will keep the
code simple, and no need to alloc the memory every time when we
find a ITS affinity entry, you need to free that memory when
ITS probing is done.

I will post a patch later, please take a look if it's ok to you :)

Thanks
Hanjun

Hanjun Guo

2017-07-22 03:44:30 UTC

Permalink

Post by Marc Zyngier

Post by Hanjun Guo

I'm not seeing this on the D05 I have access to. Actually, I'm not
seeing any data related to SRAT and the ITS. Is that a different firmware?

Yes, actually it's just a updated version in this week, that's why
the bug wasn't be spotted when Ganapat's patch was merged.

Post by Marc Zyngier

Post by Hanjun Guo

Oops, need to check if the its_srat_maps valid or not here, please
comment on what else I'm missing or wrong, then I will prepare another
version.

Please post a patch that has all of the fixes, and I'll review that.

Will do that later.

Thanks
Hanjun