[PATCH v3 0/2] acpi, gicv3-its, numa: Adding numa node mapping for ITS

[PATCH v3 0/2] acpi, gicv3-its, numa: Adding numa node mapping for ITS

[Ganapatrao Kulkarni]

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

[PATCH v3 0/2] acpi, gicv3-its, numa: Adding numa node mapping for ITS

[Ganapatrao Kulkarni]

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

[Devel] [PATCH v3 0/2] acpi, gicv3-its, numa: Adding numa node mapping for ITS

[Ganapatrao Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 697 bytes --]

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

[PATCH v3 1/2] ACPICA: ACPI 6.2: Add support for new SRAT subtable

[Ganapatrao Kulkarni]

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 <ganapatrao.kulkarni@cavium.com>
---
 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

[PATCH v3 1/2] ACPICA: ACPI 6.2: Add support for new SRAT subtable

[Ganapatrao Kulkarni]

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 <ganapatrao.kulkarni@cavium.com>
---
 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

[Devel] [PATCH v3 1/2] ACPICA: ACPI 6.2: Add support for new SRAT subtable

[Ganapatrao Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

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 <ganapatrao.kulkarni(a)cavium.com>
---
 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

[PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Ganapatrao Kulkarni]

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 <ganapatrao.kulkarni@cavium.com>
---
 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

[PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Ganapatrao Kulkarni]

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 <ganapatrao.kulkarni@cavium.com>
---
 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

[Devel] [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Ganapatrao Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 3454 bytes --]

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 <ganapatrao.kulkarni(a)cavium.com>
---
 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

Re: [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Jayachandran C]

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 <ganapatrao.kulkarni@cavium.com>
> ---
>  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.

[PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Jayachandran C]

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 <ganapatrao.kulkarni@cavium.com>
> ---
>  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.

Re: [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Marc Zyngier]

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 <ganapatrao.kulkarni@cavium.com>
>> ---
>>  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...

[PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Marc Zyngier]

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 <ganapatrao.kulkarni@cavium.com>
>> ---
>>  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...

Re: [Devel] [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Marc Zyngier]

[-- Attachment #1: Type: text/plain, Size: 2322 bytes --]

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 <ganapatrao.kulkarni(a)cavium.com>
>> ---
>>  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...

Re: [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Marc Zyngier]

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 <ganapatrao.kulkarni@cavium.com>
> ---
>  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...

[PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Marc Zyngier]

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 <ganapatrao.kulkarni@cavium.com>
> ---
>  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...

Re: [Devel] [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Marc Zyngier]

[-- Attachment #1: Type: text/plain, Size: 4078 bytes --]

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 <ganapatrao.kulkarni(a)cavium.com>
> ---
>  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...

Re: [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Lorenzo Pieralisi]

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 <ganapatrao.kulkarni@cavium.com>
> ---
>  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
> 

[PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Lorenzo Pieralisi]

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 <ganapatrao.kulkarni@cavium.com>
> ---
>  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
> 

Re: [Devel] [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Lorenzo Pieralisi]

[-- Attachment #1: Type: text/plain, Size: 4141 bytes --]

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 <ganapatrao.kulkarni(a)cavium.com>
> ---
>  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
> 

Re: [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Ganapatrao Kulkarni]

On Wed, Jun 21, 2017 at 2:28 PM, Marc Zyngier <marc.zyngier@arm.com> 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 <ganapatrao.kulkarni@cavium.com>
>> ---
>>  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

Re: [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Ganapatrao Kulkarni]

On Wed, Jun 21, 2017 at 2:28 PM, Marc Zyngier <marc.zyngier@arm.com> 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 <ganapatrao.kulkarni@cavium.com>
>> ---
>>  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

[PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Ganapatrao Kulkarni]

On Wed, Jun 21, 2017 at 2:28 PM, Marc Zyngier <marc.zyngier@arm.com> 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 <ganapatrao.kulkarni@cavium.com>
>> ---
>>  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

Re: [Devel] [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Ganapatrao Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 5118 bytes --]

On Wed, Jun 21, 2017 at 2:28 PM, Marc Zyngier <marc.zyngier(a)arm.com> 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 <ganapatrao.kulkarni(a)cavium.com>
>> ---
>>  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

Re: [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Ganapatrao Kulkarni]

On Wed, Jun 21, 2017 at 2:58 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> 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 <ganapatrao.kulkarni@cavium.com>
>> ---
>>  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

Re: [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Ganapatrao Kulkarni]

On Wed, Jun 21, 2017 at 2:58 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> 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 <ganapatrao.kulkarni@cavium.com>
>> ---
>>  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

[PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Ganapatrao Kulkarni]

On Wed, Jun 21, 2017 at 2:58 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> 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 <ganapatrao.kulkarni@cavium.com>
>> ---
>>  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

Re: [Devel] [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Ganapatrao Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 4950 bytes --]

On Wed, Jun 21, 2017 at 2:58 PM, Lorenzo Pieralisi
<lorenzo.pieralisi(a)arm.com> 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 <ganapatrao.kulkarni(a)cavium.com>
>> ---
>>  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

Re: [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Jayachandran C]

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 <ganapatrao.kulkarni@cavium.com>
> >> ---
> >>  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.

Re: [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Jayachandran C]

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 <ganapatrao.kulkarni@cavium.com>
> >> ---
> >>  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.

[PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units

[Jayachandran C]

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 <ganapatrao.kulkarni@cavium.com>
> >> ---
> >>  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.