Messages in this thread | | | From | Ard Biesheuvel <> | Date | Thu, 13 Dec 2018 12:35:28 +0100 | Subject | Re: [PATCH v7 11/25] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking |
| |
On Thu, 13 Dec 2018 at 09:54, Julien Thierry <julien.thierry@arm.com> wrote: > > > > 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... >
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.
| |