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 12:02:40 +0000 |
| |
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.
Thanks,
-- Julien Thierry
| |