Re: [PATCH v4 15/18] irqchip/apple-aic: Add support for the Apple Interrupt Controller

[Hector Martin <marcan@marcan.st>]

On 07/04/2021 03.16, Marc Zyngier wrote:
> Hi Hector,
> 
> On Fri, 02 Apr 2021 10:05:39 +0100,
> Hector Martin <marcan@marcan.st> wrote:
>> +		/*
>> +		 * In EL1 the non-redirected registers are the guest's,
>> +		 * not EL2's, so remap the hwirqs to match.
>> +		 */
>> +		if (!is_kernel_in_hyp_mode()) {
>> +			switch (fwspec->param[1]) {
>> +			case AIC_TMR_GUEST_PHYS:
>> +				*hwirq = ic->nr_hw + AIC_TMR_HV_PHYS;
>> +				break;
>> +			case AIC_TMR_GUEST_VIRT:
>> +				*hwirq = ic->nr_hw + AIC_TMR_HV_VIRT;
>> +				break;
>> +			case AIC_TMR_HV_PHYS:
>> +			case AIC_TMR_HV_VIRT:
>> +				return -ENOENT;
>> +			default:
>> +				break;
>> +			}
>> +		}
> 
> Urgh, this is nasty. You are internally remapping the hwirq from one
> timer to another in order to avoid accessing the enable register
> which happens to be an EL2 only register?

The remapping is to make the IRQs route properly at all.

There are EL2 and EL0 timers, and on GIC each timer goes to its own IRQ. 
But here there are no real IRQs, everything's a FIQ. However, thanks to 
VHE, the EL2 timer shows up as the EL0 timer, and the EL0 timer is 
accessed via EL02 registers, when in EL2. So in EL2/VHE mode, "HV" means 
EL0 and "guest" means EL02, while in EL1, there is no HV and "guest" 
means EL0. And since we figure out which IRQ fired by reading timer 
registers, this is what matters. So I map the guest IRQs to the HV 
hwirqs in EL1 mode, which makes this all work out. Then the timer code 
goes and ends up undoing all this logic again, so we map to separate 
fake "IRQs" only to end up right back at using the same timer registers 
anuway :-)

Really, the ugliness here is that the constant meaning is overloaded. In 
fwspec context they mean what they say on the tin, while in hwirq 
context "HV" means EL0 and "guest" means EL02 (other FIQs would be 
passed through unchanged). Perhaps some additional defines might help 
clarify this? Say, at the top of this file (not in the binding),

/*
  * Pass-through mapping from real timers to the correct registers to
  * access them in EL2/VHE mode. When running in EL1, this gets
  * overridden to access the guest timer using EL0 registers.
  */
#define AIC_TMR_EL0_PHYS AIC_TMR_HV_PHYS
#define AIC_TMR_EL0_VIRT AIC_TMR_HV_VIRT
#define AIC_TMR_EL02_PHYS AIC_TMR_GUEST_PHYS
#define AIC_TMR_EL02_VIRT AIC_TMR_GUEST_VIRT

Then the irqchip/FIQ dispatch side can use the EL* constants, the 
default pass-through mapping is appropriate for VHE/EL2 mode, and 
translation can adjust it for EL1 mode.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub