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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Thu, 5 Oct 2017 19:00:44 +0100
From:   Marc Zyngier <marc.zyngier@....com>
To:     shankerd@...eaurora.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>
Subject: Re: [PATCH v2] irqchip/gicv3: Add support for Range Selector (RS)
 feature

On 05/10/17 18:49, Shanker Donthineni wrote:
> 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@...eaurora.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;

Ah, I managed to kill a single character that made it work.
At some point, it said:

		need_rss |= MPIDR_RS(mpidr) || MPIDR_RS(cpu_logical_map(i);

And looking at it a bit more closely, you can init need_rss to
MPIDR_RS(mpidr) at the beginning of the function, and make this

		need_rss |= MPIDR_RS(cpu_logical_map(i);

Have a play with it anyway, I haven't looked too closely at it...

>  		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.

Great, thanks.

	M.
-- 
Jazz is not dead. It just smells funny...

Powered by blists - more mailing lists