On Thu, Dec 06, 2018 at 09:50:18AM +0000, Julien Thierry wrote:

On 05/12/18 18:26, Catalin Marinas wrote:

quoted

On Wed, Dec 05, 2018 at 04:55:54PM +0000, Julien Thierry wrote:

quoted

On 04/12/18 17:36, Catalin Marinas wrote:

quoted

On Mon, Nov 12, 2018 at 11:57:01AM +0000, Julien Thierry wrote:

quoted

diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
index 24692ed..e0a32e4 100644
--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h

@@ -18,7 +18,27 @@
 
 #ifdef __KERNEL__
 
+#include <asm/alternative.h>
+#include <asm/cpufeature.h>
 #include <asm/ptrace.h>
+#include <asm/sysreg.h>
+
+
+/*
+ * When ICC_PMR_EL1 is used for interrupt masking, only the bit indicating
+ * whether the normal interrupts are masked is kept along with the daif
+ * flags.
+ */
+#define ARCH_FLAG_PMR_EN 0x1
+
+#define MAKE_ARCH_FLAGS(daif, pmr)					\
+	((daif) | (((pmr) >> GIC_PRIO_STATUS_SHIFT) & ARCH_FLAG_PMR_EN))
+
+#define ARCH_FLAGS_GET_PMR(flags)				 \
+	((((flags) & ARCH_FLAG_PMR_EN) << GIC_PRIO_STATUS_SHIFT) \
+		| GIC_PRIO_IRQOFF)
+
+#define ARCH_FLAGS_GET_DAIF(flags) ((flags) & ~ARCH_FLAG_PMR_EN)

I wonder whether we could just use the PSR_I_BIT here to decide whether
to set the GIC_PRIO_IRQ{ON,OFF}. We could clear the PSR_I_BIT in
_restore_daif() with an alternative.

So, the issue with it is that some contexts might be using PSR.I to
disable interrupts (any contexts with async errors or debug exceptions
disabled, kvm guest entry paths, pseudo-NMIs, ...).

If any of these contexts calls local_irq_save()/local_irq_restore() or
local_daif_save()/local_daif_restore(), by only relying on PSR_I_BIT to
represent the PMR status, we might end up clearing PSR.I when we shouldn't.

I'm not sure whether there are no callers of these functions in those
context. But if that is the case, we could simplify things, yes.

There are callers of local_daif_save() (3) and local_daif_mask() (7) but
do they all need to disable the pseudo-NMIs?

Hmmm, I really think that both of those should be disabling NMIs.
Otherwise, if we take an NMI, the first thing the el1_irq handler is
going to do is "enable_da_f()" which could lead to potential issues.

One thing that could be done is:
- local_daif_save() and local_daif_mask() both mask all daif bits
(taking care to represent PMR value in the I bit of the saved flags)
- local_daif_restore() restores da_f as expected and decides values to
put for PMR and PSR.I as follows:
	* do the da_f restore
	* if PSR.A bit is cleared in the saved flags, then we also do a start_nmi()

However, this would not work with a local_daif_save()/restore() on the
return path of an NMI because I think it is the only context with NMIs
"stopped" that can take aborts. I can add a WARN_ON(in_nmi()) for
local_daif_restore() if that doesn't affect performance too much.

FTR, as we discussed this in the office, the conclusion (IIUC) we got to
was: leave the *_daif_*() functions unchanged, touching all the
corresponding PSTATE bits, but change the arch_local_irq_*() macros to
only touch the PMR when the feature is enabled.

-- 
Catalin

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