Re: [PATCH v2 08/25] arm64: Always keep DAIF.[IF] in sync

[Hector Martin]

On 17/02/2021 21.22, Mark Rutland wrote:
> > Root irqchip drivers can discriminate between IRQs and FIQs by checking
> > the ISR_EL1 system register.
>
> I think we can remove this note for now. If we go with seperate handlers
> this won't be necessary, and if not this would be better placed on a
> commit adding the FIQ handling capability.

Indeed, this doesn't make sense any more. Changed for v3.

> Judging by `git grep -Wi daif -- arch/arm64`, with this patch applied,
> we'll also need fixups in:
>
> * gic_arch_enable_irqs() in arch/arm64/include/asm/arch_gicv3.h
> * save_and_disable_irq() in arch/arm64/include/asm/assembler.h (noted below)
> * local_daif_save_flags() in arch/arm64/include/asm/daifflags.h
>    (the fake DAIF should have F set too)
> * __cpu_do_idle_irqprio() in arch/arm64/kernel/process.c

Good catches. A few of those are irrelevant for M1 but need to be done 
now that we're making this change globally, others I just missed from 
the beginning.

There's also an incorrect comment in entry.S:

	/*
	 * DA_F were cleared at start of handling. If anything is set in
	 * DAIF, we come back from an NMI, so skip preemption
	 */
	mrs	x0, daif
	orr	x24, x24, x0

Now only DA__ are cleared. This actually pairs with 
gic_arch_enable_irqs() and begs the question: in priority masking 
systems, do we unmask both IRQ and FIQ (the gic_arch_enable_irqs 
change), or do we leave FIQ masked (which instead would need an AND in 
that part of entry.S so as to not consider FIQ masked as meaning we're 
coming back from an NMI)?

And a minor related one: should init_gic_priority_masking() WARN if FIQ 
is masked too? This probably goes with the above.

Either way, this was nontrivial to make sense of, so I'll make that 
entry.S comment clearer while I'm touching it.

> I think save_and_diable_irq below needs to be updated too, since it
> only sets DAIF.I and leaves DAIF.F as-is.

Totally missed this one! Fixed for v3.

> > - * FIQ is never expected, but we mask it when we disable debug exceptions, and
> > - * unmask it at all other times.
> > + * FIQ is never expected on most platforms, but we keep it synchronized
> > + * with the IRQ mask status. On platforms that do not expect FIQ, that vector
> > + * triggers a kernel panic. On platforms that do, the FIQ vector is unified
> > + * with the IRQ vector.
> >    */
>
> Can we please delete this bit, though? Now that we say IRQ and FIQ are
> masked/unmasked together, I don't think the rest is necessary to
> understand the masking logic, and it's one less thing to keep in sync
> with changes to the entry code.

Gone :)

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