Messages in this thread Patch in this message | | | Subject | Re: [PATCH v2] irqchip/gicv3: Add support for Range Selector (RS) feature | From | Shanker Donthineni <> | Date | Thu, 5 Oct 2017 12:49:34 -0500 |
| |
Hi Marc,
On 10/05/2017 05:43 AM, Marc Zyngier wrote: > On 20/09/17 04:21, Shanker Donthineni wrote: >> A new feature Range Selector (RS) has been added to GIC specification >> in order to support more than 16 CPUs at affinity level 0. New fields >> are introduced in SGI system registers (ICC_SGI0R_EL1, ICC_SGI1R_EL1 >> and ICC_ASGI1R_EL1) to relax an artificial limit of 16 at level 0. >> >> - A new RSS field in ICC_CTLR_EL3, ICC_CTLR_EL1 and ICV_CTLR_EL1: >> [18] - Range Selector Support (RSS) >> 0b0 = Targeted SGIs with affinity level 0 values of 0-15 are supported. >> 0b1 = Targeted SGIs with affinity level 0 values of 0-255 are supported. >> >> - A new RS field in ICC_SGI0R_EL1, ICC_SGI1R_EL1 and ICC_ASGI1R_EL1: >> [47:44] - RangeSelector (RS) which group of 16 TargetList[n] field >> TargetList[n] represents aff0 value ((RS*16)+n) >> When ICC_CTLR_EL3.RSS==0 or ICC_CTLR_EL1.RSS==0, RS is RES0. >> >> - A new RSS field in GICD_TYPER: >> [26] - Range Selector Support (RSS) >> 0b0 = Targeted SGIs with affinity level 0 values of 0-15 are supported. >> 0b1 = Targeted SGIs with affinity level 0 values of 0-255 are supported. >> >> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org> >> --- >> Changes since v1: >> - Addrssed Marc's review comments on RSS validation checks. >> - Apply code changes to gic_compute_target_list() to support RSS feature. >> - Fix the compilation on ARM32. >> >> arch/arm/include/asm/arch_gicv3.h | 5 ++++ >> arch/arm64/include/asm/arch_gicv3.h | 5 ++++ >> drivers/irqchip/irq-gic-v3.c | 60 +++++++++++++++++++++++++++++++------ >> include/linux/irqchip/arm-gic-v3.h | 4 +++ >> 4 files changed, 65 insertions(+), 9 deletions(-) >> >> diff --git a/arch/arm/include/asm/arch_gicv3.h b/arch/arm/include/asm/arch_gicv3.h >> index 2747590..4a3b3c2 100644 >> --- a/arch/arm/include/asm/arch_gicv3.h >> +++ b/arch/arm/include/asm/arch_gicv3.h >> @@ -196,6 +196,11 @@ static inline void gic_write_ctlr(u32 val) >> isb(); >> } >> >> +static inline u32 gic_read_ctlr(void) >> +{ >> + return read_sysreg(ICC_CTLR); >> +} >> + >> static inline void gic_write_grpen1(u32 val) >> { >> write_sysreg(val, ICC_IGRPEN1); >> diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h >> index 8cef47f..aa2c795 100644 >> --- a/arch/arm64/include/asm/arch_gicv3.h >> +++ b/arch/arm64/include/asm/arch_gicv3.h >> @@ -87,6 +87,11 @@ static inline void gic_write_ctlr(u32 val) >> isb(); >> } >> >> +static inline u32 gic_read_ctlr(void) >> +{ >> + return read_sysreg_s(SYS_ICC_CTLR_EL1); >> +} >> + >> static inline void gic_write_grpen1(u32 val) >> { >> write_sysreg_s(val, SYS_ICC_IGRPEN1_EL1); >> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c >> index 984c3ec..2d56f2b 100644 >> --- a/drivers/irqchip/irq-gic-v3.c >> +++ b/drivers/irqchip/irq-gic-v3.c >> @@ -55,6 +55,7 @@ struct gic_chip_data { >> struct irq_domain *domain; >> u64 redist_stride; >> u32 nr_redist_regions; >> + bool has_rss; >> unsigned int irq_nr; >> struct partition_desc *ppi_descs[16]; >> }; >> @@ -63,7 +64,9 @@ struct gic_chip_data { >> static struct static_key supports_deactivate = STATIC_KEY_INIT_TRUE; >> >> static struct gic_kvm_info gic_v3_kvm_info; >> +static DEFINE_PER_CPU(bool, has_rss); >> >> +#define MPIDR_RS(mpidr) (((mpidr) & 0xF0UL) >> 4) >> #define gic_data_rdist() (this_cpu_ptr(gic_data.rdists.rdist)) >> #define gic_data_rdist_rd_base() (gic_data_rdist()->rd_base) >> #define gic_data_rdist_sgi_base() (gic_data_rdist_rd_base() + SZ_64K) >> @@ -483,6 +486,10 @@ static int gic_populate_rdist(void) >> >> static void gic_cpu_sys_reg_init(void) >> { >> + int i, cpu = smp_processor_id(); >> + u64 mpidr = cpu_logical_map(cpu); >> + bool need_rss = false; >> + >> /* >> * Need to check that the SRE bit has actually been set. If >> * not, it means that SRE is disabled at EL2. We're going to >> @@ -514,6 +521,40 @@ static void gic_cpu_sys_reg_init(void) >> >> /* ... and let's hit the road... */ >> gic_write_grpen1(1); >> + >> + /* Keep the RSS capability status in per_cpu variable */ >> + per_cpu(has_rss, cpu) = !!(gic_read_ctlr() & ICC_CTLR_EL1_RSS); >> + >> + /* Check all the CPUs have capable of sending SGIs to other CPUs */ >> + for_each_online_cpu(i) { >> + if (cpu == i) >> + continue; >> + >> + if (MPIDR_RS(mpidr) && (!per_cpu(has_rss, i))) { >> + pr_crit("CPU%d has no RSS, SGIs aren't reachable to CPU%d", >> + i, cpu); >> + continue; >> + } >> + >> + if (MPIDR_RS(cpu_logical_map(i)) && (!per_cpu(has_rss, cpu))) { >> + pr_crit("CPU%d has no RSS, SGIs aren't reachable to CPU%d", >> + cpu, i); >> + continue; >> + } >> + >> + if (MPIDR_RS(mpidr) || MPIDR_RS(cpu_logical_map(i))) >> + need_rss = true; >> + } > > The logic feels a bit clumsy, and fails to warn if the first CPU needs > RSS while the distributor doesn't have it. Yes, that's the ultimate > corner case, but hey, we might as well do the right thing... ;-) >
Agree with you it won't warn on systems booted with a single core. In case of multicore boot it warns while booting the secondary cores and covers all the combination of CPUs. Anyway I like simplified idea with a fewer lines of code that does the same purpose.
> How about something like: > > for_each_online_cpu(i) { > bool have_rss = per_cpu(has_rss, i) || per_cpu(has_rss, cpu); > need_rss = MPIDR_RS(mpidr) || MPIDR_RS(cpu_logical_map(i); > > if (need_rss != have_rss) > pr_crit("CPU%d (%lx) can't SGI CPU%d (%lx), no RSS\n", > cpu, mpidr, i, cpu_logical_map(i)); > } >
This code should work with minor changes on top of the above code snippet.
for_each_online_cpu(i) { bool have_rss = per_cpu(has_rss, i) && per_cpu(has_rss, cpu); need_rss = MPIDR_RS(mpidr) || MPIDR_RS(cpu_logical_map(i)) || need_rss; if (need_rss != have_rss) pr_crit("CPU%d (%lx) can't SGI CPU%d (%lx), no RSS\n", cpu, mpidr, i, cpu_logical_map(i)); }
> which is informative enough. > >> + >> + /** >> + * GIC spec says, when ICC_CTLR_EL1.RSS==1 and GICD_TYPER.RSS==0, >> + * writing ICC_ASGI1R_EL1 register with RS != 0 is a CONSTRAINED >> + * UNPREDICTABLE choice of : >> + * - The write is ignored. >> + * - The RS field is treated as 0. >> + */ >> + if (need_rss && (!gic_data.has_rss)) >> + pr_crit("RSS support is required but GICD doesn't support it\n"); > > This can be turn to a pr_crit_once, as there is no need to scream for > each CPU (the thing is dead anyway). >
I'll fix in v3.
>> } >> >> static int gic_dist_supports_lpis(void) >> @@ -548,6 +589,9 @@ static void gic_cpu_init(void) >> >> #ifdef CONFIG_SMP >> >> +#define MPIDR_TO_SGI_RS(mpidr) (MPIDR_RS(mpidr) << ICC_SGI1R_RS_SHIFT) >> +#define MPIDR_TO_SGI_CLUSTER_ID(mpidr) (mpidr & ~0xFUL) >> + >> static int gic_starting_cpu(unsigned int cpu) >> { >> gic_cpu_init(); >> @@ -562,13 +606,6 @@ static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask, >> u16 tlist = 0; >> >> while (cpu < nr_cpu_ids) { >> - /* >> - * If we ever get a cluster of more than 16 CPUs, just >> - * scream and skip that CPU. >> - */ >> - if (WARN_ON((mpidr & 0xff) >= 16)) >> - goto out; >> - >> tlist |= 1 << (mpidr & 0xf); >> >> next_cpu = cpumask_next(cpu, mask); >> @@ -578,7 +615,7 @@ static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask, >> >> mpidr = cpu_logical_map(cpu); >> >> - if (cluster_id != (mpidr & ~0xffUL)) { >> + if (cluster_id != MPIDR_TO_SGI_CLUSTER_ID(mpidr)) { >> cpu--; >> goto out; >> } >> @@ -600,6 +637,7 @@ static void gic_send_sgi(u64 cluster_id, u16 tlist, unsigned int irq) >> MPIDR_TO_SGI_AFFINITY(cluster_id, 2) | >> irq << ICC_SGI1R_SGI_ID_SHIFT | >> MPIDR_TO_SGI_AFFINITY(cluster_id, 1) | >> + MPIDR_TO_SGI_RS(cluster_id) | >> tlist << ICC_SGI1R_TARGET_LIST_SHIFT); >> >> pr_debug("CPU%d: ICC_SGI1R_EL1 %llx\n", smp_processor_id(), val); >> @@ -620,7 +658,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) >> smp_wmb(); >> >> for_each_cpu(cpu, mask) { >> - unsigned long cluster_id = cpu_logical_map(cpu) & ~0xffUL; >> + u64 cluster_id = MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu)); >> u16 tlist; >> >> tlist = gic_compute_target_list(&cpu, mask, cluster_id); >> @@ -959,6 +997,10 @@ static int __init gic_init_bases(void __iomem *dist_base, >> goto out_free; >> } >> >> + gic_data.has_rss = !!(typer & GICD_TYPER_RSS); >> + pr_info("Distributor has %s Range Selector support\n", >> + gic_data.has_rss ? "" : "no"); >> + >> set_handle_irq(gic_handle_irq); >> >> if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis()) >> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h >> index 6a1f87f..eb9ff9b 100644 >> --- a/include/linux/irqchip/arm-gic-v3.h >> +++ b/include/linux/irqchip/arm-gic-v3.h >> @@ -68,6 +68,7 @@ >> #define GICD_CTLR_ENABLE_SS_G1 (1U << 1) >> #define GICD_CTLR_ENABLE_SS_G0 (1U << 0) >> >> +#define GICD_TYPER_RSS (1U << 26) >> #define GICD_TYPER_LPIS (1U << 17) >> #define GICD_TYPER_MBIS (1U << 16) >> >> @@ -377,6 +378,7 @@ >> #define ICC_CTLR_EL1_SEIS_MASK (0x1 << ICC_CTLR_EL1_SEIS_SHIFT) >> #define ICC_CTLR_EL1_A3V_SHIFT 15 >> #define ICC_CTLR_EL1_A3V_MASK (0x1 << ICC_CTLR_EL1_A3V_SHIFT) >> +#define ICC_CTLR_EL1_RSS (0x1 << 18) >> #define ICC_PMR_EL1_SHIFT 0 >> #define ICC_PMR_EL1_MASK (0xff << ICC_PMR_EL1_SHIFT) >> #define ICC_BPR0_EL1_SHIFT 0 >> @@ -465,6 +467,8 @@ >> #define ICC_SGI1R_AFFINITY_2_SHIFT 32 >> #define ICC_SGI1R_AFFINITY_2_MASK (0xffULL << ICC_SGI1R_AFFINITY_2_SHIFT) >> #define ICC_SGI1R_IRQ_ROUTING_MODE_BIT 40 >> +#define ICC_SGI1R_RS_SHIFT 44 >> +#define ICC_SGI1R_RS_MASK (0xfULL << ICC_SGI1R_RS_SHIFT) >> #define ICC_SGI1R_AFFINITY_3_SHIFT 48 >> #define ICC_SGI1R_AFFINITY_3_MASK (0xffULL << ICC_SGI1R_AFFINITY_3_SHIFT) >> >> > > Thanks, > > M. >
-- Shanker Donthineni Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
| |