Messages in this thread | | | Subject | Re: [PATCH v7 11/25] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking | From | Julien Thierry <> | Date | Thu, 13 Dec 2018 15:03:57 +0000 |
| |
On 13/12/2018 12:02, Julien Thierry wrote: > > > On 13/12/2018 11:35, Ard Biesheuvel wrote: >> 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. >> > > Good, thanks for the confirmation. I'll do that for the next version of > the series. >
Argh, not as simple as I had expected.
Turns out include/linux/efi.h does not include asm/efi.h (including it at the beginning of the file breaks the build because asm/efi.h misses the efi type definitions.
So a thing like:
#ifndef efi_get_irqflags #define efi_get_irqflags(flags) local_save_flags(flags) #endif
in include/linux/efi.h cannot be overridden.
Either I would need to introduce the definitions arm, arm64 and x86 (I don't think there are other arch supporting EFI right now) or I'll need to come up with another solution.
-- Julien Thierry
| |