Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752471AbdFLKwU (ORCPT ); Mon, 12 Jun 2017 06:52:20 -0400 Received: from foss.arm.com ([217.140.101.70]:60272 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751969AbdFLKwS (ORCPT ); Mon, 12 Jun 2017 06:52:18 -0400 Date: Mon, 12 Jun 2017 11:53:35 +0100 From: Lorenzo Pieralisi To: Ganapatrao Kulkarni Cc: linux-acpi@vger.kernel.org, devel@acpica.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, lv.zheng@intel.com, robert.moore@intel.com, catalin.marinas@arm.com, will.deacon@arm.com, tglx@linutronix.de, jason@lakedaemon.net, marc.zyngier@arm.com, jnair@caviumnetworks.com, gpkulkarni@gmail.com Subject: Re: [PATCH 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units Message-ID: <20170612105335.GA30594@red-moon> References: <1496753787-27735-1-git-send-email-ganapatrao.kulkarni@cavium.com> <1496753787-27735-3-git-send-email-ganapatrao.kulkarni@cavium.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1496753787-27735-3-git-send-email-ganapatrao.kulkarni@cavium.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8357 Lines: 259 On Tue, Jun 06, 2017 at 06:26:27PM +0530, Ganapatrao Kulkarni wrote: > Add code to parse SRAT ITS Affinity sub table as defined in ACPI 6.2 > Later in driver probe, its devices are mapped to numa node Nit: Capitalize "its". > using its id to proximity domain mapping. Ditto. > Signed-off-by: Ganapatrao Kulkarni > --- > arch/arm64/include/asm/acpi.h | 2 ++ > arch/arm64/kernel/acpi_numa.c | 59 ++++++++++++++++++++++++++++++++++++++++ > drivers/acpi/numa.c | 32 +++++++++++++++++++++- > drivers/irqchip/irq-gic-v3-its.c | 3 +- > include/linux/acpi.h | 3 ++ > 5 files changed, 97 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > index 0e99978..60ea7d1 100644 > --- a/arch/arm64/include/asm/acpi.h > +++ b/arch/arm64/include/asm/acpi.h > @@ -143,9 +143,11 @@ static inline void arch_apei_flush_tlb_one(unsigned long addr) > #ifdef CONFIG_ACPI_NUMA > int arm64_acpi_numa_init(void); > int acpi_numa_get_nid(unsigned int cpu, u64 hwid); > +int acpi_numa_get_its_nid(u32 its_id); > #else > static inline int arm64_acpi_numa_init(void) { return -ENOSYS; } > static inline int acpi_numa_get_nid(unsigned int cpu, u64 hwid) { return NUMA_NO_NODE; } > +static inline int acpi_numa_get_its_nid(u32 its_id) { return NUMA_NO_NODE; } > #endif /* CONFIG_ACPI_NUMA */ > > #define ACPI_TABLE_UPGRADE_MAX_PHYS MEMBLOCK_ALLOC_ACCESSIBLE > diff --git a/arch/arm64/kernel/acpi_numa.c b/arch/arm64/kernel/acpi_numa.c > index f01fab6..f7b2ea6 100644 > --- a/arch/arm64/kernel/acpi_numa.c > +++ b/arch/arm64/kernel/acpi_numa.c > @@ -29,15 +29,24 @@ > #include > > static int cpus_in_srat; > +static int its_in_srat; > > struct __node_cpu_hwid { > u32 node_id; /* logical node containing this CPU */ > u64 cpu_hwid; /* MPIDR for this CPU */ > }; > > +struct __node_its_id { > + u32 node_id; /* numa node id */ > + u32 its_id ; /* GIC ITS ID */ ^ Remove space. > +}; > + > static struct __node_cpu_hwid early_node_cpu_hwid[NR_CPUS] = { > [0 ... NR_CPUS - 1] = {NUMA_NO_NODE, PHYS_CPUID_INVALID} }; > > +static struct __node_its_id early_node_its_id[MAX_NUMNODES] = { Mind explaining to us where does MAX_NUMNODES as an array size limit stem from here ? > +[0 ... MAX_NUMNODES - 1] = {NUMA_NO_NODE, UINT_MAX} }; > + > int acpi_numa_get_nid(unsigned int cpu, u64 hwid) > { > int i; > @@ -50,6 +59,18 @@ int acpi_numa_get_nid(unsigned int cpu, u64 hwid) > return NUMA_NO_NODE; > } > > +int acpi_numa_get_its_nid(u32 its_id) > +{ > + int i; > + > + for (i = 0; i < its_in_srat; i++) { > + if (its_id == early_node_its_id[i].its_id) > + return early_node_its_id[i].node_id; > + } > + > + return NUMA_NO_NODE; > +} > + > /* Callback for Proximity Domain -> ACPI processor UID mapping */ > void __init acpi_numa_gicc_affinity_init(struct acpi_srat_gicc_affinity *pa) > { > @@ -100,6 +121,44 @@ void __init acpi_numa_gicc_affinity_init(struct acpi_srat_gicc_affinity *pa) > pxm, mpidr, node); > } > > +/* Callback for ITS ACPI Proximity Domain mapping */ > +void __init acpi_numa_its_affinity_init(struct acpi_srat_its_affinity *pa) > +{ > + int pxm, node; > + > + if (srat_disabled()) > + return; > + > + if (pa->header.length < sizeof(struct acpi_srat_its_affinity)) { > + pr_err("SRAT: Invalid SRAT header length: %d\n", > + pa->header.length); > + bad_srat(); > + return; > + } > + > + if (its_in_srat >= MAX_NUMNODES) { I do not understand why maxcount == MAX_NUMNODES in the first place. > + pr_warn_once("SRAT: its count exceeding max count[%d]\n", > + MAX_NUMNODES); > + return; > + } > + > + pxm = pa->proximity_domain; > + node = acpi_map_pxm_to_node(pxm); > + > + if (node == NUMA_NO_NODE || node >= MAX_NUMNODES) { > + pr_err("SRAT: Too many proximity domains %d\n", pxm); Wrong error log message or at least inconsistent (yes, GICC code where you copied this code from needs patching too). > + bad_srat(); > + return; > + } > + > + early_node_its_id[its_in_srat].node_id = node; > + early_node_its_id[its_in_srat].its_id = its_in_srat; ^ One space is enough (again - that's through for GICC code too). > + node_set(node, numa_nodes_parsed); > + pr_info("SRAT: PXM %d -> ITS %d -> Node %d\n", > + pxm, its_in_srat, node); > + its_in_srat++; > +} > + > int __init arm64_acpi_numa_init(void) > { > int ret; > diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c > index edb0c79..9c04dba 100644 > --- a/drivers/acpi/numa.c > +++ b/drivers/acpi/numa.c > @@ -182,6 +182,16 @@ int acpi_map_pxm_to_online_node(int pxm) > } > break; > > + case ACPI_SRAT_TYPE_GIC_ITS_AFFINITY: > + { > + struct acpi_srat_its_affinity *p = > + (struct acpi_srat_its_affinity *)header; > + pr_debug("SRAT ITS [0x%04x]) in proximity domain %d\n", > + p->its_id, > + p->proximity_domain); > + } > + break; > + > default: > pr_warn("Found unsupported SRAT entry (type = 0x%x)\n", > header->type); > @@ -390,6 +400,24 @@ static int __init acpi_parse_slit(struct acpi_table_header *table) > return 0; > } > > +static int __init > +acpi_parse_its_affinity(struct acpi_subtable_header *header, > + const unsigned long end) > +{ > + struct acpi_srat_its_affinity *its_affinity; > + > + its_affinity = (struct acpi_srat_its_affinity *)header; > + if (!its_affinity) > + return -EINVAL; > + > + acpi_table_print_srat_entry(header); > + > + /* let architecture-dependent part to do it */ > + acpi_numa_its_affinity_init(its_affinity); > + > + return 0; > +} > + > static int __initdata parsed_numa_memblks; > > static int __init > @@ -445,7 +473,7 @@ int __init acpi_numa_init(void) > > /* SRAT: Static Resource Affinity Table */ > if (!acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat)) { > - struct acpi_subtable_proc srat_proc[3]; > + struct acpi_subtable_proc srat_proc[4]; > > memset(srat_proc, 0, sizeof(srat_proc)); > srat_proc[0].id = ACPI_SRAT_TYPE_CPU_AFFINITY; > @@ -454,6 +482,8 @@ int __init acpi_numa_init(void) > srat_proc[1].handler = acpi_parse_x2apic_affinity; > srat_proc[2].id = ACPI_SRAT_TYPE_GICC_AFFINITY; > srat_proc[2].handler = acpi_parse_gicc_affinity; > + srat_proc[3].id = ACPI_SRAT_TYPE_GIC_ITS_AFFINITY; > + srat_proc[3].handler = acpi_parse_its_affinity; > > acpi_table_parse_entries_array(ACPI_SIG_SRAT, > sizeof(struct acpi_table_srat), > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > index 45ea1933..84936da 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c > @@ -1861,7 +1861,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_numa_get_its_nid(its_entry->translation_id)); If that's the only usage I wonder whether we really need all arm64 arch code/data, instead of parsing the SRAT in ITS code driver straight away at probe, retrieve its node and be done with this. I understand you replicated what x86/GICC does with APIC code, I would like to understand though if we see a reason why (or better, why we keep the relevant stashed data in arch/arm64 instead of the ITS driver). Thanks, Lorenzo > if (!err) > return 0; > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 137e4a3..e5e8ae3 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -260,9 +260,12 @@ int acpi_table_parse_madt(enum acpi_madt_type id, > > #ifdef CONFIG_ARM64 > void acpi_numa_gicc_affinity_init(struct acpi_srat_gicc_affinity *pa); > +void acpi_numa_its_affinity_init(struct acpi_srat_its_affinity *pa); > #else > static inline void > acpi_numa_gicc_affinity_init(struct acpi_srat_gicc_affinity *pa) { } > +static inline void > +acpi_numa_its_affinity_init(struct acpi_srat_its_affinity *pa) { } > #endif > > int acpi_numa_memory_affinity_init (struct acpi_srat_mem_affinity *ma); > -- > 1.8.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html