All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hector Martin <marcan@marcan.st>
To: Marc Zyngier <maz@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org,
	Rob Herring <robh@kernel.org>, Arnd Bergmann <arnd@kernel.org>,
	Olof Johansson <olof@lixom.net>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Mark Kettenis <mark.kettenis@xs4all.nl>,
	Tony Lindgren <tony@atomide.com>,
	Mohamed Mediouni <mohamed.mediouni@caramail.com>,
	Stan Skowronek <stan@corellium.com>,
	Alexander Graf <graf@amazon.com>, Will Deacon <will@kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Christoph Hellwig <hch@infradead.org>,
	"David S. Miller" <davem@davemloft.net>,
	devicetree@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 15/18] irqchip/apple-aic: Add support for the Apple Interrupt Controller
Date: Wed, 7 Apr 2021 04:21:46 +0900	[thread overview]
Message-ID: <5a4a0ab4-5a4e-1f1c-f6c6-97439b95e7ee@marcan.st> (raw)
In-Reply-To: <87ft03p9cd.wl-maz@kernel.org>

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

WARNING: multiple messages have this Message-ID
From: Hector Martin <marcan@marcan.st>
To: Marc Zyngier <maz@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org,
	Rob Herring <robh@kernel.org>, Arnd Bergmann <arnd@kernel.org>,
	Olof Johansson <olof@lixom.net>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Mark Kettenis <mark.kettenis@xs4all.nl>,
	Tony Lindgren <tony@atomide.com>,
	Mohamed Mediouni <mohamed.mediouni@caramail.com>,
	Stan Skowronek <stan@corellium.com>,
	Alexander Graf <graf@amazon.com>, Will Deacon <will@kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Christoph Hellwig <hch@infradead.org>,
	"David S. Miller" <davem@davemloft.net>,
	devicetree@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 15/18] irqchip/apple-aic: Add support for the Apple Interrupt Controller
Date: Wed, 7 Apr 2021 04:21:46 +0900	[thread overview]
Message-ID: <5a4a0ab4-5a4e-1f1c-f6c6-97439b95e7ee@marcan.st> (raw)
In-Reply-To: <87ft03p9cd.wl-maz@kernel.org>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-04-06 19:22 UTC|newest]

