| 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
| ||
|
Message-ID: <4597dc21-6f91-a51d-3513-d982738c04ea@somainline.org>
Date: Thu, 29 Sep 2022 19:10:30 +0200
From: Konrad Dybcio <konrad.dybcio@...ainline.org>
To: Sven Peter <sven@...npeter.dev>, asahi@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org
Cc: towinchenmi@...il.com, Hector Martin <marcan@...can.st>,
Alyssa Rosenzweig <alyssa@...enzweig.io>,
Thomas Gleixner <tglx@...utronix.de>,
Marc Zyngier <maz@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs
On 29/09/2022 16:50, Sven Peter wrote:
> On Thu, Sep 29, 2022, at 16:40, Konrad Dybcio wrote:
>> Add support for A7-A11 SoCs by if-ing out some features only present
>> on:
>>
>> * A11 & newer (implementation-defined IPI & UNCORE registers)
>> * A11[1] & newer (fast IPI support).
>>
>> Also, annotate IPI regs support in the aic struct so that the driver
>> can tell whether the SoC supports these, as they are written to,
>> even if fast IPI is disabled. This in turn causes a crash on older
>> platforms, as the implemention-defined registers either do
>> something else or are not supposed to be touched - definitely not a
>> NOP though.
>>
>> [1] A11 is supposed to use this feature, but it currently doesn't work
>> for reasons unknown and hence remains disabled. It can easily be enabled
>> on A11 only, as there is a SoC-specific compatible in the DT with a
>> fallback to apple,aic, so that the interrupt controller gets to probe
>> regardless of whether IPI Sn_... registers are used or not.
>> That said, it is not yet necessary, especially with only one core up,
>> and it has worked a-ok so far.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@...ainline.org>
>> ---
>> Changes since v1:
>> - remove EL2 register check (dts change covered this)
>> - use static_branch instead of ifs
>> - rename "uncore2 registers" to "uncore registers" in added code and
>> update the commit message accordingly
>> - create a "legacy" config struct for pre-A11 targets
>> - rewrite the commit message a bit to match actual status
>>
>> drivers/irqchip/irq-apple-aic.c | 56 ++++++++++++++++++++++++---------
>> 1 file changed, 41 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
>> index 1c2813ad8bbe..cdef99bfcfb3 100644
>> --- a/drivers/irqchip/irq-apple-aic.c
>> +++ b/drivers/irqchip/irq-apple-aic.c
>> @@ -229,6 +229,7 @@
>> #define AIC_TMR_EL02_VIRT AIC_TMR_GUEST_VIRT
>>
>> static DEFINE_STATIC_KEY_TRUE(use_fast_ipi);
>> +static DEFINE_STATIC_KEY_TRUE(has_uncore_regs);
>>
>> struct aic_info {
>> int version;
>> @@ -246,6 +247,7 @@ struct aic_info {
>>
>> /* Features */
>> bool fast_ipi;
>> + bool uncore_regs;
>> };
>>
>> static const struct aic_info aic1_info = {
>> @@ -253,6 +255,8 @@ static const struct aic_info aic1_info = {
>>
>> .event = AIC_EVENT,
>> .target_cpu = AIC_TARGET_CPU,
>> +
>> + .uncore_regs = true,
>> };
>>
>> static const struct aic_info aic1_fipi_info = {
>> @@ -264,6 +268,13 @@ static const struct aic_info aic1_fipi_info = {
>> .fast_ipi = true,
>> };
>>
>> +static const struct aic_info aic1_legacy_info = {
>> + .version = 1,
>> +
>> + .event = AIC_EVENT,
>> + .target_cpu = AIC_TARGET_CPU,
>> +};
>> +
>> static const struct aic_info aic2_info = {
>> .version = 2,
>>
>> @@ -273,6 +284,10 @@ static const struct aic_info aic2_info = {
>> };
>>
>> static const struct of_device_id aic_info_match[] = {
>> + {
>> + .compatible = "apple,s5l8960x-aic",
>> + .data = &aic1_legacy_info,
>> + },
> Maybe I'm confused but shouldn't this be the apple,aic fallback and uncore_regs
> should be enabled for e.g. t8103-aic then?
Yes, looks like..
>
>> {
>> .compatible = "apple,t8103-aic",
>> .data = &aic1_fipi_info,
>> @@ -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 (static_branch_likely(&use_fast_ipi)) {
>> + 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);
>> + }
> This doesn't make much sense:
>
> if (A) {
> if (B) {
> if (A) { // A is already guaranteed to be true here, why check it again?
> // ...
> } else {
> // how can this ever be reached then?
> }
> }
> }
Good point, I went too far with squashing "has IPI regs" and "use fast
IPI" together and
didn't notice it's now unreachable..
Konrad
>
>> }
>> }
>>
>> @@ -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 (static_branch_likely(&has_uncore_regs)) {
>> + 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));
>> + }
>> }
>> }
>>
>> @@ -944,7 +963,8 @@ static int aic_init_cpu(unsigned int cpu)
>> /* Mask all hard-wired per-CPU IRQ/FIQ sources */
>>
>> /* Pending Fast IPI FIQs */
>> - write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
>> + if (static_branch_likely(&use_fast_ipi))
>> + write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
>>
>> /* Timer FIQs */
>> sysreg_clear_set(cntp_ctl_el0, 0, ARCH_TIMER_CTRL_IT_MASK);
>> @@ -965,8 +985,9 @@ static int aic_init_cpu(unsigned int cpu)
>> FIELD_PREP(PMCR0_IMODE, PMCR0_IMODE_OFF));
>>
>> /* Uncore PMC FIQ */
>> - sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
>> - FIELD_PREP(UPMCR0_IMODE, UPMCR0_IMODE_OFF));
>> + if (static_branch_likely(&has_uncore_regs))
>> + sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
>> + FIELD_PREP(UPMCR0_IMODE, UPMCR0_IMODE_OFF));
>>
>> /* Commit all of the above */
>> isb();
>> @@ -1125,6 +1146,11 @@ static int __init aic_of_ic_init(struct
>> device_node *node, struct device_node *p
>> else
>> static_branch_disable(&use_fast_ipi);
>>
>> + if (irqc->info.uncore_regs)
>> + static_branch_enable(&has_uncore_regs);
>> + else
>> + static_branch_disable(&has_uncore_regs);
>> +
>> irqc->info.die_stride = off - start_off;
>>
>> irqc->hw_domain = irq_domain_create_tree(of_node_to_fwnode(node),
>> --
>> 2.30.2
>
> Sven
Powered by blists - more mailing lists