lkml.org 
[lkml]   [2018]   [Dec]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v6 08/24] arm64: Unmask PMR before going idle
From
Date


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

\
 
 \ /
  Last update: 2018-12-03 11:39    [W:0.101 / U:44.844 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site