Re: [PATCH v7 11/25] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking

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

On 12/12/2018 18:10, Ard Biesheuvel wrote:
> On Wed, 12 Dec 2018 at 18:59, Julien Thierry <julien.thierry@arm.com> wrote:
>>
>>
>>
>> On 12/12/2018 17:27, Ard Biesheuvel wrote:
>>> On Wed, 12 Dec 2018 at 17:48, Julien Thierry <julien.thierry@arm.com> wrote:
>>>>
>>>> Instead disabling interrupts by setting the PSR.I bit, use a priority
>>>> higher than the one used for interrupts to mask them via PMR.
>>>>
>>>> When using PMR to disable interrupts, the value of PMR will be used
>>>> instead of PSR.[DAIF] for the irqflags.
>>>>
>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>>>> Suggested-by: Daniel Thompson <daniel.thompson@linaro.org>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Cc: Oleg Nesterov <oleg@redhat.com>
>>>> ---
>>>>  arch/arm64/include/asm/efi.h      |   5 +-
>>>>  arch/arm64/include/asm/irqflags.h | 123 +++++++++++++++++++++++++++++---------
>>>>  2 files changed, 99 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
>>>> index 7ed3208..a9d3ebc 100644
>>>> --- a/arch/arm64/include/asm/efi.h
>>>> +++ b/arch/arm64/include/asm/efi.h
>>>> @@ -42,7 +42,10 @@
>>>>
>>>>  efi_status_t __efi_rt_asm_wrapper(void *, const char *, ...);
>>>>
>>>> -#define ARCH_EFI_IRQ_FLAGS_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
>>>> +#define ARCH_EFI_IRQ_FLAGS_MASK                                                \
>>>> +       (system_uses_irq_prio_masking() ?                               \
>>>> +               GIC_PRIO_IRQON :                                        \
>>>> +               (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT))
>>>>
>>>
>>> This mask is used to determine whether we return from a firmware call
>>> with a different value for the I flag than we entered it with. So
>>> instead of changing the mask, we should change the way we record DAIF,
>>> given that the firmware is still going to poke the I bit if it
>>> misbehaves, regardless of whether the OS happens to use priorities for
>>> interrupt masking.
>>>
>>
>> Thanks for pointing that out, so this change makes little sense...
>>
>> The annoying part is that the flag checking takes place in the arch
>> agnostic code.
>>
>> Would introducing some overriddable efi_get_flags() or efi_save_flags()
>> that default to local_save_flags() seem like an acceptable solution?
>>
>> This way I could override it for arm64 and still return the DAIF bits.
>>
> 
> I don't follow the reasoning below about irqflags exactly, but is
> there any way we could simply but both PMR and DAIF in flags? We could
> even update the mask here to ensure that the firmware doesn't corrupt
> the PMR.
> 

So, that was the case in my previous versions of the series, and as you
said, that covered checking both DAIF bits and PMR on return from EFI
services. But Catalin suggested that irqflags could just use PMR when we
enable the priority masking feature. Catalin's suggestion does simplify
things, except for this part.

However, it doesn't seem to far-fetched to me that the architecture
could have a more generic way to tell the EFI driver "this is the set of
stuff that I care about and you should return from runtime services with
this stuff in the same state as before" without the "set of stuff" being
limited to irqflags.

But maybe this would be over-engineering just to deal with my use-case...

Thanks,

-- 
Julien Thierry