Re: [PATCH v6 08/24] arm64: Unmask PMR before going idle

[Julien Thierry <julien.thierry@arm.com>]

On 30/11/18 13:37, Mark Rutland wrote:
> On Fri, Nov 30, 2018 at 10:55:47AM +0000, Julien Thierry wrote:
>> On 29/11/18 17:44, Mark Rutland wrote:
>>> On Mon, Nov 12, 2018 at 11:56:59AM +0000, Julien Thierry wrote:
> 
>>>> +	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.
> 
>>>>  	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.
> 
>>>
>>>> +	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?
> 

I'm not sure I understand what you're suggesting. Using individual
accessors for PMR and PSR.I instead of arch_local_irq_save?

I am just a bit concerned about having too many functions to play with
either (especially since most of the time those functions end up being
single use).

Thanks,

-- 
Julien Thierry