| Message ID | 1544633245-6036-12-git-send-email-julien.thierry@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: provide pseudo NMI with GICv3 | expand |
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. It also means that the NMI concept is a best effort thing only, given that uncooperative firmware could prevent them from being delivered. > /* arch specific definitions used by the stub code */ > > diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h > index 24692ed..fa3b06f 100644 > --- a/arch/arm64/include/asm/irqflags.h > +++ b/arch/arm64/include/asm/irqflags.h > @@ -18,7 +18,9 @@ > > #ifdef __KERNEL__ > > +#include <asm/alternative.h> > #include <asm/ptrace.h> > +#include <asm/sysreg.h> > > /* > * Aarch64 has flags for masking: Debug, Asynchronous (serror), Interrupts and > @@ -36,47 +38,96 @@ > /* > * CPU interrupt mask handling. > */ > -static inline unsigned long arch_local_irq_save(void) > -{ > - unsigned long flags; > - asm volatile( > - "mrs %0, daif // arch_local_irq_save\n" > - "msr daifset, #2" > - : "=r" (flags) > - : > - : "memory"); > - return flags; > -} > - > static inline void arch_local_irq_enable(void) > { > - asm volatile( > - "msr daifclr, #2 // arch_local_irq_enable" > - : > + unsigned long unmasked = GIC_PRIO_IRQON; > + > + asm volatile(ALTERNATIVE( > + "msr daifclr, #2 // arch_local_irq_enable\n" > + "nop", > + "msr_s " __stringify(SYS_ICC_PMR_EL1) ",%0\n" > + "dsb sy", > + ARM64_HAS_IRQ_PRIO_MASKING) > : > + : "r" (unmasked) > : "memory"); > } > > static inline void arch_local_irq_disable(void) > { > - asm volatile( > - "msr daifset, #2 // arch_local_irq_disable" > - : > + unsigned long masked = GIC_PRIO_IRQOFF; > + > + asm volatile(ALTERNATIVE( > + "msr daifset, #2 // arch_local_irq_disable", > + "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0", > + ARM64_HAS_IRQ_PRIO_MASKING) > : > + : "r" (masked) > : "memory"); > } > > /* > + * Having two ways to control interrupt status is a bit complicated. Some > + * locations like exception entries will have PSR.I bit set by the architecture > + * while PMR is unmasked. > + * We need the irqflags to represent that interrupts are disabled in such cases. > + * > + * For this, we lower the value read from PMR when the I bit is set so it is > + * considered as an irq masking priority. (With PMR, lower value means masking > + * more interrupts). > + */ > +#define _get_irqflags(daif_bits, pmr) \ > +({ \ > + unsigned long flags; \ > + \ > + BUILD_BUG_ON(GIC_PRIO_IRQOFF < (GIC_PRIO_IRQON & ~PSR_I_BIT)); \ > + asm volatile(ALTERNATIVE( \ > + "mov %0, %1\n" \ > + "nop\n" \ > + "nop", \ > + "and %0, %1, #" __stringify(PSR_I_BIT) "\n" \ > + "mvn %0, %0\n" \ > + "and %0, %0, %2", \ > + ARM64_HAS_IRQ_PRIO_MASKING) \ > + : "=&r" (flags) \ > + : "r" (daif_bits), "r" (pmr) \ > + : "memory"); \ > + \ > + flags; \ > +}) > + > +/* > * Save the current interrupt enable state. > */ > static inline unsigned long arch_local_save_flags(void) > { > - unsigned long flags; > - asm volatile( > - "mrs %0, daif // arch_local_save_flags" > - : "=r" (flags) > + unsigned long daif_bits; > + unsigned long pmr; // Only used if alternative is on > + > + daif_bits = read_sysreg(daif); > + > + // Get PMR > + asm volatile(ALTERNATIVE( > + "nop", > + "mrs_s %0, " __stringify(SYS_ICC_PMR_EL1), > + ARM64_HAS_IRQ_PRIO_MASKING) > + : "=&r" (pmr) > : > : "memory"); > + > + return _get_irqflags(daif_bits, pmr); > +} > + > +#undef _get_irqflags > + > +static inline unsigned long arch_local_irq_save(void) > +{ > + unsigned long flags; > + > + flags = arch_local_save_flags(); > + > + arch_local_irq_disable(); > + > return flags; > } > > @@ -85,16 +136,32 @@ static inline unsigned long arch_local_save_flags(void) > */ > static inline void arch_local_irq_restore(unsigned long flags) > { > - asm volatile( > - "msr daif, %0 // arch_local_irq_restore" > - : > - : "r" (flags) > - : "memory"); > + asm volatile(ALTERNATIVE( > + "msr daif, %0\n" > + "nop", > + "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0\n" > + "dsb sy", > + ARM64_HAS_IRQ_PRIO_MASKING) > + : "+r" (flags) > + : > + : "memory"); > } > > static inline int arch_irqs_disabled_flags(unsigned long flags) > { > - return flags & PSR_I_BIT; > + int res; > + > + asm volatile(ALTERNATIVE( > + "and %w0, %w1, #" __stringify(PSR_I_BIT) "\n" > + "nop", > + "cmp %w1, #" __stringify(GIC_PRIO_IRQOFF) "\n" > + "cset %w0, ls", > + ARM64_HAS_IRQ_PRIO_MASKING) > + : "=&r" (res) > + : "r" ((int) flags) > + : "memory"); > + > + return res; > } > #endif > #endif > -- > 1.9.1 >
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. > It also means that the NMI concept is a best effort thing only, given > that uncooperative firmware could prevent them from being delivered. > Yes, but we would get an error on the EFI side if it starts setting the I bit. The NMI would be mostly a debug tool at that point anyway, so I guess if external forces are against us, it is not really worth fighting for it. Thanks, > > >> /* arch specific definitions used by the stub code */ >> >> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h >> index 24692ed..fa3b06f 100644 >> --- a/arch/arm64/include/asm/irqflags.h >> +++ b/arch/arm64/include/asm/irqflags.h >> @@ -18,7 +18,9 @@ >> >> #ifdef __KERNEL__ >> >> +#include <asm/alternative.h> >> #include <asm/ptrace.h> >> +#include <asm/sysreg.h> >> >> /* >> * Aarch64 has flags for masking: Debug, Asynchronous (serror), Interrupts and >> @@ -36,47 +38,96 @@ >> /* >> * CPU interrupt mask handling. >> */ >> -static inline unsigned long arch_local_irq_save(void) >> -{ >> - unsigned long flags; >> - asm volatile( >> - "mrs %0, daif // arch_local_irq_save\n" >> - "msr daifset, #2" >> - : "=r" (flags) >> - : >> - : "memory"); >> - return flags; >> -} >> - >> static inline void arch_local_irq_enable(void) >> { >> - asm volatile( >> - "msr daifclr, #2 // arch_local_irq_enable" >> - : >> + unsigned long unmasked = GIC_PRIO_IRQON; >> + >> + asm volatile(ALTERNATIVE( >> + "msr daifclr, #2 // arch_local_irq_enable\n" >> + "nop", >> + "msr_s " __stringify(SYS_ICC_PMR_EL1) ",%0\n" >> + "dsb sy", >> + ARM64_HAS_IRQ_PRIO_MASKING) >> : >> + : "r" (unmasked) >> : "memory"); >> } >> >> static inline void arch_local_irq_disable(void) >> { >> - asm volatile( >> - "msr daifset, #2 // arch_local_irq_disable" >> - : >> + unsigned long masked = GIC_PRIO_IRQOFF; >> + >> + asm volatile(ALTERNATIVE( >> + "msr daifset, #2 // arch_local_irq_disable", >> + "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0", >> + ARM64_HAS_IRQ_PRIO_MASKING) >> : >> + : "r" (masked) >> : "memory"); >> } >> >> /* >> + * Having two ways to control interrupt status is a bit complicated. Some >> + * locations like exception entries will have PSR.I bit set by the architecture >> + * while PMR is unmasked. >> + * We need the irqflags to represent that interrupts are disabled in such cases. >> + * >> + * For this, we lower the value read from PMR when the I bit is set so it is >> + * considered as an irq masking priority. (With PMR, lower value means masking >> + * more interrupts). >> + */ >> +#define _get_irqflags(daif_bits, pmr) \ >> +({ \ >> + unsigned long flags; \ >> + \ >> + BUILD_BUG_ON(GIC_PRIO_IRQOFF < (GIC_PRIO_IRQON & ~PSR_I_BIT)); \ >> + asm volatile(ALTERNATIVE( \ >> + "mov %0, %1\n" \ >> + "nop\n" \ >> + "nop", \ >> + "and %0, %1, #" __stringify(PSR_I_BIT) "\n" \ >> + "mvn %0, %0\n" \ >> + "and %0, %0, %2", \ >> + ARM64_HAS_IRQ_PRIO_MASKING) \ >> + : "=&r" (flags) \ >> + : "r" (daif_bits), "r" (pmr) \ >> + : "memory"); \ >> + \ >> + flags; \ >> +}) >> + >> +/* >> * Save the current interrupt enable state. >> */ >> static inline unsigned long arch_local_save_flags(void) >> { >> - unsigned long flags; >> - asm volatile( >> - "mrs %0, daif // arch_local_save_flags" >> - : "=r" (flags) >> + unsigned long daif_bits; >> + unsigned long pmr; // Only used if alternative is on >> + >> + daif_bits = read_sysreg(daif); >> + >> + // Get PMR >> + asm volatile(ALTERNATIVE( >> + "nop", >> + "mrs_s %0, " __stringify(SYS_ICC_PMR_EL1), >> + ARM64_HAS_IRQ_PRIO_MASKING) >> + : "=&r" (pmr) >> : >> : "memory"); >> + >> + return _get_irqflags(daif_bits, pmr); >> +} >> + >> +#undef _get_irqflags >> + >> +static inline unsigned long arch_local_irq_save(void) >> +{ >> + unsigned long flags; >> + >> + flags = arch_local_save_flags(); >> + >> + arch_local_irq_disable(); >> + >> return flags; >> } >> >> @@ -85,16 +136,32 @@ static inline unsigned long arch_local_save_flags(void) >> */ >> static inline void arch_local_irq_restore(unsigned long flags) >> { >> - asm volatile( >> - "msr daif, %0 // arch_local_irq_restore" >> - : >> - : "r" (flags) >> - : "memory"); >> + asm volatile(ALTERNATIVE( >> + "msr daif, %0\n" >> + "nop", >> + "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0\n" >> + "dsb sy", >> + ARM64_HAS_IRQ_PRIO_MASKING) >> + : "+r" (flags) >> + : >> + : "memory"); >> } >> >> static inline int arch_irqs_disabled_flags(unsigned long flags) >> { >> - return flags & PSR_I_BIT; >> + int res; >> + >> + asm volatile(ALTERNATIVE( >> + "and %w0, %w1, #" __stringify(PSR_I_BIT) "\n" >> + "nop", >> + "cmp %w1, #" __stringify(GIC_PRIO_IRQOFF) "\n" >> + "cset %w0, ls", >> + ARM64_HAS_IRQ_PRIO_MASKING) >> + : "=&r" (res) >> + : "r" ((int) flags) >> + : "memory"); >> + >> + return res; >> } >> #endif >> #endif >> -- >> 1.9.1 >>
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. > > It also means that the NMI concept is a best effort thing only, given > > that uncooperative firmware could prevent them from being delivered. > > > > Yes, but we would get an error on the EFI side if it starts setting the > I bit. The NMI would be mostly a debug tool at that point anyway, so I > guess if external forces are against us, it is not really worth fighting > for it. > > Thanks, > > > > > > >> /* arch specific definitions used by the stub code */ > >> > >> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h > >> index 24692ed..fa3b06f 100644 > >> --- a/arch/arm64/include/asm/irqflags.h > >> +++ b/arch/arm64/include/asm/irqflags.h > >> @@ -18,7 +18,9 @@ > >> > >> #ifdef __KERNEL__ > >> > >> +#include <asm/alternative.h> > >> #include <asm/ptrace.h> > >> +#include <asm/sysreg.h> > >> > >> /* > >> * Aarch64 has flags for masking: Debug, Asynchronous (serror), Interrupts and > >> @@ -36,47 +38,96 @@ > >> /* > >> * CPU interrupt mask handling. > >> */ > >> -static inline unsigned long arch_local_irq_save(void) > >> -{ > >> - unsigned long flags; > >> - asm volatile( > >> - "mrs %0, daif // arch_local_irq_save\n" > >> - "msr daifset, #2" > >> - : "=r" (flags) > >> - : > >> - : "memory"); > >> - return flags; > >> -} > >> - > >> static inline void arch_local_irq_enable(void) > >> { > >> - asm volatile( > >> - "msr daifclr, #2 // arch_local_irq_enable" > >> - : > >> + unsigned long unmasked = GIC_PRIO_IRQON; > >> + > >> + asm volatile(ALTERNATIVE( > >> + "msr daifclr, #2 // arch_local_irq_enable\n" > >> + "nop", > >> + "msr_s " __stringify(SYS_ICC_PMR_EL1) ",%0\n" > >> + "dsb sy", > >> + ARM64_HAS_IRQ_PRIO_MASKING) > >> : > >> + : "r" (unmasked) > >> : "memory"); > >> } > >> > >> static inline void arch_local_irq_disable(void) > >> { > >> - asm volatile( > >> - "msr daifset, #2 // arch_local_irq_disable" > >> - : > >> + unsigned long masked = GIC_PRIO_IRQOFF; > >> + > >> + asm volatile(ALTERNATIVE( > >> + "msr daifset, #2 // arch_local_irq_disable", > >> + "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0", > >> + ARM64_HAS_IRQ_PRIO_MASKING) > >> : > >> + : "r" (masked) > >> : "memory"); > >> } > >> > >> /* > >> + * Having two ways to control interrupt status is a bit complicated. Some > >> + * locations like exception entries will have PSR.I bit set by the architecture > >> + * while PMR is unmasked. > >> + * We need the irqflags to represent that interrupts are disabled in such cases. > >> + * > >> + * For this, we lower the value read from PMR when the I bit is set so it is > >> + * considered as an irq masking priority. (With PMR, lower value means masking > >> + * more interrupts). > >> + */ > >> +#define _get_irqflags(daif_bits, pmr) \ > >> +({ \ > >> + unsigned long flags; \ > >> + \ > >> + BUILD_BUG_ON(GIC_PRIO_IRQOFF < (GIC_PRIO_IRQON & ~PSR_I_BIT)); \ > >> + asm volatile(ALTERNATIVE( \ > >> + "mov %0, %1\n" \ > >> + "nop\n" \ > >> + "nop", \ > >> + "and %0, %1, #" __stringify(PSR_I_BIT) "\n" \ > >> + "mvn %0, %0\n" \ > >> + "and %0, %0, %2", \ > >> + ARM64_HAS_IRQ_PRIO_MASKING) \ > >> + : "=&r" (flags) \ > >> + : "r" (daif_bits), "r" (pmr) \ > >> + : "memory"); \ > >> + \ > >> + flags; \ > >> +}) > >> + > >> +/* > >> * Save the current interrupt enable state. > >> */ > >> static inline unsigned long arch_local_save_flags(void) > >> { > >> - unsigned long flags; > >> - asm volatile( > >> - "mrs %0, daif // arch_local_save_flags" > >> - : "=r" (flags) > >> + unsigned long daif_bits; > >> + unsigned long pmr; // Only used if alternative is on > >> + > >> + daif_bits = read_sysreg(daif); > >> + > >> + // Get PMR > >> + asm volatile(ALTERNATIVE( > >> + "nop", > >> + "mrs_s %0, " __stringify(SYS_ICC_PMR_EL1), > >> + ARM64_HAS_IRQ_PRIO_MASKING) > >> + : "=&r" (pmr) > >> : > >> : "memory"); > >> + > >> + return _get_irqflags(daif_bits, pmr); > >> +} > >> + > >> +#undef _get_irqflags > >> + > >> +static inline unsigned long arch_local_irq_save(void) > >> +{ > >> + unsigned long flags; > >> + > >> + flags = arch_local_save_flags(); > >> + > >> + arch_local_irq_disable(); > >> + > >> return flags; > >> } > >> > >> @@ -85,16 +136,32 @@ static inline unsigned long arch_local_save_flags(void) > >> */ > >> static inline void arch_local_irq_restore(unsigned long flags) > >> { > >> - asm volatile( > >> - "msr daif, %0 // arch_local_irq_restore" > >> - : > >> - : "r" (flags) > >> - : "memory"); > >> + asm volatile(ALTERNATIVE( > >> + "msr daif, %0\n" > >> + "nop", > >> + "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0\n" > >> + "dsb sy", > >> + ARM64_HAS_IRQ_PRIO_MASKING) > >> + : "+r" (flags) > >> + : > >> + : "memory"); > >> } > >> > >> static inline int arch_irqs_disabled_flags(unsigned long flags) > >> { > >> - return flags & PSR_I_BIT; > >> + int res; > >> + > >> + asm volatile(ALTERNATIVE( > >> + "and %w0, %w1, #" __stringify(PSR_I_BIT) "\n" > >> + "nop", > >> + "cmp %w1, #" __stringify(GIC_PRIO_IRQOFF) "\n" > >> + "cset %w0, ls", > >> + ARM64_HAS_IRQ_PRIO_MASKING) > >> + : "=&r" (res) > >> + : "r" ((int) flags) > >> + : "memory"); > >> + > >> + return res; > >> } > >> #endif > >> #endif > >> -- > >> 1.9.1 > >> > > -- > Julien Thierry
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,
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.
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,
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.
Hi, On 13/12/2018 15:03, Julien Thierry wrote: > > 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. > Would the following patch be acceptable for the EFI generic side? If it is, I'll add it to the next iteration of this series. Thanks, Julien --> From 7acaa8e17142263addafb18ae10bd5d2d49cfb39 Mon Sep 17 00:00:00 2001 From: Julien Thierry <julien.thierry@arm.com> Date: Fri, 14 Dec 2018 14:20:13 +0000 Subject: [RFC] efi: Let architectures decide the flags that should be saved/restored Currently, irqflags are saved before calling runtime services and checked for mismatch on return. Add a config option to let architectures define a set of flags to be checked and (if needed) restored when coming back from runtime services. This allows to use check flags that are not necesarly related to irqflags. Signed-off-by: Julien Thierry <julien.thierry@arm.com> --- arch/Kconfig | 8 ++++++++ drivers/firmware/efi/runtime-wrappers.c | 4 ++-- include/linux/efi.h | 12 ++++++++++-- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/arch/Kconfig b/arch/Kconfig index e1e540f..cbec325 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -695,6 +695,14 @@ config HAVE_ARCH_HASH file which provides platform-specific implementations of some functions in <linux/hash.h> or fs/namei.c. +config HAVE_GENERIC_EFI_FLAGS + bool + default n + help + Architecture defines a set of flags that EFI runtime services + should take care to restore when returning to the OS. + If this is not set, the set of flags defaults to the arch irqflags. + config ISA_BUS_API def_bool ISA diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c index 8903b9c..6dafa04 100644 --- a/drivers/firmware/efi/runtime-wrappers.c +++ b/drivers/firmware/efi/runtime-wrappers.c @@ -93,7 +93,7 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call) { unsigned long cur_flags, mismatch; - local_save_flags(cur_flags); + efi_save_flags(cur_flags); mismatch = flags ^ cur_flags; if (!WARN_ON_ONCE(mismatch & ARCH_EFI_IRQ_FLAGS_MASK)) @@ -102,7 +102,7 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call) add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_NOW_UNRELIABLE); pr_err_ratelimited(FW_BUG "IRQ flags corrupted (0x%08lx=>0x%08lx) by EFI %s\n", flags, cur_flags, call); - local_irq_restore(flags); + efi_restore_flags(flags); } /* diff --git a/include/linux/efi.h b/include/linux/efi.h index 100ce4a..41c110a 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -1594,6 +1594,14 @@ enum efi_secureboot_mode { void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table); +#ifdef CONFIG_HAVE_GENERIC_EFI_FLAGS +#define efi_save_flags(state_flags) arch_efi_save_flags(state_flags) +#define efi_restore_flags(state_flags) arch_efi_restore_flags(state_flags) +#else +#define efi_save_flags(state_flags) local_save_flags(state_flags) +#define efi_restore_flags(state_flags) local_irq_restore(state_flags) +#endif + /* * Arch code can implement the following three template macros, avoiding * reptition for the void/non-void return cases of {__,}efi_call_virt(): @@ -1621,7 +1629,7 @@ enum efi_secureboot_mode { \ arch_efi_call_virt_setup(); \ \ - local_save_flags(__flags); \ + efi_save_flags(__flags); \ __s = arch_efi_call_virt(p, f, args); \ efi_call_virt_check_flags(__flags, __stringify(f)); \ \ @@ -1636,7 +1644,7 @@ enum efi_secureboot_mode { \ arch_efi_call_virt_setup(); \ \ - local_save_flags(__flags); \ + efi_save_flags(__flags); \ arch_efi_call_virt(p, f, args); \ efi_call_virt_check_flags(__flags, __stringify(f)); \ \
On Fri, 14 Dec 2018 at 16:23, Julien Thierry <julien.thierry@arm.com> wrote: > > Hi, > > On 13/12/2018 15:03, Julien Thierry wrote: > > > > 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. > > > It might be a bit nasty, but can we put the #ifndef above in runtime-wrappers.c directly? The only reference in linux/efi.h is a macro, so that shouldn't matter afaict. > Would the following patch be acceptable for the EFI generic side? > > If it is, I'll add it to the next iteration of this series. > > Thanks, > > Julien > > --> > > From 7acaa8e17142263addafb18ae10bd5d2d49cfb39 Mon Sep 17 00:00:00 2001 > From: Julien Thierry <julien.thierry@arm.com> > Date: Fri, 14 Dec 2018 14:20:13 +0000 > Subject: [RFC] efi: Let architectures decide the flags that should be > saved/restored > > Currently, irqflags are saved before calling runtime services and > checked for mismatch on return. > > Add a config option to let architectures define a set of flags to be > checked and (if needed) restored when coming back from runtime services. > This allows to use check flags that are not necesarly related to > irqflags. > > Signed-off-by: Julien Thierry <julien.thierry@arm.com> > --- > arch/Kconfig | 8 ++++++++ > drivers/firmware/efi/runtime-wrappers.c | 4 ++-- > include/linux/efi.h | 12 ++++++++++-- > 3 files changed, 20 insertions(+), 4 deletions(-) > > diff --git a/arch/Kconfig b/arch/Kconfig > index e1e540f..cbec325 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -695,6 +695,14 @@ config HAVE_ARCH_HASH > file which provides platform-specific implementations of some > functions in <linux/hash.h> or fs/namei.c. > > +config HAVE_GENERIC_EFI_FLAGS > + bool > + default n > + help > + Architecture defines a set of flags that EFI runtime services > + should take care to restore when returning to the OS. > + If this is not set, the set of flags defaults to the arch irqflags. > + > config ISA_BUS_API > def_bool ISA > > diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c > index 8903b9c..6dafa04 100644 > --- a/drivers/firmware/efi/runtime-wrappers.c > +++ b/drivers/firmware/efi/runtime-wrappers.c > @@ -93,7 +93,7 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call) > { > unsigned long cur_flags, mismatch; > > - local_save_flags(cur_flags); > + efi_save_flags(cur_flags); > > mismatch = flags ^ cur_flags; > if (!WARN_ON_ONCE(mismatch & ARCH_EFI_IRQ_FLAGS_MASK)) > @@ -102,7 +102,7 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call) > add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_NOW_UNRELIABLE); > pr_err_ratelimited(FW_BUG "IRQ flags corrupted (0x%08lx=>0x%08lx) by EFI %s\n", > flags, cur_flags, call); > - local_irq_restore(flags); > + efi_restore_flags(flags); > } > > /* > diff --git a/include/linux/efi.h b/include/linux/efi.h > index 100ce4a..41c110a 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -1594,6 +1594,14 @@ enum efi_secureboot_mode { > > void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table); > > +#ifdef CONFIG_HAVE_GENERIC_EFI_FLAGS > +#define efi_save_flags(state_flags) arch_efi_save_flags(state_flags) > +#define efi_restore_flags(state_flags) arch_efi_restore_flags(state_flags) > +#else > +#define efi_save_flags(state_flags) local_save_flags(state_flags) > +#define efi_restore_flags(state_flags) local_irq_restore(state_flags) > +#endif > + > /* > * Arch code can implement the following three template macros, avoiding > * reptition for the void/non-void return cases of {__,}efi_call_virt(): > @@ -1621,7 +1629,7 @@ enum efi_secureboot_mode { > \ > arch_efi_call_virt_setup(); \ > \ > - local_save_flags(__flags); \ > + efi_save_flags(__flags); \ > __s = arch_efi_call_virt(p, f, args); \ > efi_call_virt_check_flags(__flags, __stringify(f)); \ > \ > @@ -1636,7 +1644,7 @@ enum efi_secureboot_mode { > \ > arch_efi_call_virt_setup(); \ > \ > - local_save_flags(__flags); \ > + efi_save_flags(__flags); \ > arch_efi_call_virt(p, f, args); \ > efi_call_virt_check_flags(__flags, __stringify(f)); \ > \ > -- > 1.9.1 > > > > > -- > Julien Thierry
On 14/12/2018 15:49, Ard Biesheuvel wrote: > On Fri, 14 Dec 2018 at 16:23, Julien Thierry <julien.thierry@arm.com> wrote: >> >> Hi, >> >> On 13/12/2018 15:03, Julien Thierry wrote: >>> >>> 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. >>> >> > > It might be a bit nasty, but can we put the #ifndef above in > runtime-wrappers.c directly? The only reference in linux/efi.h is a > macro, so that shouldn't matter afaict. > Sadly, in arch/x86/platform/uv/bios_uv.c, uv_bios_call() has a reference to the macro efi_call_virt_pointer() which wouldn't be able to see the definition in runtime-wrappers.c Otherwise, we could've moved efi_call_virt_pointer() and __efi_call_virt_pointer in runtime-wrappers.c and things would not have been as nasty. But no, I don't think we can do that without breaking some x86 build :( . Thanks, > >> Would the following patch be acceptable for the EFI generic side? >> >> If it is, I'll add it to the next iteration of this series. >> >> Thanks, >> >> Julien >> >> --> >> >> From 7acaa8e17142263addafb18ae10bd5d2d49cfb39 Mon Sep 17 00:00:00 2001 >> From: Julien Thierry <julien.thierry@arm.com> >> Date: Fri, 14 Dec 2018 14:20:13 +0000 >> Subject: [RFC] efi: Let architectures decide the flags that should be >> saved/restored >> >> Currently, irqflags are saved before calling runtime services and >> checked for mismatch on return. >> >> Add a config option to let architectures define a set of flags to be >> checked and (if needed) restored when coming back from runtime services. >> This allows to use check flags that are not necesarly related to >> irqflags. >> >> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >> --- >> arch/Kconfig | 8 ++++++++ >> drivers/firmware/efi/runtime-wrappers.c | 4 ++-- >> include/linux/efi.h | 12 ++++++++++-- >> 3 files changed, 20 insertions(+), 4 deletions(-) >> >> diff --git a/arch/Kconfig b/arch/Kconfig >> index e1e540f..cbec325 100644 >> --- a/arch/Kconfig >> +++ b/arch/Kconfig >> @@ -695,6 +695,14 @@ config HAVE_ARCH_HASH >> file which provides platform-specific implementations of some >> functions in <linux/hash.h> or fs/namei.c. >> >> +config HAVE_GENERIC_EFI_FLAGS >> + bool >> + default n >> + help >> + Architecture defines a set of flags that EFI runtime services >> + should take care to restore when returning to the OS. >> + If this is not set, the set of flags defaults to the arch irqflags. >> + >> config ISA_BUS_API >> def_bool ISA >> >> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c >> index 8903b9c..6dafa04 100644 >> --- a/drivers/firmware/efi/runtime-wrappers.c >> +++ b/drivers/firmware/efi/runtime-wrappers.c >> @@ -93,7 +93,7 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call) >> { >> unsigned long cur_flags, mismatch; >> >> - local_save_flags(cur_flags); >> + efi_save_flags(cur_flags); >> >> mismatch = flags ^ cur_flags; >> if (!WARN_ON_ONCE(mismatch & ARCH_EFI_IRQ_FLAGS_MASK)) >> @@ -102,7 +102,7 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call) >> add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_NOW_UNRELIABLE); >> pr_err_ratelimited(FW_BUG "IRQ flags corrupted (0x%08lx=>0x%08lx) by EFI %s\n", >> flags, cur_flags, call); >> - local_irq_restore(flags); >> + efi_restore_flags(flags); >> } >> >> /* >> diff --git a/include/linux/efi.h b/include/linux/efi.h >> index 100ce4a..41c110a 100644 >> --- a/include/linux/efi.h >> +++ b/include/linux/efi.h >> @@ -1594,6 +1594,14 @@ enum efi_secureboot_mode { >> >> void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table); >> >> +#ifdef CONFIG_HAVE_GENERIC_EFI_FLAGS >> +#define efi_save_flags(state_flags) arch_efi_save_flags(state_flags) >> +#define efi_restore_flags(state_flags) arch_efi_restore_flags(state_flags) >> +#else >> +#define efi_save_flags(state_flags) local_save_flags(state_flags) >> +#define efi_restore_flags(state_flags) local_irq_restore(state_flags) >> +#endif >> + >> /* >> * Arch code can implement the following three template macros, avoiding >> * reptition for the void/non-void return cases of {__,}efi_call_virt(): >> @@ -1621,7 +1629,7 @@ enum efi_secureboot_mode { >> \ >> arch_efi_call_virt_setup(); \ >> \ >> - local_save_flags(__flags); \ >> + efi_save_flags(__flags); \ >> __s = arch_efi_call_virt(p, f, args); \ >> efi_call_virt_check_flags(__flags, __stringify(f)); \ >> \ >> @@ -1636,7 +1644,7 @@ enum efi_secureboot_mode { >> \ >> arch_efi_call_virt_setup(); \ >> \ >> - local_save_flags(__flags); \ >> + efi_save_flags(__flags); \ >> arch_efi_call_virt(p, f, args); \ >> efi_call_virt_check_flags(__flags, __stringify(f)); \ >> \ >> -- >> 1.9.1 >> >> >> >> >> -- >> Julien Thierry
From: Jian-Lin Chen <lecopzer.chen@mediatek.com> On Wed, 12 Dec 2018 at 17:48, Julien Thierry <julien.thierry@arm.com> wrote: > static inline void arch_local_irq_enable(void) > { > - asm volatile( > - "msr daifclr, #2 // arch_local_irq_enable" > - : > + unsigned long unmasked = GIC_PRIO_IRQON; > + Should we need a WARN_ON() to check if the daif_I bit is masked, or explicitly unmasked I bit here? If I bit was masked and someone calls arch_local_irq_enable(), they still couldn't recieve any interrupt. > + asm volatile(ALTERNATIVE( > + "msr daifclr, #2 // arch_local_irq_enable\n" > + "nop", > + "msr_s " __stringify(SYS_ICC_PMR_EL1) ",%0\n" > + "dsb sy", > + ARM64_HAS_IRQ_PRIO_MASKING) > : > + : "r" (unmasked) > : "memory"); > } > > static inline void arch_local_irq_disable(void) > { > - asm volatile( > - "msr daifset, #2 // arch_local_irq_disable" > - : > + unsigned long masked = GIC_PRIO_IRQOFF; > + > + asm volatile(ALTERNATIVE( > + "msr daifset, #2 // arch_local_irq_disable", > + "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0", May be a "dsb sy" here? > + ARM64_HAS_IRQ_PRIO_MASKING) > : > + : "r" (masked) > : "memory"); > }
Hi Jian-Lin, Thanks for looking at this. On 16/12/2018 14:47, Jian-Lin Chen wrote: > From: Jian-Lin Chen <lecopzer.chen@mediatek.com> > > > On Wed, 12 Dec 2018 at 17:48, Julien Thierry <julien.thierry@arm.com> wrote: >> static inline void arch_local_irq_enable(void) >> { >> - asm volatile( >> - "msr daifclr, #2 // arch_local_irq_enable" >> - : >> + unsigned long unmasked = GIC_PRIO_IRQON; >> + > > Should we need a WARN_ON() to check if the daif_I bit is masked, or > explicitly unmasked I bit here? > While I would agree, adding the WARN_ON() will add some non-negligible overhead, especially if we need to read the daif flags to check it. Since these functions are called often in the whole system and using PMR already makes things a bit slower, I'd prefer to avoid checks in here. > If I bit was masked and someone calls arch_local_irq_enable(), they still > couldn't recieve any interrupt. > > >> + asm volatile(ALTERNATIVE( >> + "msr daifclr, #2 // arch_local_irq_enable\n" >> + "nop", >> + "msr_s " __stringify(SYS_ICC_PMR_EL1) ",%0\n" >> + "dsb sy", >> + ARM64_HAS_IRQ_PRIO_MASKING) >> : >> + : "r" (unmasked) >> : "memory"); >> } >> >> static inline void arch_local_irq_disable(void) >> { >> - asm volatile( >> - "msr daifset, #2 // arch_local_irq_disable" >> - : >> + unsigned long masked = GIC_PRIO_IRQOFF; >> + >> + asm volatile(ALTERNATIVE( >> + "msr daifset, #2 // arch_local_irq_disable", >> + "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0", > > May be a "dsb sy" here? So, we need a "dsb sy" when unmasking interrupts because this ensures the redistributor sees the latest PMR value and starts forwarding lower priority interrupts again. When we disable interrupts however, the GIC CPU interface guarantees that no interrupts of lower priority than the current value of PMR will be taken. So we don't really need the redistributor to immediately see the new value of PMR as the logic in the GIC CPU interface is good enough for our goal. Thanks,
HI Julien, Thanks a lot for your reply, since I'm working on this patch in ARM (32 bits), so I have to dig into the details. Julien Thierry <julien.thierry@arm.com> 於 2018年12月17日 週一 下午5:26寫道： > > Hi Jian-Lin, > > Thanks for looking at this. > > On 16/12/2018 14:47, Jian-Lin Chen wrote: > > From: Jian-Lin Chen <lecopzer.chen@mediatek.com> > > > > > > On Wed, 12 Dec 2018 at 17:48, Julien Thierry <julien.thierry@arm.com> wrote: > >> static inline void arch_local_irq_enable(void) > >> { > >> - asm volatile( > >> - "msr daifclr, #2 // arch_local_irq_enable" > >> - : > >> + unsigned long unmasked = GIC_PRIO_IRQON; > >> + > > > > Should we need a WARN_ON() to check if the daif_I bit is masked, or > > explicitly unmasked I bit here? > > > > While I would agree, adding the WARN_ON() will add some non-negligible > overhead, especially if we need to read the daif flags to check it. > > Since these functions are called often in the whole system and using PMR > already makes things a bit slower, I'd prefer to avoid checks in here. Ok, so we have to find a better place to check it. I have no idea so far... > > > If I bit was masked and someone calls arch_local_irq_enable(), they still > > couldn't recieve any interrupt. > > > > > >> + asm volatile(ALTERNATIVE( > >> + "msr daifclr, #2 // arch_local_irq_enable\n" > >> + "nop", > >> + "msr_s " __stringify(SYS_ICC_PMR_EL1) ",%0\n" > >> + "dsb sy", > >> + ARM64_HAS_IRQ_PRIO_MASKING) > >> : > >> + : "r" (unmasked) > >> : "memory"); > >> } > >> > >> static inline void arch_local_irq_disable(void) > >> { > >> - asm volatile( > >> - "msr daifset, #2 // arch_local_irq_disable" > >> - : > >> + unsigned long masked = GIC_PRIO_IRQOFF; > >> + > >> + asm volatile(ALTERNATIVE( > >> + "msr daifset, #2 // arch_local_irq_disable", > >> + "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0", > > > > May be a "dsb sy" here? > > So, we need a "dsb sy" when unmasking interrupts because this ensures > the redistributor sees the latest PMR value and starts forwarding lower > priority interrupts again. > > When we disable interrupts however, the GIC CPU interface guarantees > that no interrupts of lower priority than the current value of PMR will > be taken. So we don't really need the redistributor to immediately see > the new value of PMR as the logic in the GIC CPU interface is good > enough for our goal. > Got it, thanks for the detail! > Thanks, > > -- > Julien Thierry
Hi Ard, On 14/12/2018 16:40, Julien Thierry wrote: > > > On 14/12/2018 15:49, Ard Biesheuvel wrote: >> On Fri, 14 Dec 2018 at 16:23, Julien Thierry <julien.thierry@arm.com> wrote: >>> >>> Hi, >>> >>> On 13/12/2018 15:03, Julien Thierry wrote: >>>> >>>> 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. >>>> >>> >> >> It might be a bit nasty, but can we put the #ifndef above in >> runtime-wrappers.c directly? The only reference in linux/efi.h is a >> macro, so that shouldn't matter afaict. >> > > Sadly, in arch/x86/platform/uv/bios_uv.c, uv_bios_call() has a reference > to the macro efi_call_virt_pointer() which wouldn't be able to see the > definition in runtime-wrappers.c > > Otherwise, we could've moved efi_call_virt_pointer() and > __efi_call_virt_pointer in runtime-wrappers.c and things would not have > been as nasty. > > But no, I don't think we can do that without breaking some x86 build :( . > Since the above does not work, would the solution with the HAVE_GENERIC_EFI_FLAGS below be acceptable to you? Or would you rather I defined helpers in <asm/efi.h> for all arm/arm64/x86? Or neither and I shall find another way? Thanks, Julien >> >>> Would the following patch be acceptable for the EFI generic side? >>> >>> If it is, I'll add it to the next iteration of this series. >>> >>> Thanks, >>> >>> Julien >>> >>> --> >>> >>> From 7acaa8e17142263addafb18ae10bd5d2d49cfb39 Mon Sep 17 00:00:00 2001 >>> From: Julien Thierry <julien.thierry@arm.com> >>> Date: Fri, 14 Dec 2018 14:20:13 +0000 >>> Subject: [RFC] efi: Let architectures decide the flags that should be >>> saved/restored >>> >>> Currently, irqflags are saved before calling runtime services and >>> checked for mismatch on return. >>> >>> Add a config option to let architectures define a set of flags to be >>> checked and (if needed) restored when coming back from runtime services. >>> This allows to use check flags that are not necesarly related to >>> irqflags. >>> >>> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >>> --- >>> arch/Kconfig | 8 ++++++++ >>> drivers/firmware/efi/runtime-wrappers.c | 4 ++-- >>> include/linux/efi.h | 12 ++++++++++-- >>> 3 files changed, 20 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/Kconfig b/arch/Kconfig >>> index e1e540f..cbec325 100644 >>> --- a/arch/Kconfig >>> +++ b/arch/Kconfig >>> @@ -695,6 +695,14 @@ config HAVE_ARCH_HASH >>> file which provides platform-specific implementations of some >>> functions in <linux/hash.h> or fs/namei.c. >>> >>> +config HAVE_GENERIC_EFI_FLAGS >>> + bool >>> + default n >>> + help >>> + Architecture defines a set of flags that EFI runtime services >>> + should take care to restore when returning to the OS. >>> + If this is not set, the set of flags defaults to the arch irqflags. >>> + >>> config ISA_BUS_API >>> def_bool ISA >>> >>> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c >>> index 8903b9c..6dafa04 100644 >>> --- a/drivers/firmware/efi/runtime-wrappers.c >>> +++ b/drivers/firmware/efi/runtime-wrappers.c >>> @@ -93,7 +93,7 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call) >>> { >>> unsigned long cur_flags, mismatch; >>> >>> - local_save_flags(cur_flags); >>> + efi_save_flags(cur_flags); >>> >>> mismatch = flags ^ cur_flags; >>> if (!WARN_ON_ONCE(mismatch & ARCH_EFI_IRQ_FLAGS_MASK)) >>> @@ -102,7 +102,7 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call) >>> add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_NOW_UNRELIABLE); >>> pr_err_ratelimited(FW_BUG "IRQ flags corrupted (0x%08lx=>0x%08lx) by EFI %s\n", >>> flags, cur_flags, call); >>> - local_irq_restore(flags); >>> + efi_restore_flags(flags); >>> } >>> >>> /* >>> diff --git a/include/linux/efi.h b/include/linux/efi.h >>> index 100ce4a..41c110a 100644 >>> --- a/include/linux/efi.h >>> +++ b/include/linux/efi.h >>> @@ -1594,6 +1594,14 @@ enum efi_secureboot_mode { >>> >>> void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table); >>> >>> +#ifdef CONFIG_HAVE_GENERIC_EFI_FLAGS >>> +#define efi_save_flags(state_flags) arch_efi_save_flags(state_flags) >>> +#define efi_restore_flags(state_flags) arch_efi_restore_flags(state_flags) >>> +#else >>> +#define efi_save_flags(state_flags) local_save_flags(state_flags) >>> +#define efi_restore_flags(state_flags) local_irq_restore(state_flags) >>> +#endif >>> + >>> /* >>> * Arch code can implement the following three template macros, avoiding >>> * reptition for the void/non-void return cases of {__,}efi_call_virt(): >>> @@ -1621,7 +1629,7 @@ enum efi_secureboot_mode { >>> \ >>> arch_efi_call_virt_setup(); \ >>> \ >>> - local_save_flags(__flags); \ >>> + efi_save_flags(__flags); \ >>> __s = arch_efi_call_virt(p, f, args); \ >>> efi_call_virt_check_flags(__flags, __stringify(f)); \ >>> \ >>> @@ -1636,7 +1644,7 @@ enum efi_secureboot_mode { >>> \ >>> arch_efi_call_virt_setup(); \ >>> \ >>> - local_save_flags(__flags); \ >>> + efi_save_flags(__flags); \ >>> arch_efi_call_virt(p, f, args); \ >>> efi_call_virt_check_flags(__flags, __stringify(f)); \ >>> \ >>> -- >>> 1.9.1 >>> >>> >>> >>> >>> -- >>> Julien Thierry >
On Wed, 19 Dec 2018 at 18:01, Julien Thierry <julien.thierry@arm.com> wrote: > > Hi Ard, > > On 14/12/2018 16:40, Julien Thierry wrote: > > > > > > On 14/12/2018 15:49, Ard Biesheuvel wrote: > >> On Fri, 14 Dec 2018 at 16:23, Julien Thierry <julien.thierry@arm.com> wrote: > >>> > >>> Hi, > >>> > >>> On 13/12/2018 15:03, Julien Thierry wrote: > >>>> > >>>> 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. > >>>> > >>> > >> > >> It might be a bit nasty, but can we put the #ifndef above in > >> runtime-wrappers.c directly? The only reference in linux/efi.h is a > >> macro, so that shouldn't matter afaict. > >> > > > > Sadly, in arch/x86/platform/uv/bios_uv.c, uv_bios_call() has a reference > > to the macro efi_call_virt_pointer() which wouldn't be able to see the > > definition in runtime-wrappers.c > > > > Otherwise, we could've moved efi_call_virt_pointer() and > > __efi_call_virt_pointer in runtime-wrappers.c and things would not have > > been as nasty. > > > > But no, I don't think we can do that without breaking some x86 build :( . > > > > Since the above does not work, would the solution with the > HAVE_GENERIC_EFI_FLAGS below be acceptable to you? Or would you rather I > defined helpers in <asm/efi.h> for all arm/arm64/x86? > > Or neither and I shall find another way? > Would it be possible to introduce a function efi_call_virt_save_flags() [as a counterpart to efi_call_virt_check_flags()], and put the implementation in runtime-wrappers.c (which already includes asm/efi.h)? That should allow you to put arch-specific hooks in asm/efi.h, and use them in the implementation of efi_call_virt_save_flags(). AFAICT, that removes the need for Kconfig glue.
On 20/12/2018 17:53, Ard Biesheuvel wrote: > On Wed, 19 Dec 2018 at 18:01, Julien Thierry <julien.thierry@arm.com> wrote: >> >> Hi Ard, >> >> On 14/12/2018 16:40, Julien Thierry wrote: >>> >>> >>> On 14/12/2018 15:49, Ard Biesheuvel wrote: >>>> On Fri, 14 Dec 2018 at 16:23, Julien Thierry <julien.thierry@arm.com> wrote: >>>>> >>>>> Hi, >>>>> >>>>> On 13/12/2018 15:03, Julien Thierry wrote: >>>>>> >>>>>> 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. >>>>>> >>>>> >>>> >>>> It might be a bit nasty, but can we put the #ifndef above in >>>> runtime-wrappers.c directly? The only reference in linux/efi.h is a >>>> macro, so that shouldn't matter afaict. >>>> >>> >>> Sadly, in arch/x86/platform/uv/bios_uv.c, uv_bios_call() has a reference >>> to the macro efi_call_virt_pointer() which wouldn't be able to see the >>> definition in runtime-wrappers.c >>> >>> Otherwise, we could've moved efi_call_virt_pointer() and >>> __efi_call_virt_pointer in runtime-wrappers.c and things would not have >>> been as nasty. >>> >>> But no, I don't think we can do that without breaking some x86 build :( . >>> >> >> Since the above does not work, would the solution with the >> HAVE_GENERIC_EFI_FLAGS below be acceptable to you? Or would you rather I >> defined helpers in <asm/efi.h> for all arm/arm64/x86? >> >> Or neither and I shall find another way? >> > > Would it be possible to introduce a function > > efi_call_virt_save_flags() > > [as a counterpart to efi_call_virt_check_flags()], and put the > implementation in runtime-wrappers.c (which already includes > asm/efi.h)? > > That should allow you to put arch-specific hooks in asm/efi.h, and use > them in the implementation of efi_call_virt_save_flags(). AFAICT, that > removes the need for Kconfig glue. > Yes, that seems to work and does not break other arch. I'll go with that in my next version of the patches. Thanks,
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)) /* arch specific definitions used by the stub code */ diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h index 24692ed..fa3b06f 100644 --- a/arch/arm64/include/asm/irqflags.h +++ b/arch/arm64/include/asm/irqflags.h @@ -18,7 +18,9 @@ #ifdef __KERNEL__ +#include <asm/alternative.h> #include <asm/ptrace.h> +#include <asm/sysreg.h> /* * Aarch64 has flags for masking: Debug, Asynchronous (serror), Interrupts and @@ -36,47 +38,96 @@ /* * CPU interrupt mask handling. */ -static inline unsigned long arch_local_irq_save(void) -{ - unsigned long flags; - asm volatile( - "mrs %0, daif // arch_local_irq_save\n" - "msr daifset, #2" - : "=r" (flags) - : - : "memory"); - return flags; -} - static inline void arch_local_irq_enable(void) { - asm volatile( - "msr daifclr, #2 // arch_local_irq_enable" - : + unsigned long unmasked = GIC_PRIO_IRQON; + + asm volatile(ALTERNATIVE( + "msr daifclr, #2 // arch_local_irq_enable\n" + "nop", + "msr_s " __stringify(SYS_ICC_PMR_EL1) ",%0\n" + "dsb sy", + ARM64_HAS_IRQ_PRIO_MASKING) : + : "r" (unmasked) : "memory"); } static inline void arch_local_irq_disable(void) { - asm volatile( - "msr daifset, #2 // arch_local_irq_disable" - : + unsigned long masked = GIC_PRIO_IRQOFF; + + asm volatile(ALTERNATIVE( + "msr daifset, #2 // arch_local_irq_disable", + "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0", + ARM64_HAS_IRQ_PRIO_MASKING) : + : "r" (masked) : "memory"); } /* + * Having two ways to control interrupt status is a bit complicated. Some + * locations like exception entries will have PSR.I bit set by the architecture + * while PMR is unmasked. + * We need the irqflags to represent that interrupts are disabled in such cases. + * + * For this, we lower the value read from PMR when the I bit is set so it is + * considered as an irq masking priority. (With PMR, lower value means masking + * more interrupts). + */ +#define _get_irqflags(daif_bits, pmr) \ +({ \ + unsigned long flags; \ + \ + BUILD_BUG_ON(GIC_PRIO_IRQOFF < (GIC_PRIO_IRQON & ~PSR_I_BIT)); \ + asm volatile(ALTERNATIVE( \ + "mov %0, %1\n" \ + "nop\n" \ + "nop", \ + "and %0, %1, #" __stringify(PSR_I_BIT) "\n" \ + "mvn %0, %0\n" \ + "and %0, %0, %2", \ + ARM64_HAS_IRQ_PRIO_MASKING) \ + : "=&r" (flags) \ + : "r" (daif_bits), "r" (pmr) \ + : "memory"); \ + \ + flags; \ +}) + +/* * Save the current interrupt enable state. */ static inline unsigned long arch_local_save_flags(void) { - unsigned long flags; - asm volatile( - "mrs %0, daif // arch_local_save_flags" - : "=r" (flags) + unsigned long daif_bits; + unsigned long pmr; // Only used if alternative is on + + daif_bits = read_sysreg(daif); + + // Get PMR + asm volatile(ALTERNATIVE( + "nop", + "mrs_s %0, " __stringify(SYS_ICC_PMR_EL1), + ARM64_HAS_IRQ_PRIO_MASKING) + : "=&r" (pmr) : : "memory"); + + return _get_irqflags(daif_bits, pmr); +} + +#undef _get_irqflags + +static inline unsigned long arch_local_irq_save(void) +{ + unsigned long flags; + + flags = arch_local_save_flags(); + + arch_local_irq_disable(); + return flags; } @@ -85,16 +136,32 @@ static inline unsigned long arch_local_save_flags(void) */ static inline void arch_local_irq_restore(unsigned long flags) { - asm volatile( - "msr daif, %0 // arch_local_irq_restore" - : - : "r" (flags) - : "memory"); + asm volatile(ALTERNATIVE( + "msr daif, %0\n" + "nop", + "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0\n" + "dsb sy", + ARM64_HAS_IRQ_PRIO_MASKING) + : "+r" (flags) + : + : "memory"); } static inline int arch_irqs_disabled_flags(unsigned long flags) { - return flags & PSR_I_BIT; + int res; + + asm volatile(ALTERNATIVE( + "and %w0, %w1, #" __stringify(PSR_I_BIT) "\n" + "nop", + "cmp %w1, #" __stringify(GIC_PRIO_IRQOFF) "\n" + "cset %w0, ls", + ARM64_HAS_IRQ_PRIO_MASKING) + : "=&r" (res) + : "r" ((int) flags) + : "memory"); + + return res; } #endif #endif
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(-)