| 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: <acd89b38-89e1-f6d6-2f98-01d94d9984bb@somainline.org>
Date: Tue, 12 Jul 2022 21:23:31 +0200
From: Konrad Dybcio <konrad.dybcio@...ainline.org>
To: Marc Zyngier <maz@...nel.org>
Cc: ~postmarketos/upstreaming@...ts.sr.ht, martin.botka@...ainline.org,
angelogioacchino.delregno@...ainline.org,
marijn.suijten@...ainline.org, jamipkettunen@...ainline.org,
Hector Martin <marcan@...can.st>,
Sven Peter <sven@...npeter.dev>,
Alyssa Rosenzweig <alyssa@...enzweig.io>,
Thomas Gleixner <tglx@...utronix.de>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs
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@...ainline.org> 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@...ainline.org>
>> ---
>> 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.
>
>> default:
>> break;
>> }
>> @@ -944,7 +964,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 (aic_irqc->info.ipi_regs)
>> + 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 +986,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 (aic_irqc->info.uncore2_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();
>
> I must be missing something though. Where is the code that actually
> enables support for the SoCs mentioned in $SUBJECT?
In this peculiar case, enabling support means stripping away the so-called
'features', otherwise the interrupt controller won't budge.
Konrad
>
> Thanks,
>
> M.
>
Powered by blists - more mailing lists