On Fri, Nov 30, 2018 at 10:55:47AM +0000, Julien Thierry wrote:

On 29/11/18 17:44, Mark Rutland wrote:

quoted

On Mon, Nov 12, 2018 at 11:56:59AM +0000, Julien Thierry wrote:

quoted

quoted

+	mov	x2, #GIC_PRIO_IRQON
+	msr_s	SYS_ICC_PMR_EL1, x2		// unmask PMR
 	dsb	sy				// WFI may enter a low-power mode

Is the DSB SY sufficient and necessary to synchronise the update of
SYS_ICC_PMR_EL1? We don't need an ISB too?

DSB SY is necessary when we unmask interrupts to make sure that the
redistributor sees the update to PMR before we do WFI. My understanding
is that the resdistributor is free to stop forwarding interrupts to the
CPU interface if from its point of view those interrupts don't have a
high enough priority.

As for the ISB, I don't think we need one because writes to PMR are
self-synchronizing, so the write to PMR should be seen before DSB SY and
wfi.

Having looked at ARM IHI 0069D, 8.1.6 "Observability of the effects of
accesses to the GIC registers", I think I agree. 

My specific concern was that a CPU might complete the DSB before the
MSR, but I think it's clear per the GIC spec it's clear that an ISB is
not expected between the MSR and DSB, even if that's unusual.

quoted

quoted

 	wfi
+	msr_s	SYS_ICC_PMR_EL1, x1		// restore PMR

Likewise, we don't need any barriers here before we poke DAIF?

Here we don't need DSB SY because the value being restored is either:
- GIC_PRIO_IRQON which is the same as the current value, the
redistributor is already aware of it.
- GIC_PRIO_IRQOFF and the self-synchronization of PMR ensures that no
interrupts with priorities lower than the value of PMR can be taken
(this does not require to be seen by the redistributor).

For the ISB, I have this small doubt about whether it is needed between
WFI and MSR PMR. But there is this bit in the ARM ARM section D12.1.3
"General behavior of accesses to the AArch64 System registers",
subsection "Synchronization requirements for AArch64 System registers":

"Direct writes using the instructions in Table D11-2 on page D11-2660
require synchronization before software can rely on the effects of
changes to the System registers to affect instructions appearing in
program order after the direct write to the System register. Direct
writes to these registers are not allowed to affect any instructions
appearing in program order before the direct write."

ICC_PMR_EL1 is part of the mentioned table.

I think that's a defect in the ARM ARM, given it disagrees with the GIC
spec.

And reordering the direct write to PMR before the WFI would definitely
affect the WFI instruction, so my interpretation is that this would
not be allowed by the architecture. So I don't think we need the ISB
either, but my understanding could be wrong.

We already assume that a DSB can't be re-ordered w.r.t. the WFI, so as
long as the DSB can't complete before the MSR, I think we're good.

quoted

quoted

+	msr	daif, x0			// restore I bit
 	ret
 ENDPROC(cpu_do_idle)

If we build without CONFIG_ARM64_PSEUDO_NMI surely we don't want to emit
the alternative?

How about we move this to C, and have something like the below?

For the !CONFIG_ARM64_PSEUDO_NMI case it generates identical assembly to the
existing cpu_do_idle(). Note that I've assumed we don't need barriers, which
(as above) I'm not certain of.

Thanks,
Mark.

---->8----

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 7f1628effe6d..ccd2ad8c5e2f 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c

@@ -73,6 +73,40 @@ EXPORT_SYMBOL_GPL(pm_power_off);
 
 void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
 
+static inline void __cpu_do_idle(void)
+{
+       /* WFI may enter a low-power mode */
+       dsb(sy);
+       wfi();
+}
+
+/*
+ * When using priority masking we need to take extra care, etc.
+ */
+static inline void __cpu_do_idle_irqprio(void)
+{
+       unsigned long flags = arch_local_irq_save();

The issue with this is that in patch 10, arch_local_irq_* functions
toggle PMR rather than PSR.I.

I could use local_daif_mask but I don't think disabling debug and async
is good. Otherwise and can do a small bit of  inline assembly and have
something like:

Can we factor out the existing arch_local_irq_save() somehow?

Thanks,
Mark.

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