Re: [PATCH v2 3/5] arm64: Fix incorrect irqflag restore for priority masking

[Julien Thierry]

On 14/05/2019 13:01, Robin Murphy wrote:
> On 14/05/2019 10:25, Julien Thierry wrote:
> [...]
>>>> +static inline int arch_irqs_disabled_flags(unsigned long flags)
>>>> +{
>>>> +ÂÂÂ int res;
>>>> +
>>>> +ÂÂÂ asm volatile(ALTERNATIVE(
>>>> +ÂÂÂÂÂÂÂ "andÂÂÂ %w0, %w1, #" __stringify(PSR_I_BIT) "\n"
>>>> +ÂÂÂÂÂÂÂ "nop",
>>>> +ÂÂÂÂÂÂÂ "cmpÂÂÂ %w1, #" __stringify(GIC_PRIO_IRQON) "\n"
>>>> +ÂÂÂÂÂÂÂ "csetÂÂÂ %w0, ne",
>>>> +ÂÂÂÂÂÂÂ ARM64_HAS_IRQ_PRIO_MASKING)
>>>> +ÂÂÂÂÂÂÂ : "=&r" (res)
>>>> +ÂÂÂÂÂÂÂ : "r" ((int) flags)
>>>> +ÂÂÂÂÂÂÂ : "memory");
>>>
>>> I wonder if this should have "cc" as part of the clobber list.
>>
>> Is there any special semantic to "cc" on arm64? All I can find is that
>> in the general case it indicates that it is modifying the "flags"
>> register.
>>
>> Is your suggestion only for the PMR case? Or is it something that we
>> should add regardless of PMR?
>> The latter makes sense to me, but for the former, I fail to understand
>> why this should affect only PMR.
> 
> The PMR case really ought to have have a cc clobber, because who knows
> what this may end up inlined into, and compilers can get pretty
> aggressive with instruction scheduling in ways which leave a live value
> in CPSR across sizeable chunks of other code. It's true that the non-PMR
> case doesn't need it, but the surrounding code still needs to be
> generated to accommodate both possible versions of the alternative. From
> the look of the rest of the patch, the existing pseudo-NMI code has this
> bug in a few places.
> 
> Technically you could omit it when ARM64_PSEUDO_NMI is configured out
> entirely, but at that point you may as well omit the whole alternative
> as well. It's probably not worth the bother unless it proves to have a
> significant impact on codegen overall. On which note the memory clobber
> also seems superfluous either way :/
> 

Right, I see. I misunderstood what was meant by "cc" indicating that the
assembly modified the flags.

Due to the context I interpreted it as irqflags whereas it concerns the
condition flags (hence the 'c' I presume...). It all makes more sense now.

> That said, now that I've been looking at it for this long, if the aim is
> just to create a zero/nonzero value then couldn't the PMR case just be
> "eor %w0, %w1, #GIC_PRIO_IRQON" and avoid the need for clobbers at all?
> 

Yes, definitely seems like it would be better! I'll take that
suggestion, thanks.

Cheers,

-- 
Julien Thierry