| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 12 Jun 2017 11:53:35 +0100
From: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To: Ganapatrao Kulkarni <ganapatrao.kulkarni@...ium.com>
Cc: linux-acpi@...r.kernel.org, devel@...ica.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
lv.zheng@...el.com, robert.moore@...el.com,
catalin.marinas@....com, will.deacon@....com, tglx@...utronix.de,
jason@...edaemon.net, marc.zyngier@....com,
jnair@...iumnetworks.com, gpkulkarni@...il.com
Subject: Re: [PATCH 2/2] acpi, gicv3-its, numa: Adding numa node mapping for
gic-its units
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 <ganapatrao.kulkarni@...ium.com>
> ---
> 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 <asm/numa.h>
>
> 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@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists