* [PATCH v3 0/2] arm64: Relax ICC_PMR_EL1 synchronisation when possible @ 2019-10-02 9:06 ` Marc Zyngier 0 siblings, 0 replies; 14+ messages in thread From: Marc Zyngier @ 2019-10-02 9:06 UTC (permalink / raw) To: Will Deacon, Catalin Marinas Cc: linux-arm-kernel, Suzuki K Poulose, linux-kernel, James Morse, uohanjun, huawei.libin, liwei391, Julien Thierry This is a very late update on [1], fixing the 32bit compilation issue that was present in v2, and adding an extra message in the kernel log to find out what is going on. [1] http://lore.kernel.org/r/20190802125208.73162-1-maz@kernel.org Marc Zyngier (2): arm64: Relax ICC_PMR_EL1 accesses when ICC_CTLR_EL1.PMHE is clear arm64: Document ICC_CTLR_EL3.PMHE setting requirements Documentation/arm64/booting.rst | 3 +++ arch/arm64/include/asm/barrier.h | 12 ++++++++++++ arch/arm64/include/asm/daifflags.h | 3 ++- arch/arm64/include/asm/irqflags.h | 19 ++++++++++--------- arch/arm64/include/asm/kvm_host.h | 3 +-- arch/arm64/kernel/entry.S | 6 ++++-- arch/arm64/kvm/hyp/switch.c | 4 ++-- drivers/irqchip/irq-gic-v3.c | 20 ++++++++++++++++++++ include/linux/irqchip/arm-gic-v3.h | 2 ++ 9 files changed, 56 insertions(+), 16 deletions(-) -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 0/2] arm64: Relax ICC_PMR_EL1 synchronisation when possible @ 2019-10-02 9:06 ` Marc Zyngier 0 siblings, 0 replies; 14+ messages in thread From: Marc Zyngier @ 2019-10-02 9:06 UTC (permalink / raw) To: Will Deacon, Catalin Marinas Cc: Suzuki K Poulose, James Morse, Julien Thierry, huawei.libin, uohanjun, liwei391, linux-arm-kernel, linux-kernel This is a very late update on [1], fixing the 32bit compilation issue that was present in v2, and adding an extra message in the kernel log to find out what is going on. [1] http://lore.kernel.org/r/20190802125208.73162-1-maz@kernel.org Marc Zyngier (2): arm64: Relax ICC_PMR_EL1 accesses when ICC_CTLR_EL1.PMHE is clear arm64: Document ICC_CTLR_EL3.PMHE setting requirements Documentation/arm64/booting.rst | 3 +++ arch/arm64/include/asm/barrier.h | 12 ++++++++++++ arch/arm64/include/asm/daifflags.h | 3 ++- arch/arm64/include/asm/irqflags.h | 19 ++++++++++--------- arch/arm64/include/asm/kvm_host.h | 3 +-- arch/arm64/kernel/entry.S | 6 ++++-- arch/arm64/kvm/hyp/switch.c | 4 ++-- drivers/irqchip/irq-gic-v3.c | 20 ++++++++++++++++++++ include/linux/irqchip/arm-gic-v3.h | 2 ++ 9 files changed, 56 insertions(+), 16 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 1/2] arm64: Relax ICC_PMR_EL1 accesses when ICC_CTLR_EL1.PMHE is clear 2019-10-02 9:06 ` Marc Zyngier @ 2019-10-02 9:06 ` Marc Zyngier -1 siblings, 0 replies; 14+ messages in thread From: Marc Zyngier @ 2019-10-02 9:06 UTC (permalink / raw) To: Will Deacon, Catalin Marinas Cc: linux-arm-kernel, Suzuki K Poulose, linux-kernel, James Morse, uohanjun, huawei.libin, liwei391, Julien Thierry The GICv3 architecture specification is incredibly misleading when it comes to PMR and the requirement for a DSB. It turns out that this DSB is only required if the CPU interface sends an Upstream Control message to the redistributor in order to update the RD's view of PMR. This message is only sent when ICC_CTLR_EL1.PMHE is set, which isn't the case in Linux. It can still be set from EL3, so some special care is required. But the upshot is that in the (hopefuly large) majority of the cases, we can drop the DSB altogether. This relies on a new static key being set if the boot CPU has PMHE set. The drawback is that this static key has to be exported to modules. Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: James Morse <james.morse@arm.com> Cc: Julien Thierry <julien.thierry.kdev@gmail.com> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/include/asm/barrier.h | 12 ++++++++++++ arch/arm64/include/asm/daifflags.h | 3 ++- arch/arm64/include/asm/irqflags.h | 19 ++++++++++--------- arch/arm64/include/asm/kvm_host.h | 3 +-- arch/arm64/kernel/entry.S | 6 ++++-- arch/arm64/kvm/hyp/switch.c | 4 ++-- drivers/irqchip/irq-gic-v3.c | 20 ++++++++++++++++++++ include/linux/irqchip/arm-gic-v3.h | 2 ++ 8 files changed, 53 insertions(+), 16 deletions(-) diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h index e0e2b1946f42..7d9cc5ec4971 100644 --- a/arch/arm64/include/asm/barrier.h +++ b/arch/arm64/include/asm/barrier.h @@ -29,6 +29,18 @@ SB_BARRIER_INSN"nop\n", \ ARM64_HAS_SB)) +#ifdef CONFIG_ARM64_PSEUDO_NMI +#define pmr_sync() \ + do { \ + extern struct static_key_false gic_pmr_sync; \ + \ + if (static_branch_unlikely(&gic_pmr_sync)) \ + dsb(sy); \ + } while(0) +#else +#define pmr_sync() do {} while (0) +#endif + #define mb() dsb(sy) #define rmb() dsb(ld) #define wmb() dsb(st) diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h index 063c964af705..53cd5fab79a8 100644 --- a/arch/arm64/include/asm/daifflags.h +++ b/arch/arm64/include/asm/daifflags.h @@ -8,6 +8,7 @@ #include <linux/irqflags.h> #include <asm/arch_gicv3.h> +#include <asm/barrier.h> #include <asm/cpufeature.h> #define DAIF_PROCCTX 0 @@ -65,7 +66,7 @@ static inline void local_daif_restore(unsigned long flags) if (system_uses_irq_prio_masking()) { gic_write_pmr(GIC_PRIO_IRQON); - dsb(sy); + pmr_sync(); } } else if (system_uses_irq_prio_masking()) { u64 pmr; diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h index 1a59f0ed1ae3..aa4b6521ef14 100644 --- a/arch/arm64/include/asm/irqflags.h +++ b/arch/arm64/include/asm/irqflags.h @@ -6,6 +6,7 @@ #define __ASM_IRQFLAGS_H #include <asm/alternative.h> +#include <asm/barrier.h> #include <asm/ptrace.h> #include <asm/sysreg.h> @@ -34,14 +35,14 @@ static inline void arch_local_irq_enable(void) } asm volatile(ALTERNATIVE( - "msr daifclr, #2 // arch_local_irq_enable\n" - "nop", - __msr_s(SYS_ICC_PMR_EL1, "%0") - "dsb sy", + "msr daifclr, #2 // arch_local_irq_enable", + __msr_s(SYS_ICC_PMR_EL1, "%0"), ARM64_HAS_IRQ_PRIO_MASKING) : : "r" ((unsigned long) GIC_PRIO_IRQON) : "memory"); + + pmr_sync(); } static inline void arch_local_irq_disable(void) @@ -116,14 +117,14 @@ static inline unsigned long arch_local_irq_save(void) static inline void arch_local_irq_restore(unsigned long flags) { asm volatile(ALTERNATIVE( - "msr daif, %0\n" - "nop", - __msr_s(SYS_ICC_PMR_EL1, "%0") - "dsb sy", - ARM64_HAS_IRQ_PRIO_MASKING) + "msr daif, %0", + __msr_s(SYS_ICC_PMR_EL1, "%0"), + ARM64_HAS_IRQ_PRIO_MASKING) : : "r" (flags) : "memory"); + + pmr_sync(); } #endif /* __ASM_IRQFLAGS_H */ diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index f656169db8c3..5ecb091c8576 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -600,8 +600,7 @@ static inline void kvm_arm_vhe_guest_enter(void) * local_daif_mask() already sets GIC_PRIO_PSR_I_SET, we just need a * dsb to ensure the redistributor is forwards EL2 IRQs to the CPU. */ - if (system_uses_irq_prio_masking()) - dsb(sy); + pmr_sync(); } static inline void kvm_arm_vhe_guest_exit(void) diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 84a822748c84..3e0eb4f5253b 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -269,8 +269,10 @@ alternative_else_nop_endif alternative_if ARM64_HAS_IRQ_PRIO_MASKING ldr x20, [sp, #S_PMR_SAVE] msr_s SYS_ICC_PMR_EL1, x20 - /* Ensure priority change is seen by redistributor */ - dsb sy + mrs_s x21, SYS_ICC_CTLR_EL1 + tbz x21, #6, .L__skip_pmr_sync\@ // Check for ICC_CTLR_EL1.PMHE + dsb sy // Ensure priority change is seen by redistributor +.L__skip_pmr_sync\@: alternative_else_nop_endif ldp x21, x22, [sp, #S_PC] // load ELR, SPSR diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c index bd978ad71936..85832c5bf9f5 100644 --- a/arch/arm64/kvm/hyp/switch.c +++ b/arch/arm64/kvm/hyp/switch.c @@ -12,7 +12,7 @@ #include <kvm/arm_psci.h> -#include <asm/arch_gicv3.h> +#include <asm/barrier.h> #include <asm/cpufeature.h> #include <asm/kprobes.h> #include <asm/kvm_asm.h> @@ -605,7 +605,7 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu) */ if (system_uses_irq_prio_masking()) { gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET); - dsb(sy); + pmr_sync(); } vcpu = kern_hyp_va(vcpu); diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index 422664ac5f53..0abc5a13adaa 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -87,6 +87,15 @@ static DEFINE_STATIC_KEY_TRUE(supports_deactivate_key); */ static DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis); +/* + * Global static key controlling whether an update to PMR allowing more + * interrupts requires to be propagated to the redistributor (DSB SY). + * And this needs to be exported for modules to be able to enable + * interrupts... + */ +DEFINE_STATIC_KEY_FALSE(gic_pmr_sync); +EXPORT_SYMBOL(gic_pmr_sync); + /* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */ static refcount_t *ppi_nmi_refs; @@ -1502,6 +1511,17 @@ static void gic_enable_nmi_support(void) for (i = 0; i < gic_data.ppi_nr; i++) refcount_set(&ppi_nmi_refs[i], 0); + /* + * Linux itself doesn't use 1:N distribution, so has no need to + * set PMHE. The only reason to have it set is if EL3 requires it + * (and we can't change it). + */ + if (gic_read_ctlr() & ICC_CTLR_EL1_PMHE_MASK) + static_branch_enable(&gic_pmr_sync); + + pr_info("%s ICC_PMR_EL1 synchronisation\n", + static_branch_unlikely(&gic_pmr_sync) ? "Forcing" : "Relaxing"); + static_branch_enable(&supports_pseudo_nmis); if (static_branch_likely(&supports_deactivate_key)) diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h index 5cc10cf7cb3e..a0bde9e12efa 100644 --- a/include/linux/irqchip/arm-gic-v3.h +++ b/include/linux/irqchip/arm-gic-v3.h @@ -487,6 +487,8 @@ #define ICC_CTLR_EL1_EOImode_MASK (1 << ICC_CTLR_EL1_EOImode_SHIFT) #define ICC_CTLR_EL1_CBPR_SHIFT 0 #define ICC_CTLR_EL1_CBPR_MASK (1 << ICC_CTLR_EL1_CBPR_SHIFT) +#define ICC_CTLR_EL1_PMHE_SHIFT 6 +#define ICC_CTLR_EL1_PMHE_MASK (1 << ICC_CTLR_EL1_PMHE_SHIFT) #define ICC_CTLR_EL1_PRI_BITS_SHIFT 8 #define ICC_CTLR_EL1_PRI_BITS_MASK (0x7 << ICC_CTLR_EL1_PRI_BITS_SHIFT) #define ICC_CTLR_EL1_ID_BITS_SHIFT 11 -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 1/2] arm64: Relax ICC_PMR_EL1 accesses when ICC_CTLR_EL1.PMHE is clear @ 2019-10-02 9:06 ` Marc Zyngier 0 siblings, 0 replies; 14+ messages in thread From: Marc Zyngier @ 2019-10-02 9:06 UTC (permalink / raw) To: Will Deacon, Catalin Marinas Cc: Suzuki K Poulose, James Morse, Julien Thierry, huawei.libin, uohanjun, liwei391, linux-arm-kernel, linux-kernel The GICv3 architecture specification is incredibly misleading when it comes to PMR and the requirement for a DSB. It turns out that this DSB is only required if the CPU interface sends an Upstream Control message to the redistributor in order to update the RD's view of PMR. This message is only sent when ICC_CTLR_EL1.PMHE is set, which isn't the case in Linux. It can still be set from EL3, so some special care is required. But the upshot is that in the (hopefuly large) majority of the cases, we can drop the DSB altogether. This relies on a new static key being set if the boot CPU has PMHE set. The drawback is that this static key has to be exported to modules. Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: James Morse <james.morse@arm.com> Cc: Julien Thierry <julien.thierry.kdev@gmail.com> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/include/asm/barrier.h | 12 ++++++++++++ arch/arm64/include/asm/daifflags.h | 3 ++- arch/arm64/include/asm/irqflags.h | 19 ++++++++++--------- arch/arm64/include/asm/kvm_host.h | 3 +-- arch/arm64/kernel/entry.S | 6 ++++-- arch/arm64/kvm/hyp/switch.c | 4 ++-- drivers/irqchip/irq-gic-v3.c | 20 ++++++++++++++++++++ include/linux/irqchip/arm-gic-v3.h | 2 ++ 8 files changed, 53 insertions(+), 16 deletions(-) diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h index e0e2b1946f42..7d9cc5ec4971 100644 --- a/arch/arm64/include/asm/barrier.h +++ b/arch/arm64/include/asm/barrier.h @@ -29,6 +29,18 @@ SB_BARRIER_INSN"nop\n", \ ARM64_HAS_SB)) +#ifdef CONFIG_ARM64_PSEUDO_NMI +#define pmr_sync() \ + do { \ + extern struct static_key_false gic_pmr_sync; \ + \ + if (static_branch_unlikely(&gic_pmr_sync)) \ + dsb(sy); \ + } while(0) +#else +#define pmr_sync() do {} while (0) +#endif + #define mb() dsb(sy) #define rmb() dsb(ld) #define wmb() dsb(st) diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h index 063c964af705..53cd5fab79a8 100644 --- a/arch/arm64/include/asm/daifflags.h +++ b/arch/arm64/include/asm/daifflags.h @@ -8,6 +8,7 @@ #include <linux/irqflags.h> #include <asm/arch_gicv3.h> +#include <asm/barrier.h> #include <asm/cpufeature.h> #define DAIF_PROCCTX 0 @@ -65,7 +66,7 @@ static inline void local_daif_restore(unsigned long flags) if (system_uses_irq_prio_masking()) { gic_write_pmr(GIC_PRIO_IRQON); - dsb(sy); + pmr_sync(); } } else if (system_uses_irq_prio_masking()) { u64 pmr; diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h index 1a59f0ed1ae3..aa4b6521ef14 100644 --- a/arch/arm64/include/asm/irqflags.h +++ b/arch/arm64/include/asm/irqflags.h @@ -6,6 +6,7 @@ #define __ASM_IRQFLAGS_H #include <asm/alternative.h> +#include <asm/barrier.h> #include <asm/ptrace.h> #include <asm/sysreg.h> @@ -34,14 +35,14 @@ static inline void arch_local_irq_enable(void) } asm volatile(ALTERNATIVE( - "msr daifclr, #2 // arch_local_irq_enable\n" - "nop", - __msr_s(SYS_ICC_PMR_EL1, "%0") - "dsb sy", + "msr daifclr, #2 // arch_local_irq_enable", + __msr_s(SYS_ICC_PMR_EL1, "%0"), ARM64_HAS_IRQ_PRIO_MASKING) : : "r" ((unsigned long) GIC_PRIO_IRQON) : "memory"); + + pmr_sync(); } static inline void arch_local_irq_disable(void) @@ -116,14 +117,14 @@ static inline unsigned long arch_local_irq_save(void) static inline void arch_local_irq_restore(unsigned long flags) { asm volatile(ALTERNATIVE( - "msr daif, %0\n" - "nop", - __msr_s(SYS_ICC_PMR_EL1, "%0") - "dsb sy", - ARM64_HAS_IRQ_PRIO_MASKING) + "msr daif, %0", + __msr_s(SYS_ICC_PMR_EL1, "%0"), + ARM64_HAS_IRQ_PRIO_MASKING) : : "r" (flags) : "memory"); + + pmr_sync(); } #endif /* __ASM_IRQFLAGS_H */ diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index f656169db8c3..5ecb091c8576 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -600,8 +600,7 @@ static inline void kvm_arm_vhe_guest_enter(void) * local_daif_mask() already sets GIC_PRIO_PSR_I_SET, we just need a * dsb to ensure the redistributor is forwards EL2 IRQs to the CPU. */ - if (system_uses_irq_prio_masking()) - dsb(sy); + pmr_sync(); } static inline void kvm_arm_vhe_guest_exit(void) diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 84a822748c84..3e0eb4f5253b 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -269,8 +269,10 @@ alternative_else_nop_endif alternative_if ARM64_HAS_IRQ_PRIO_MASKING ldr x20, [sp, #S_PMR_SAVE] msr_s SYS_ICC_PMR_EL1, x20 - /* Ensure priority change is seen by redistributor */ - dsb sy + mrs_s x21, SYS_ICC_CTLR_EL1 + tbz x21, #6, .L__skip_pmr_sync\@ // Check for ICC_CTLR_EL1.PMHE + dsb sy // Ensure priority change is seen by redistributor +.L__skip_pmr_sync\@: alternative_else_nop_endif ldp x21, x22, [sp, #S_PC] // load ELR, SPSR diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c index bd978ad71936..85832c5bf9f5 100644 --- a/arch/arm64/kvm/hyp/switch.c +++ b/arch/arm64/kvm/hyp/switch.c @@ -12,7 +12,7 @@ #include <kvm/arm_psci.h> -#include <asm/arch_gicv3.h> +#include <asm/barrier.h> #include <asm/cpufeature.h> #include <asm/kprobes.h> #include <asm/kvm_asm.h> @@ -605,7 +605,7 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu) */ if (system_uses_irq_prio_masking()) { gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET); - dsb(sy); + pmr_sync(); } vcpu = kern_hyp_va(vcpu); diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index 422664ac5f53..0abc5a13adaa 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -87,6 +87,15 @@ static DEFINE_STATIC_KEY_TRUE(supports_deactivate_key); */ static DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis); +/* + * Global static key controlling whether an update to PMR allowing more + * interrupts requires to be propagated to the redistributor (DSB SY). + * And this needs to be exported for modules to be able to enable + * interrupts... + */ +DEFINE_STATIC_KEY_FALSE(gic_pmr_sync); +EXPORT_SYMBOL(gic_pmr_sync); + /* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */ static refcount_t *ppi_nmi_refs; @@ -1502,6 +1511,17 @@ static void gic_enable_nmi_support(void) for (i = 0; i < gic_data.ppi_nr; i++) refcount_set(&ppi_nmi_refs[i], 0); + /* + * Linux itself doesn't use 1:N distribution, so has no need to + * set PMHE. The only reason to have it set is if EL3 requires it + * (and we can't change it). + */ + if (gic_read_ctlr() & ICC_CTLR_EL1_PMHE_MASK) + static_branch_enable(&gic_pmr_sync); + + pr_info("%s ICC_PMR_EL1 synchronisation\n", + static_branch_unlikely(&gic_pmr_sync) ? "Forcing" : "Relaxing"); + static_branch_enable(&supports_pseudo_nmis); if (static_branch_likely(&supports_deactivate_key)) diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h index 5cc10cf7cb3e..a0bde9e12efa 100644 --- a/include/linux/irqchip/arm-gic-v3.h +++ b/include/linux/irqchip/arm-gic-v3.h @@ -487,6 +487,8 @@ #define ICC_CTLR_EL1_EOImode_MASK (1 << ICC_CTLR_EL1_EOImode_SHIFT) #define ICC_CTLR_EL1_CBPR_SHIFT 0 #define ICC_CTLR_EL1_CBPR_MASK (1 << ICC_CTLR_EL1_CBPR_SHIFT) +#define ICC_CTLR_EL1_PMHE_SHIFT 6 +#define ICC_CTLR_EL1_PMHE_MASK (1 << ICC_CTLR_EL1_PMHE_SHIFT) #define ICC_CTLR_EL1_PRI_BITS_SHIFT 8 #define ICC_CTLR_EL1_PRI_BITS_MASK (0x7 << ICC_CTLR_EL1_PRI_BITS_SHIFT) #define ICC_CTLR_EL1_ID_BITS_SHIFT 11 -- 2.20.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] arm64: Relax ICC_PMR_EL1 accesses when ICC_CTLR_EL1.PMHE is clear 2019-10-02 9:06 ` Marc Zyngier @ 2019-10-23 8:38 ` liwei (GF) -1 siblings, 0 replies; 14+ messages in thread From: liwei (GF) @ 2019-10-23 8:38 UTC (permalink / raw) To: Marc Zyngier, Will Deacon, Catalin Marinas Cc: Suzuki K Poulose, linux-kernel, James Morse, Julien Thierry, huawei.libin, guohanjun, linux-arm-kernel Hi Marc, On 2019/10/2 17:06, Marc Zyngier wrote: > The GICv3 architecture specification is incredibly misleading when it > comes to PMR and the requirement for a DSB. It turns out that this DSB > is only required if the CPU interface sends an Upstream Control > message to the redistributor in order to update the RD's view of PMR. > > This message is only sent when ICC_CTLR_EL1.PMHE is set, which isn't > the case in Linux. It can still be set from EL3, so some special care > is required. But the upshot is that in the (hopefuly large) majority > of the cases, we can drop the DSB altogether. > > This relies on a new static key being set if the boot CPU has PMHE > set. The drawback is that this static key has to be exported to > modules. > > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: James Morse <james.morse@arm.com> > Cc: Julien Thierry <julien.thierry.kdev@gmail.com> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/include/asm/barrier.h | 12 ++++++++++++ > arch/arm64/include/asm/daifflags.h | 3 ++- > arch/arm64/include/asm/irqflags.h | 19 ++++++++++--------- > arch/arm64/include/asm/kvm_host.h | 3 +-- > arch/arm64/kernel/entry.S | 6 ++++-- > arch/arm64/kvm/hyp/switch.c | 4 ++-- > drivers/irqchip/irq-gic-v3.c | 20 ++++++++++++++++++++ > include/linux/irqchip/arm-gic-v3.h | 2 ++ > 8 files changed, 53 insertions(+), 16 deletions(-) > > diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h > index e0e2b1946f42..7d9cc5ec4971 100644 > --- a/arch/arm64/include/asm/barrier.h > +++ b/arch/arm64/include/asm/barrier.h > @@ -29,6 +29,18 @@ > SB_BARRIER_INSN"nop\n", \ > ARM64_HAS_SB)) > > +#ifdef CONFIG_ARM64_PSEUDO_NMI > +#define pmr_sync() \ > + do { \ > + extern struct static_key_false gic_pmr_sync; \ > + \ > + if (static_branch_unlikely(&gic_pmr_sync)) \ > + dsb(sy); \ > + } while(0) > +#else > +#define pmr_sync() do {} while (0) > +#endif > + Thank you for solving this problem, it helps a lot indeed. The pmr_sync() will call dsb(sy) when ARM64_PSEUDO_NMI=y and gic_pmr_sync=force, but if pseudo nmi is not enabled through boot option, it just take one more redundant calling than before at the following two place. I think change dsb(sy) to + asm volatile(ALTERNATIVE("nop", "dsb sy", \ + ARM64_HAS_IRQ_PRIO_MASKING) \ + : : : "memory"); \ may be more appropriate. Thanks, Wei > > @@ -34,14 +35,14 @@ static inline void arch_local_irq_enable(void) > } > > asm volatile(ALTERNATIVE( > - "msr daifclr, #2 // arch_local_irq_enable\n" > - "nop", > - __msr_s(SYS_ICC_PMR_EL1, "%0") > - "dsb sy", > + "msr daifclr, #2 // arch_local_irq_enable", > + __msr_s(SYS_ICC_PMR_EL1, "%0"), > ARM64_HAS_IRQ_PRIO_MASKING) > : > : "r" ((unsigned long) GIC_PRIO_IRQON) > : "memory"); > + > + pmr_sync(); > } > > static inline void arch_local_irq_disable(void) > @@ -116,14 +117,14 @@ static inline unsigned long arch_local_irq_save(void) > static inline void arch_local_irq_restore(unsigned long flags) > { > asm volatile(ALTERNATIVE( > - "msr daif, %0\n" > - "nop", > - __msr_s(SYS_ICC_PMR_EL1, "%0") > - "dsb sy", > - ARM64_HAS_IRQ_PRIO_MASKING) > + "msr daif, %0", > + __msr_s(SYS_ICC_PMR_EL1, "%0"), > + ARM64_HAS_IRQ_PRIO_MASKING) > : > : "r" (flags) > : "memory"); > + > + pmr_sync(); > } > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] arm64: Relax ICC_PMR_EL1 accesses when ICC_CTLR_EL1.PMHE is clear @ 2019-10-23 8:38 ` liwei (GF) 0 siblings, 0 replies; 14+ messages in thread From: liwei (GF) @ 2019-10-23 8:38 UTC (permalink / raw) To: Marc Zyngier, Will Deacon, Catalin Marinas Cc: Suzuki K Poulose, James Morse, Julien Thierry, huawei.libin, guohanjun, linux-arm-kernel, linux-kernel Hi Marc, On 2019/10/2 17:06, Marc Zyngier wrote: > The GICv3 architecture specification is incredibly misleading when it > comes to PMR and the requirement for a DSB. It turns out that this DSB > is only required if the CPU interface sends an Upstream Control > message to the redistributor in order to update the RD's view of PMR. > > This message is only sent when ICC_CTLR_EL1.PMHE is set, which isn't > the case in Linux. It can still be set from EL3, so some special care > is required. But the upshot is that in the (hopefuly large) majority > of the cases, we can drop the DSB altogether. > > This relies on a new static key being set if the boot CPU has PMHE > set. The drawback is that this static key has to be exported to > modules. > > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: James Morse <james.morse@arm.com> > Cc: Julien Thierry <julien.thierry.kdev@gmail.com> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/include/asm/barrier.h | 12 ++++++++++++ > arch/arm64/include/asm/daifflags.h | 3 ++- > arch/arm64/include/asm/irqflags.h | 19 ++++++++++--------- > arch/arm64/include/asm/kvm_host.h | 3 +-- > arch/arm64/kernel/entry.S | 6 ++++-- > arch/arm64/kvm/hyp/switch.c | 4 ++-- > drivers/irqchip/irq-gic-v3.c | 20 ++++++++++++++++++++ > include/linux/irqchip/arm-gic-v3.h | 2 ++ > 8 files changed, 53 insertions(+), 16 deletions(-) > > diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h > index e0e2b1946f42..7d9cc5ec4971 100644 > --- a/arch/arm64/include/asm/barrier.h > +++ b/arch/arm64/include/asm/barrier.h > @@ -29,6 +29,18 @@ > SB_BARRIER_INSN"nop\n", \ > ARM64_HAS_SB)) > > +#ifdef CONFIG_ARM64_PSEUDO_NMI > +#define pmr_sync() \ > + do { \ > + extern struct static_key_false gic_pmr_sync; \ > + \ > + if (static_branch_unlikely(&gic_pmr_sync)) \ > + dsb(sy); \ > + } while(0) > +#else > +#define pmr_sync() do {} while (0) > +#endif > + Thank you for solving this problem, it helps a lot indeed. The pmr_sync() will call dsb(sy) when ARM64_PSEUDO_NMI=y and gic_pmr_sync=force, but if pseudo nmi is not enabled through boot option, it just take one more redundant calling than before at the following two place. I think change dsb(sy) to + asm volatile(ALTERNATIVE("nop", "dsb sy", \ + ARM64_HAS_IRQ_PRIO_MASKING) \ + : : : "memory"); \ may be more appropriate. Thanks, Wei > > @@ -34,14 +35,14 @@ static inline void arch_local_irq_enable(void) > } > > asm volatile(ALTERNATIVE( > - "msr daifclr, #2 // arch_local_irq_enable\n" > - "nop", > - __msr_s(SYS_ICC_PMR_EL1, "%0") > - "dsb sy", > + "msr daifclr, #2 // arch_local_irq_enable", > + __msr_s(SYS_ICC_PMR_EL1, "%0"), > ARM64_HAS_IRQ_PRIO_MASKING) > : > : "r" ((unsigned long) GIC_PRIO_IRQON) > : "memory"); > + > + pmr_sync(); > } > > static inline void arch_local_irq_disable(void) > @@ -116,14 +117,14 @@ static inline unsigned long arch_local_irq_save(void) > static inline void arch_local_irq_restore(unsigned long flags) > { > asm volatile(ALTERNATIVE( > - "msr daif, %0\n" > - "nop", > - __msr_s(SYS_ICC_PMR_EL1, "%0") > - "dsb sy", > - ARM64_HAS_IRQ_PRIO_MASKING) > + "msr daif, %0", > + __msr_s(SYS_ICC_PMR_EL1, "%0"), > + ARM64_HAS_IRQ_PRIO_MASKING) > : > : "r" (flags) > : "memory"); > + > + pmr_sync(); > } > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] arm64: Relax ICC_PMR_EL1 accesses when ICC_CTLR_EL1.PMHE is clear 2019-10-23 8:38 ` liwei (GF) @ 2019-10-23 12:13 ` Marc Zyngier -1 siblings, 0 replies; 14+ messages in thread From: Marc Zyngier @ 2019-10-23 12:13 UTC (permalink / raw) To: liwei (GF) Cc: Suzuki K Poulose, Catalin Marinas, linux-kernel, James Morse, linux-arm-kernel, huawei.libin, guohanjun, Will Deacon, Julien Thierry Hi Wei, On 2019-10-23 09:38, liwei (GF) wrote: > Hi Marc, > > On 2019/10/2 17:06, Marc Zyngier wrote: >> The GICv3 architecture specification is incredibly misleading when >> it >> comes to PMR and the requirement for a DSB. It turns out that this >> DSB >> is only required if the CPU interface sends an Upstream Control >> message to the redistributor in order to update the RD's view of >> PMR. >> >> This message is only sent when ICC_CTLR_EL1.PMHE is set, which isn't >> the case in Linux. It can still be set from EL3, so some special >> care >> is required. But the upshot is that in the (hopefuly large) majority >> of the cases, we can drop the DSB altogether. >> >> This relies on a new static key being set if the boot CPU has PMHE >> set. The drawback is that this static key has to be exported to >> modules. >> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will@kernel.org> >> Cc: James Morse <james.morse@arm.com> >> Cc: Julien Thierry <julien.thierry.kdev@gmail.com> >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> arch/arm64/include/asm/barrier.h | 12 ++++++++++++ >> arch/arm64/include/asm/daifflags.h | 3 ++- >> arch/arm64/include/asm/irqflags.h | 19 ++++++++++--------- >> arch/arm64/include/asm/kvm_host.h | 3 +-- >> arch/arm64/kernel/entry.S | 6 ++++-- >> arch/arm64/kvm/hyp/switch.c | 4 ++-- >> drivers/irqchip/irq-gic-v3.c | 20 ++++++++++++++++++++ >> include/linux/irqchip/arm-gic-v3.h | 2 ++ >> 8 files changed, 53 insertions(+), 16 deletions(-) >> >> diff --git a/arch/arm64/include/asm/barrier.h >> b/arch/arm64/include/asm/barrier.h >> index e0e2b1946f42..7d9cc5ec4971 100644 >> --- a/arch/arm64/include/asm/barrier.h >> +++ b/arch/arm64/include/asm/barrier.h >> @@ -29,6 +29,18 @@ >> SB_BARRIER_INSN"nop\n", \ >> ARM64_HAS_SB)) >> >> +#ifdef CONFIG_ARM64_PSEUDO_NMI >> +#define pmr_sync() \ >> + do { \ >> + extern struct static_key_false gic_pmr_sync; \ >> + \ >> + if (static_branch_unlikely(&gic_pmr_sync)) \ >> + dsb(sy); \ >> + } while(0) >> +#else >> +#define pmr_sync() do {} while (0) >> +#endif >> + > > Thank you for solving this problem, it helps a lot indeed. > > The pmr_sync() will call dsb(sy) when ARM64_PSEUDO_NMI=y and > gic_pmr_sync=force, > but if pseudo nmi is not enabled through boot option, it just take > one more > redundant calling than before at the following two place. > > I think change dsb(sy) to > + asm volatile(ALTERNATIVE("nop", "dsb sy", > \ > + ARM64_HAS_IRQ_PRIO_MASKING) \ > + : : : "memory"); \ > may be more appropriate. I'm not sure I understand what you mean. The static key defaults to false, so if pseudo_nmi is not enabled, this dsb(sy) is simply never executed. Am I missing something obvious? Thanks, M. > > Thanks, > Wei > >> >> @@ -34,14 +35,14 @@ static inline void arch_local_irq_enable(void) >> } >> >> asm volatile(ALTERNATIVE( >> - "msr daifclr, #2 // arch_local_irq_enable\n" >> - "nop", >> - __msr_s(SYS_ICC_PMR_EL1, "%0") >> - "dsb sy", >> + "msr daifclr, #2 // arch_local_irq_enable", >> + __msr_s(SYS_ICC_PMR_EL1, "%0"), >> ARM64_HAS_IRQ_PRIO_MASKING) >> : >> : "r" ((unsigned long) GIC_PRIO_IRQON) >> : "memory"); >> + >> + pmr_sync(); >> } >> >> static inline void arch_local_irq_disable(void) >> @@ -116,14 +117,14 @@ static inline unsigned long >> arch_local_irq_save(void) >> static inline void arch_local_irq_restore(unsigned long flags) >> { >> asm volatile(ALTERNATIVE( >> - "msr daif, %0\n" >> - "nop", >> - __msr_s(SYS_ICC_PMR_EL1, "%0") >> - "dsb sy", >> - ARM64_HAS_IRQ_PRIO_MASKING) >> + "msr daif, %0", >> + __msr_s(SYS_ICC_PMR_EL1, "%0"), >> + ARM64_HAS_IRQ_PRIO_MASKING) >> : >> : "r" (flags) >> : "memory"); >> + >> + pmr_sync(); >> } >> -- Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] arm64: Relax ICC_PMR_EL1 accesses when ICC_CTLR_EL1.PMHE is clear @ 2019-10-23 12:13 ` Marc Zyngier 0 siblings, 0 replies; 14+ messages in thread From: Marc Zyngier @ 2019-10-23 12:13 UTC (permalink / raw) To: liwei (GF) Cc: Will Deacon, Catalin Marinas, Suzuki K Poulose, James Morse, Julien Thierry, huawei.libin, guohanjun, linux-arm-kernel, linux-kernel Hi Wei, On 2019-10-23 09:38, liwei (GF) wrote: > Hi Marc, > > On 2019/10/2 17:06, Marc Zyngier wrote: >> The GICv3 architecture specification is incredibly misleading when >> it >> comes to PMR and the requirement for a DSB. It turns out that this >> DSB >> is only required if the CPU interface sends an Upstream Control >> message to the redistributor in order to update the RD's view of >> PMR. >> >> This message is only sent when ICC_CTLR_EL1.PMHE is set, which isn't >> the case in Linux. It can still be set from EL3, so some special >> care >> is required. But the upshot is that in the (hopefuly large) majority >> of the cases, we can drop the DSB altogether. >> >> This relies on a new static key being set if the boot CPU has PMHE >> set. The drawback is that this static key has to be exported to >> modules. >> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will@kernel.org> >> Cc: James Morse <james.morse@arm.com> >> Cc: Julien Thierry <julien.thierry.kdev@gmail.com> >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> arch/arm64/include/asm/barrier.h | 12 ++++++++++++ >> arch/arm64/include/asm/daifflags.h | 3 ++- >> arch/arm64/include/asm/irqflags.h | 19 ++++++++++--------- >> arch/arm64/include/asm/kvm_host.h | 3 +-- >> arch/arm64/kernel/entry.S | 6 ++++-- >> arch/arm64/kvm/hyp/switch.c | 4 ++-- >> drivers/irqchip/irq-gic-v3.c | 20 ++++++++++++++++++++ >> include/linux/irqchip/arm-gic-v3.h | 2 ++ >> 8 files changed, 53 insertions(+), 16 deletions(-) >> >> diff --git a/arch/arm64/include/asm/barrier.h >> b/arch/arm64/include/asm/barrier.h >> index e0e2b1946f42..7d9cc5ec4971 100644 >> --- a/arch/arm64/include/asm/barrier.h >> +++ b/arch/arm64/include/asm/barrier.h >> @@ -29,6 +29,18 @@ >> SB_BARRIER_INSN"nop\n", \ >> ARM64_HAS_SB)) >> >> +#ifdef CONFIG_ARM64_PSEUDO_NMI >> +#define pmr_sync() \ >> + do { \ >> + extern struct static_key_false gic_pmr_sync; \ >> + \ >> + if (static_branch_unlikely(&gic_pmr_sync)) \ >> + dsb(sy); \ >> + } while(0) >> +#else >> +#define pmr_sync() do {} while (0) >> +#endif >> + > > Thank you for solving this problem, it helps a lot indeed. > > The pmr_sync() will call dsb(sy) when ARM64_PSEUDO_NMI=y and > gic_pmr_sync=force, > but if pseudo nmi is not enabled through boot option, it just take > one more > redundant calling than before at the following two place. > > I think change dsb(sy) to > + asm volatile(ALTERNATIVE("nop", "dsb sy", > \ > + ARM64_HAS_IRQ_PRIO_MASKING) \ > + : : : "memory"); \ > may be more appropriate. I'm not sure I understand what you mean. The static key defaults to false, so if pseudo_nmi is not enabled, this dsb(sy) is simply never executed. Am I missing something obvious? Thanks, M. > > Thanks, > Wei > >> >> @@ -34,14 +35,14 @@ static inline void arch_local_irq_enable(void) >> } >> >> asm volatile(ALTERNATIVE( >> - "msr daifclr, #2 // arch_local_irq_enable\n" >> - "nop", >> - __msr_s(SYS_ICC_PMR_EL1, "%0") >> - "dsb sy", >> + "msr daifclr, #2 // arch_local_irq_enable", >> + __msr_s(SYS_ICC_PMR_EL1, "%0"), >> ARM64_HAS_IRQ_PRIO_MASKING) >> : >> : "r" ((unsigned long) GIC_PRIO_IRQON) >> : "memory"); >> + >> + pmr_sync(); >> } >> >> static inline void arch_local_irq_disable(void) >> @@ -116,14 +117,14 @@ static inline unsigned long >> arch_local_irq_save(void) >> static inline void arch_local_irq_restore(unsigned long flags) >> { >> asm volatile(ALTERNATIVE( >> - "msr daif, %0\n" >> - "nop", >> - __msr_s(SYS_ICC_PMR_EL1, "%0") >> - "dsb sy", >> - ARM64_HAS_IRQ_PRIO_MASKING) >> + "msr daif, %0", >> + __msr_s(SYS_ICC_PMR_EL1, "%0"), >> + ARM64_HAS_IRQ_PRIO_MASKING) >> : >> : "r" (flags) >> : "memory"); >> + >> + pmr_sync(); >> } >> -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] arm64: Relax ICC_PMR_EL1 accesses when ICC_CTLR_EL1.PMHE is clear 2019-10-23 12:13 ` Marc Zyngier @ 2019-10-26 1:42 ` liwei (GF) -1 siblings, 0 replies; 14+ messages in thread From: liwei (GF) @ 2019-10-26 1:42 UTC (permalink / raw) To: Marc Zyngier Cc: Suzuki K Poulose, Catalin Marinas, linux-kernel, James Morse, linux-arm-kernel, huawei.libin, guohanjun, Will Deacon, Julien Thierry Hi Marc, On 2019/10/23 20:13, Marc Zyngier wrote: > Hi Wei, > > On 2019-10-23 09:38, liwei (GF) wrote: >> Hi Marc, >> >> On 2019/10/2 17:06, Marc Zyngier wrote: >>> The GICv3 architecture specification is incredibly misleading when it >>> comes to PMR and the requirement for a DSB. It turns out that this DSB >>> is only required if the CPU interface sends an Upstream Control >>> message to the redistributor in order to update the RD's view of PMR. >>> >>> This message is only sent when ICC_CTLR_EL1.PMHE is set, which isn't >>> the case in Linux. It can still be set from EL3, so some special care >>> is required. But the upshot is that in the (hopefuly large) majority >>> of the cases, we can drop the DSB altogether. >>> >>> This relies on a new static key being set if the boot CPU has PMHE >>> set. The drawback is that this static key has to be exported to >>> modules. >>> >>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>> Cc: Will Deacon <will@kernel.org> >>> Cc: James Morse <james.morse@arm.com> >>> Cc: Julien Thierry <julien.thierry.kdev@gmail.com> >>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >>> Signed-off-by: Marc Zyngier <maz@kernel.org> >>> --- >>> arch/arm64/include/asm/barrier.h | 12 ++++++++++++ >>> arch/arm64/include/asm/daifflags.h | 3 ++- >>> arch/arm64/include/asm/irqflags.h | 19 ++++++++++--------- >>> arch/arm64/include/asm/kvm_host.h | 3 +-- >>> arch/arm64/kernel/entry.S | 6 ++++-- >>> arch/arm64/kvm/hyp/switch.c | 4 ++-- >>> drivers/irqchip/irq-gic-v3.c | 20 ++++++++++++++++++++ >>> include/linux/irqchip/arm-gic-v3.h | 2 ++ >>> 8 files changed, 53 insertions(+), 16 deletions(-) >>> >>> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h >>> index e0e2b1946f42..7d9cc5ec4971 100644 >>> --- a/arch/arm64/include/asm/barrier.h >>> +++ b/arch/arm64/include/asm/barrier.h >>> @@ -29,6 +29,18 @@ >>> SB_BARRIER_INSN"nop\n", \ >>> ARM64_HAS_SB)) >>> >>> +#ifdef CONFIG_ARM64_PSEUDO_NMI >>> +#define pmr_sync() \ >>> + do { \ >>> + extern struct static_key_false gic_pmr_sync; \ >>> + \ >>> + if (static_branch_unlikely(&gic_pmr_sync)) \ >>> + dsb(sy); \ >>> + } while(0) >>> +#else >>> +#define pmr_sync() do {} while (0) >>> +#endif >>> + >> >> Thank you for solving this problem, it helps a lot indeed. >> >> The pmr_sync() will call dsb(sy) when ARM64_PSEUDO_NMI=y and >> gic_pmr_sync=force, >> but if pseudo nmi is not enabled through boot option, it just take one more >> redundant calling than before at the following two place. >> >> I think change dsb(sy) to >> + asm volatile(ALTERNATIVE("nop", "dsb sy", \ >> + ARM64_HAS_IRQ_PRIO_MASKING) \ >> + : : : "memory"); \ >> may be more appropriate. > > I'm not sure I understand what you mean. The static key defaults to false, > so if pseudo_nmi is not enabled, this dsb(sy) is simply never executed. > > Am I missing something obvious? > > Thanks, > > M. > You are right, my mistake. Sorry for confusing you. Thanks, Wei _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] arm64: Relax ICC_PMR_EL1 accesses when ICC_CTLR_EL1.PMHE is clear @ 2019-10-26 1:42 ` liwei (GF) 0 siblings, 0 replies; 14+ messages in thread From: liwei (GF) @ 2019-10-26 1:42 UTC (permalink / raw) To: Marc Zyngier Cc: Will Deacon, Catalin Marinas, Suzuki K Poulose, James Morse, Julien Thierry, huawei.libin, guohanjun, linux-arm-kernel, linux-kernel Hi Marc, On 2019/10/23 20:13, Marc Zyngier wrote: > Hi Wei, > > On 2019-10-23 09:38, liwei (GF) wrote: >> Hi Marc, >> >> On 2019/10/2 17:06, Marc Zyngier wrote: >>> The GICv3 architecture specification is incredibly misleading when it >>> comes to PMR and the requirement for a DSB. It turns out that this DSB >>> is only required if the CPU interface sends an Upstream Control >>> message to the redistributor in order to update the RD's view of PMR. >>> >>> This message is only sent when ICC_CTLR_EL1.PMHE is set, which isn't >>> the case in Linux. It can still be set from EL3, so some special care >>> is required. But the upshot is that in the (hopefuly large) majority >>> of the cases, we can drop the DSB altogether. >>> >>> This relies on a new static key being set if the boot CPU has PMHE >>> set. The drawback is that this static key has to be exported to >>> modules. >>> >>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>> Cc: Will Deacon <will@kernel.org> >>> Cc: James Morse <james.morse@arm.com> >>> Cc: Julien Thierry <julien.thierry.kdev@gmail.com> >>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >>> Signed-off-by: Marc Zyngier <maz@kernel.org> >>> --- >>> arch/arm64/include/asm/barrier.h | 12 ++++++++++++ >>> arch/arm64/include/asm/daifflags.h | 3 ++- >>> arch/arm64/include/asm/irqflags.h | 19 ++++++++++--------- >>> arch/arm64/include/asm/kvm_host.h | 3 +-- >>> arch/arm64/kernel/entry.S | 6 ++++-- >>> arch/arm64/kvm/hyp/switch.c | 4 ++-- >>> drivers/irqchip/irq-gic-v3.c | 20 ++++++++++++++++++++ >>> include/linux/irqchip/arm-gic-v3.h | 2 ++ >>> 8 files changed, 53 insertions(+), 16 deletions(-) >>> >>> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h >>> index e0e2b1946f42..7d9cc5ec4971 100644 >>> --- a/arch/arm64/include/asm/barrier.h >>> +++ b/arch/arm64/include/asm/barrier.h >>> @@ -29,6 +29,18 @@ >>> SB_BARRIER_INSN"nop\n", \ >>> ARM64_HAS_SB)) >>> >>> +#ifdef CONFIG_ARM64_PSEUDO_NMI >>> +#define pmr_sync() \ >>> + do { \ >>> + extern struct static_key_false gic_pmr_sync; \ >>> + \ >>> + if (static_branch_unlikely(&gic_pmr_sync)) \ >>> + dsb(sy); \ >>> + } while(0) >>> +#else >>> +#define pmr_sync() do {} while (0) >>> +#endif >>> + >> >> Thank you for solving this problem, it helps a lot indeed. >> >> The pmr_sync() will call dsb(sy) when ARM64_PSEUDO_NMI=y and >> gic_pmr_sync=force, >> but if pseudo nmi is not enabled through boot option, it just take one more >> redundant calling than before at the following two place. >> >> I think change dsb(sy) to >> + asm volatile(ALTERNATIVE("nop", "dsb sy", \ >> + ARM64_HAS_IRQ_PRIO_MASKING) \ >> + : : : "memory"); \ >> may be more appropriate. > > I'm not sure I understand what you mean. The static key defaults to false, > so if pseudo_nmi is not enabled, this dsb(sy) is simply never executed. > > Am I missing something obvious? > > Thanks, > > M. > You are right, my mistake. Sorry for confusing you. Thanks, Wei ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 2/2] arm64: Document ICC_CTLR_EL3.PMHE setting requirements 2019-10-02 9:06 ` Marc Zyngier @ 2019-10-02 9:06 ` Marc Zyngier -1 siblings, 0 replies; 14+ messages in thread From: Marc Zyngier @ 2019-10-02 9:06 UTC (permalink / raw) To: Will Deacon, Catalin Marinas Cc: linux-arm-kernel, Suzuki K Poulose, linux-kernel, James Morse, uohanjun, huawei.libin, liwei391, Julien Thierry It goes without saying, but better saying it: the kernel expects ICC_CTLR_EL3.PMHE to have the same value across all CPUs, and for that setting not to change during the lifetime of the kernel. Signed-off-by: Marc Zyngier <maz@kernel.org> --- Documentation/arm64/booting.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/arm64/booting.rst b/Documentation/arm64/booting.rst index d3f3a60fbf25..5d78a6f5b0ae 100644 --- a/Documentation/arm64/booting.rst +++ b/Documentation/arm64/booting.rst @@ -213,6 +213,9 @@ Before jumping into the kernel, the following conditions must be met: - ICC_SRE_EL3.Enable (bit 3) must be initialiased to 0b1. - ICC_SRE_EL3.SRE (bit 0) must be initialised to 0b1. + - ICC_CTLR_EL3.PMHE (bit 6) must be set to the same value across + all CPUs the kernel is executing on, and must stay constant + for the lifetime of the kernel. - If the kernel is entered at EL1: -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 2/2] arm64: Document ICC_CTLR_EL3.PMHE setting requirements @ 2019-10-02 9:06 ` Marc Zyngier 0 siblings, 0 replies; 14+ messages in thread From: Marc Zyngier @ 2019-10-02 9:06 UTC (permalink / raw) To: Will Deacon, Catalin Marinas Cc: Suzuki K Poulose, James Morse, Julien Thierry, huawei.libin, uohanjun, liwei391, linux-arm-kernel, linux-kernel It goes without saying, but better saying it: the kernel expects ICC_CTLR_EL3.PMHE to have the same value across all CPUs, and for that setting not to change during the lifetime of the kernel. Signed-off-by: Marc Zyngier <maz@kernel.org> --- Documentation/arm64/booting.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/arm64/booting.rst b/Documentation/arm64/booting.rst index d3f3a60fbf25..5d78a6f5b0ae 100644 --- a/Documentation/arm64/booting.rst +++ b/Documentation/arm64/booting.rst @@ -213,6 +213,9 @@ Before jumping into the kernel, the following conditions must be met: - ICC_SRE_EL3.Enable (bit 3) must be initialiased to 0b1. - ICC_SRE_EL3.SRE (bit 0) must be initialised to 0b1. + - ICC_CTLR_EL3.PMHE (bit 6) must be set to the same value across + all CPUs the kernel is executing on, and must stay constant + for the lifetime of the kernel. - If the kernel is entered at EL1: -- 2.20.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 0/2] arm64: Relax ICC_PMR_EL1 synchronisation when possible 2019-10-02 9:06 ` Marc Zyngier @ 2019-10-15 17:30 ` Catalin Marinas -1 siblings, 0 replies; 14+ messages in thread From: Catalin Marinas @ 2019-10-15 17:30 UTC (permalink / raw) To: Marc Zyngier Cc: linux-arm-kernel, Suzuki K Poulose, liwei391, linux-kernel, James Morse, uohanjun, huawei.libin, Will Deacon, Julien Thierry On Wed, Oct 02, 2019 at 10:06:11AM +0100, Marc Zyngier wrote: > This is a very late update on [1], fixing the 32bit compilation issue that > was present in v2, and adding an extra message in the kernel log to find out > what is going on. > > [1] http://lore.kernel.org/r/20190802125208.73162-1-maz@kernel.org > > Marc Zyngier (2): > arm64: Relax ICC_PMR_EL1 accesses when ICC_CTLR_EL1.PMHE is clear > arm64: Document ICC_CTLR_EL3.PMHE setting requirements Queued for 5.5. Thanks. -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 0/2] arm64: Relax ICC_PMR_EL1 synchronisation when possible @ 2019-10-15 17:30 ` Catalin Marinas 0 siblings, 0 replies; 14+ messages in thread From: Catalin Marinas @ 2019-10-15 17:30 UTC (permalink / raw) To: Marc Zyngier Cc: Will Deacon, Suzuki K Poulose, James Morse, Julien Thierry, huawei.libin, uohanjun, liwei391, linux-arm-kernel, linux-kernel On Wed, Oct 02, 2019 at 10:06:11AM +0100, Marc Zyngier wrote: > This is a very late update on [1], fixing the 32bit compilation issue that > was present in v2, and adding an extra message in the kernel log to find out > what is going on. > > [1] http://lore.kernel.org/r/20190802125208.73162-1-maz@kernel.org > > Marc Zyngier (2): > arm64: Relax ICC_PMR_EL1 accesses when ICC_CTLR_EL1.PMHE is clear > arm64: Document ICC_CTLR_EL3.PMHE setting requirements Queued for 5.5. Thanks. -- Catalin ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-10-26 1:42 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-10-02 9:06 [PATCH v3 0/2] arm64: Relax ICC_PMR_EL1 synchronisation when possible Marc Zyngier 2019-10-02 9:06 ` Marc Zyngier 2019-10-02 9:06 ` [PATCH v3 1/2] arm64: Relax ICC_PMR_EL1 accesses when ICC_CTLR_EL1.PMHE is clear Marc Zyngier 2019-10-02 9:06 ` Marc Zyngier 2019-10-23 8:38 ` liwei (GF) 2019-10-23 8:38 ` liwei (GF) 2019-10-23 12:13 ` Marc Zyngier 2019-10-23 12:13 ` Marc Zyngier 2019-10-26 1:42 ` liwei (GF) 2019-10-26 1:42 ` liwei (GF) 2019-10-02 9:06 ` [PATCH v3 2/2] arm64: Document ICC_CTLR_EL3.PMHE setting requirements Marc Zyngier 2019-10-02 9:06 ` Marc Zyngier 2019-10-15 17:30 ` [PATCH v3 0/2] arm64: Relax ICC_PMR_EL1 synchronisation when possible Catalin Marinas 2019-10-15 17:30 ` Catalin Marinas
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.