ACPI 6.2 have added SRAT subtable to define proximity domain for ITS
devices. This patchset updates acpi header file from acpica repo and
adds numa node mapping for ITS devices.
v3:
- Use static array to stash parsed data from srat its table.
v2:
- Incorporated comments from Lorenzo Pieralisi and Marc Zyngier.
v1: first patch
Ganapatrao Kulkarni (2):
ACPICA: ACPI 6.2: Add support for new SRAT subtable
acpi, gicv3-its, numa: Adding numa node mapping for gic-its units
drivers/irqchip/irq-gic-v3-its.c | 80 +++++++++++++++++++++++++++++++++++++++-
include/acpi/actbl1.h | 12 +++++-
2 files changed, 90 insertions(+), 2 deletions(-)
--
1.8.1.4
Add GIC ITS Affinity (ACPI 6.2) subtable to SRAT table.
ACPICA commit 5bc67f63918da249bfe279ee461d152bb3e6f55b
Link: https://github.com/acpica/acpica/commit/5bc67f6
Signed-off-by: Ganapatrao Kulkarni <[email protected]>
---
include/acpi/actbl1.h | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
index b4ce55c..253c9db 100644
--- a/include/acpi/actbl1.h
+++ b/include/acpi/actbl1.h
@@ -1192,7 +1192,8 @@ enum acpi_srat_type {
ACPI_SRAT_TYPE_MEMORY_AFFINITY = 1,
ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY = 2,
ACPI_SRAT_TYPE_GICC_AFFINITY = 3,
- ACPI_SRAT_TYPE_RESERVED = 4 /* 4 and greater are reserved */
+ ACPI_SRAT_TYPE_GIC_ITS_AFFINITY = 4, /* ACPI 6.2 */
+ ACPI_SRAT_TYPE_RESERVED = 5 /* 5 and greater are reserved */
};
/*
@@ -1260,6 +1261,15 @@ struct acpi_srat_gicc_affinity {
u32 clock_domain;
};
+/* 4: GIC ITS Affinity (ACPI 6.2) */
+
+struct acpi_srat_its_affinity {
+ struct acpi_subtable_header header;
+ u32 proximity_domain;
+ u16 reserved;
+ u32 its_id;
+};
+
/* Flags for struct acpi_srat_gicc_affinity */
#define ACPI_SRAT_GICC_ENABLED (1) /* 00: Use affinity structure */
--
1.8.1.4
Add code to parse SRAT ITS Affinity sub table as defined in ACPI 6.2
Later in per device probe, ITS devices are mapped to
numa node using ITS id to proximity domain mapping.
Signed-off-by: Ganapatrao Kulkarni <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 80 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 79 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 45ea1933..88cfb32 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1833,6 +1833,82 @@ static int __init its_of_probe(struct device_node *node)
#define ACPI_GICV3_ITS_MEM_SIZE (SZ_128K)
+#ifdef CONFIG_ACPI_NUMA
+struct its_srat_map {
+ u32 numa_node; /* numa node id */
+ u32 its_id; /* GIC ITS ID */
+};
+
+static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata = {
+ [0 ... MAX_NUMNODES - 1] = {NUMA_NO_NODE, UINT_MAX} };
+
+static int its_in_srat __initdata;
+
+static int __init
+acpi_get_its_numa_node(u32 its_id)
+{
+ int i;
+
+ for (i = 0; i < its_in_srat; i++) {
+ if (its_id == its_srat_maps[i].its_id)
+ return its_srat_maps[i].numa_node;
+ }
+ return NUMA_NO_NODE;
+}
+
+static int __init
+gic_acpi_parse_srat_its(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ int pxm, node;
+ struct acpi_srat_its_affinity *its_affinity;
+
+ its_affinity = (struct acpi_srat_its_affinity *)header;
+ if (!its_affinity)
+ return -EINVAL;
+
+ if (its_affinity->header.length <
+ sizeof(struct acpi_srat_its_affinity)) {
+ pr_err("SRAT:ITS: Invalid SRAT header length: %d\n",
+ its_affinity->header.length);
+ return -EINVAL;
+ }
+
+ if (its_in_srat >= MAX_NUMNODES) {
+ pr_err("SRAT:ITS: ITS devices exceeding max count[%d]\n",
+ MAX_NUMNODES);
+ return -EINVAL;
+ }
+
+ pxm = its_affinity->proximity_domain;
+ node = acpi_map_pxm_to_node(pxm);
+
+ if (node == NUMA_NO_NODE || node >= MAX_NUMNODES) {
+ pr_err("SRAT:ITS: Invalid numa node %d\n", node);
+ return -EINVAL;
+ }
+
+ its_srat_maps[its_in_srat].numa_node = node;
+ its_srat_maps[its_in_srat].its_id = its_affinity->its_id;
+ its_in_srat++;
+ pr_info("ACPI: NUMA: SRAT: ITS: PXM %d -> ITS_ID %d -> NODE %d\n",
+ pxm, its_affinity->its_id, node);
+
+ return 0;
+}
+
+static int __init acpi_table_parse_srat_its(void)
+{
+ return acpi_table_parse_entries(ACPI_SIG_SRAT,
+ sizeof(struct acpi_table_srat),
+ ACPI_SRAT_TYPE_GIC_ITS_AFFINITY,
+ gic_acpi_parse_srat_its, 0);
+}
+#else
+#define acpi_table_parse_srat_its() do { } while (0)
+#define acpi_get_its_numa_node(its_id) NUMA_NO_NODE
+#endif
+
static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
const unsigned long end)
{
@@ -1861,7 +1937,8 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
goto dom_err;
}
- err = its_probe_one(&res, dom_handle, NUMA_NO_NODE);
+ err = its_probe_one(&res, dom_handle,
+ acpi_get_its_numa_node(its_entry->translation_id));
if (!err)
return 0;
@@ -1873,6 +1950,7 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
static void __init its_acpi_probe(void)
{
+ acpi_table_parse_srat_its();
acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
gic_acpi_parse_madt_its, 0);
}
--
1.8.1.4
On Wed, Jun 21, 2017 at 11:45:43AM +0530, Ganapatrao Kulkarni wrote:
> Add code to parse SRAT ITS Affinity sub table as defined in ACPI 6.2
> Later in per device probe, ITS devices are mapped to
> numa node using ITS id to proximity domain mapping.
>
> Signed-off-by: Ganapatrao Kulkarni <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 80 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 45ea1933..88cfb32 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1833,6 +1833,82 @@ static int __init its_of_probe(struct device_node *node)
>
> #define ACPI_GICV3_ITS_MEM_SIZE (SZ_128K)
>
> +#ifdef CONFIG_ACPI_NUMA
> +struct its_srat_map {
> + u32 numa_node; /* numa node id */
> + u32 its_id; /* GIC ITS ID */
> +};
> +
> +static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata = {
> + [0 ... MAX_NUMNODES - 1] = {NUMA_NO_NODE, UINT_MAX} };
> +
> +static int its_in_srat __initdata;
> +
> +static int __init
> +acpi_get_its_numa_node(u32 its_id)
> +{
> + int i;
> +
> + for (i = 0; i < its_in_srat; i++) {
> + if (its_id == its_srat_maps[i].its_id)
> + return its_srat_maps[i].numa_node;
> + }
> + return NUMA_NO_NODE;
> +}
> +
> +static int __init
> +gic_acpi_parse_srat_its(struct acpi_subtable_header *header,
> + const unsigned long end)
> +{
> + int pxm, node;
> + struct acpi_srat_its_affinity *its_affinity;
> +
> + its_affinity = (struct acpi_srat_its_affinity *)header;
> + if (!its_affinity)
> + return -EINVAL;
> +
> + if (its_affinity->header.length <
> + sizeof(struct acpi_srat_its_affinity)) {
> + pr_err("SRAT:ITS: Invalid SRAT header length: %d\n",
> + its_affinity->header.length);
The original driver does not use pr_fmt properly, it may be worth
fixing that up rather than having prefixes, here...
> + return -EINVAL;
> + }
> +
> + if (its_in_srat >= MAX_NUMNODES) {
> + pr_err("SRAT:ITS: ITS devices exceeding max count[%d]\n",
> + MAX_NUMNODES);
> + return -EINVAL;
> + }
> +
> + pxm = its_affinity->proximity_domain;
> + node = acpi_map_pxm_to_node(pxm);
> +
> + if (node == NUMA_NO_NODE || node >= MAX_NUMNODES) {
> + pr_err("SRAT:ITS: Invalid numa node %d\n", node);
here.
> + return -EINVAL;
> + }
> +
> + its_srat_maps[its_in_srat].numa_node = node;
> + its_srat_maps[its_in_srat].its_id = its_affinity->its_id;
> + its_in_srat++;
> + pr_info("ACPI: NUMA: SRAT: ITS: PXM %d -> ITS_ID %d -> NODE %d\n",
> + pxm, its_affinity->its_id, node);
also here.
> +
> + return 0;
> +}
> +
> +static int __init acpi_table_parse_srat_its(void)
> +{
> + return acpi_table_parse_entries(ACPI_SIG_SRAT,
> + sizeof(struct acpi_table_srat),
> + ACPI_SRAT_TYPE_GIC_ITS_AFFINITY,
> + gic_acpi_parse_srat_its, 0);
> +}
> +#else
> +#define acpi_table_parse_srat_its() do { } while (0)
The function implementation returns a value, but this does not, it is
better to be consistent here.
> +#define acpi_get_its_numa_node(its_id) NUMA_NO_NODE
> +#endif
> +
> static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
> const unsigned long end)
> {
> @@ -1861,7 +1937,8 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
> goto dom_err;
> }
>
> - err = its_probe_one(&res, dom_handle, NUMA_NO_NODE);
> + err = its_probe_one(&res, dom_handle,
> + acpi_get_its_numa_node(its_entry->translation_id));
> if (!err)
> return 0;
>
> @@ -1873,6 +1950,7 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>
> static void __init its_acpi_probe(void)
> {
> + acpi_table_parse_srat_its();
> acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
> gic_acpi_parse_madt_its, 0);
> }
JC.
On 21/06/17 08:09, Jayachandran C wrote:
> On Wed, Jun 21, 2017 at 11:45:43AM +0530, Ganapatrao Kulkarni wrote:
>> Add code to parse SRAT ITS Affinity sub table as defined in ACPI 6.2
>> Later in per device probe, ITS devices are mapped to
>> numa node using ITS id to proximity domain mapping.
>>
>> Signed-off-by: Ganapatrao Kulkarni <[email protected]>
>> ---
>> drivers/irqchip/irq-gic-v3-its.c | 80 +++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 79 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 45ea1933..88cfb32 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -1833,6 +1833,82 @@ static int __init its_of_probe(struct device_node *node)
>>
>> #define ACPI_GICV3_ITS_MEM_SIZE (SZ_128K)
>>
>> +#ifdef CONFIG_ACPI_NUMA
>> +struct its_srat_map {
>> + u32 numa_node; /* numa node id */
>> + u32 its_id; /* GIC ITS ID */
>> +};
>> +
>> +static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata = {
>> + [0 ... MAX_NUMNODES - 1] = {NUMA_NO_NODE, UINT_MAX} };
>> +
>> +static int its_in_srat __initdata;
>> +
>> +static int __init
>> +acpi_get_its_numa_node(u32 its_id)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < its_in_srat; i++) {
>> + if (its_id == its_srat_maps[i].its_id)
>> + return its_srat_maps[i].numa_node;
>> + }
>> + return NUMA_NO_NODE;
>> +}
>> +
>> +static int __init
>> +gic_acpi_parse_srat_its(struct acpi_subtable_header *header,
>> + const unsigned long end)
>> +{
>> + int pxm, node;
>> + struct acpi_srat_its_affinity *its_affinity;
>> +
>> + its_affinity = (struct acpi_srat_its_affinity *)header;
>> + if (!its_affinity)
>> + return -EINVAL;
>> +
>> + if (its_affinity->header.length <
>> + sizeof(struct acpi_srat_its_affinity)) {
>> + pr_err("SRAT:ITS: Invalid SRAT header length: %d\n",
>> + its_affinity->header.length);
>
> The original driver does not use pr_fmt properly, it may be worth
> fixing that up rather than having prefixes, here...
Irrespective of what the "proper" usage is, this kind of cleanup doesn't
belong in this series. Feel free to submit a separate patch.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 21/06/17 07:15, Ganapatrao Kulkarni wrote:
> Add code to parse SRAT ITS Affinity sub table as defined in ACPI 6.2
> Later in per device probe, ITS devices are mapped to
> numa node using ITS id to proximity domain mapping.
>
> Signed-off-by: Ganapatrao Kulkarni <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 80 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 45ea1933..88cfb32 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1833,6 +1833,82 @@ static int __init its_of_probe(struct device_node *node)
>
> #define ACPI_GICV3_ITS_MEM_SIZE (SZ_128K)
>
> +#ifdef CONFIG_ACPI_NUMA
> +struct its_srat_map {
> + u32 numa_node; /* numa node id */
> + u32 its_id; /* GIC ITS ID */
> +};
> +
> +static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata = {
> + [0 ... MAX_NUMNODES - 1] = {NUMA_NO_NODE, UINT_MAX} };
> +
> +static int its_in_srat __initdata;
> +
> +static int __init
> +acpi_get_its_numa_node(u32 its_id)
Please keep these on the same line.
> +{
> + int i;
> +
> + for (i = 0; i < its_in_srat; i++) {
> + if (its_id == its_srat_maps[i].its_id)
> + return its_srat_maps[i].numa_node;
> + }
> + return NUMA_NO_NODE;
> +}
> +
> +static int __init
> +gic_acpi_parse_srat_its(struct acpi_subtable_header *header,
> + const unsigned long end)
Same remark.
> +{
> + int pxm, node;
> + struct acpi_srat_its_affinity *its_affinity;
> +
> + its_affinity = (struct acpi_srat_its_affinity *)header;
> + if (!its_affinity)
> + return -EINVAL;
> +
> + if (its_affinity->header.length <
> + sizeof(struct acpi_srat_its_affinity)) {
Same thing.
> + pr_err("SRAT:ITS: Invalid SRAT header length: %d\n",
> + its_affinity->header.length);
> + return -EINVAL;
> + }
> +
> + if (its_in_srat >= MAX_NUMNODES) {
> + pr_err("SRAT:ITS: ITS devices exceeding max count[%d]\n",
> + MAX_NUMNODES);
> + return -EINVAL;
> + }
> +
> + pxm = its_affinity->proximity_domain;
> + node = acpi_map_pxm_to_node(pxm);
> +
> + if (node == NUMA_NO_NODE || node >= MAX_NUMNODES) {
> + pr_err("SRAT:ITS: Invalid numa node %d\n", node);
> + return -EINVAL;
> + }
So if you find an entry that doesn't match the current kernel
configuration, you drop all the subsequent entries? That doesn't feel right.
> +
> + its_srat_maps[its_in_srat].numa_node = node;
> + its_srat_maps[its_in_srat].its_id = its_affinity->its_id;
> + its_in_srat++;
> + pr_info("ACPI: NUMA: SRAT: ITS: PXM %d -> ITS_ID %d -> NODE %d\n",
> + pxm, its_affinity->its_id, node);
> +
> + return 0;
> +}
> +
> +static int __init acpi_table_parse_srat_its(void)
> +{
> + return acpi_table_parse_entries(ACPI_SIG_SRAT,
> + sizeof(struct acpi_table_srat),
> + ACPI_SRAT_TYPE_GIC_ITS_AFFINITY,
> + gic_acpi_parse_srat_its, 0);
If you don't check the return value, there is no point returning it.
> +}
> +#else
> +#define acpi_table_parse_srat_its() do { } while (0)
> +#define acpi_get_its_numa_node(its_id) NUMA_NO_NODE
> +#endif
> +
> static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
> const unsigned long end)
> {
> @@ -1861,7 +1937,8 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
> goto dom_err;
> }
>
> - err = its_probe_one(&res, dom_handle, NUMA_NO_NODE);
> + err = its_probe_one(&res, dom_handle,
> + acpi_get_its_numa_node(its_entry->translation_id));
> if (!err)
> return 0;
>
> @@ -1873,6 +1950,7 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>
> static void __init its_acpi_probe(void)
> {
> + acpi_table_parse_srat_its();
> acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
> gic_acpi_parse_madt_its, 0);
> }
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On Wed, Jun 21, 2017 at 11:45:43AM +0530, Ganapatrao Kulkarni wrote:
> Add code to parse SRAT ITS Affinity sub table as defined in ACPI 6.2
> Later in per device probe, ITS devices are mapped to
> numa node using ITS id to proximity domain mapping.
>
> Signed-off-by: Ganapatrao Kulkarni <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 80 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 45ea1933..88cfb32 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1833,6 +1833,82 @@ static int __init its_of_probe(struct device_node *node)
>
> #define ACPI_GICV3_ITS_MEM_SIZE (SZ_128K)
>
> +#ifdef CONFIG_ACPI_NUMA
> +struct its_srat_map {
> + u32 numa_node; /* numa node id */
> + u32 its_id; /* GIC ITS ID */
> +};
> +
> +static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata = {
> + [0 ... MAX_NUMNODES - 1] = {NUMA_NO_NODE, UINT_MAX} };
Nit: UINT_MAX is as valid as 0 as its_id (but the intent here is to make
it look like an invalid value), given that the its_in_srat counter
defines what entries are valid I do not necessarily see the point in
initializing with something that is a valid value != 0.
> +static int its_in_srat __initdata;
> +
> +static int __init
> +acpi_get_its_numa_node(u32 its_id)
> +{
> + int i;
> +
> + for (i = 0; i < its_in_srat; i++) {
> + if (its_id == its_srat_maps[i].its_id)
> + return its_srat_maps[i].numa_node;
> + }
> + return NUMA_NO_NODE;
> +}
> +
> +static int __init
> +gic_acpi_parse_srat_its(struct acpi_subtable_header *header,
> + const unsigned long end)
> +{
> + int pxm, node;
> + struct acpi_srat_its_affinity *its_affinity;
> +
> + its_affinity = (struct acpi_srat_its_affinity *)header;
> + if (!its_affinity)
> + return -EINVAL;
> +
> + if (its_affinity->header.length <
> + sizeof(struct acpi_srat_its_affinity)) {
> + pr_err("SRAT:ITS: Invalid SRAT header length: %d\n",
> + its_affinity->header.length);
> + return -EINVAL;
> + }
> +
> + if (its_in_srat >= MAX_NUMNODES) {
> + pr_err("SRAT:ITS: ITS devices exceeding max count[%d]\n",
> + MAX_NUMNODES);
> + return -EINVAL;
> + }
> +
> + pxm = its_affinity->proximity_domain;
> + node = acpi_map_pxm_to_node(pxm);
> +
> + if (node == NUMA_NO_NODE || node >= MAX_NUMNODES) {
> + pr_err("SRAT:ITS: Invalid numa node %d\n", node);
> + return -EINVAL;
As Marc mentioned you should bail out here but continue parsing entries,
aka "return 0";
Thanks,
Lorenzo
> + }
> +
> + its_srat_maps[its_in_srat].numa_node = node;
> + its_srat_maps[its_in_srat].its_id = its_affinity->its_id;
> + its_in_srat++;
> + pr_info("ACPI: NUMA: SRAT: ITS: PXM %d -> ITS_ID %d -> NODE %d\n",
> + pxm, its_affinity->its_id, node);
> +
> + return 0;
> +}
> +
> +static int __init acpi_table_parse_srat_its(void)
> +{
> + return acpi_table_parse_entries(ACPI_SIG_SRAT,
> + sizeof(struct acpi_table_srat),
> + ACPI_SRAT_TYPE_GIC_ITS_AFFINITY,
> + gic_acpi_parse_srat_its, 0);
> +}
> +#else
> +#define acpi_table_parse_srat_its() do { } while (0)
> +#define acpi_get_its_numa_node(its_id) NUMA_NO_NODE
> +#endif
> +
> static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
> const unsigned long end)
> {
> @@ -1861,7 +1937,8 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
> goto dom_err;
> }
>
> - err = its_probe_one(&res, dom_handle, NUMA_NO_NODE);
> + err = its_probe_one(&res, dom_handle,
> + acpi_get_its_numa_node(its_entry->translation_id));
> if (!err)
> return 0;
>
> @@ -1873,6 +1950,7 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>
> static void __init its_acpi_probe(void)
> {
> + acpi_table_parse_srat_its();
> acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
> gic_acpi_parse_madt_its, 0);
> }
> --
> 1.8.1.4
>
On Wed, Jun 21, 2017 at 2:28 PM, Marc Zyngier <[email protected]> wrote:
> On 21/06/17 07:15, Ganapatrao Kulkarni wrote:
>> Add code to parse SRAT ITS Affinity sub table as defined in ACPI 6.2
>> Later in per device probe, ITS devices are mapped to
>> numa node using ITS id to proximity domain mapping.
>>
>> Signed-off-by: Ganapatrao Kulkarni <[email protected]>
>> ---
>> drivers/irqchip/irq-gic-v3-its.c | 80 +++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 79 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 45ea1933..88cfb32 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -1833,6 +1833,82 @@ static int __init its_of_probe(struct device_node *node)
>>
>> #define ACPI_GICV3_ITS_MEM_SIZE (SZ_128K)
>>
>> +#ifdef CONFIG_ACPI_NUMA
>> +struct its_srat_map {
>> + u32 numa_node; /* numa node id */
>> + u32 its_id; /* GIC ITS ID */
>> +};
>> +
>> +static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata = {
>> + [0 ... MAX_NUMNODES - 1] = {NUMA_NO_NODE, UINT_MAX} };
>> +
>> +static int its_in_srat __initdata;
>> +
>> +static int __init
>> +acpi_get_its_numa_node(u32 its_id)
>
> Please keep these on the same line.
sure.
>
>> +{
>> + int i;
>> +
>> + for (i = 0; i < its_in_srat; i++) {
>> + if (its_id == its_srat_maps[i].its_id)
>> + return its_srat_maps[i].numa_node;
>> + }
>> + return NUMA_NO_NODE;
>> +}
>> +
>> +static int __init
>> +gic_acpi_parse_srat_its(struct acpi_subtable_header *header,
>> + const unsigned long end)
>
> Same remark.
ok
>
>> +{
>> + int pxm, node;
>> + struct acpi_srat_its_affinity *its_affinity;
>> +
>> + its_affinity = (struct acpi_srat_its_affinity *)header;
>> + if (!its_affinity)
>> + return -EINVAL;
>> +
>> + if (its_affinity->header.length <
>> + sizeof(struct acpi_srat_its_affinity)) {
>
> Same thing.
ok
>
>> + pr_err("SRAT:ITS: Invalid SRAT header length: %d\n",
>> + its_affinity->header.length);
>> + return -EINVAL;
>> + }
>> +
>> + if (its_in_srat >= MAX_NUMNODES) {
>> + pr_err("SRAT:ITS: ITS devices exceeding max count[%d]\n",
>> + MAX_NUMNODES);
>> + return -EINVAL;
>> + }
>> +
>> + pxm = its_affinity->proximity_domain;
>> + node = acpi_map_pxm_to_node(pxm);
>> +
>> + if (node == NUMA_NO_NODE || node >= MAX_NUMNODES) {
>> + pr_err("SRAT:ITS: Invalid numa node %d\n", node);
>> + return -EINVAL;
>> + }
>
> So if you find an entry that doesn't match the current kernel
> configuration, you drop all the subsequent entries? That doesn't feel right.
thanks, it is reasonable to print warning to notify that
mapping is broken for some tables and continue.
anyway device which does not have mapping gets mapped default to NUMA_NO_NODE.
>
>> +
>> + its_srat_maps[its_in_srat].numa_node = node;
>> + its_srat_maps[its_in_srat].its_id = its_affinity->its_id;
>> + its_in_srat++;
>> + pr_info("ACPI: NUMA: SRAT: ITS: PXM %d -> ITS_ID %d -> NODE %d\n",
>> + pxm, its_affinity->its_id, node);
>> +
>> + return 0;
>> +}
>> +
>> +static int __init acpi_table_parse_srat_its(void)
>> +{
>> + return acpi_table_parse_entries(ACPI_SIG_SRAT,
>> + sizeof(struct acpi_table_srat),
>> + ACPI_SRAT_TYPE_GIC_ITS_AFFINITY,
>> + gic_acpi_parse_srat_its, 0);
>
> If you don't check the return value, there is no point returning it.
thanks, return value is don't care, i will change accordingly.
>
>> +}
>> +#else
>> +#define acpi_table_parse_srat_its() do { } while (0)
>> +#define acpi_get_its_numa_node(its_id) NUMA_NO_NODE
>> +#endif
>> +
>> static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>> const unsigned long end)
>> {
>> @@ -1861,7 +1937,8 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>> goto dom_err;
>> }
>>
>> - err = its_probe_one(&res, dom_handle, NUMA_NO_NODE);
>> + err = its_probe_one(&res, dom_handle,
>> + acpi_get_its_numa_node(its_entry->translation_id));
>> if (!err)
>> return 0;
>>
>> @@ -1873,6 +1950,7 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>>
>> static void __init its_acpi_probe(void)
>> {
>> + acpi_table_parse_srat_its();
>> acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
>> gic_acpi_parse_madt_its, 0);
>> }
>>
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
thanks
Ganapat
On Wed, Jun 21, 2017 at 2:58 PM, Lorenzo Pieralisi
<[email protected]> wrote:
> On Wed, Jun 21, 2017 at 11:45:43AM +0530, Ganapatrao Kulkarni wrote:
>> Add code to parse SRAT ITS Affinity sub table as defined in ACPI 6.2
>> Later in per device probe, ITS devices are mapped to
>> numa node using ITS id to proximity domain mapping.
>>
>> Signed-off-by: Ganapatrao Kulkarni <[email protected]>
>> ---
>> drivers/irqchip/irq-gic-v3-its.c | 80 +++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 79 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 45ea1933..88cfb32 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -1833,6 +1833,82 @@ static int __init its_of_probe(struct device_node *node)
>>
>> #define ACPI_GICV3_ITS_MEM_SIZE (SZ_128K)
>>
>> +#ifdef CONFIG_ACPI_NUMA
>> +struct its_srat_map {
>> + u32 numa_node; /* numa node id */
>> + u32 its_id; /* GIC ITS ID */
>> +};
>> +
>> +static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata = {
>> + [0 ... MAX_NUMNODES - 1] = {NUMA_NO_NODE, UINT_MAX} };
>
> Nit: UINT_MAX is as valid as 0 as its_id (but the intent here is to make
> it look like an invalid value), given that the its_in_srat counter
> defines what entries are valid I do not necessarily see the point in
> initializing with something that is a valid value != 0.
ok, this array may not need any init,
>
>> +static int its_in_srat __initdata;
>> +
>> +static int __init
>> +acpi_get_its_numa_node(u32 its_id)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < its_in_srat; i++) {
>> + if (its_id == its_srat_maps[i].its_id)
>> + return its_srat_maps[i].numa_node;
>> + }
>> + return NUMA_NO_NODE;
>> +}
>> +
>> +static int __init
>> +gic_acpi_parse_srat_its(struct acpi_subtable_header *header,
>> + const unsigned long end)
>> +{
>> + int pxm, node;
>> + struct acpi_srat_its_affinity *its_affinity;
>> +
>> + its_affinity = (struct acpi_srat_its_affinity *)header;
>> + if (!its_affinity)
>> + return -EINVAL;
>> +
>> + if (its_affinity->header.length <
>> + sizeof(struct acpi_srat_its_affinity)) {
>> + pr_err("SRAT:ITS: Invalid SRAT header length: %d\n",
>> + its_affinity->header.length);
>> + return -EINVAL;
>> + }
>> +
>> + if (its_in_srat >= MAX_NUMNODES) {
>> + pr_err("SRAT:ITS: ITS devices exceeding max count[%d]\n",
>> + MAX_NUMNODES);
>> + return -EINVAL;
>> + }
>> +
>> + pxm = its_affinity->proximity_domain;
>> + node = acpi_map_pxm_to_node(pxm);
>> +
>> + if (node == NUMA_NO_NODE || node >= MAX_NUMNODES) {
>> + pr_err("SRAT:ITS: Invalid numa node %d\n", node);
>> + return -EINVAL;
>
> As Marc mentioned you should bail out here but continue parsing entries,
> aka "return 0";
agreed.
>
> Thanks,
> Lorenzo
>
>> + }
>> +
>> + its_srat_maps[its_in_srat].numa_node = node;
>> + its_srat_maps[its_in_srat].its_id = its_affinity->its_id;
>> + its_in_srat++;
>> + pr_info("ACPI: NUMA: SRAT: ITS: PXM %d -> ITS_ID %d -> NODE %d\n",
>> + pxm, its_affinity->its_id, node);
>> +
>> + return 0;
>> +}
>> +
>> +static int __init acpi_table_parse_srat_its(void)
>> +{
>> + return acpi_table_parse_entries(ACPI_SIG_SRAT,
>> + sizeof(struct acpi_table_srat),
>> + ACPI_SRAT_TYPE_GIC_ITS_AFFINITY,
>> + gic_acpi_parse_srat_its, 0);
>> +}
>> +#else
>> +#define acpi_table_parse_srat_its() do { } while (0)
>> +#define acpi_get_its_numa_node(its_id) NUMA_NO_NODE
>> +#endif
>> +
>> static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>> const unsigned long end)
>> {
>> @@ -1861,7 +1937,8 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>> goto dom_err;
>> }
>>
>> - err = its_probe_one(&res, dom_handle, NUMA_NO_NODE);
>> + err = its_probe_one(&res, dom_handle,
>> + acpi_get_its_numa_node(its_entry->translation_id));
>> if (!err)
>> return 0;
>>
>> @@ -1873,6 +1950,7 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>>
>> static void __init its_acpi_probe(void)
>> {
>> + acpi_table_parse_srat_its();
>> acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
>> gic_acpi_parse_madt_its, 0);
>> }
>> --
>> 1.8.1.4
>>
thanks
Ganapat
On Wed, Jun 21, 2017 at 09:44:58AM +0100, Marc Zyngier wrote:
> On 21/06/17 08:09, Jayachandran C wrote:
> > On Wed, Jun 21, 2017 at 11:45:43AM +0530, Ganapatrao Kulkarni wrote:
> >> Add code to parse SRAT ITS Affinity sub table as defined in ACPI 6.2
> >> Later in per device probe, ITS devices are mapped to
> >> numa node using ITS id to proximity domain mapping.
> >>
> >> Signed-off-by: Ganapatrao Kulkarni <[email protected]>
> >> ---
> >> drivers/irqchip/irq-gic-v3-its.c | 80 +++++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 79 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> >> index 45ea1933..88cfb32 100644
> >> --- a/drivers/irqchip/irq-gic-v3-its.c
> >> +++ b/drivers/irqchip/irq-gic-v3-its.c
> >> @@ -1833,6 +1833,82 @@ static int __init its_of_probe(struct device_node *node)
> >>
> >> #define ACPI_GICV3_ITS_MEM_SIZE (SZ_128K)
> >>
> >> +#ifdef CONFIG_ACPI_NUMA
> >> +struct its_srat_map {
> >> + u32 numa_node; /* numa node id */
> >> + u32 its_id; /* GIC ITS ID */
> >> +};
> >> +
> >> +static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata = {
> >> + [0 ... MAX_NUMNODES - 1] = {NUMA_NO_NODE, UINT_MAX} };
> >> +
> >> +static int its_in_srat __initdata;
> >> +
> >> +static int __init
> >> +acpi_get_its_numa_node(u32 its_id)
> >> +{
> >> + int i;
> >> +
> >> + for (i = 0; i < its_in_srat; i++) {
> >> + if (its_id == its_srat_maps[i].its_id)
> >> + return its_srat_maps[i].numa_node;
> >> + }
> >> + return NUMA_NO_NODE;
> >> +}
> >> +
> >> +static int __init
> >> +gic_acpi_parse_srat_its(struct acpi_subtable_header *header,
> >> + const unsigned long end)
> >> +{
> >> + int pxm, node;
> >> + struct acpi_srat_its_affinity *its_affinity;
> >> +
> >> + its_affinity = (struct acpi_srat_its_affinity *)header;
> >> + if (!its_affinity)
> >> + return -EINVAL;
> >> +
> >> + if (its_affinity->header.length <
> >> + sizeof(struct acpi_srat_its_affinity)) {
> >> + pr_err("SRAT:ITS: Invalid SRAT header length: %d\n",
> >> + its_affinity->header.length);
> >
> > The original driver does not use pr_fmt properly, it may be worth
> > fixing that up rather than having prefixes, here...
>
> Irrespective of what the "proper" usage is
The prefixes of the prints in this driver are a bit inconsistent. And
there are a few error messages like the "Failed to allocate PROPBASE"
and "Bad LPI chunk %d\n" without any prefix at all. pr_fmt would be
helpful to clean it up.
> this kind of cleanup doesn't belong in this series.
Right. I think the printed messages in this patch can be improved quite
a bit. currently the tags and spacing are inconsistent, something like:
| pr_err("SRAT: Invalid header length %d in ITS affinity\n", ...
| pr_err("SRAT: Invalid NUMA node %d in ITS affinity\n", node);
| pr_info("SRAT: PXM %d -> ITS %d -> Node %d\n",
would be better in my opinion.
> Feel free to submit a separate patch.
The driver also prints 2 lines per CPU on boot which can get to a few
hundred lines on our test machines. So there are a few things worth
fixing up here... maybe in another kernel dev cycle.
JC.