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

[Ard Biesheuvel]

On Thu, 13 Dec 2018 at 09:54, Julien Thierry <julien.thierry@xxxxxxx> wrote:
>
>
>
> On 12/12/2018 18:10, Ard Biesheuvel wrote:
> > On Wed, 12 Dec 2018 at 18:59, Julien Thierry <julien.thierry@xxxxxxx> wrote:
> >>
> >>
> >>
> >> On 12/12/2018 17:27, Ard Biesheuvel wrote:
> >>> On Wed, 12 Dec 2018 at 17:48, Julien Thierry <julien.thierry@xxxxxxx> 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@xxxxxxx>
> >>>> Suggested-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> >>>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> >>>> Cc: Will Deacon <will.deacon@xxxxxxx>
> >>>> Cc: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> >>>> Cc: Oleg Nesterov <oleg@xxxxxxxxxx>
> >>>> ---
> >>>>  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...
>

No, that makes sense. As you said, you can just create a
efi_get_irqflags() helper that defaults to what we are using now, and
can be overridden to just return DAIF in our case.