From: Julien Thierry <julien.thierry@arm.com> To: Dave Martin <Dave.Martin@arm.com> Cc: linux-arm-kernel@lists.infradead.org, mark.rutland@arm.com, daniel.thompson@linaro.org, marc.zyngier@arm.com, catalin.marinas@arm.com, Ard Biesheuvel <ard.biesheuvel@linaro.org>, will.deacon@arm.com, linux-kernel@vger.kernel.org, christoffer.dall@arm.com, james.morse@arm.com, Oleg Nesterov <oleg@redhat.com>, joel@joelfernandes.org Subject: Re: [PATCH v8 12/26] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking Date: Fri, 18 Jan 2019 17:27:29 +0000 [thread overview] Message-ID: <2bdc47ca-5aa0-24a2-b7fe-7b59158698e2@arm.com> (raw) In-Reply-To: <20190118163556.GB3578@e103592.cambridge.arm.com> On 18/01/2019 16:35, Dave Martin wrote: > On Tue, Jan 08, 2019 at 02:07:30PM +0000, Julien Thierry 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 | 11 ++++ >> arch/arm64/include/asm/irqflags.h | 123 +++++++++++++++++++++++++++++--------- >> 2 files changed, 106 insertions(+), 28 deletions(-) >> >> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h >> index 7ed3208..134ff6e 100644 >> --- a/arch/arm64/include/asm/efi.h >> +++ b/arch/arm64/include/asm/efi.h >> @@ -44,6 +44,17 @@ >> >> #define ARCH_EFI_IRQ_FLAGS_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT) >> >> +#define arch_efi_save_flags(state_flags) \ >> + do { \ >> + (state_flags) = read_sysreg(daif); \ >> + } while (0) >> + >> +#define arch_efi_restore_flags(state_flags) \ >> + do { \ >> + write_sysreg(state_flags, daif); \ >> + } while (0) >> + >> + > > Randomly commenting a few minor nits as I glance down my mailbox... > > There's no need to protect single statements with do { } while(0). > > Just protect an expression statement that could be misparsed with ( ). > > -> > > #define arch_efi_save_flags(state_flags) ((state_flags) = read_sysreg(daif)) For the efi_save_flags(), I wanted to avoid it getting used as an expression. Would casting the assignment expression to (void) be acceptable? > #define arch_efi_restore_flags(state_flags) write_sysreg(state_flags, daif) For this one, write_sysreg() is already a statement, so yes, I definitely don't need a do { } while (0) here. > > [...] > >> 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 @@ > > [...] > >> /* >> + * 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; \ >> +}) > > Nit: does this need to be a macro? > > ({ ... }) is mildly gross and it's preferable to avoid it if the code > works just as well without... > > pmr would need to be passed as a pointer, with "r" (*pmr) in the asm, > but I think it would compile down to precisely the same code. > The only motivation for it to be a macro was to be able to #undef it after its use. But with Catalin's suggestion, looks like we can makes things simple and avoid having a separate macro/function. >> + >> +/* >> * 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) > > Why earlyclobber? >>> : >> : "memory"); > > [...] > >> @@ -85,16 +136,32 @@ static inline unsigned long arch_local_save_flags(void) > > [...] > >> 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) > > Why earlyclobber? %0 is not written before the reading of any input > argument so far as I can see, in either alternative. > I didn't really understand what the earlyclobber semantic was, thanks for explaining it. Thanks, -- Julien Thierry
WARNING: multiple messages have this Message-ID (diff)
From: Julien Thierry <julien.thierry@arm.com> To: Dave Martin <Dave.Martin@arm.com> Cc: mark.rutland@arm.com, daniel.thompson@linaro.org, Ard Biesheuvel <ard.biesheuvel@linaro.org>, marc.zyngier@arm.com, catalin.marinas@arm.com, will.deacon@arm.com, linux-kernel@vger.kernel.org, christoffer.dall@arm.com, james.morse@arm.com, Oleg Nesterov <oleg@redhat.com>, joel@joelfernandes.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v8 12/26] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking Date: Fri, 18 Jan 2019 17:27:29 +0000 [thread overview] Message-ID: <2bdc47ca-5aa0-24a2-b7fe-7b59158698e2@arm.com> (raw) In-Reply-To: <20190118163556.GB3578@e103592.cambridge.arm.com> On 18/01/2019 16:35, Dave Martin wrote: > On Tue, Jan 08, 2019 at 02:07:30PM +0000, Julien Thierry 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 | 11 ++++ >> arch/arm64/include/asm/irqflags.h | 123 +++++++++++++++++++++++++++++--------- >> 2 files changed, 106 insertions(+), 28 deletions(-) >> >> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h >> index 7ed3208..134ff6e 100644 >> --- a/arch/arm64/include/asm/efi.h >> +++ b/arch/arm64/include/asm/efi.h >> @@ -44,6 +44,17 @@ >> >> #define ARCH_EFI_IRQ_FLAGS_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT) >> >> +#define arch_efi_save_flags(state_flags) \ >> + do { \ >> + (state_flags) = read_sysreg(daif); \ >> + } while (0) >> + >> +#define arch_efi_restore_flags(state_flags) \ >> + do { \ >> + write_sysreg(state_flags, daif); \ >> + } while (0) >> + >> + > > Randomly commenting a few minor nits as I glance down my mailbox... > > There's no need to protect single statements with do { } while(0). > > Just protect an expression statement that could be misparsed with ( ). > > -> > > #define arch_efi_save_flags(state_flags) ((state_flags) = read_sysreg(daif)) For the efi_save_flags(), I wanted to avoid it getting used as an expression. Would casting the assignment expression to (void) be acceptable? > #define arch_efi_restore_flags(state_flags) write_sysreg(state_flags, daif) For this one, write_sysreg() is already a statement, so yes, I definitely don't need a do { } while (0) here. > > [...] > >> 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 @@ > > [...] > >> /* >> + * 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; \ >> +}) > > Nit: does this need to be a macro? > > ({ ... }) is mildly gross and it's preferable to avoid it if the code > works just as well without... > > pmr would need to be passed as a pointer, with "r" (*pmr) in the asm, > but I think it would compile down to precisely the same code. > The only motivation for it to be a macro was to be able to #undef it after its use. But with Catalin's suggestion, looks like we can makes things simple and avoid having a separate macro/function. >> + >> +/* >> * 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) > > Why earlyclobber? >>> : >> : "memory"); > > [...] > >> @@ -85,16 +136,32 @@ static inline unsigned long arch_local_save_flags(void) > > [...] > >> 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) > > Why earlyclobber? %0 is not written before the reading of any input > argument so far as I can see, in either alternative. > I didn't really understand what the earlyclobber semantic was, thanks for explaining it. Thanks, -- Julien Thierry _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-01-18 17:27 UTC|newest] Thread overview: 117+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-01-08 14:07 [PATCH v8 00/26] arm64: provide pseudo NMI with GICv3 Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 01/26] arm64: Fix HCR.TGE status for NMI contexts Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-14 15:56 ` Catalin Marinas 2019-01-14 15:56 ` Catalin Marinas 2019-01-14 16:12 ` Julien Thierry 2019-01-14 16:12 ` Julien Thierry 2019-01-14 17:25 ` James Morse 2019-01-14 17:25 ` James Morse 2019-01-16 13:35 ` Sasha Levin 2019-01-16 13:35 ` Sasha Levin 2019-01-28 9:16 ` Marc Zyngier 2019-01-28 9:16 ` Marc Zyngier 2019-01-28 9:16 ` Marc Zyngier 2019-01-08 14:07 ` [PATCH v8 02/26] arm64: Remove unused daif related functions/macros Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 03/26] arm64: cpufeature: Set SYSREG_GIC_CPUIF as a boot system feature Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 04/26] arm64: cpufeature: Add cpufeature for IRQ priority masking Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 05/26] arm/arm64: gic-v3: Add PMR and RPR accessors Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 06/26] irqchip/gic-v3: Switch to PMR masking before calling IRQ handler Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 07/26] arm64: ptrace: Provide definitions for PMR values Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-14 16:12 ` Catalin Marinas 2019-01-14 16:12 ` Catalin Marinas 2019-01-08 14:07 ` [PATCH v8 08/26] arm64: Make PMR part of task context Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-18 16:10 ` Catalin Marinas 2019-01-18 16:10 ` Catalin Marinas 2019-01-08 14:07 ` [PATCH v8 09/26] arm64: Unmask PMR before going idle Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-18 16:23 ` Catalin Marinas 2019-01-18 16:23 ` Catalin Marinas 2019-01-18 17:17 ` Julien Thierry 2019-01-18 17:17 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 10/26] arm64: kvm: Unmask PMR before entering guest Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-18 16:25 ` Catalin Marinas 2019-01-18 16:25 ` Catalin Marinas 2019-01-08 14:07 ` [PATCH v8 11/26] efi: Let architectures decide the flags that should be saved/restored Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-18 16:26 ` Catalin Marinas 2019-01-18 16:26 ` Catalin Marinas 2019-01-08 14:07 ` [PATCH v8 12/26] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 15:40 ` Dave Martin 2019-01-08 15:40 ` Dave Martin 2019-01-08 15:51 ` Marc Zyngier 2019-01-08 15:51 ` Marc Zyngier 2019-01-08 16:45 ` Dave Martin 2019-01-08 16:45 ` Dave Martin 2019-01-08 17:16 ` Marc Zyngier 2019-01-08 17:16 ` Marc Zyngier 2019-01-08 18:01 ` Dave Martin 2019-01-08 18:01 ` Dave Martin 2019-01-08 17:58 ` Julien Thierry 2019-01-08 17:58 ` Julien Thierry 2019-01-08 18:37 ` Dave Martin 2019-01-08 18:37 ` Dave Martin 2019-01-18 16:09 ` Catalin Marinas 2019-01-18 16:09 ` Catalin Marinas 2019-01-18 16:57 ` Julien Thierry 2019-01-18 16:57 ` Julien Thierry 2019-01-18 17:30 ` Catalin Marinas 2019-01-18 17:30 ` Catalin Marinas 2019-01-18 17:33 ` Catalin Marinas 2019-01-18 17:33 ` Catalin Marinas 2019-01-21 8:45 ` Julien Thierry 2019-01-21 8:45 ` Julien Thierry 2019-01-18 16:35 ` Dave Martin 2019-01-18 16:35 ` Dave Martin 2019-01-18 17:27 ` Julien Thierry [this message] 2019-01-18 17:27 ` Julien Thierry 2019-01-18 18:23 ` Dave Martin 2019-01-18 18:23 ` Dave Martin 2019-01-08 14:07 ` [PATCH v8 13/26] arm64: daifflags: Include PMR in daifflags restore operations Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-18 16:43 ` Catalin Marinas 2019-01-18 16:43 ` Catalin Marinas 2019-01-08 14:07 ` [PATCH v8 14/26] arm64: alternative: Allow alternative status checking per cpufeature Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 15/26] arm64: alternative: Apply alternatives early in boot process Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:51 ` Suzuki K Poulose 2019-01-08 14:51 ` Suzuki K Poulose 2019-01-08 15:20 ` Julien Thierry 2019-01-08 15:20 ` Julien Thierry 2019-01-08 17:40 ` Suzuki K Poulose 2019-01-08 17:40 ` Suzuki K Poulose 2019-01-10 10:50 ` Julien Thierry 2019-01-10 10:50 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 16/26] irqchip/gic-v3: Factor group0 detection into functions Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 17/26] arm64: Switch to PMR masking when starting CPUs Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 18/26] arm64: gic-v3: Implement arch support for priority masking Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 19/26] irqchip/gic-v3: Detect if GIC can support pseudo-NMIs Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 20/26] irqchip/gic-v3: Handle pseudo-NMIs Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 21/26] irqchip/gic: Add functions to access irq priorities Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 22/26] irqchip/gic-v3: Allow interrupts to be set as pseudo-NMI Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 23/26] arm64: Handle serror in NMI context Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 24/26] arm64: Skip preemption when exiting an NMI Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 25/26] arm64: Skip irqflags tracing for NMI in IRQs disabled context Julien Thierry 2019-01-08 14:07 ` Julien Thierry 2019-01-08 14:07 ` [PATCH v8 26/26] arm64: Enable the support of pseudo-NMIs Julien Thierry 2019-01-08 14:07 ` Julien Thierry
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=2bdc47ca-5aa0-24a2-b7fe-7b59158698e2@arm.com \ --to=julien.thierry@arm.com \ --cc=Dave.Martin@arm.com \ --cc=ard.biesheuvel@linaro.org \ --cc=catalin.marinas@arm.com \ --cc=christoffer.dall@arm.com \ --cc=daniel.thompson@linaro.org \ --cc=james.morse@arm.com \ --cc=joel@joelfernandes.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=marc.zyngier@arm.com \ --cc=mark.rutland@arm.com \ --cc=oleg@redhat.com \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.