Re: [PATCH 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs

[Sven Peter]

On Tue, Jul 12, 2022, at 21:23, Konrad Dybcio wrote:
> On 12.07.2022 21:12, Marc Zyngier wrote:
>> Hi Konrad,
>> 
>> Please add a cover letter when sending more than a single patch.
>> 
>> On Tue, 12 Jul 2022 17:09:19 +0100,
>> Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxx> wrote:
>>>
>>> Add support for A7-A11 SoCs by if-ing out some features only present on
>>> A12 & newer (UNCORE2 registers) or M1 & newer (EL2 registers - the
>>> older SoCs don't implement EL2).
>>>
>>> Also, annotate IPI regs support (A11 and newer*) so that the driver can
>>> tell whether the SoC supports these (they are written to even if fast
>>> IPI is disabled, when the registers are there of course).
>>>
>>> *A11 is supposed to use this feature, but it is currently not working.
>>> That said, it is not yet necessary, especially with only one core up,
>>> and it works a-ok using the same featureset as earlier SoCs.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxx>
>>> ---
>>>  drivers/irqchip/irq-apple-aic.c | 54 +++++++++++++++++++++++----------
>>>  1 file changed, 38 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
>>> index 12dd48727a15..36f4b52addc2 100644
>>> --- a/drivers/irqchip/irq-apple-aic.c
>>> +++ b/drivers/irqchip/irq-apple-aic.c
>>> @@ -245,7 +245,10 @@ struct aic_info {
>>>  	u32 die_stride;
>>>  
>>>  	/* Features */
>>> +	bool el2_regs;
>>>  	bool fast_ipi;
>>> +	bool ipi_regs;
>>> +	bool uncore2_regs;
>>>  };
>>>  
>>>  static const struct aic_info aic1_info = {
>>> @@ -261,7 +264,10 @@ static const struct aic_info aic1_fipi_info = {
>>>  	.event		= AIC_EVENT,
>>>  	.target_cpu	= AIC_TARGET_CPU,
>>>  
>>> +	.el2_regs	= true,
>>>  	.fast_ipi	= true,
>>> +	.ipi_regs	= true,
>>> +	.uncore2_regs	= true,
>>>  };
>>>  
>>>  static const struct aic_info aic2_info = {
>>> @@ -269,7 +275,10 @@ static const struct aic_info aic2_info = {
>>>  
>>>  	.irq_cfg	= AIC2_IRQ_CFG,
>>>  
>>> +	.el2_regs	= true,
>>>  	.fast_ipi	= true,
>>> +	.ipi_regs	= true,
>>> +	.uncore2_regs	= true,
>> 
>> So to sum it up, all recent cores have all the cool features, and the
>> older ones have none of them. Surely we can do better than adding 3
>> fields that have the same value. Turn 'fast_ipi' into something that
>> means 'full_fat', and key everything on that.
>> 
>> And if this is meant to evolve into a more differentiated set of
>> features, the usual idiom is to have a set of flags as part of an
>> unsigned long instead of a set of booleans.
> The latter would be true if a bootrom exploit or any equivalent means
> of booting Linux would be found for A12 (M1 is family with A14 for context).
>
> We can think of 4 feature levels, I think:
>
> A7-A10: 'nothing fancy'
> A11: fast_ipi (broken currently, need to investigate)
> A12: A11 + UNCORE2 regs
> M1+: A12 + EL2
>
> We *could* squash the A12-A14 case into M1, but then if a means of booting
> Linux appears, this would have to be untangled again..
>
>> 
>>>  };
>>>  
>>>  static const struct of_device_id aic_info_match[] = {
>>> @@ -452,6 +461,9 @@ static unsigned long aic_fiq_get_idx(struct irq_data *d)
>>>  
>>>  static void aic_fiq_set_mask(struct irq_data *d)
>>>  {
>>> +	if (!aic_irqc->info.el2_regs)
>>> +		return;
>> 
>> Why? AIC_TMR_EL02_PHYS is defined as the interrupt that fires in the
>> context of a guest. There is no guest here (no EL2 either), so what
>> you should have as interrupt number is AIC_TMR_EL0_{PHYS,VIRT}, and
>> this change becomes irrelevant (nothing to mask). Which is also what
>> happens when running an M1 under the m1n1 hypervisor.
> This func accesses impl-defined regs that are not present on earlier SoCs.
>
>> 
>>> +
>>>  	/* Only the guest timers have real mask bits, unfortunately. */
>>>  	switch (aic_fiq_get_idx(d)) {
>>>  	case AIC_TMR_EL02_PHYS:
>>> @@ -469,6 +481,9 @@ static void aic_fiq_set_mask(struct irq_data *d)
>>>  
>>>  static void aic_fiq_clear_mask(struct irq_data *d)
>>>  {
>>> +	if (!aic_irqc->info.el2_regs)
>>> +		return;
>>> +
>>>  	switch (aic_fiq_get_idx(d)) {
>>>  	case AIC_TMR_EL02_PHYS:
>>>  		sysreg_clear_set_s(SYS_IMP_APL_VM_TMR_FIQ_ENA_EL2, 0, VM_TMR_FIQ_ENABLE_P);
>>> @@ -524,12 +539,14 @@ static void __exception_irq_entry aic_handle_fiq(struct pt_regs *regs)
>>>  	 * we check for everything here, even things we don't support yet.
>>>  	 */
>>>  
>>> -	if (read_sysreg_s(SYS_IMP_APL_IPI_SR_EL1) & IPI_SR_PENDING) {
>>> -		if (static_branch_likely(&use_fast_ipi)) {
>>> -			aic_handle_ipi(regs);
>>> -		} else {
>>> -			pr_err_ratelimited("Fast IPI fired. Acking.\n");
>>> -			write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
>>> +	if (aic_irqc->info.ipi_regs) {
>> 
>> This is probably the hottest path in the whole kernel. Do we want an
>> extra read here? Absolutely not. At the very least, this should be a
>> static key.
> Yeah, makes sense..
>
>
>> 
>>> +		if (read_sysreg_s(SYS_IMP_APL_IPI_SR_EL1) & IPI_SR_PENDING) {
>>> +			if (static_branch_likely(&use_fast_ipi)) {
>>> +				aic_handle_ipi(regs);
>>> +			} else {
>>> +				pr_err_ratelimited("Fast IPI fired. Acking.\n");
>>> +				write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
>>> +			}
>>>  		}
>>>  	}
>>>  
>>> @@ -566,12 +583,14 @@ static void __exception_irq_entry aic_handle_fiq(struct pt_regs *regs)
>>>  					  AIC_FIQ_HWIRQ(irq));
>>>  	}
>>>  
>>> -	if (FIELD_GET(UPMCR0_IMODE, read_sysreg_s(SYS_IMP_APL_UPMCR0_EL1)) == UPMCR0_IMODE_FIQ &&
>>> -			(read_sysreg_s(SYS_IMP_APL_UPMSR_EL1) & UPMSR_IACT)) {
>>> -		/* Same story with uncore PMCs */
>>> -		pr_err_ratelimited("Uncore PMC FIQ fired. Masking.\n");
>>> -		sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
>>> -				   FIELD_PREP(UPMCR0_IMODE, UPMCR0_IMODE_OFF));
>>> +	if (aic_irqc->info.uncore2_regs) {
>> 
>> Same thing.
>> 
>>> +		if (FIELD_GET(UPMCR0_IMODE, read_sysreg_s(SYS_IMP_APL_UPMCR0_EL1)) == UPMCR0_IMODE_FIQ &&
>>> +				(read_sysreg_s(SYS_IMP_APL_UPMSR_EL1) & UPMSR_IACT)) {
>>> +			/* Same story with uncore PMCs */
>>> +			pr_err_ratelimited("Uncore PMC FIQ fired. Masking.\n");
>>> +			sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
>>> +					FIELD_PREP(UPMCR0_IMODE, UPMCR0_IMODE_OFF));
>>> +		}
>>>  	}
>>>  }
>>>  
>>> @@ -676,7 +695,8 @@ static int aic_irq_domain_translate(struct irq_domain *id,
>>>  				break;
>>>  			case AIC_TMR_HV_PHYS:
>>>  			case AIC_TMR_HV_VIRT:
>>> -				return -ENOENT;
>>> +				if (aic_irqc->info.el2_regs)
>>> +					return -ENOENT;
>> 
>> See my comment above about the use of these interrupt numbers.
> `if (!is_kernel_in_hyp_mode()) {` always evaluates to true, since there's
> no EL2. Hence, accessing AIC_TMR_HV_{VIRT,PHYS} makes this return ENOENT,
> which means timer can't probe and that's no bueno.

Sounds like an issue with your device tree. There should be no reference to
AIC_TMR_HV_{VIRT,PHYS} in there.


Sven