Thread overview: 63+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-02  9:05 [PATCH v4 00/18] Apple M1 SoC platform bring-up Hector Martin
2021-04-02  9:05 ` Hector Martin
2021-04-02  9:05 ` [PATCH v4 01/18] dt-bindings: vendor-prefixes: Add apple prefix Hector Martin
2021-04-02  9:05   ` Hector Martin
2021-04-02  9:05 ` [PATCH v4 02/18] dt-bindings: arm: apple: Add bindings for Apple ARM platforms Hector Martin
2021-04-02  9:05   ` Hector Martin
2021-04-02  9:05 ` [PATCH v4 03/18] dt-bindings: arm: cpus: Add apple,firestorm & icestorm compatibles Hector Martin
2021-04-02  9:05   ` [PATCH v4 03/18] dt-bindings: arm: cpus: Add apple, firestorm " Hector Martin
2021-04-02  9:05 ` [PATCH v4 04/18] arm64: cputype: Add CPU implementor & types for the Apple M1 cores Hector Martin
2021-04-02  9:05   ` Hector Martin
2021-04-02  9:05 ` [PATCH v4 05/18] dt-bindings: timer: arm,arch_timer: Add interrupt-names support Hector Martin
2021-04-02  9:05   ` [PATCH v4 05/18] dt-bindings: timer: arm, arch_timer: " Hector Martin
2021-04-06 16:44   ` [PATCH v4 05/18] dt-bindings: timer: arm,arch_timer: " Rob Herring
2021-04-06 16:44     ` Rob Herring
2021-04-02  9:05 ` [PATCH v4 06/18] arm64: arch_timer: Implement support for interrupt-names Hector Martin
2021-04-02  9:05   ` Hector Martin
2021-04-02  9:05 ` [PATCH v4 07/18] asm-generic/io.h: Add a non-posted variant of ioremap() Hector Martin
2021-04-02  9:05   ` Hector Martin
2021-04-02  9:05 ` [PATCH v4 08/18] docs: driver-api: device-io: Document I/O access functions Hector Martin
2021-04-02  9:05   ` Hector Martin
2021-04-02  9:05 ` [PATCH v4 09/18] docs: driver-api: device-io: Document ioremap() variants & access funcs Hector Martin
2021-04-02  9:05   ` Hector Martin
2021-04-02  9:05 ` [PATCH v4 10/18] arm64: Implement ioremap_np() to map MMIO as nGnRnE Hector Martin
2021-04-02  9:05   ` Hector Martin
2021-04-02  9:05 ` [PATCH v4 11/18] asm-generic/io.h: implement pci_remap_cfgspace using ioremap_np Hector Martin
2021-04-02  9:05   ` Hector Martin
2021-04-07 13:27   ` Andy Shevchenko
2021-04-07 13:27     ` Andy Shevchenko
2021-04-07 21:03     ` Will Deacon
2021-04-07 21:03       ` Will Deacon
2021-04-08 11:01       ` Hector Martin
2021-04-08 11:01         ` Hector Martin
2021-04-08 11:24         ` Andy Shevchenko
2021-04-08 11:24           ` Andy Shevchenko
2021-04-02  9:05 ` [PATCH v4 12/18] of/address: Add infrastructure to declare MMIO as non-posted Hector Martin
2021-04-02  9:05   ` Hector Martin
2021-04-06 16:47   ` Rob Herring
2021-04-06 16:47     ` Rob Herring
2021-04-06 16:59     ` Hector Martin
2021-04-06 16:59       ` Hector Martin
2021-04-02  9:05 ` [PATCH v4 13/18] arm64: Move ICH_ sysreg bits from arm-gic-v3.h to sysreg.h Hector Martin
2021-04-02  9:05   ` Hector Martin
2021-04-02  9:05 ` [PATCH v4 14/18] dt-bindings: interrupt-controller: Add DT bindings for apple-aic Hector Martin
2021-04-02  9:05   ` Hector Martin
2021-04-02  9:05 ` [PATCH v4 15/18] irqchip/apple-aic: Add support for the Apple Interrupt Controller Hector Martin
2021-04-02  9:05   ` Hector Martin
2021-04-06 18:16   ` Marc Zyngier
2021-04-06 18:16     ` Marc Zyngier
2021-04-06 19:21     ` Hector Martin [this message]
2021-04-06 19:21       ` Hector Martin
2021-04-07 21:09   ` Will Deacon
2021-04-07 21:09     ` Will Deacon
2021-04-08 11:02     ` Hector Martin
2021-04-08 11:02       ` Hector Martin
2021-04-02  9:05 ` [PATCH v4 16/18] arm64: Kconfig: Introduce CONFIG_ARCH_APPLE Hector Martin
2021-04-02  9:05   ` Hector Martin
2021-04-02  9:05 ` [PATCH v4 17/18] dt-bindings: display: Add apple,simple-framebuffer Hector Martin
2021-04-02  9:05   ` Hector Martin
2021-04-02  9:05 ` [PATCH v4 18/18] arm64: apple: Add initial Apple Mac mini (M1, 2020) devicetree Hector Martin
2021-04-02  9:05   ` Hector Martin
2021-04-02 22:48   ` Konrad Dybcio
2021-04-06 16:56   ` Rob Herring
2021-04-06 16:56     ` Rob Herring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5a4a0ab4-5a4e-1f1c-f6c6-97439b95e7ee@marcan.st \
    --to=marcan@marcan.st \
    --cc=andy.shevchenko@gmail.com \
    --cc=arnd@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=corbet@lwn.net \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=graf@amazon.com \
    --cc=hch@infradead.org \
    --cc=krzk@kernel.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.kettenis@xs4all.nl \
    --cc=mark.rutland@arm.com \
    --cc=maz@kernel.org \
    --cc=mohamed.mediouni@caramail.com \
    --cc=olof@lixom.net \
    --cc=robh@kernel.org \
    --cc=stan@corellium.com \
    --cc=tony@atomide.com \
    --cc=will@kernel.org \
    --subject='Re: [PATCH v4 15/18] irqchip/apple-aic: Add support for the Apple Interrupt Controller' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.