2018-11-12 11:58:16

by Julien Thierry

Subject: [PATCH v6 00/24] arm64: provide pseudo NMI with GICv3

Hi,

This series is a continuation of the work started by Daniel [1]. The goal
is to use GICv3 interrupt priorities to simulate an NMI.

The patches depend on the core API for NMIs patches [2].

To achieve this, set two priorities, one for standard interrupts and
another, higher priority, for NMIs. Whenever we want to disable interrupts,
we mask the standard priority instead so NMIs can still be raised. Some
corner cases though still require to actually mask all interrupts
effectively disabling the NMI.

Daniel Thompson ran some benchmarks [3] on a previous version showing a
small (<1%) performance drop when using interrupt priorities on Cortex-A53
and GIC-500.

Currently, only PPIs and SPIs can be set as NMIs. IPIs being currently
hardcoded IRQ numbers, there isn't a generic interface to set SGIs as NMI
for now. LPIs being controlled by the ITS cannot be delivered as NMI.
When an NMI is active on a CPU, no other NMI can be triggered on the CPU.

Requirements to use this:
- Have GICv3
- SCR_EL3.FIQ is set to 1 when linux runs or have single security state
- Select Kernel Feature -> Use ICC system registers for IRQ masking

* Patches 1 and 2 are just a bit of cleanup

* Patch 3 introduces a CPU feature to check if priority masking should be
used

* Patches 4 and 5 add the support for priority masking in GICv3 driver

* Patches 6 to 11 add the support for priority masking the arch/arm64
code

* Patches 12 and 13 allow us to apply alternatives earlier in the boot
process

* Patches 14 to 16 starts the PMR masking on cpu startup and provides
primitives for arm64 GICv3 driver to perform priority masking

* Patches 17 to 20 Add support for pseudo-NMIs in GICv3 driver

* Patches 21 to 23 Add support for receiving NMIs in arch/arm64

* Patch 24 adds the build config and command line option to enable
pseudo-NMIs

Changes since v5[4]:
* Rebased on v4.20-rc2
* Reorganized patches again
* Renamed compile option + PMR values defines
* Write PMR enablement for cpu startup in C
* Fix wrong array size passed to __apply_alternatives
* Do not touch PMR in pt_regs when not using irq masking
* Remove ISB between PMR and IAR -> turns out it is not needed
* Simplify irqflags code after introducing PMR in it
* Use ref count API to track PPIs set as NMI
* Simplify NMI exit path

Changes since V4[5]:
* Rebased to v4.19-rc1
* Adapted GIC driver to the core NMI API
* Added option to disable priority masking on command line
* Added Daniel's Tested-by on patches related replacing PSR.I toggling with
PMR masking
* Fix scope matching for alternative features.
* Spotted some more places using PSR.I or daif and replaced with generic
interrupt functions

Changes since V3[6]:
* Big refactoring. As suggested by Marc Z., some of the bigger patches
needed to be split into smaller one.
* Try to reduce the amount of #ifdef for the new feature by introducing
an individual cpufeature for priority masking
* Do not track which alternatives have been applied (was a bit dodgy
anyway), and use an alternative for VHE cpu_enable callback
* Fix a build failure with arm by adding the correct RPR accessors
* Added Suggested-by tags for changes from comming or inspired by Daniel's
series. Do let me know if you feel I missed something and am not giving
you due credit.

Changes since V2[7]:
* Series rebase to v4.17-rc6
* Adapt pathces 1 and 2 to the rework of cpufeatures framework
* Use the group0 detection scheme in the GICv3 driver to identify
the priority view, and drop the use of a fake interrupt
* Add the case for a GIC configured in a single security state
* Use local_daif_restore instead of local_irq_enable the first time
we enable interrupts after a bp hardening in the handling of a kernel
entry. Otherwise PRS.I remains set...

Changes since V1[8]:
* Series rebased to v4.15-rc8.
* Check for arm64_early_features in this_cpu_has_cap (spotted by Suzuki).
* Fix issue where debug exception were not masked when enabling debug in
mdscr_el1.

Changes since RFC[9]:
* The series was rebased to v4.15-rc2 which implied some changes mainly
related to the work on exception entries and daif flags by James Morse.
- The first patch in the previous series was dropped because no longer
applicable.
- With the semantics James introduced of "inheriting" daif flags,
handling of PMR on exception entry is simplified as PMR is not altered
by taking an exception and already inherited from previous state.
- James pointed out that taking a PseudoNMI before reading the FAR_EL1
register should not be allowed as per the TRM (D10.2.29):
"FAR_EL1 is made UNKNOWN on an exception return from EL1."
So in this submission PSR.I bit is cleared only after FAR_EL1 is read.
* For KVM, only deal with PMR unmasking/restoring in common code, and VHE
specific code makes sure PSR.I bit is set when necessary.
* When detecting the GIC priority view (patch 5), wait for an actual
interrupt instead of trying only once.


[1] http://www.spinics.net/lists/arm-kernel/msg525077.html
[2] https://lkml.org/lkml/2018/11/12/2113
[3] https://lkml.org/lkml/2018/7/20/803
[4] https://lkml.org/lkml/2018/8/28/693
[5] https://lkml.org/lkml/2018/7/24/321
[6] https://lkml.org/lkml/2018/5/21/276
[7] https://lkml.org/lkml/2018/1/17/335
[8] https://www.spinics.net/lists/arm-kernel/msg620763.html
[9] https://www.spinics.net/lists/arm-kernel/msg610736.html

Cheers,

Julien

-->

Daniel Thompson (1):
arm64: alternative: Apply alternatives early in boot process

Julien Thierry (23):
arm64: Remove unused daif related functions/macros
arm64: cpufeature: Set SYSREG_GIC_CPUIF as a boot system feature
arm64: cpufeature: Add cpufeature for IRQ priority masking
arm/arm64: gic-v3: Add PMR and RPR accessors
irqchip/gic-v3: Switch to PMR masking before calling IRQ handler
arm64: ptrace: Provide definitions for PMR values
arm64: Make PMR part of task context
arm64: Unmask PMR before going idle
arm64: kvm: Unmask PMR before entering guest
arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking
arm64: daifflags: Include PMR in daifflags restore operations
arm64: alternative: Allow alternative status checking per cpufeature
irqchip/gic-v3: Factor group0 detection into functions
arm64: Switch to PMR masking when starting CPUs
arm64: gic-v3: Implement arch support for priority masking
irqchip/gic-v3: Detect if GIC can support pseudo-NMIs
irqchip/gic-v3: Handle pseudo-NMIs
irqchip/gic: Add functions to access irq priorities
irqchip/gic-v3: Allow interrupts to be set as pseudo-NMI
arm64: Handle serror in NMI context
arm64: Skip preemption when exiting an NMI
arm64: Skip irqflags tracing for NMI in IRQs disabled context
arm64: Enable the support of pseudo-NMIs

Documentation/admin-guide/kernel-parameters.txt | 6 +
Documentation/arm64/booting.txt | 5 +
arch/arm/include/asm/arch_gicv3.h | 33 ++++
arch/arm64/Kconfig | 14 ++
arch/arm64/include/asm/alternative.h | 4 +-
arch/arm64/include/asm/arch_gicv3.h | 32 +++
arch/arm64/include/asm/assembler.h | 10 +-
arch/arm64/include/asm/cpucaps.h | 3 +-
arch/arm64/include/asm/cpufeature.h | 10 +
arch/arm64/include/asm/daifflags.h | 20 +-
arch/arm64/include/asm/efi.h | 3 +-
arch/arm64/include/asm/irqflags.h | 132 ++++++++++---
arch/arm64/include/asm/kvm_host.h | 12 ++
arch/arm64/include/asm/processor.h | 3 +
arch/arm64/include/asm/ptrace.h | 20 +-
arch/arm64/kernel/alternative.c | 60 +++++-
arch/arm64/kernel/asm-offsets.c | 1 +
arch/arm64/kernel/cpufeature.c | 42 +++-
arch/arm64/kernel/entry.S | 40 ++++
arch/arm64/kernel/process.c | 6 +
arch/arm64/kernel/smp.c | 34 ++++
arch/arm64/kernel/traps.c | 8 +-
arch/arm64/kvm/hyp/switch.c | 16 ++
arch/arm64/mm/proc.S | 18 ++
drivers/irqchip/irq-gic-common.c | 10 +
drivers/irqchip/irq-gic-common.h | 2 +
drivers/irqchip/irq-gic-v3.c | 250 +++++++++++++++++++++---
include/linux/irqchip/arm-gic-common.h | 2 +-
28 files changed, 695 insertions(+), 101 deletions(-)

--
1.9.1

2018-11-12 11:59:02

by Julien Thierry

Subject: [PATCH v6 18/24] irqchip/gic-v3: Handle pseudo-NMIs

Provide a higher priority to be used for pseudo-NMIs. When such an
interrupt is received, keep interrupts fully disabled at CPU level to
prevent receiving other pseudo-NMIs while handling the current one.

Signed-off-by: Julien Thierry <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Marc Zyngier <[email protected]>
---
drivers/irqchip/irq-gic-v3.c | 35 +++++++++++++++++++++++++++++------
1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 5245791..c763f1a 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -41,6 +41,8 @@

#include "irq-gic-common.h"

+#define GICD_INT_NMI_PRI 0xa0
+
struct redist_region {
void __iomem *redist_base;
phys_addr_t phys_base;
@@ -375,6 +377,16 @@ static u64 gic_mpidr_to_affinity(unsigned long mpidr)
return aff;
}

+static inline void gic_deactivate_unexpected_irq(u32 irqnr)
+{
+ if (static_branch_likely(&supports_deactivate_key)) {
+ if (irqnr < 8192)
+ gic_write_dir(irqnr);
+ } else {
+ gic_write_eoir(irqnr);
+ }
+}
+
static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
{
u32 irqnr;
@@ -384,6 +396,22 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
if (likely(irqnr > 15 && irqnr < 1020) || irqnr >= 8192) {
int err;

+ if (gic_supports_nmi() &&
+ unlikely(gic_read_rpr() == GICD_INT_NMI_PRI)) {
+ if (static_branch_likely(&supports_deactivate_key))
+ gic_write_eoir(irqnr);
+ /*
+ * Leave the PSR.I bit set to prevent other NMIs to be
+ * received while handling this one.
+ * PSR.I will be restored when we ERET to the
+ * interrupted context.
+ */
+ err = handle_domain_nmi(gic_data.domain, irqnr, regs);
+ if (err)
+ gic_deactivate_unexpected_irq(irqnr);
+ return;
+ }
+
if (gic_prio_masking_enabled()) {
gic_pmr_mask_irqs();
gic_arch_enable_irqs();
@@ -397,12 +425,7 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
err = handle_domain_irq(gic_data.domain, irqnr, regs);
if (err) {
WARN_ONCE(true, "Unexpected interrupt received!\n");
- if (static_branch_likely(&supports_deactivate_key)) {
- if (irqnr < 8192)
- gic_write_dir(irqnr);
- } else {
- gic_write_eoir(irqnr);
- }
+ gic_deactivate_unexpected_irq(irqnr);
}
return;
}
--
1.9.1

2018-11-12 11:59:02

by Julien Thierry

Subject: [PATCH v6 17/24] irqchip/gic-v3: Detect if GIC can support pseudo-NMIs

The values non secure EL1 needs to use for PMR and RPR registers depends on
the value of SCR_EL3.FIQ.

The values non secure EL1 sees from the distributor and redistributor
depend on whether security is enabled for the GIC or not.

To avoid having to deal with two sets of values for PMR
masking/unmasking, only enable pseudo-NMIs when GIC has non-secure view
of priorities.

Also, add firmware requirements related to SCR_EL3.

Signed-off-by: Julien Thierry <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Marc Zyngier <[email protected]>
---
Documentation/arm64/booting.txt | 5 ++++
drivers/irqchip/irq-gic-v3.c | 58 ++++++++++++++++++++++++++++++++++++-----
2 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
index 8d0df62..e387938 100644
--- a/Documentation/arm64/booting.txt
+++ b/Documentation/arm64/booting.txt
@@ -188,6 +188,11 @@ Before jumping into the kernel, the following conditions must be met:
the kernel image will be entered must be initialised by software at a
higher exception level to prevent execution in an UNKNOWN state.

+ - SCR_EL3.FIQ must have the same value across all CPUs the kernel is
+ executing on.
+ - The value of SCR_EL3.FIQ must be the same as the one present at boot
+ time whenever the kernel is executing.
+
For systems with a GICv3 interrupt controller to be used in v3 mode:
- If EL3 is present:
ICC_SRE_EL3.Enable (bit 3) must be initialiased to 0b1.
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 7f0b2e8..5245791 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -63,6 +63,31 @@ struct gic_chip_data {
static struct gic_chip_data gic_data __read_mostly;
static DEFINE_STATIC_KEY_TRUE(supports_deactivate_key);

+/*
+ * The behaviours of RPR and PMR registers differ depending on the value of
+ * SCR_EL3.FIQ, and the behaviour of non-secure priority registers of the
+ * distributor and redistributors depends on whether security is enabled in the
+ * GIC.
+ *
+ * When security is enabled, non-secure priority values from the (re)distributor
+ * are presented to the GIC CPUIF as follow:
+ * (GIC_(R)DIST_PRI[irq] >> 1) | 0x80;
+ *
+ * If SCR_EL3.FIQ == 1, the values writen to/read from PMR and RPR at non-secure
+ * EL1 are subject to a similar operation thus matching the priorities presented
+ * from the (re)distributor when security is enabled.
+ *
+ * see GICv3/GICv4 Architecture Specification (IHI0069D):
+ * - section 4.8.1 Non-secure accesses to register fields for Secure interrupt
+ * priorities.
+ * - Figure 4-7 Secure read of the priority field for a Non-secure Group 1
+ * interrupt.
+ *
+ * For now, we only support pseudo-NMIs if we have non-secure view of
+ * priorities.
+ */
+static DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis);
+
static struct gic_kvm_info gic_v3_kvm_info;
static DEFINE_PER_CPU(bool, has_rss);

@@ -226,6 +251,12 @@ static void gic_unmask_irq(struct irq_data *d)
gic_poke_irq(d, GICD_ISENABLER);
}

+static inline bool gic_supports_nmi(void)
+{
+ return IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) &&
+ static_branch_likely(&supports_pseudo_nmis);
+}
+
static int gic_irq_set_irqchip_state(struct irq_data *d,
enum irqchip_irq_state which, bool val)
{
@@ -572,6 +603,12 @@ static void gic_update_vlpi_properties(void)
!gic_data.rdists.has_direct_lpi ? "no " : "");
}

+/* Check whether it's single security state view */
+static inline bool gic_dist_security_disabled(void)
+{
+ return readl_relaxed(gic_data.dist_base + GICD_CTLR) & GICD_CTLR_DS;
+}
+
static void gic_cpu_sys_reg_init(void)
{
int i, cpu = smp_processor_id();
@@ -597,6 +634,9 @@ static void gic_cpu_sys_reg_init(void)
/* Set priority mask register */
if (!gic_prio_masking_enabled())
write_gicreg(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
+ else if (gic_supports_nmi() && group0)
+ /* Mismatch configuration with boot CPU */
+ WARN_ON(!gic_dist_security_disabled());

/*
* Some firmwares hand over to the kernel with the BPR changed from
@@ -851,12 +891,6 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
#endif

#ifdef CONFIG_CPU_PM
-/* Check whether it's single security state view */
-static bool gic_dist_security_disabled(void)
-{
- return readl_relaxed(gic_data.dist_base + GICD_CTLR) & GICD_CTLR_DS;
-}
-
static int gic_cpu_pm_notifier(struct notifier_block *self,
unsigned long cmd, void *v)
{
@@ -1100,6 +1134,11 @@ static int partition_domain_translate(struct irq_domain *d,
.select = gic_irq_domain_select,
};

+static void gic_enable_nmi_support(void)
+{
+ static_branch_enable(&supports_pseudo_nmis);
+}
+
static int __init gic_init_bases(void __iomem *dist_base,
struct redist_region *rdist_regs,
u32 nr_redist_regions,
@@ -1169,6 +1208,13 @@ static int __init gic_init_bases(void __iomem *dist_base,
its_cpu_init();
}

+ if (gic_prio_masking_enabled()) {
+ if (!gic_has_group0() || gic_dist_security_disabled())
+ gic_enable_nmi_support();
+ else
+ pr_warn("SCR_EL3.FIQ is cleared, cannot enable use of pseudo-NMIs\n");
+ }
+
return 0;

out_free:
--
1.9.1

2018-11-12 11:59:15

by Julien Thierry

Subject: [PATCH v6 22/24] arm64: Skip preemption when exiting an NMI

Handling of an NMI should not set any TIF flags. For NMIs received from
EL0 the current exit path is safe to use.

However, an NMI received at EL1 could have interrupted some task context
that has set the TIF_NEED_RESCHED flag. Preempting a task should not
happen as a result of an NMI.

Skip preemption after handling an NMI from EL1.

Signed-off-by: Julien Thierry <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Marc Zyngier <[email protected]>
---
arch/arm64/kernel/entry.S | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index eb8120e..e02ee55 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -636,6 +636,14 @@ el1_irq:

#ifdef CONFIG_PREEMPT
ldr w24, [tsk, #TSK_TI_PREEMPT] // get preempt count
+alternative_if ARM64_HAS_IRQ_PRIO_MASKING
+ /*
+ * DA_F were cleared at start of handling. If anything is set in DAIF,
+ * we come back from an NMI, so skip preemption
+ */
+ mrs x0, daif
+ orr w24, w24, w0
+alternative_else_nop_endif
cbnz w24, 1f // preempt count != 0
ldr x0, [tsk, #TSK_TI_FLAGS] // get flags
tbz x0, #TIF_NEED_RESCHED, 1f // needs rescheduling?
--
1.9.1

2018-11-12 11:59:18

by Julien Thierry

Subject: [PATCH v6 24/24] arm64: Enable the support of pseudo-NMIs

Add a build option and a command line parameter to build and enable the
support of pseudo-NMIs.

Signed-off-by: Julien Thierry <[email protected]>
Suggested-by: Daniel Thompson <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
---
Documentation/admin-guide/kernel-parameters.txt | 6 ++++++
arch/arm64/Kconfig | 14 ++++++++++++++
arch/arm64/kernel/cpufeature.c | 11 ++++++++++-
3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 81d1d5a..63dbd56 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1183,6 +1183,12 @@
to discrete, to make X server driver able to add WB
entry later. This parameter enables that.

+ enable_pseudo_nmi [ARM64]
+ Enables support for pseudo-NMIs in the kernel. This
+ requires both the kernel to be built with
+ CONFIG_ARM64_PSEUDO_NMI and to be running on a
+ platform with GICv3.
+
enable_timer_pin_1 [X86]
Enable PIN 1 of APIC timer
Can be useful to work around chipset bugs
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 787d785..4729cf0 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -958,6 +958,20 @@ config ARM64_SSBD

If unsure, say Y.

+config ARM64_PSEUDO_NMI
+ bool "Support for NMI-like interrupts"
+ select CONFIG_ARM_GIC_V3
+ help
+ Adds support for mimicking Non-Maskable Interrupts through the use of
+ GIC interrupt priority. This support requires version 3 or later of
+ Arm GIC.
+
+ This high priority configuration for interrupts need to be
+ explicitly enabled through the new kernel parameter
+ "enable_pseudo_nmi".
+
+ If unsure, say N
+
menuconfig ARMV8_DEPRECATED
bool "Emulate deprecated/obsolete ARMv8 instructions"
depends on COMPAT
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 58a4978..acf1d06 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1149,10 +1149,19 @@ static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused)
#endif /* CONFIG_ARM64_RAS_EXTN */

#ifdef CONFIG_ARM64_PSEUDO_NMI
+static bool enable_pseudo_nmi;
+
+static int __init early_enable_pseudo_nmi(char *p)
+{
+ enable_pseudo_nmi = true;
+ return 0;
+}
+early_param("enable_pseudo_nmi", early_enable_pseudo_nmi);
+
static bool can_use_gic_priorities(const struct arm64_cpu_capabilities *entry,
int scope)
{
- return false;
+ return enable_pseudo_nmi && has_useable_gicv3_cpuif(entry, scope);
}
#endif

--
1.9.1

2018-11-12 11:59:22

by Julien Thierry

Subject: [PATCH v6 21/24] arm64: Handle serror in NMI context

Per definition of the daifflags, Serrors can occur during any interrupt
context, that includes NMI contexts. Trying to nmi_enter in an nmi context
will crash.

Skip nmi_enter/nmi_exit when serror occurred during an NMI.

Suggested-by: James Morse <[email protected]>
Signed-off-by: Julien Thierry <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Dave Martin <[email protected]>
Cc: James Morse <[email protected]>
---
arch/arm64/kernel/traps.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 5f4d9ac..66344cd 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -897,13 +897,17 @@ bool arm64_is_fatal_ras_serror(struct pt_regs *regs, unsigned int esr)

asmlinkage void do_serror(struct pt_regs *regs, unsigned int esr)
{
- nmi_enter();
+ const bool was_in_nmi = in_nmi();
+
+ if (!was_in_nmi)
+ nmi_enter();

/* non-RAS errors are not containable */
if (!arm64_is_ras_serror(esr) || arm64_is_fatal_ras_serror(regs, esr))
arm64_serror_panic(regs, esr);

- nmi_exit();
+ if (!was_in_nmi)
+ nmi_exit();
}

void __pte_error(const char *file, int line, unsigned long val)
--
1.9.1

2018-11-12 11:59:28

by Julien Thierry

Subject: [PATCH v6 20/24] irqchip/gic-v3: Allow interrupts to be set as pseudo-NMI

Implement NMI callbacks for GICv3 irqchip. Install NMI safe handlers
when setting up interrupt line as NMI.

Only SPIs and PPIs are allowed to be set up as NMI.

Signed-off-by: Julien Thierry <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Marc Zyngier <[email protected]>
---
drivers/irqchip/irq-gic-v3.c | 84 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 84 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index c763f1a..f22ae49 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -27,6 +27,7 @@
#include <linux/of_address.h>
#include <linux/of_irq.h>
#include <linux/percpu.h>
+#include <linux/refcount.h>
#include <linux/slab.h>

#include <linux/irqchip.h>
@@ -90,6 +91,9 @@ struct gic_chip_data {
*/
static DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis);

+/* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */
+static refcount_t ppi_nmi_refs[16];
+
static struct gic_kvm_info gic_v3_kvm_info;
static DEFINE_PER_CPU(bool, has_rss);

@@ -314,6 +318,72 @@ static int gic_irq_get_irqchip_state(struct irq_data *d,
return 0;
}

+static int gic_irq_nmi_setup(struct irq_data *d)
+{
+ struct irq_desc *desc = irq_to_desc(d->irq);
+
+ if (!gic_supports_nmi())
+ return -EINVAL;
+
+ if (gic_peek_irq(d, GICD_ISENABLER)) {
+ pr_err("Cannot set NMI property of enabled IRQ %u\n", d->irq);
+ return -EINVAL;
+ }
+
+ /*
+ * A secondary irq_chip should be in charge of LPI request,
+ * it should not be possible to get there
+ */
+ if (WARN_ON(gic_irq(d) >= 8192))
+ return -EINVAL;
+
+ /* desc lock should already be held */
+ if (gic_irq(d) < 32) {
+ /* Setting up PPI as NMI, only switch handler for first NMI */
+ if (!refcount_inc_not_zero(&ppi_nmi_refs[gic_irq(d) - 16])) {
+ refcount_set(&ppi_nmi_refs[gic_irq(d) - 16], 1);
+ desc->handle_irq = handle_percpu_devid_fasteoi_nmi;
+ }
+ } else {
+ desc->handle_irq = handle_fasteoi_nmi;
+ }
+
+ gic_set_irq_prio(gic_irq(d), gic_dist_base(d), GICD_INT_NMI_PRI);
+
+ return 0;
+}
+
+static void gic_irq_nmi_teardown(struct irq_data *d)
+{
+ struct irq_desc *desc = irq_to_desc(d->irq);
+
+ if (WARN_ON(!gic_supports_nmi()))
+ return;
+
+ if (gic_peek_irq(d, GICD_ISENABLER)) {
+ pr_err("Cannot set NMI property of enabled IRQ %u\n", d->irq);
+ return;
+ }
+
+ /*
+ * A secondary irq_chip should be in charge of LPI request,
+ * it should not be possible to get there
+ */
+ if (WARN_ON(gic_irq(d) >= 8192))
+ return;
+
+ /* desc lock should already be held */
+ if (gic_irq(d) < 32) {
+ /* Tearing down NMI, only switch handler for last NMI */
+ if (refcount_dec_and_test(&ppi_nmi_refs[gic_irq(d) - 16]))
+ desc->handle_irq = handle_percpu_devid_irq;
+ } else {
+ desc->handle_irq = handle_fasteoi_irq;
+ }
+
+ gic_set_irq_prio(gic_irq(d), gic_dist_base(d), GICD_INT_DEF_PRI);
+}
+
static void gic_eoi_irq(struct irq_data *d)
{
gic_write_eoir(gic_irq(d));
@@ -950,6 +1020,8 @@ static inline void gic_cpu_pm_init(void) { }
.irq_set_affinity = gic_set_affinity,
.irq_get_irqchip_state = gic_irq_get_irqchip_state,
.irq_set_irqchip_state = gic_irq_set_irqchip_state,
+ .irq_nmi_setup = gic_irq_nmi_setup,
+ .irq_nmi_teardown = gic_irq_nmi_teardown,
.flags = IRQCHIP_SET_TYPE_MASKED |
IRQCHIP_SKIP_SET_WAKE |
IRQCHIP_MASK_ON_SUSPEND,
@@ -965,6 +1037,8 @@ static inline void gic_cpu_pm_init(void) { }
.irq_get_irqchip_state = gic_irq_get_irqchip_state,
.irq_set_irqchip_state = gic_irq_set_irqchip_state,
.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity,
+ .irq_nmi_setup = gic_irq_nmi_setup,
+ .irq_nmi_teardown = gic_irq_nmi_teardown,
.flags = IRQCHIP_SET_TYPE_MASKED |
IRQCHIP_SKIP_SET_WAKE |
IRQCHIP_MASK_ON_SUSPEND,
@@ -1159,7 +1233,17 @@ static int partition_domain_translate(struct irq_domain *d,

static void gic_enable_nmi_support(void)
{
+ int i;
+
+ for (i = 0; i < 16; i++)
+ refcount_set(&ppi_nmi_refs[i], 0);
+
static_branch_enable(&supports_pseudo_nmis);
+
+ if (static_branch_likely(&supports_deactivate_key))
+ gic_eoimode1_chip.flags |= IRQCHIP_SUPPORTS_NMI;
+ else
+ gic_chip.flags |= IRQCHIP_SUPPORTS_NMI;
}

static int __init gic_init_bases(void __iomem *dist_base,
--
1.9.1

2018-11-12 11:59:33

by Julien Thierry

Subject: [PATCH v6 19/24] irqchip/gic: Add functions to access irq priorities

Add accessors to the GIC distributor/redistributors priority registers.

Signed-off-by: Julien Thierry <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Marc Zyngier <[email protected]>
---
drivers/irqchip/irq-gic-common.c | 10 ++++++++++
drivers/irqchip/irq-gic-common.h | 2 ++
2 files changed, 12 insertions(+)

diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index 01e673c..910746f 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -98,6 +98,16 @@ int gic_configure_irq(unsigned int irq, unsigned int type,
return ret;
}

+void gic_set_irq_prio(unsigned int irq, void __iomem *base, u8 prio)
+{
+ writeb_relaxed(prio, base + GIC_DIST_PRI + irq);
+}
+
+u8 gic_get_irq_prio(unsigned int irq, void __iomem *base)
+{
+ return readb_relaxed(base + GIC_DIST_PRI + irq);
+}
+
void gic_dist_config(void __iomem *base, int gic_irqs,
void (*sync_access)(void))
{
diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
index 3919cd7..1586dbd 100644
--- a/drivers/irqchip/irq-gic-common.h
+++ b/drivers/irqchip/irq-gic-common.h
@@ -35,6 +35,8 @@ void gic_dist_config(void __iomem *base, int gic_irqs,
void gic_cpu_config(void __iomem *base, void (*sync_access)(void));
void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
void *data);
+void gic_set_irq_prio(unsigned int irq, void __iomem *base, u8 prio);
+u8 gic_get_irq_prio(unsigned int irq, void __iomem *base);

void gic_set_kvm_info(const struct gic_kvm_info *info);

--
1.9.1

2018-11-12 11:59:46

by Julien Thierry

Subject: [PATCH v6 16/24] arm64: gic-v3: Implement arch support for priority masking

Implement architecture specific primitive allowing the GICv3 driver to
use priorities to mask interrupts.

Lower the default priority of interrupts to a value maskable with
priority mask used for PMR. This is safe to do as both arm and arm64
only use one priority and are do not currently care about which priority
value it is, as long as all interrupts use the same priority.

Signed-off-by: Julien Thierry <[email protected]>
Suggested-by: Daniel Thompson <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
---
arch/arm64/include/asm/arch_gicv3.h | 8 ++++----
include/linux/irqchip/arm-gic-common.h | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
index 3f8d5f4..154612a 100644
--- a/arch/arm64/include/asm/arch_gicv3.h
+++ b/arch/arm64/include/asm/arch_gicv3.h
@@ -22,6 +22,7 @@

#ifndef __ASSEMBLY__

+#include <linux/irqchip/arm-gic-common.h>
#include <linux/stringify.h>
#include <asm/barrier.h>
#include <asm/cacheflush.h>
@@ -162,14 +163,13 @@ static inline bool gic_prio_masking_enabled(void)

static inline void gic_pmr_mask_irqs(void)
{
- /* Should not get called yet. */
- WARN_ON_ONCE(true);
+ BUILD_BUG_ON(GICD_INT_DEF_PRI <= GIC_PRIO_IRQOFF);
+ gic_write_pmr(GIC_PRIO_IRQOFF);
}

static inline void gic_arch_enable_irqs(void)
{
- /* Should not get called yet. */
- WARN_ON_ONCE(true);
+ asm volatile ("msr daifclr, #2" : : : "memory");
}

#endif /* __ASSEMBLY__ */
diff --git a/include/linux/irqchip/arm-gic-common.h b/include/linux/irqchip/arm-gic-common.h
index 9a1a479..2c9a4b3 100644
--- a/include/linux/irqchip/arm-gic-common.h
+++ b/include/linux/irqchip/arm-gic-common.h
@@ -13,7 +13,7 @@
#include <linux/types.h>
#include <linux/ioport.h>

-#define GICD_INT_DEF_PRI 0xa0
+#define GICD_INT_DEF_PRI 0xc0
#define GICD_INT_DEF_PRI_X4 ((GICD_INT_DEF_PRI << 24) |\
(GICD_INT_DEF_PRI << 16) |\
(GICD_INT_DEF_PRI << 8) |\
--
1.9.1

2018-11-12 11:59:48

by Julien Thierry

Subject: [PATCH v6 23/24] arm64: Skip irqflags tracing for NMI in IRQs disabled context

When an NMI is raised while interrupts where disabled, the IRQ tracing
already is in the correct state (i.e. hardirqs_off) and should be left
as such when returning to the interrupted context.

Check whether PMR was masking interrupts when the NMI was raised and
skip IRQ tracing if necessary.

Signed-off-by: Julien Thierry <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Marc Zyngier <[email protected]>
---
arch/arm64/kernel/entry.S | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index e02ee55..f175f18 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -629,7 +629,17 @@ el1_irq:
kernel_entry 1
enable_da_f
#ifdef CONFIG_TRACE_IRQFLAGS
+#ifdef CONFIG_ARM64_PSEUDO_NMI
+alternative_if ARM64_HAS_IRQ_PRIO_MASKING
+ ldr x20, [sp, #S_PMR_SAVE]
+alternative_else
+ mov x20, #GIC_PRIO_IRQON
+alternative_endif
+ /* Irqs were disabled, don't trace */
+ tbz x20, GIC_PRIO_STATUS_SHIFT, 1f
+#endif
bl trace_hardirqs_off
+1:
#endif

irq_handler
@@ -651,8 +661,17 @@ alternative_else_nop_endif
1:
#endif
#ifdef CONFIG_TRACE_IRQFLAGS
+#ifdef CONFIG_ARM64_PSEUDO_NMI
+ /*
+ * if IRQs were disabled when we received the interrupt, we have an NMI
+ * and we are not re-enabling interrupt upon eret. Skip tracing.
+ */
+ tbz x20, GIC_PRIO_STATUS_SHIFT, 1f
+#endif
bl trace_hardirqs_on
+1:
#endif
+
kernel_exit 1
ENDPROC(el1_irq)

--
1.9.1

2018-11-12 11:59:52

by Julien Thierry

Subject: [PATCH v6 15/24] arm64: Switch to PMR masking when starting CPUs

Once the boot CPU has been prepared or a new secondary CPU has been
brought up, use ICC_PMR_EL1 to mask interrupts on that CPU and clear
PSR.I bit.

Since ICC_PMR_EL1 is initialized at CPU bringup, avoid overwriting
it in the GICv3 driver.

Signed-off-by: Julien Thierry <[email protected]>
Suggested-by: Daniel Thompson <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: James Morse <[email protected]>
Cc: Marc Zyngier <[email protected]>
---
arch/arm64/kernel/smp.c | 27 +++++++++++++++++++++++++++
drivers/irqchip/irq-gic-v3.c | 8 +++++++-
2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 8dc9dde..e495360 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -35,6 +35,7 @@
#include <linux/smp.h>
#include <linux/seq_file.h>
#include <linux/irq.h>
+#include <linux/irqchip/arm-gic-v3.h>
#include <linux/percpu.h>
#include <linux/clockchips.h>
#include <linux/completion.h>
@@ -175,6 +176,25 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
return ret;
}

+static void init_gic_priority_masking(void)
+{
+ u32 gic_sre = gic_read_sre();
+ u32 cpuflags;
+
+ if (WARN_ON(!(gic_sre & ICC_SRE_EL1_SRE)))
+ return;
+
+ WARN_ON(!irqs_disabled());
+
+ gic_write_pmr(GIC_PRIO_IRQOFF);
+
+ cpuflags = read_sysreg(daif);
+
+ /* We can only unmask PSR.I if we can take aborts */
+ if (!(cpuflags & PSR_A_BIT))
+ write_sysreg(cpuflags & ~PSR_I_BIT, daif);
+}
+
/*
* This is the secondary CPU boot entry. We're using this CPUs
* idle thread stack, but a set of temporary page tables.
@@ -211,6 +231,9 @@ asmlinkage notrace void secondary_start_kernel(void)
*/
check_local_cpu_capabilities();

+ if (system_supports_irq_prio_masking())
+ init_gic_priority_masking();
+
if (cpu_ops[cpu]->cpu_postboot)
cpu_ops[cpu]->cpu_postboot();

@@ -421,6 +444,10 @@ void __init smp_prepare_boot_cpu(void)
* and/or scheduling is enabled.
*/
apply_boot_alternatives();
+
+ /* Conditionally switch to GIC PMR for interrupt masking */
+ if (system_supports_irq_prio_masking())
+ init_gic_priority_masking();
}

static u64 __init of_get_cpu_mpidr(struct device_node *dn)
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index dbf5247..7f0b2e8 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -414,6 +414,9 @@ static u32 gic_get_pribits(void)
static bool gic_has_group0(void)
{
u32 val;
+ u32 old_pmr;
+
+ old_pmr = gic_read_pmr();

/*
* Let's find out if Group0 is under control of EL3 or not by
@@ -429,6 +432,8 @@ static bool gic_has_group0(void)
gic_write_pmr(BIT(8 - gic_get_pribits()));
val = gic_read_pmr();

+ gic_write_pmr(old_pmr);
+
return val != 0;
}

@@ -590,7 +595,8 @@ static void gic_cpu_sys_reg_init(void)
group0 = gic_has_group0();

/* Set priority mask register */
- write_gicreg(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
+ if (!gic_prio_masking_enabled())
+ write_gicreg(DEFAULT_PMR_VALUE, ICC_PMR_EL1);

/*
* Some firmwares hand over to the kernel with the BPR changed from
--
1.9.1

2018-11-12 11:59:57

by Julien Thierry

Subject: [PATCH v6 11/24] arm64: daifflags: Include PMR in daifflags restore operations

The addition of PMR should not bypass the semantics of daifflags.

When DA_F are set, I bit is also set as no interrupts (even of higher
priority) is allowed.

When DA_F are cleared, I bit is cleared and interrupt enabling/disabling
goes through ICC_PMR_EL1.

Signed-off-by: Julien Thierry <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: James Morse <[email protected]>
---
arch/arm64/include/asm/daifflags.h | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
index 546bc39..31936b3 100644
--- a/arch/arm64/include/asm/daifflags.h
+++ b/arch/arm64/include/asm/daifflags.h
@@ -18,8 +18,14 @@

#include <linux/irqflags.h>

-#define DAIF_PROCCTX 0
-#define DAIF_PROCCTX_NOIRQ PSR_I_BIT
+#include <asm/cpufeature.h>
+
+#define DAIF_PROCCTX MAKE_ARCH_FLAGS(0, GIC_PRIO_IRQON)
+
+#define DAIF_PROCCTX_NOIRQ \
+ (system_supports_irq_prio_masking() ? \
+ MAKE_ARCH_FLAGS(0, GIC_PRIO_IRQOFF) : \
+ MAKE_ARCH_FLAGS(PSR_I_BIT, GIC_PRIO_IRQON))

/* mask/save/unmask/restore all exceptions, including interrupts. */
static inline void local_daif_mask(void)
--
1.9.1

2018-11-12 11:59:58

by Julien Thierry

Subject: [PATCH v6 14/24] irqchip/gic-v3: Factor group0 detection into functions

The code to detect whether Linux has access to group0 interrupts can
prove useful in other parts of the driver.

Provide a separate function to do this.

Signed-off-by: Julien Thierry <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Marc Zyngier <[email protected]>
---
drivers/irqchip/irq-gic-v3.c | 55 +++++++++++++++++++++++++++++---------------
1 file changed, 36 insertions(+), 19 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index e5d8c14..dbf5247 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -399,6 +399,39 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
}
}

+static u32 gic_get_pribits(void)
+{
+ u32 pribits;
+
+ pribits = gic_read_ctlr();
+ pribits &= ICC_CTLR_EL1_PRI_BITS_MASK;
+ pribits >>= ICC_CTLR_EL1_PRI_BITS_SHIFT;
+ pribits++;
+
+ return pribits;
+}
+
+static bool gic_has_group0(void)
+{
+ u32 val;
+
+ /*
+ * Let's find out if Group0 is under control of EL3 or not by
+ * setting the highest possible, non-zero priority in PMR.
+ *
+ * If SCR_EL3.FIQ is set, the priority gets shifted down in
+ * order for the CPU interface to set bit 7, and keep the
+ * actual priority in the non-secure range. In the process, it
+ * looses the least significant bit and the actual priority
+ * becomes 0x80. Reading it back returns 0, indicating that
+ * we're don't have access to Group0.
+ */
+ gic_write_pmr(BIT(8 - gic_get_pribits()));
+ val = gic_read_pmr();
+
+ return val != 0;
+}
+
static void __init gic_dist_init(void)
{
unsigned int i;
@@ -540,7 +573,7 @@ static void gic_cpu_sys_reg_init(void)
u64 mpidr = cpu_logical_map(cpu);
u64 need_rss = MPIDR_RS(mpidr);
bool group0;
- u32 val, pribits;
+ u32 pribits;

/*
* Need to check that the SRE bit has actually been set. If
@@ -552,25 +585,9 @@ static void gic_cpu_sys_reg_init(void)
if (!gic_enable_sre())
pr_err("GIC: unable to set SRE (disabled at EL2), panic ahead\n");

- pribits = gic_read_ctlr();
- pribits &= ICC_CTLR_EL1_PRI_BITS_MASK;
- pribits >>= ICC_CTLR_EL1_PRI_BITS_SHIFT;
- pribits++;
+ pribits = gic_get_pribits();

- /*
- * Let's find out if Group0 is under control of EL3 or not by
- * setting the highest possible, non-zero priority in PMR.
- *
- * If SCR_EL3.FIQ is set, the priority gets shifted down in
- * order for the CPU interface to set bit 7, and keep the
- * actual priority in the non-secure range. In the process, it
- * looses the least significant bit and the actual priority
- * becomes 0x80. Reading it back returns 0, indicating that
- * we're don't have access to Group0.
- */
- write_gicreg(BIT(8 - pribits), ICC_PMR_EL1);
- val = read_gicreg(ICC_PMR_EL1);
- group0 = val != 0;
+ group0 = gic_has_group0();

/* Set priority mask register */
write_gicreg(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
--
1.9.1

2018-11-12 12:00:05

by Julien Thierry

Subject: [PATCH v6 13/24] arm64: alternative: Apply alternatives early in boot process

From: Daniel Thompson <[email protected]>

Currently alternatives are applied very late in the boot process (and
a long time after we enable scheduling). Some alternative sequences,
such as those that alter the way CPU context is stored, must be applied
much earlier in the boot sequence.

Introduce apply_boot_alternatives() to allow some alternatives to be
applied immediately after we detect the CPU features of the boot CPU.

Signed-off-by: Daniel Thompson <[email protected]>
[[email protected]: rename to fit new cpufeature framework better,
apply BOOT_SCOPE feature early in boot]
Signed-off-by: Julien Thierry <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Christoffer Dall <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
---
arch/arm64/include/asm/alternative.h | 1 +
arch/arm64/include/asm/cpufeature.h | 4 ++++
arch/arm64/kernel/alternative.c | 43 +++++++++++++++++++++++++++++++-----
arch/arm64/kernel/cpufeature.c | 6 +++++
arch/arm64/kernel/smp.c | 7 ++++++
5 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
index 9806a23..b9f8d78 100644
--- a/arch/arm64/include/asm/alternative.h
+++ b/arch/arm64/include/asm/alternative.h
@@ -25,6 +25,7 @@ struct alt_instr {
typedef void (*alternative_cb_t)(struct alt_instr *alt,
__le32 *origptr, __le32 *updptr, int nr_inst);

+void __init apply_boot_alternatives(void);
void __init apply_alternatives_all(void);
bool alternative_is_applied(u16 cpufeature);

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index a6e063f..2d5f18f 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -357,6 +357,10 @@ static inline int cpucap_default_scope(const struct arm64_cpu_capabilities *cap)
extern struct static_key_false cpu_hwcap_keys[ARM64_NCAPS];
extern struct static_key_false arm64_const_caps_ready;

+/* ARM64 CAPS + alternative_cb */
+#define ARM64_NPATCHABLE (ARM64_NCAPS + 1)
+extern DECLARE_BITMAP(boot_capabilities, ARM64_NPATCHABLE);
+
bool this_cpu_has_cap(unsigned int cap);

static inline bool cpu_have_feature(unsigned int num)
diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index c947d22..a9b4677 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -155,7 +155,8 @@ static void clean_dcache_range_nopatch(u64 start, u64 end)
} while (cur += d_size, cur < end);
}

-static void __apply_alternatives(void *alt_region, bool is_module)
+static void __apply_alternatives(void *alt_region, bool is_module,
+ unsigned long *feature_mask)
{
struct alt_instr *alt;
struct alt_region *region = alt_region;
@@ -165,6 +166,9 @@ static void __apply_alternatives(void *alt_region, bool is_module)
for (alt = region->begin; alt < region->end; alt++) {
int nr_inst;

+ if (!test_bit(alt->cpufeature, feature_mask))
+ continue;
+
/* Use ARM64_CB_PATCH as an unconditional patch */
if (alt->cpufeature < ARM64_CB_PATCH &&
!cpus_have_cap(alt->cpufeature))
@@ -203,8 +207,11 @@ static void __apply_alternatives(void *alt_region, bool is_module)
__flush_icache_all();
isb();

- /* We applied all that was available */
- bitmap_copy(applied_alternatives, cpu_hwcaps, ARM64_NCAPS);
+ /* Ignore ARM64_CB bit from feature mask */
+ bitmap_or(applied_alternatives, applied_alternatives,
+ feature_mask, ARM64_NCAPS);
+ bitmap_and(applied_alternatives, applied_alternatives,
+ cpu_hwcaps, ARM64_NCAPS);
}
}

@@ -225,8 +232,13 @@ static int __apply_alternatives_multi_stop(void *unused)
cpu_relax();
isb();
} else {
+ DECLARE_BITMAP(remaining_capabilities, ARM64_NPATCHABLE);
+
+ bitmap_complement(remaining_capabilities, boot_capabilities,
+ ARM64_NPATCHABLE);
+
BUG_ON(all_alternatives_applied);
- __apply_alternatives(&region, false);
+ __apply_alternatives(&region, false, remaining_capabilities);
/* Barriers provided by the cache flushing */
WRITE_ONCE(all_alternatives_applied, 1);
}
@@ -240,6 +252,24 @@ void __init apply_alternatives_all(void)
stop_machine(__apply_alternatives_multi_stop, NULL, cpu_online_mask);
}

+/*
+ * This is called very early in the boot process (directly after we run
+ * a feature detect on the boot CPU). No need to worry about other CPUs
+ * here.
+ */
+void __init apply_boot_alternatives(void)
+{
+ struct alt_region region = {
+ .begin = (struct alt_instr *)__alt_instructions,
+ .end = (struct alt_instr *)__alt_instructions_end,
+ };
+
+ /* If called on non-boot cpu things could go wrong */
+ WARN_ON(smp_processor_id() != 0);
+
+ __apply_alternatives(&region, false, &boot_capabilities[0]);
+}
+
#ifdef CONFIG_MODULES
void apply_alternatives_module(void *start, size_t length)
{
@@ -247,7 +277,10 @@ void apply_alternatives_module(void *start, size_t length)
.begin = start,
.end = start + length,
};
+ DECLARE_BITMAP(all_capabilities, ARM64_NPATCHABLE);
+
+ bitmap_fill(all_capabilities, ARM64_NPATCHABLE);

- __apply_alternatives(&region, true);
+ __apply_alternatives(&region, true, &all_capabilities[0]);
}
#endif
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index dcf5d14..58a4978 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -53,6 +53,9 @@
DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
EXPORT_SYMBOL(cpu_hwcaps);

+/* Need also bit for ARM64_CB_PATCH */
+DECLARE_BITMAP(boot_capabilities, ARM64_NPATCHABLE);
+
/*
* Flag to indicate if we have computed the system wide
* capabilities based on the boot time active CPUs. This
@@ -1536,6 +1539,9 @@ static void __update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
if (!cpus_have_cap(caps->capability) && caps->desc)
pr_info("%s %s\n", info, caps->desc);
cpus_set_cap(caps->capability);
+
+ if (caps->type & SCOPE_BOOT_CPU)
+ set_bit(caps->capability, boot_capabilities);
}
}

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 96b8f2f..8dc9dde 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -414,6 +414,13 @@ void __init smp_prepare_boot_cpu(void)
*/
jump_label_init();
cpuinfo_store_boot_cpu();
+
+ /*
+ * We now know enough about the boot CPU to apply the
+ * alternatives that cannot wait until interrupt handling
+ * and/or scheduling is enabled.
+ */
+ apply_boot_alternatives();
}

static u64 __init of_get_cpu_mpidr(struct device_node *dn)
--
1.9.1

2018-11-12 12:00:16

by Julien Thierry

Subject: [PATCH v6 10/24] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking

Instead disabling interrupts by setting the PSR.I bit, use a priority
higher than the one used for interrupts to mask them via PMR.

The value chosen for PMR to enable/disable interrupts encodes the status
of interrupts on a single bit. This information is stored in the irqflags
values used when saving/restoring IRQ status.

Signed-off-by: Julien Thierry <[email protected]>
Suggested-by: Daniel Thompson <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Oleg Nesterov <[email protected]>
---
arch/arm64/include/asm/efi.h | 3 +-
arch/arm64/include/asm/irqflags.h | 132 +++++++++++++++++++++++++++++---------
2 files changed, 105 insertions(+), 30 deletions(-)

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 7ed3208..3e06891 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -42,7 +42,8 @@

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 \
+ (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT | ARCH_FLAG_PMR_EN)

/* 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..e0a32e4 100644
--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h
@@ -18,7 +18,27 @@

#ifdef __KERNEL__

+#include <asm/alternative.h>
+#include <asm/cpufeature.h>
#include <asm/ptrace.h>
+#include <asm/sysreg.h>
+
+
+/*
+ * When ICC_PMR_EL1 is used for interrupt masking, only the bit indicating
+ * whether the normal interrupts are masked is kept along with the daif
+ * flags.
+ */
+#define ARCH_FLAG_PMR_EN 0x1
+
+#define MAKE_ARCH_FLAGS(daif, pmr) \
+ ((daif) | (((pmr) >> GIC_PRIO_STATUS_SHIFT) & ARCH_FLAG_PMR_EN))
+
+#define ARCH_FLAGS_GET_PMR(flags) \
+ ((((flags) & ARCH_FLAG_PMR_EN) << GIC_PRIO_STATUS_SHIFT) \
+ | GIC_PRIO_IRQOFF)
+
+#define ARCH_FLAGS_GET_DAIF(flags) ((flags) & ~ARCH_FLAG_PMR_EN)

/*
* Aarch64 has flags for masking: Debug, Asynchronous (serror), Interrupts and
@@ -34,35 +54,62 @@
*/

/*
- * CPU interrupt mask handling.
+ * Local definitions to help us manage between PMR and daif
*/
-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;
-}
+#define _save_daif(dest) \
+ asm volatile("mrs %0, daif" : "=&r" (dest) : : "memory")
+
+#define _restore_daif(daif) \
+ asm volatile("msr daif, %0" : : "r" (daif) : "memory")

+#define _save_pmr(dest) \
+ asm volatile(ALTERNATIVE( \
+ "mov %0, #" __stringify(GIC_PRIO_IRQON), \
+ "mrs_s %0, " __stringify(SYS_ICC_PMR_EL1), \
+ ARM64_HAS_IRQ_PRIO_MASKING) \
+ : "=&r" (dest) \
+ : \
+ : "memory")
+
+#define _restore_pmr(pmr) \
+ asm volatile(ALTERNATIVE( \
+ "nop\n" \
+ "nop", \
+ "msr_s " __stringify(SYS_ICC_PMR_EL1) ",%0\n" \
+ "dsb sy", \
+ ARM64_HAS_IRQ_PRIO_MASKING) \
+ : \
+ : "r" (pmr) \
+ : "memory")
+
+/*
+ * CPU interrupt mask handling.
+ */
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");
}

@@ -71,12 +118,24 @@ static inline void arch_local_irq_disable(void)
*/
static inline unsigned long arch_local_save_flags(void)
{
+ unsigned long daif_flags;
+ unsigned long pmr;
+
+ _save_daif(daif_flags);
+
+ _save_pmr(pmr);
+
+ return MAKE_ARCH_FLAGS(daif_flags, pmr);
+}
+
+static inline unsigned long arch_local_irq_save(void)
+{
unsigned long flags;
- asm volatile(
- "mrs %0, daif // arch_local_save_flags"
- : "=r" (flags)
- :
- : "memory");
+
+ flags = arch_local_save_flags();
+
+ arch_local_irq_disable();
+
return flags;
}

@@ -85,16 +144,31 @@ 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");
+ unsigned long pmr = ARCH_FLAGS_GET_PMR(flags);
+
+ flags = ARCH_FLAGS_GET_DAIF(flags);
+
+ /*
+ * Code switching from PSR.I interrupt disabling to PMR masking
+ * should not lie between consecutive calls to local_irq_save()
+ * and local_irq_restore() in the same context.
+ * So restoring PMR and then the daif flags should be safe.
+ */
+ _restore_pmr(pmr);
+
+ _restore_daif(flags);
}

static inline int arch_irqs_disabled_flags(unsigned long flags)
{
- return flags & PSR_I_BIT;
+ return (ARCH_FLAGS_GET_DAIF(flags) & (PSR_I_BIT)) |
+ !(ARCH_FLAGS_GET_PMR(flags) & GIC_PRIO_STATUS_BIT);
}
+
+#undef _save_daif
+#undef _restore_daif
+#undef _save_pmr
+#undef _restore_pmr
+
#endif
#endif
--
1.9.1

2018-11-12 12:00:24

by Julien Thierry

Subject: [PATCH v6 09/24] arm64: kvm: Unmask PMR before entering guest

Interrupts masked by ICC_PMR_EL1 will not be signaled to the CPU. This
means that hypervisor will not receive masked interrupts while running a
guest.

Avoid this by making sure ICC_PMR_EL1 is unmasked when we enter a guest.

Signed-off-by: Julien Thierry <[email protected]>
Cc: Christoffer Dall <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: [email protected]
---
arch/arm64/include/asm/kvm_host.h | 12 ++++++++++++
arch/arm64/kvm/hyp/switch.c | 16 ++++++++++++++++
2 files changed, 28 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 52fbc82..31c7e9a 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -24,6 +24,7 @@

#include <linux/types.h>
#include <linux/kvm_types.h>
+#include <asm/arch_gicv3.h>
#include <asm/cpufeature.h>
#include <asm/daifflags.h>
#include <asm/fpsimd.h>
@@ -471,6 +472,17 @@ static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
static inline void kvm_arm_vhe_guest_enter(void)
{
local_daif_mask();
+
+ /*
+ * Having IRQs masked via PMR when entering the guest means the GIC
+ * will not signal the CPU of interrupts of lower priority, and the
+ * only way to get out will be via guest exceptions.
+ * Naturally, we want to avoid this.
+ */
+ if (system_supports_irq_prio_masking()) {
+ gic_write_pmr(GIC_PRIO_IRQON);
+ dsb(sy);
+ }
}

static inline void kvm_arm_vhe_guest_exit(void)
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 7cc175c..e5ea193 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -22,6 +22,7 @@

#include <kvm/arm_psci.h>

+#include <asm/arch_gicv3.h>
#include <asm/cpufeature.h>
#include <asm/kvm_asm.h>
#include <asm/kvm_emulate.h>
@@ -537,6 +538,17 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
struct kvm_cpu_context *guest_ctxt;
u64 exit_code;

+ /*
+ * Having IRQs masked via PMR when entering the guest means the GIC
+ * will not signal the CPU of interrupts of lower priority, and the
+ * only way to get out will be via guest exceptions.
+ * Naturally, we want to avoid this.
+ */
+ if (system_supports_irq_prio_masking()) {
+ gic_write_pmr(GIC_PRIO_IRQON);
+ dsb(sy);
+ }
+
vcpu = kern_hyp_va(vcpu);

host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
@@ -589,6 +601,10 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
*/
__debug_switch_to_host(vcpu);

+ /* Returning to host will clear PSR.I, remask PMR if needed */
+ if (system_supports_irq_prio_masking())
+ gic_write_pmr(GIC_PRIO_IRQOFF);
+
return exit_code;
}

--
1.9.1

2018-11-12 12:00:43

by Julien Thierry

Subject: [PATCH v6 07/24] arm64: Make PMR part of task context

If ICC_PMR_EL1 is used to mask interrupts, its value should be
saved/restored whenever a task is context switched out/in or
gets an exception.

Add PMR to the registers to save in the pt_regs struct upon kernel entry,
and restore it before ERET. Also, initialize it to a sane value when
creating new tasks.

Signed-off-by: Julien Thierry <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Dave Martin <[email protected]>
---
arch/arm64/include/asm/processor.h | 3 +++
arch/arm64/include/asm/ptrace.h | 14 +++++++++++---
arch/arm64/kernel/asm-offsets.c | 1 +
arch/arm64/kernel/entry.S | 13 +++++++++++++
arch/arm64/kernel/process.c | 6 ++++++
5 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
index 6b0d4df..b2315ef 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -168,6 +168,9 @@ static inline void start_thread_common(struct pt_regs *regs, unsigned long pc)
memset(regs, 0, sizeof(*regs));
forget_syscall(regs);
regs->pc = pc;
+
+ if (system_supports_irq_prio_masking())
+ regs->pmr_save = GIC_PRIO_IRQON;
}

static inline void start_thread(struct pt_regs *regs, unsigned long pc,
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index ce6998c..0ad46f5 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -19,6 +19,8 @@
#ifndef __ASM_PTRACE_H
#define __ASM_PTRACE_H

+#include <asm/cpufeature.h>
+
#include <uapi/asm/ptrace.h>

/* Current Exception Level values, as contained in CurrentEL */
@@ -173,7 +175,8 @@ struct pt_regs {
#endif

u64 orig_addr_limit;
- u64 unused; // maintain 16 byte alignment
+ /* Only valid when ARM64_HAS_IRQ_PRIO_MASKING is enabled. */
+ u64 pmr_save;
u64 stackframe[2];
};

@@ -208,8 +211,13 @@ static inline void forget_syscall(struct pt_regs *regs)
#define processor_mode(regs) \
((regs)->pstate & PSR_MODE_MASK)

-#define interrupts_enabled(regs) \
- (!((regs)->pstate & PSR_I_BIT))
+#define irqs_priority_unmasked(regs) \
+ (system_supports_irq_prio_masking() ? \
+ (regs)->pmr_save & GIC_PRIO_STATUS_BIT : \
+ true)
+
+#define interrupts_enabled(regs) \
+ (!((regs)->pstate & PSR_I_BIT) && irqs_priority_unmasked(regs))

#define fast_interrupts_enabled(regs) \
(!((regs)->pstate & PSR_F_BIT))
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 323aeb5..bab4122 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -78,6 +78,7 @@ int main(void)
DEFINE(S_ORIG_X0, offsetof(struct pt_regs, orig_x0));
DEFINE(S_SYSCALLNO, offsetof(struct pt_regs, syscallno));
DEFINE(S_ORIG_ADDR_LIMIT, offsetof(struct pt_regs, orig_addr_limit));
+ DEFINE(S_PMR_SAVE, offsetof(struct pt_regs, pmr_save));
DEFINE(S_STACKFRAME, offsetof(struct pt_regs, stackframe));
DEFINE(S_FRAME_SIZE, sizeof(struct pt_regs));
BLANK();
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 039144e..eb8120e 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -249,6 +249,12 @@ alternative_else_nop_endif
msr sp_el0, tsk
.endif

+ /* Save pmr */
+alternative_if ARM64_HAS_IRQ_PRIO_MASKING
+ mrs_s x20, SYS_ICC_PMR_EL1
+ str x20, [sp, #S_PMR_SAVE]
+alternative_else_nop_endif
+
/*
* Registers that may be useful after this macro is invoked:
*
@@ -269,6 +275,13 @@ alternative_else_nop_endif
/* No need to restore UAO, it will be restored from SPSR_EL1 */
.endif

+ /* Restore pmr */
+alternative_if ARM64_HAS_IRQ_PRIO_MASKING
+ ldr x20, [sp, #S_PMR_SAVE]
+ msr_s SYS_ICC_PMR_EL1, x20
+ dsb sy
+alternative_else_nop_endif
+
ldp x21, x22, [sp, #S_PC] // load ELR, SPSR
.if \el == 0
ct_user_enter
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index d9a4c2d..71e8850 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -231,6 +231,9 @@ void __show_regs(struct pt_regs *regs)

printk("sp : %016llx\n", sp);

+ if (system_supports_irq_prio_masking())
+ printk("pmr_save: %08llx\n", regs->pmr_save);
+
i = top_reg;

while (i >= 0) {
@@ -362,6 +365,9 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
if (arm64_get_ssbd_state() == ARM64_SSBD_FORCE_DISABLE)
childregs->pstate |= PSR_SSBS_BIT;

+ if (system_supports_irq_prio_masking())
+ childregs->pmr_save = GIC_PRIO_IRQON;
+
p->thread.cpu_context.x19 = stack_start;
p->thread.cpu_context.x20 = stk_sz;
}
--
1.9.1

2018-11-12 12:00:49

by Julien Thierry

Subject: [PATCH v6 12/24] arm64: alternative: Allow alternative status checking per cpufeature

In preparation for the application of alternatives at different points
during the boot process, provide the possibility to check whether
alternatives for a feature of interest was already applied instead of
having a global boolean for all alternatives.

Make VHE enablement code check for the VHE feature instead of considering
all alternatives.

Signed-off-by: Julien Thierry <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Christoffer Dall <[email protected]>
---
arch/arm64/include/asm/alternative.h | 3 +--
arch/arm64/kernel/alternative.c | 21 +++++++++++++++++----
arch/arm64/kernel/cpufeature.c | 2 +-
3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
index 4b650ec..9806a23 100644
--- a/arch/arm64/include/asm/alternative.h
+++ b/arch/arm64/include/asm/alternative.h
@@ -14,8 +14,6 @@
#include <linux/stddef.h>
#include <linux/stringify.h>

-extern int alternatives_applied;
-
struct alt_instr {
s32 orig_offset; /* offset to original instruction */
s32 alt_offset; /* offset to replacement instruction */
@@ -28,6 +26,7 @@ typedef void (*alternative_cb_t)(struct alt_instr *alt,
__le32 *origptr, __le32 *updptr, int nr_inst);

void __init apply_alternatives_all(void);
+bool alternative_is_applied(u16 cpufeature);

#ifdef CONFIG_MODULES
void apply_alternatives_module(void *start, size_t length);
diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index b5d6039..c947d22 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -32,13 +32,23 @@
#define ALT_ORIG_PTR(a) __ALT_PTR(a, orig_offset)
#define ALT_REPL_PTR(a) __ALT_PTR(a, alt_offset)

-int alternatives_applied;
+static int all_alternatives_applied;
+
+static DECLARE_BITMAP(applied_alternatives, ARM64_NCAPS);

struct alt_region {
struct alt_instr *begin;
struct alt_instr *end;
};

+bool alternative_is_applied(u16 cpufeature)
+{
+ if (WARN_ON(cpufeature >= ARM64_NCAPS))
+ return false;
+
+ return test_bit(cpufeature, applied_alternatives);
+}
+
/*
* Check if the target PC is within an alternative block.
*/
@@ -192,6 +202,9 @@ static void __apply_alternatives(void *alt_region, bool is_module)
dsb(ish);
__flush_icache_all();
isb();
+
+ /* We applied all that was available */
+ bitmap_copy(applied_alternatives, cpu_hwcaps, ARM64_NCAPS);
}
}

@@ -208,14 +221,14 @@ static int __apply_alternatives_multi_stop(void *unused)

/* We always have a CPU 0 at this point (__init) */
if (smp_processor_id()) {
- while (!READ_ONCE(alternatives_applied))
+ while (!READ_ONCE(all_alternatives_applied))
cpu_relax();
isb();
} else {
- BUG_ON(alternatives_applied);
+ BUG_ON(all_alternatives_applied);
__apply_alternatives(&region, false);
/* Barriers provided by the cache flushing */
- WRITE_ONCE(alternatives_applied, 1);
+ WRITE_ONCE(all_alternatives_applied, 1);
}

return 0;
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 1b5b553..dcf5d14 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1068,7 +1068,7 @@ static void cpu_copy_el2regs(const struct arm64_cpu_capabilities *__unused)
* that, freshly-onlined CPUs will set tpidr_el2, so we don't need to
* do anything here.
*/
- if (!alternatives_applied)
+ if (!alternative_is_applied(ARM64_HAS_VIRT_HOST_EXTN))
write_sysreg(read_sysreg(tpidr_el1), tpidr_el2);
}
#endif
--
1.9.1

2018-11-12 12:01:04

by Julien Thierry

Subject: [PATCH v6 04/24] arm/arm64: gic-v3: Add PMR and RPR accessors

Add helper functions to access system registers related to interrupt
priorities: PMR and RPR.

Signed-off-by: Julien Thierry <[email protected]>
Cc: Russell King <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Marc Zyngier <[email protected]>
---
arch/arm/include/asm/arch_gicv3.h | 16 ++++++++++++++++
arch/arm64/include/asm/arch_gicv3.h | 15 +++++++++++++++
2 files changed, 31 insertions(+)

diff --git a/arch/arm/include/asm/arch_gicv3.h b/arch/arm/include/asm/arch_gicv3.h
index 0bd5307..bef0b5d 100644
--- a/arch/arm/include/asm/arch_gicv3.h
+++ b/arch/arm/include/asm/arch_gicv3.h
@@ -34,6 +34,7 @@
#define ICC_SRE __ACCESS_CP15(c12, 0, c12, 5)
#define ICC_IGRPEN1 __ACCESS_CP15(c12, 0, c12, 7)
#define ICC_BPR1 __ACCESS_CP15(c12, 0, c12, 3)
+#define ICC_RPR __ACCESS_CP15(c12, 0, c11, 3)

#define __ICC_AP0Rx(x) __ACCESS_CP15(c12, 0, c8, 4 | x)
#define ICC_AP0R0 __ICC_AP0Rx(0)
@@ -245,6 +246,21 @@ static inline void gic_write_bpr1(u32 val)
write_sysreg(val, ICC_BPR1);
}

+static inline u32 gic_read_pmr(void)
+{
+ return read_sysreg(ICC_PMR);
+}
+
+static inline void gic_write_pmr(u32 val)
+{
+ write_sysreg(val, ICC_PMR);
+}
+
+static inline u32 gic_read_rpr(void)
+{
+ return read_sysreg(ICC_RPR);
+}
+
/*
* Even in 32bit systems that use LPAE, there is no guarantee that the I/O
* interface provides true 64bit atomic accesses, so using strd/ldrd doesn't
diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
index e278f94..37193e2 100644
--- a/arch/arm64/include/asm/arch_gicv3.h
+++ b/arch/arm64/include/asm/arch_gicv3.h
@@ -114,6 +114,21 @@ static inline void gic_write_bpr1(u32 val)
write_sysreg_s(val, SYS_ICC_BPR1_EL1);
}

+static inline u32 gic_read_pmr(void)
+{
+ return read_sysreg_s(SYS_ICC_PMR_EL1);
+}
+
+static inline void gic_write_pmr(u32 val)
+{
+ write_sysreg_s(val, SYS_ICC_PMR_EL1);
+}
+
+static inline u32 gic_read_rpr(void)
+{
+ return read_sysreg_s(SYS_ICC_RPR_EL1);
+}
+
#define gic_read_typer(c) readq_relaxed(c)
#define gic_write_irouter(v, c) writeq_relaxed(v, c)
#define gic_read_lpir(c) readq_relaxed(c)
--
1.9.1

2018-11-12 12:01:42

by Julien Thierry

Subject: [PATCH v6 02/24] arm64: cpufeature: Set SYSREG_GIC_CPUIF as a boot system feature

It is not supported to have some CPUs using GICv3 sysreg CPU interface
while some others do not.

Once ICC_SRE_EL1.SRE is set on a CPU, the bit cannot be cleared. Since
matching this feature require setting ICC_SRE_EL1.SRE, it cannot be
turned off if found on a CPU.

Set the feature as STRICT_BOOT, if boot CPU has it, all other CPUs are
required to have it.

Signed-off-by: Julien Thierry <[email protected]>
Suggested-by: Daniel Thompson <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Marc Zyngier <[email protected]>
---
arch/arm64/kernel/cpufeature.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index af50064..03a9d96 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1149,7 +1149,7 @@ static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused)
{
.desc = "GIC system register CPU interface",
.capability = ARM64_HAS_SYSREG_GIC_CPUIF,
- .type = ARM64_CPUCAP_SYSTEM_FEATURE,
+ .type = ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE,
.matches = has_useable_gicv3_cpuif,
.sys_reg = SYS_ID_AA64PFR0_EL1,
.field_pos = ID_AA64PFR0_GIC_SHIFT,
--
1.9.1

2018-11-12 12:01:56

by Julien Thierry

Subject: [PATCH v6 08/24] arm64: Unmask PMR before going idle

CPU does not received signals for interrupts with a priority masked by
ICC_PMR_EL1. This means the CPU might not come back from a WFI
instruction.

Make sure ICC_PMR_EL1 does not mask interrupts when doing a WFI.

Signed-off-by: Julien Thierry <[email protected]>
Suggested-by: Daniel Thompson <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
---
arch/arm64/mm/proc.S | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)

diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 2c75b0b..3c7064c 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -20,6 +20,7 @@

#include <linux/init.h>
#include <linux/linkage.h>
+#include <linux/irqchip/arm-gic-v3.h>
#include <asm/assembler.h>
#include <asm/asm-offsets.h>
#include <asm/hwcap.h>
@@ -53,10 +54,27 @@
* cpu_do_idle()
*
* Idle the processor (wait for interrupt).
+ *
+ * If the CPU supports priority masking we must do additional work to
+ * ensure that interrupts are not masked at the PMR (because the core will
+ * not wake up if we block the wake up signal in the interrupt controller).
*/
ENTRY(cpu_do_idle)
+alternative_if_not ARM64_HAS_IRQ_PRIO_MASKING
+ dsb sy // WFI may enter a low-power mode
+ wfi
+ ret
+alternative_else
+ mrs x0, daif // save I bit
+ msr daifset, #2 // set I bit
+ mrs_s x1, SYS_ICC_PMR_EL1 // save PMR
+alternative_endif
+ mov x2, #GIC_PRIO_IRQON
+ msr_s SYS_ICC_PMR_EL1, x2 // unmask PMR
dsb sy // WFI may enter a low-power mode
wfi
+ msr_s SYS_ICC_PMR_EL1, x1 // restore PMR
+ msr daif, x0 // restore I bit
ret
ENDPROC(cpu_do_idle)

--
1.9.1

2018-11-12 12:02:10

by Julien Thierry

Subject: [PATCH v6 06/24] arm64: ptrace: Provide definitions for PMR values

Introduce fixed values for PMR that are going to be used to mask and
unmask interrupts by priority. These values are chosent in such a way
that a single bit (GIC_PMR_UNMASKED_BIT) encodes the information whether
interrupts are masked or not.

Signed-off-by: Julien Thierry <[email protected]>
Suggested-by: Daniel Thompson <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
---
arch/arm64/include/asm/ptrace.h | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index fce22c4..ce6998c 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -25,6 +25,12 @@
#define CurrentEL_EL1 (1 << 2)
#define CurrentEL_EL2 (2 << 2)

+/* PMR values used to mask/unmask interrupts */
+#define GIC_PRIO_IRQON 0xf0
+#define GIC_PRIO_STATUS_SHIFT 6
+#define GIC_PRIO_STATUS_BIT (1 << GIC_PRIO_STATUS_SHIFT)
+#define GIC_PRIO_IRQOFF (GIC_PRIO_IRQON ^ GIC_PRIO_STATUS_BIT)
+
/* Additional SPSR bits not exposed in the UABI */
#define PSR_IL_BIT (1 << 20)

--
1.9.1

2018-11-12 12:02:12

by Julien Thierry

Subject: [PATCH v6 05/24] irqchip/gic-v3: Switch to PMR masking before calling IRQ handler

Mask the IRQ priority through PMR and re-enable IRQs at CPU level,
allowing only higher priority interrupts to be received during interrupt
handling.

Signed-off-by: Julien Thierry <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Marc Zyngier <[email protected]>
---
arch/arm/include/asm/arch_gicv3.h | 17 +++++++++++++++++
arch/arm64/include/asm/arch_gicv3.h | 17 +++++++++++++++++
drivers/irqchip/irq-gic-v3.c | 10 ++++++++++
3 files changed, 44 insertions(+)

diff --git a/arch/arm/include/asm/arch_gicv3.h b/arch/arm/include/asm/arch_gicv3.h
index bef0b5d..f6f485f 100644
--- a/arch/arm/include/asm/arch_gicv3.h
+++ b/arch/arm/include/asm/arch_gicv3.h
@@ -363,5 +363,22 @@ static inline void gits_write_vpendbaser(u64 val, void * __iomem addr)

#define gits_read_vpendbaser(c) __gic_readq_nonatomic(c)

+static inline bool gic_prio_masking_enabled(void)
+{
+ return false;
+}
+
+static inline void gic_pmr_mask_irqs(void)
+{
+ /* Should not get called. */
+ WARN_ON_ONCE(true);
+}
+
+static inline void gic_arch_enable_irqs(void)
+{
+ /* Should not get called. */
+ WARN_ON_ONCE(true);
+}
+
#endif /* !__ASSEMBLY__ */
#endif /* !__ASM_ARCH_GICV3_H */
diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
index 37193e2..3f8d5f4 100644
--- a/arch/arm64/include/asm/arch_gicv3.h
+++ b/arch/arm64/include/asm/arch_gicv3.h
@@ -155,5 +155,22 @@ static inline u32 gic_read_rpr(void)
#define gits_write_vpendbaser(v, c) writeq_relaxed(v, c)
#define gits_read_vpendbaser(c) readq_relaxed(c)

+static inline bool gic_prio_masking_enabled(void)
+{
+ return system_supports_irq_prio_masking();
+}
+
+static inline void gic_pmr_mask_irqs(void)
+{
+ /* Should not get called yet. */
+ WARN_ON_ONCE(true);
+}
+
+static inline void gic_arch_enable_irqs(void)
+{
+ /* Should not get called yet. */
+ WARN_ON_ONCE(true);
+}
+
#endif /* __ASSEMBLY__ */
#endif /* __ASM_ARCH_GICV3_H */
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 8f87f40..e5d8c14 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -353,6 +353,11 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
if (likely(irqnr > 15 && irqnr < 1020) || irqnr >= 8192) {
int err;

+ if (gic_prio_masking_enabled()) {
+ gic_pmr_mask_irqs();
+ gic_arch_enable_irqs();
+ }
+
if (static_branch_likely(&supports_deactivate_key))
gic_write_eoir(irqnr);
else
@@ -371,6 +376,11 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
return;
}
if (irqnr < 16) {
+ if (gic_prio_masking_enabled()) {
+ gic_pmr_mask_irqs();
+ gic_arch_enable_irqs();
+ }
+
gic_write_eoir(irqnr);
if (static_branch_likely(&supports_deactivate_key))
gic_write_dir(irqnr);
--
1.9.1

2018-11-12 12:02:21

by Julien Thierry

Subject: [PATCH v6 01/24] arm64: Remove unused daif related functions/macros

There are some helpers to modify PSR.[DAIF] bits that are not referenced
anywhere. The less these bits are available outside of local_irq_*
functions the better.

Get rid of those unused helpers.

Signed-off-by: Julien Thierry <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: James Morse <[email protected]>
---
arch/arm64/include/asm/assembler.h | 10 +---------
arch/arm64/include/asm/daifflags.h | 10 ----------
2 files changed, 1 insertion(+), 19 deletions(-)

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index 6142402..6e76c8e 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -60,16 +60,8 @@
.endm

/*
- * Enable and disable interrupts.
+ * Save/restore interrupts.
*/
- .macro disable_irq
- msr daifset, #2
- .endm
-
- .macro enable_irq
- msr daifclr, #2
- .endm
-
.macro save_and_disable_irq, flags
mrs \flags, daif
msr daifset, #2
diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
index 8d91f22..546bc39 100644
--- a/arch/arm64/include/asm/daifflags.h
+++ b/arch/arm64/include/asm/daifflags.h
@@ -43,16 +43,6 @@ static inline unsigned long local_daif_save(void)
return flags;
}

-static inline void local_daif_unmask(void)
-{
- trace_hardirqs_on();
- asm volatile(
- "msr daifclr, #0xf // local_daif_unmask"
- :
- :
- : "memory");
-}
-
static inline void local_daif_restore(unsigned long flags)
{
if (!arch_irqs_disabled_flags(flags))
--
1.9.1

2018-11-12 12:02:47

by Julien Thierry

Subject: [PATCH v6 03/24] arm64: cpufeature: Add cpufeature for IRQ priority masking

Add a cpufeature indicating whether a cpu supports masking interrupts
by priority.

The feature will be properly enabled in a later patch.

Signed-off-by: Julien Thierry <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
---
arch/arm64/include/asm/cpucaps.h | 3 ++-
arch/arm64/include/asm/cpufeature.h | 6 ++++++
arch/arm64/kernel/cpufeature.c | 23 +++++++++++++++++++++++
3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
index 6e2d254..f367e5c 100644
--- a/arch/arm64/include/asm/cpucaps.h
+++ b/arch/arm64/include/asm/cpucaps.h
@@ -54,7 +54,8 @@
#define ARM64_HAS_CRC32 33
#define ARM64_SSBS 34
#define ARM64_WORKAROUND_1188873 35
+#define ARM64_HAS_IRQ_PRIO_MASKING 36

-#define ARM64_NCAPS 36
+#define ARM64_NCAPS 37

#endif /* __ASM_CPUCAPS_H */
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 7e2ec64..a6e063f 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -514,6 +514,12 @@ static inline bool system_supports_cnp(void)
cpus_have_const_cap(ARM64_HAS_CNP);
}

+static inline bool system_supports_irq_prio_masking(void)
+{
+ return IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) &&
+ cpus_have_const_cap(ARM64_HAS_IRQ_PRIO_MASKING);
+}
+
#define ARM64_SSBD_UNKNOWN -1
#define ARM64_SSBD_FORCE_DISABLE 0
#define ARM64_SSBD_KERNEL 1
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 03a9d96..1b5b553 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1145,6 +1145,14 @@ static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused)
}
#endif /* CONFIG_ARM64_RAS_EXTN */

+#ifdef CONFIG_ARM64_PSEUDO_NMI
+static bool can_use_gic_priorities(const struct arm64_cpu_capabilities *entry,
+ int scope)
+{
+ return false;
+}
+#endif
+
static const struct arm64_cpu_capabilities arm64_features[] = {
{
.desc = "GIC system register CPU interface",
@@ -1368,6 +1376,21 @@ static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused)
.cpu_enable = cpu_enable_cnp,
},
#endif
+#ifdef CONFIG_ARM64_PSEUDO_NMI
+ {
+ /*
+ * Depends on having GICv3
+ */
+ .desc = "IRQ priority masking",
+ .capability = ARM64_HAS_IRQ_PRIO_MASKING,
+ .type = ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE,
+ .matches = can_use_gic_priorities,
+ .sys_reg = SYS_ID_AA64PFR0_EL1,
+ .field_pos = ID_AA64PFR0_GIC_SHIFT,
+ .sign = FTR_UNSIGNED,
+ .min_field_value = 1,
+ },
+#endif
{},
};

--
1.9.1

2018-11-12 12:02:55

by Julien Thierry

Subject: Re: [PATCH v6 00/24] arm64: provide pseudo NMI with GICv3

Hi,

I currently have an issue with my public repository, I'll try to post a
branch to fetch this series from as soon as possible.

Thanks,

Julien

On 12/11/18 11:56, Julien Thierry wrote:
> Hi,
>
> This series is a continuation of the work started by Daniel [1]. The goal
> is to use GICv3 interrupt priorities to simulate an NMI.
>
> The patches depend on the core API for NMIs patches [2].
>
> To achieve this, set two priorities, one for standard interrupts and
> another, higher priority, for NMIs. Whenever we want to disable interrupts,
> we mask the standard priority instead so NMIs can still be raised. Some
> corner cases though still require to actually mask all interrupts
> effectively disabling the NMI.
>
> Daniel Thompson ran some benchmarks [3] on a previous version showing a
> small (<1%) performance drop when using interrupt priorities on Cortex-A53
> and GIC-500.
>
> Currently, only PPIs and SPIs can be set as NMIs. IPIs being currently
> hardcoded IRQ numbers, there isn't a generic interface to set SGIs as NMI
> for now. LPIs being controlled by the ITS cannot be delivered as NMI.
> When an NMI is active on a CPU, no other NMI can be triggered on the CPU.
>
> Requirements to use this:
> - Have GICv3
> - SCR_EL3.FIQ is set to 1 when linux runs or have single security state
> - Select Kernel Feature -> Use ICC system registers for IRQ masking
>
> * Patches 1 and 2 are just a bit of cleanup
>
> * Patch 3 introduces a CPU feature to check if priority masking should be
> used
>
> * Patches 4 and 5 add the support for priority masking in GICv3 driver
>
> * Patches 6 to 11 add the support for priority masking the arch/arm64
> code
>
> * Patches 12 and 13 allow us to apply alternatives earlier in the boot
> process
>
> * Patches 14 to 16 starts the PMR masking on cpu startup and provides
> primitives for arm64 GICv3 driver to perform priority masking
>
> * Patches 17 to 20 Add support for pseudo-NMIs in GICv3 driver
>
> * Patches 21 to 23 Add support for receiving NMIs in arch/arm64
>
> * Patch 24 adds the build config and command line option to enable
> pseudo-NMIs
>
> Changes since v5[4]:
> * Rebased on v4.20-rc2
> * Reorganized patches again
> * Renamed compile option + PMR values defines
> * Write PMR enablement for cpu startup in C
> * Fix wrong array size passed to __apply_alternatives
> * Do not touch PMR in pt_regs when not using irq masking
> * Remove ISB between PMR and IAR -> turns out it is not needed
> * Simplify irqflags code after introducing PMR in it
> * Use ref count API to track PPIs set as NMI
> * Simplify NMI exit path
>
> Changes since V4[5]:
> * Rebased to v4.19-rc1
> * Adapted GIC driver to the core NMI API
> * Added option to disable priority masking on command line
> * Added Daniel's Tested-by on patches related replacing PSR.I toggling with
> PMR masking
> * Fix scope matching for alternative features.
> * Spotted some more places using PSR.I or daif and replaced with generic
> interrupt functions
>
> Changes since V3[6]:
> * Big refactoring. As suggested by Marc Z., some of the bigger patches
> needed to be split into smaller one.
> * Try to reduce the amount of #ifdef for the new feature by introducing
> an individual cpufeature for priority masking
> * Do not track which alternatives have been applied (was a bit dodgy
> anyway), and use an alternative for VHE cpu_enable callback
> * Fix a build failure with arm by adding the correct RPR accessors
> * Added Suggested-by tags for changes from comming or inspired by Daniel's
> series. Do let me know if you feel I missed something and am not giving
> you due credit.
>
> Changes since V2[7]:
> * Series rebase to v4.17-rc6
> * Adapt pathces 1 and 2 to the rework of cpufeatures framework
> * Use the group0 detection scheme in the GICv3 driver to identify
> the priority view, and drop the use of a fake interrupt
> * Add the case for a GIC configured in a single security state
> * Use local_daif_restore instead of local_irq_enable the first time
> we enable interrupts after a bp hardening in the handling of a kernel
> entry. Otherwise PRS.I remains set...
>
> Changes since V1[8]:
> * Series rebased to v4.15-rc8.
> * Check for arm64_early_features in this_cpu_has_cap (spotted by Suzuki).
> * Fix issue where debug exception were not masked when enabling debug in
> mdscr_el1.
>
> Changes since RFC[9]:
> * The series was rebased to v4.15-rc2 which implied some changes mainly
> related to the work on exception entries and daif flags by James Morse.
> - The first patch in the previous series was dropped because no longer
> applicable.
> - With the semantics James introduced of "inheriting" daif flags,
> handling of PMR on exception entry is simplified as PMR is not altered
> by taking an exception and already inherited from previous state.
> - James pointed out that taking a PseudoNMI before reading the FAR_EL1
> register should not be allowed as per the TRM (D10.2.29):
> "FAR_EL1 is made UNKNOWN on an exception return from EL1."
> So in this submission PSR.I bit is cleared only after FAR_EL1 is read.
> * For KVM, only deal with PMR unmasking/restoring in common code, and VHE
> specific code makes sure PSR.I bit is set when necessary.
> * When detecting the GIC priority view (patch 5), wait for an actual
> interrupt instead of trying only once.
>
>
> [1] http://www.spinics.net/lists/arm-kernel/msg525077.html
> [2] https://lkml.org/lkml/2018/11/12/2113
> [3] https://lkml.org/lkml/2018/7/20/803
> [4] https://lkml.org/lkml/2018/8/28/693
> [5] https://lkml.org/lkml/2018/7/24/321
> [6] https://lkml.org/lkml/2018/5/21/276
> [7] https://lkml.org/lkml/2018/1/17/335
> [8] https://www.spinics.net/lists/arm-kernel/msg620763.html
> [9] https://www.spinics.net/lists/arm-kernel/msg610736.html
>
> Cheers,
>
> Julien
>
> -->
>
> Daniel Thompson (1):
> arm64: alternative: Apply alternatives early in boot process
>
> Julien Thierry (23):
> arm64: Remove unused daif related functions/macros
> arm64: cpufeature: Set SYSREG_GIC_CPUIF as a boot system feature
> arm64: cpufeature: Add cpufeature for IRQ priority masking
> arm/arm64: gic-v3: Add PMR and RPR accessors
> irqchip/gic-v3: Switch to PMR masking before calling IRQ handler
> arm64: ptrace: Provide definitions for PMR values
> arm64: Make PMR part of task context
> arm64: Unmask PMR before going idle
> arm64: kvm: Unmask PMR before entering guest
> arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking
> arm64: daifflags: Include PMR in daifflags restore operations
> arm64: alternative: Allow alternative status checking per cpufeature
> irqchip/gic-v3: Factor group0 detection into functions
> arm64: Switch to PMR masking when starting CPUs
> arm64: gic-v3: Implement arch support for priority masking
> irqchip/gic-v3: Detect if GIC can support pseudo-NMIs
> irqchip/gic-v3: Handle pseudo-NMIs
> irqchip/gic: Add functions to access irq priorities
> irqchip/gic-v3: Allow interrupts to be set as pseudo-NMI
> arm64: Handle serror in NMI context
> arm64: Skip preemption when exiting an NMI
> arm64: Skip irqflags tracing for NMI in IRQs disabled context
> arm64: Enable the support of pseudo-NMIs
>
> Documentation/admin-guide/kernel-parameters.txt | 6 +
> Documentation/arm64/booting.txt | 5 +
> arch/arm/include/asm/arch_gicv3.h | 33 ++++
> arch/arm64/Kconfig | 14 ++
> arch/arm64/include/asm/alternative.h | 4 +-
> arch/arm64/include/asm/arch_gicv3.h | 32 +++
> arch/arm64/include/asm/assembler.h | 10 +-
> arch/arm64/include/asm/cpucaps.h | 3 +-
> arch/arm64/include/asm/cpufeature.h | 10 +
> arch/arm64/include/asm/daifflags.h | 20 +-
> arch/arm64/include/asm/efi.h | 3 +-
> arch/arm64/include/asm/irqflags.h | 132 ++++++++++---
> arch/arm64/include/asm/kvm_host.h | 12 ++
> arch/arm64/include/asm/processor.h | 3 +
> arch/arm64/include/asm/ptrace.h | 20 +-
> arch/arm64/kernel/alternative.c | 60 +++++-
> arch/arm64/kernel/asm-offsets.c | 1 +
> arch/arm64/kernel/cpufeature.c | 42 +++-
> arch/arm64/kernel/entry.S | 40 ++++
> arch/arm64/kernel/process.c | 6 +
> arch/arm64/kernel/smp.c | 34 ++++
> arch/arm64/kernel/traps.c | 8 +-
> arch/arm64/kvm/hyp/switch.c | 16 ++
> arch/arm64/mm/proc.S | 18 ++
> drivers/irqchip/irq-gic-common.c | 10 +
> drivers/irqchip/irq-gic-common.h | 2 +
> drivers/irqchip/irq-gic-v3.c | 250 +++++++++++++++++++++---
> include/linux/irqchip/arm-gic-common.h | 2 +-
> 28 files changed, 695 insertions(+), 101 deletions(-)
>
> --
> 1.9.1
>

--
Julien Thierry

2018-11-12 18:01:42

by Suzuki K Poulose

Subject: Re: [PATCH v6 02/24] arm64: cpufeature: Set SYSREG_GIC_CPUIF as a boot system feature

On 12/11/2018 11:56, Julien Thierry wrote:
> It is not supported to have some CPUs using GICv3 sysreg CPU interface
> while some others do not.
>
> Once ICC_SRE_EL1.SRE is set on a CPU, the bit cannot be cleared. Since
> matching this feature require setting ICC_SRE_EL1.SRE, it cannot be
> turned off if found on a CPU.
>
> Set the feature as STRICT_BOOT, if boot CPU has it, all other CPUs are
> required to have it.
>
> Signed-off-by: Julien Thierry <[email protected]>
> Suggested-by: Daniel Thompson <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> ---
> arch/arm64/kernel/cpufeature.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index af50064..03a9d96 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1149,7 +1149,7 @@ static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused)
> {
> .desc = "GIC system register CPU interface",
> .capability = ARM64_HAS_SYSREG_GIC_CPUIF,
> - .type = ARM64_CPUCAP_SYSTEM_FEATURE,
> + .type = ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE,
> .matches = has_useable_gicv3_cpuif,
> .sys_reg = SYS_ID_AA64PFR0_EL1,
> .field_pos = ID_AA64PFR0_GIC_SHIFT,
>

Reviewed-by: Suzuki K Poulose <[email protected]>

2018-11-12 18:03:45

by Suzuki K Poulose

Subject: Re: [PATCH v6 03/24] arm64: cpufeature: Add cpufeature for IRQ priority masking

On 12/11/2018 11:56, Julien Thierry wrote:
> Add a cpufeature indicating whether a cpu supports masking interrupts
> by priority.
>
> The feature will be properly enabled in a later patch.
>
> Signed-off-by: Julien Thierry <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> ---
> arch/arm64/include/asm/cpucaps.h | 3 ++-
> arch/arm64/include/asm/cpufeature.h | 6 ++++++
> arch/arm64/kernel/cpufeature.c | 23 +++++++++++++++++++++++
> 3 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> index 6e2d254..f367e5c 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -54,7 +54,8 @@
> #define ARM64_HAS_CRC32 33
> #define ARM64_SSBS 34
> #define ARM64_WORKAROUND_1188873 35
> +#define ARM64_HAS_IRQ_PRIO_MASKING 36
>
> -#define ARM64_NCAPS 36
> +#define ARM64_NCAPS 37
>
> #endif /* __ASM_CPUCAPS_H */
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 7e2ec64..a6e063f 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -514,6 +514,12 @@ static inline bool system_supports_cnp(void)
> cpus_have_const_cap(ARM64_HAS_CNP);
> }
>
> +static inline bool system_supports_irq_prio_masking(void)
> +{
> + return IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) &&
> + cpus_have_const_cap(ARM64_HAS_IRQ_PRIO_MASKING);
> +}
> +
> #define ARM64_SSBD_UNKNOWN -1
> #define ARM64_SSBD_FORCE_DISABLE 0
> #define ARM64_SSBD_KERNEL 1
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 03a9d96..1b5b553 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1145,6 +1145,14 @@ static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused)
> }
> #endif /* CONFIG_ARM64_RAS_EXTN */
>
> +#ifdef CONFIG_ARM64_PSEUDO_NMI
> +static bool can_use_gic_priorities(const struct arm64_cpu_capabilities *entry,
> + int scope)
> +{
> + return false;
> +}
> +#endif
> +
> static const struct arm64_cpu_capabilities arm64_features[] = {
> {
> .desc = "GIC system register CPU interface",
> @@ -1368,6 +1376,21 @@ static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused)
> .cpu_enable = cpu_enable_cnp,
> },
> #endif
> +#ifdef CONFIG_ARM64_PSEUDO_NMI
> + {
> + /*
> + * Depends on having GICv3
> + */
> + .desc = "IRQ priority masking",
> + .capability = ARM64_HAS_IRQ_PRIO_MASKING,
> + .type = ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE,
> + .matches = can_use_gic_priorities,
> + .sys_reg = SYS_ID_AA64PFR0_EL1,
> + .field_pos = ID_AA64PFR0_GIC_SHIFT,
> + .sign = FTR_UNSIGNED,
> + .min_field_value = 1,
> + },
> +#endif
> {},
> };
>
>

Reviewed-by: Suzuki K Poulose <[email protected]>

2018-11-13 14:46:44

by Julien Thierry

Subject: Re: [PATCH v6 00/24] arm64: provide pseudo NMI with GICv3

Hi,

This series + the core NMI patches can be fetched from:

git clone http://linux-arm.org/linux-jt.git v4.20-pseudo-nmi

Thanks,

Julien

On 12/11/18 11:56, Julien Thierry wrote:
> Hi,
>
> This series is a continuation of the work started by Daniel [1]. The goal
> is to use GICv3 interrupt priorities to simulate an NMI.
>
> The patches depend on the core API for NMIs patches [2].
>
> To achieve this, set two priorities, one for standard interrupts and
> another, higher priority, for NMIs. Whenever we want to disable interrupts,
> we mask the standard priority instead so NMIs can still be raised. Some
> corner cases though still require to actually mask all interrupts
> effectively disabling the NMI.
>
> Daniel Thompson ran some benchmarks [3] on a previous version showing a
> small (<1%) performance drop when using interrupt priorities on Cortex-A53
> and GIC-500.
>
> Currently, only PPIs and SPIs can be set as NMIs. IPIs being currently
> hardcoded IRQ numbers, there isn't a generic interface to set SGIs as NMI
> for now. LPIs being controlled by the ITS cannot be delivered as NMI.
> When an NMI is active on a CPU, no other NMI can be triggered on the CPU.
>
> Requirements to use this:
> - Have GICv3
> - SCR_EL3.FIQ is set to 1 when linux runs or have single security state
> - Select Kernel Feature -> Use ICC system registers for IRQ masking
>
> * Patches 1 and 2 are just a bit of cleanup
>
> * Patch 3 introduces a CPU feature to check if priority masking should be
> used
>
> * Patches 4 and 5 add the support for priority masking in GICv3 driver
>
> * Patches 6 to 11 add the support for priority masking the arch/arm64
> code
>
> * Patches 12 and 13 allow us to apply alternatives earlier in the boot
> process
>
> * Patches 14 to 16 starts the PMR masking on cpu startup and provides
> primitives for arm64 GICv3 driver to perform priority masking
>
> * Patches 17 to 20 Add support for pseudo-NMIs in GICv3 driver
>
> * Patches 21 to 23 Add support for receiving NMIs in arch/arm64
>
> * Patch 24 adds the build config and command line option to enable
> pseudo-NMIs
>
> Changes since v5[4]:
> * Rebased on v4.20-rc2
> * Reorganized patches again
> * Renamed compile option + PMR values defines
> * Write PMR enablement for cpu startup in C
> * Fix wrong array size passed to __apply_alternatives
> * Do not touch PMR in pt_regs when not using irq masking
> * Remove ISB between PMR and IAR -> turns out it is not needed
> * Simplify irqflags code after introducing PMR in it
> * Use ref count API to track PPIs set as NMI
> * Simplify NMI exit path
>
> Changes since V4[5]:
> * Rebased to v4.19-rc1
> * Adapted GIC driver to the core NMI API
> * Added option to disable priority masking on command line
> * Added Daniel's Tested-by on patches related replacing PSR.I toggling with
> PMR masking
> * Fix scope matching for alternative features.
> * Spotted some more places using PSR.I or daif and replaced with generic
> interrupt functions
>
> Changes since V3[6]:
> * Big refactoring. As suggested by Marc Z., some of the bigger patches
> needed to be split into smaller one.
> * Try to reduce the amount of #ifdef for the new feature by introducing
> an individual cpufeature for priority masking
> * Do not track which alternatives have been applied (was a bit dodgy
> anyway), and use an alternative for VHE cpu_enable callback
> * Fix a build failure with arm by adding the correct RPR accessors
> * Added Suggested-by tags for changes from comming or inspired by Daniel's
> series. Do let me know if you feel I missed something and am not giving
> you due credit.
>
> Changes since V2[7]:
> * Series rebase to v4.17-rc6
> * Adapt pathces 1 and 2 to the rework of cpufeatures framework
> * Use the group0 detection scheme in the GICv3 driver to identify
> the priority view, and drop the use of a fake interrupt
> * Add the case for a GIC configured in a single security state
> * Use local_daif_restore instead of local_irq_enable the first time
> we enable interrupts after a bp hardening in the handling of a kernel
> entry. Otherwise PRS.I remains set...
>
> Changes since V1[8]:
> * Series rebased to v4.15-rc8.
> * Check for arm64_early_features in this_cpu_has_cap (spotted by Suzuki).
> * Fix issue where debug exception were not masked when enabling debug in
> mdscr_el1.
>
> Changes since RFC[9]:
> * The series was rebased to v4.15-rc2 which implied some changes mainly
> related to the work on exception entries and daif flags by James Morse.
> - The first patch in the previous series was dropped because no longer
> applicable.
> - With the semantics James introduced of "inheriting" daif flags,
> handling of PMR on exception entry is simplified as PMR is not altered
> by taking an exception and already inherited from previous state.
> - James pointed out that taking a PseudoNMI before reading the FAR_EL1
> register should not be allowed as per the TRM (D10.2.29):
> "FAR_EL1 is made UNKNOWN on an exception return from EL1."
> So in this submission PSR.I bit is cleared only after FAR_EL1 is read.
> * For KVM, only deal with PMR unmasking/restoring in common code, and VHE
> specific code makes sure PSR.I bit is set when necessary.
> * When detecting the GIC priority view (patch 5), wait for an actual
> interrupt instead of trying only once.
>
>
> [1] http://www.spinics.net/lists/arm-kernel/msg525077.html
> [2] https://lkml.org/lkml/2018/11/12/2113
> [3] https://lkml.org/lkml/2018/7/20/803
> [4] https://lkml.org/lkml/2018/8/28/693
> [5] https://lkml.org/lkml/2018/7/24/321
> [6] https://lkml.org/lkml/2018/5/21/276
> [7] https://lkml.org/lkml/2018/1/17/335
> [8] https://www.spinics.net/lists/arm-kernel/msg620763.html
> [9] https://www.spinics.net/lists/arm-kernel/msg610736.html
>
> Cheers,
>
> Julien
>
> -->
>
> Daniel Thompson (1):
> arm64: alternative: Apply alternatives early in boot process
>
> Julien Thierry (23):
> arm64: Remove unused daif related functions/macros
> arm64: cpufeature: Set SYSREG_GIC_CPUIF as a boot system feature
> arm64: cpufeature: Add cpufeature for IRQ priority masking
> arm/arm64: gic-v3: Add PMR and RPR accessors
> irqchip/gic-v3: Switch to PMR masking before calling IRQ handler
> arm64: ptrace: Provide definitions for PMR values
> arm64: Make PMR part of task context
> arm64: Unmask PMR before going idle
> arm64: kvm: Unmask PMR before entering guest
> arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking
> arm64: daifflags: Include PMR in daifflags restore operations
> arm64: alternative: Allow alternative status checking per cpufeature
> irqchip/gic-v3: Factor group0 detection into functions
> arm64: Switch to PMR masking when starting CPUs
> arm64: gic-v3: Implement arch support for priority masking
> irqchip/gic-v3: Detect if GIC can support pseudo-NMIs
> irqchip/gic-v3: Handle pseudo-NMIs
> irqchip/gic: Add functions to access irq priorities
> irqchip/gic-v3: Allow interrupts to be set as pseudo-NMI
> arm64: Handle serror in NMI context
> arm64: Skip preemption when exiting an NMI
> arm64: Skip irqflags tracing for NMI in IRQs disabled context
> arm64: Enable the support of pseudo-NMIs
>
> Documentation/admin-guide/kernel-parameters.txt | 6 +
> Documentation/arm64/booting.txt | 5 +
> arch/arm/include/asm/arch_gicv3.h | 33 ++++
> arch/arm64/Kconfig | 14 ++
> arch/arm64/include/asm/alternative.h | 4 +-
> arch/arm64/include/asm/arch_gicv3.h | 32 +++
> arch/arm64/include/asm/assembler.h | 10 +-
> arch/arm64/include/asm/cpucaps.h | 3 +-
> arch/arm64/include/asm/cpufeature.h | 10 +
> arch/arm64/include/asm/daifflags.h | 20 +-
> arch/arm64/include/asm/efi.h | 3 +-
> arch/arm64/include/asm/irqflags.h | 132 ++++++++++---
> arch/arm64/include/asm/kvm_host.h | 12 ++
> arch/arm64/include/asm/processor.h | 3 +
> arch/arm64/include/asm/ptrace.h | 20 +-
> arch/arm64/kernel/alternative.c | 60 +++++-
> arch/arm64/kernel/asm-offsets.c | 1 +
> arch/arm64/kernel/cpufeature.c | 42 +++-
> arch/arm64/kernel/entry.S | 40 ++++
> arch/arm64/kernel/process.c | 6 +
> arch/arm64/kernel/smp.c | 34 ++++
> arch/arm64/kernel/traps.c | 8 +-
> arch/arm64/kvm/hyp/switch.c | 16 ++
> arch/arm64/mm/proc.S | 18 ++
> drivers/irqchip/irq-gic-common.c | 10 +
> drivers/irqchip/irq-gic-common.h | 2 +
> drivers/irqchip/irq-gic-v3.c | 250 +++++++++++++++++++++---
> include/linux/irqchip/arm-gic-common.h | 2 +-
> 28 files changed, 695 insertions(+), 101 deletions(-)
>
> --
> 1.9.1
>

--
Julien Thierry

2018-11-29 16:29:11

by Mark Rutland

Subject: Re: [PATCH v6 02/24] arm64: cpufeature: Set SYSREG_GIC_CPUIF as a boot system feature

On Mon, Nov 12, 2018 at 11:56:53AM +0000, Julien Thierry wrote:
> It is not supported to have some CPUs using GICv3 sysreg CPU interface
> while some others do not.
>
> Once ICC_SRE_EL1.SRE is set on a CPU, the bit cannot be cleared. Since
> matching this feature require setting ICC_SRE_EL1.SRE, it cannot be
> turned off if found on a CPU.
>
> Set the feature as STRICT_BOOT, if boot CPU has it, all other CPUs are
> required to have it.
>
> Signed-off-by: Julien Thierry <[email protected]>
> Suggested-by: Daniel Thompson <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Marc Zyngier <[email protected]>

Makes sense.

Reviewed-by: Mark Rutland <[email protected]>

Mark.

> ---
> arch/arm64/kernel/cpufeature.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index af50064..03a9d96 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1149,7 +1149,7 @@ static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused)
> {
> .desc = "GIC system register CPU interface",
> .capability = ARM64_HAS_SYSREG_GIC_CPUIF,
> - .type = ARM64_CPUCAP_SYSTEM_FEATURE,
> + .type = ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE,
> .matches = has_useable_gicv3_cpuif,
> .sys_reg = SYS_ID_AA64PFR0_EL1,
> .field_pos = ID_AA64PFR0_GIC_SHIFT,
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2018-11-29 16:30:36

by Mark Rutland

Subject: Re: [PATCH v6 01/24] arm64: Remove unused daif related functions/macros

On Mon, Nov 12, 2018 at 11:56:52AM +0000, Julien Thierry wrote:
> There are some helpers to modify PSR.[DAIF] bits that are not referenced
> anywhere. The less these bits are available outside of local_irq_*
> functions the better.
>
> Get rid of those unused helpers.
>
> Signed-off-by: Julien Thierry <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: James Morse <[email protected]>

Reviewed-by: Mark Rutland <[email protected]>

Mark.

> ---
> arch/arm64/include/asm/assembler.h | 10 +---------
> arch/arm64/include/asm/daifflags.h | 10 ----------
> 2 files changed, 1 insertion(+), 19 deletions(-)
>
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index 6142402..6e76c8e 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -60,16 +60,8 @@
> .endm
>
> /*
> - * Enable and disable interrupts.
> + * Save/restore interrupts.
> */
> - .macro disable_irq
> - msr daifset, #2
> - .endm
> -
> - .macro enable_irq
> - msr daifclr, #2
> - .endm
> -
> .macro save_and_disable_irq, flags
> mrs \flags, daif
> msr daifset, #2
> diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
> index 8d91f22..546bc39 100644
> --- a/arch/arm64/include/asm/daifflags.h
> +++ b/arch/arm64/include/asm/daifflags.h
> @@ -43,16 +43,6 @@ static inline unsigned long local_daif_save(void)
> return flags;
> }
>
> -static inline void local_daif_unmask(void)
> -{
> - trace_hardirqs_on();
> - asm volatile(
> - "msr daifclr, #0xf // local_daif_unmask"
> - :
> - :
> - : "memory");
> -}
> -
> static inline void local_daif_restore(unsigned long flags)
> {
> if (!arch_irqs_disabled_flags(flags))
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2018-11-29 16:34:24

by Mark Rutland

Subject: Re: [PATCH v6 04/24] arm/arm64: gic-v3: Add PMR and RPR accessors

On Mon, Nov 12, 2018 at 11:56:55AM +0000, Julien Thierry wrote:
> Add helper functions to access system registers related to interrupt
> priorities: PMR and RPR.
>
> Signed-off-by: Julien Thierry <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Marc Zyngier <[email protected]>

The AArch32 ICC_RPR encoding looks right per ARM DDI 0487D.a table G7-3,
and the rest looks sane to me.

Reviewed-by: Mark Rutland <[email protected]>

Mark.

> ---
> arch/arm/include/asm/arch_gicv3.h | 16 ++++++++++++++++
> arch/arm64/include/asm/arch_gicv3.h | 15 +++++++++++++++
> 2 files changed, 31 insertions(+)
>
> diff --git a/arch/arm/include/asm/arch_gicv3.h b/arch/arm/include/asm/arch_gicv3.h
> index 0bd5307..bef0b5d 100644
> --- a/arch/arm/include/asm/arch_gicv3.h
> +++ b/arch/arm/include/asm/arch_gicv3.h
> @@ -34,6 +34,7 @@
> #define ICC_SRE __ACCESS_CP15(c12, 0, c12, 5)
> #define ICC_IGRPEN1 __ACCESS_CP15(c12, 0, c12, 7)
> #define ICC_BPR1 __ACCESS_CP15(c12, 0, c12, 3)
> +#define ICC_RPR __ACCESS_CP15(c12, 0, c11, 3)
>
> #define __ICC_AP0Rx(x) __ACCESS_CP15(c12, 0, c8, 4 | x)
> #define ICC_AP0R0 __ICC_AP0Rx(0)
> @@ -245,6 +246,21 @@ static inline void gic_write_bpr1(u32 val)
> write_sysreg(val, ICC_BPR1);
> }
>
> +static inline u32 gic_read_pmr(void)
> +{
> + return read_sysreg(ICC_PMR);
> +}
> +
> +static inline void gic_write_pmr(u32 val)
> +{
> + write_sysreg(val, ICC_PMR);
> +}
> +
> +static inline u32 gic_read_rpr(void)
> +{
> + return read_sysreg(ICC_RPR);
> +}
> +
> /*
> * Even in 32bit systems that use LPAE, there is no guarantee that the I/O
> * interface provides true 64bit atomic accesses, so using strd/ldrd doesn't
> diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
> index e278f94..37193e2 100644
> --- a/arch/arm64/include/asm/arch_gicv3.h
> +++ b/arch/arm64/include/asm/arch_gicv3.h
> @@ -114,6 +114,21 @@ static inline void gic_write_bpr1(u32 val)
> write_sysreg_s(val, SYS_ICC_BPR1_EL1);
> }
>
> +static inline u32 gic_read_pmr(void)
> +{
> + return read_sysreg_s(SYS_ICC_PMR_EL1);
> +}
> +
> +static inline void gic_write_pmr(u32 val)
> +{
> + write_sysreg_s(val, SYS_ICC_PMR_EL1);
> +}
> +
> +static inline u32 gic_read_rpr(void)
> +{
> + return read_sysreg_s(SYS_ICC_RPR_EL1);
> +}
> +
> #define gic_read_typer(c) readq_relaxed(c)
> #define gic_write_irouter(v, c) writeq_relaxed(v, c)
> #define gic_read_lpir(c) readq_relaxed(c)
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2018-11-29 16:47:52

by Mark Rutland

Subject: Re: [PATCH v6 07/24] arm64: Make PMR part of task context

On Mon, Nov 12, 2018 at 11:56:58AM +0000, Julien Thierry wrote:
> If ICC_PMR_EL1 is used to mask interrupts, its value should be
> saved/restored whenever a task is context switched out/in or
> gets an exception.
>
> Add PMR to the registers to save in the pt_regs struct upon kernel entry,
> and restore it before ERET. Also, initialize it to a sane value when
> creating new tasks.

Could you please elaborate on when this matters?

Does this actually matter for context-switch? Can we do that in a
pseudo-NMI handler?

Or does this only matter for exception entry/return, and not
context-switch?

Thanks,
Mark.

> Signed-off-by: Julien Thierry <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Dave Martin <[email protected]>
> ---
> arch/arm64/include/asm/processor.h | 3 +++
> arch/arm64/include/asm/ptrace.h | 14 +++++++++++---
> arch/arm64/kernel/asm-offsets.c | 1 +
> arch/arm64/kernel/entry.S | 13 +++++++++++++
> arch/arm64/kernel/process.c | 6 ++++++
> 5 files changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> index 6b0d4df..b2315ef 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -168,6 +168,9 @@ static inline void start_thread_common(struct pt_regs *regs, unsigned long pc)
> memset(regs, 0, sizeof(*regs));
> forget_syscall(regs);
> regs->pc = pc;
> +
> + if (system_supports_irq_prio_masking())
> + regs->pmr_save = GIC_PRIO_IRQON;
> }
>
> static inline void start_thread(struct pt_regs *regs, unsigned long pc,
> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> index ce6998c..0ad46f5 100644
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -19,6 +19,8 @@
> #ifndef __ASM_PTRACE_H
> #define __ASM_PTRACE_H
>
> +#include <asm/cpufeature.h>
> +
> #include <uapi/asm/ptrace.h>
>
> /* Current Exception Level values, as contained in CurrentEL */
> @@ -173,7 +175,8 @@ struct pt_regs {
> #endif
>
> u64 orig_addr_limit;
> - u64 unused; // maintain 16 byte alignment
> + /* Only valid when ARM64_HAS_IRQ_PRIO_MASKING is enabled. */
> + u64 pmr_save;
> u64 stackframe[2];
> };
>
> @@ -208,8 +211,13 @@ static inline void forget_syscall(struct pt_regs *regs)
> #define processor_mode(regs) \
> ((regs)->pstate & PSR_MODE_MASK)
>
> -#define interrupts_enabled(regs) \
> - (!((regs)->pstate & PSR_I_BIT))
> +#define irqs_priority_unmasked(regs) \
> + (system_supports_irq_prio_masking() ? \
> + (regs)->pmr_save & GIC_PRIO_STATUS_BIT : \
> + true)
> +
> +#define interrupts_enabled(regs) \
> + (!((regs)->pstate & PSR_I_BIT) && irqs_priority_unmasked(regs))
>
> #define fast_interrupts_enabled(regs) \
> (!((regs)->pstate & PSR_F_BIT))
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 323aeb5..bab4122 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -78,6 +78,7 @@ int main(void)
> DEFINE(S_ORIG_X0, offsetof(struct pt_regs, orig_x0));
> DEFINE(S_SYSCALLNO, offsetof(struct pt_regs, syscallno));
> DEFINE(S_ORIG_ADDR_LIMIT, offsetof(struct pt_regs, orig_addr_limit));
> + DEFINE(S_PMR_SAVE, offsetof(struct pt_regs, pmr_save));
> DEFINE(S_STACKFRAME, offsetof(struct pt_regs, stackframe));
> DEFINE(S_FRAME_SIZE, sizeof(struct pt_regs));
> BLANK();
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 039144e..eb8120e 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -249,6 +249,12 @@ alternative_else_nop_endif
> msr sp_el0, tsk
> .endif
>
> + /* Save pmr */
> +alternative_if ARM64_HAS_IRQ_PRIO_MASKING
> + mrs_s x20, SYS_ICC_PMR_EL1
> + str x20, [sp, #S_PMR_SAVE]
> +alternative_else_nop_endif
> +
> /*
> * Registers that may be useful after this macro is invoked:
> *
> @@ -269,6 +275,13 @@ alternative_else_nop_endif
> /* No need to restore UAO, it will be restored from SPSR_EL1 */
> .endif
>
> + /* Restore pmr */
> +alternative_if ARM64_HAS_IRQ_PRIO_MASKING
> + ldr x20, [sp, #S_PMR_SAVE]
> + msr_s SYS_ICC_PMR_EL1, x20
> + dsb sy
> +alternative_else_nop_endif
> +
> ldp x21, x22, [sp, #S_PC] // load ELR, SPSR
> .if \el == 0
> ct_user_enter
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index d9a4c2d..71e8850 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -231,6 +231,9 @@ void __show_regs(struct pt_regs *regs)
>
> printk("sp : %016llx\n", sp);
>
> + if (system_supports_irq_prio_masking())
> + printk("pmr_save: %08llx\n", regs->pmr_save);
> +
> i = top_reg;
>
> while (i >= 0) {
> @@ -362,6 +365,9 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
> if (arm64_get_ssbd_state() == ARM64_SSBD_FORCE_DISABLE)
> childregs->pstate |= PSR_SSBS_BIT;
>
> + if (system_supports_irq_prio_masking())
> + childregs->pmr_save = GIC_PRIO_IRQON;
> +
> p->thread.cpu_context.x19 = stack_start;
> p->thread.cpu_context.x20 = stk_sz;
> }
> --
> 1.9.1
>

2018-11-29 17:14:45

by Mark Rutland

Subject: Re: [PATCH v6 03/24] arm64: cpufeature: Add cpufeature for IRQ priority masking

On Mon, Nov 12, 2018 at 11:56:54AM +0000, Julien Thierry wrote:
> Add a cpufeature indicating whether a cpu supports masking interrupts
> by priority.
>
> The feature will be properly enabled in a later patch.
>
> Signed-off-by: Julien Thierry <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> ---
> arch/arm64/include/asm/cpucaps.h | 3 ++-
> arch/arm64/include/asm/cpufeature.h | 6 ++++++
> arch/arm64/kernel/cpufeature.c | 23 +++++++++++++++++++++++
> 3 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> index 6e2d254..f367e5c 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -54,7 +54,8 @@
> #define ARM64_HAS_CRC32 33
> #define ARM64_SSBS 34
> #define ARM64_WORKAROUND_1188873 35
> +#define ARM64_HAS_IRQ_PRIO_MASKING 36
>
> -#define ARM64_NCAPS 36
> +#define ARM64_NCAPS 37
>
> #endif /* __ASM_CPUCAPS_H */
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 7e2ec64..a6e063f 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -514,6 +514,12 @@ static inline bool system_supports_cnp(void)
> cpus_have_const_cap(ARM64_HAS_CNP);
> }
>
> +static inline bool system_supports_irq_prio_masking(void)
> +{
> + return IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) &&
> + cpus_have_const_cap(ARM64_HAS_IRQ_PRIO_MASKING);
> +}

This should probably be s/supports/uses/.

With that:

Reviewed-by: Mark Rutland <[email protected]>

Mark.

> +
> #define ARM64_SSBD_UNKNOWN -1
> #define ARM64_SSBD_FORCE_DISABLE 0
> #define ARM64_SSBD_KERNEL 1
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 03a9d96..1b5b553 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1145,6 +1145,14 @@ static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused)
> }
> #endif /* CONFIG_ARM64_RAS_EXTN */
>
> +#ifdef CONFIG_ARM64_PSEUDO_NMI
> +static bool can_use_gic_priorities(const struct arm64_cpu_capabilities *entry,
> + int scope)
> +{
> + return false;
> +}
> +#endif
> +
> static const struct arm64_cpu_capabilities arm64_features[] = {
> {
> .desc = "GIC system register CPU interface",
> @@ -1368,6 +1376,21 @@ static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused)
> .cpu_enable = cpu_enable_cnp,
> },
> #endif
> +#ifdef CONFIG_ARM64_PSEUDO_NMI
> + {
> + /*
> + * Depends on having GICv3
> + */
> + .desc = "IRQ priority masking",
> + .capability = ARM64_HAS_IRQ_PRIO_MASKING,
> + .type = ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE,
> + .matches = can_use_gic_priorities,
> + .sys_reg = SYS_ID_AA64PFR0_EL1,
> + .field_pos = ID_AA64PFR0_GIC_SHIFT,
> + .sign = FTR_UNSIGNED,
> + .min_field_value = 1,
> + },
> +#endif
> {},
> };
>
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2018-11-29 18:15:57

by Mark Rutland

Subject: Re: [PATCH v6 05/24] irqchip/gic-v3: Switch to PMR masking before calling IRQ handler

On Mon, Nov 12, 2018 at 11:56:56AM +0000, Julien Thierry wrote:
> Mask the IRQ priority through PMR and re-enable IRQs at CPU level,
> allowing only higher priority interrupts to be received during interrupt
> handling.
>
> Signed-off-by: Julien Thierry <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> ---
> arch/arm/include/asm/arch_gicv3.h | 17 +++++++++++++++++
> arch/arm64/include/asm/arch_gicv3.h | 17 +++++++++++++++++
> drivers/irqchip/irq-gic-v3.c | 10 ++++++++++
> 3 files changed, 44 insertions(+)
>
> diff --git a/arch/arm/include/asm/arch_gicv3.h b/arch/arm/include/asm/arch_gicv3.h
> index bef0b5d..f6f485f 100644
> --- a/arch/arm/include/asm/arch_gicv3.h
> +++ b/arch/arm/include/asm/arch_gicv3.h
> @@ -363,5 +363,22 @@ static inline void gits_write_vpendbaser(u64 val, void * __iomem addr)
>
> #define gits_read_vpendbaser(c) __gic_readq_nonatomic(c)
>
> +static inline bool gic_prio_masking_enabled(void)
> +{
> + return false;
> +}
> +
> +static inline void gic_pmr_mask_irqs(void)
> +{
> + /* Should not get called. */
> + WARN_ON_ONCE(true);
> +}
> +
> +static inline void gic_arch_enable_irqs(void)
> +{
> + /* Should not get called. */
> + WARN_ON_ONCE(true);
> +}
> +
> #endif /* !__ASSEMBLY__ */
> #endif /* !__ASM_ARCH_GICV3_H */
> diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
> index 37193e2..3f8d5f4 100644
> --- a/arch/arm64/include/asm/arch_gicv3.h
> +++ b/arch/arm64/include/asm/arch_gicv3.h
> @@ -155,5 +155,22 @@ static inline u32 gic_read_rpr(void)
> #define gits_write_vpendbaser(v, c) writeq_relaxed(v, c)
> #define gits_read_vpendbaser(c) readq_relaxed(c)
>
> +static inline bool gic_prio_masking_enabled(void)
> +{
> + return system_supports_irq_prio_masking();
> +}
> +
> +static inline void gic_pmr_mask_irqs(void)
> +{
> + /* Should not get called yet. */
> + WARN_ON_ONCE(true);
> +}
> +
> +static inline void gic_arch_enable_irqs(void)
> +{
> + /* Should not get called yet. */
> + WARN_ON_ONCE(true);
> +}
> +
> #endif /* __ASSEMBLY__ */
> #endif /* __ASM_ARCH_GICV3_H */
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 8f87f40..e5d8c14 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -353,6 +353,11 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
> if (likely(irqnr > 15 && irqnr < 1020) || irqnr >= 8192) {
> int err;
>
> + if (gic_prio_masking_enabled()) {
> + gic_pmr_mask_irqs();
> + gic_arch_enable_irqs();
> + }

IIUC, if we have two pNMIs, this will allow one to preempt another, e.g.

< pNMI#1 asserted >

< CPU takes IRQ exception for pNMI #1>

irqnr = gic_read_iar(); // pNMI #1

< pNMI#2 asserted >

// masks IRQs at GIC, leaves other pNMIs unmasked
gic_pmr_mask_irqs()
gic_arch_enable_irqs();

...

< CPU takes IRQ exception for pNMI #2 >

... or is that not a problem? Is the NMI code re-entrant?

> +
> if (static_branch_likely(&supports_deactivate_key))
> gic_write_eoir(irqnr);
> else
> @@ -371,6 +376,11 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
> return;
> }
> if (irqnr < 16) {
> + if (gic_prio_masking_enabled()) {
> + gic_pmr_mask_irqs();
> + gic_arch_enable_irqs();
> + }

Can we pull this above the two cases, or is there a problem with doing
this for spurious IRQs?

Where is the corresponding unmask of the PMR, and disable of IRQs? It's
difficult to follow the logic if that's in another patch.

Thanks,
Mark.

2018-11-29 18:32:54

by Mark Rutland

Subject: Re: [PATCH v6 06/24] arm64: ptrace: Provide definitions for PMR values

On Mon, Nov 12, 2018 at 11:56:57AM +0000, Julien Thierry wrote:
> Introduce fixed values for PMR that are going to be used to mask and
> unmask interrupts by priority. These values are chosent in such a way

Nit: s/chosent/chosen/

> that a single bit (GIC_PMR_UNMASKED_BIT) encodes the information whether
> interrupts are masked or not.

There's no GIC_PMR_UNMASKED_BIT in this patch. Should that be
GIC_PRIO_STATUS_BIT?

> Signed-off-by: Julien Thierry <[email protected]>
> Suggested-by: Daniel Thompson <[email protected]>
> Cc: Oleg Nesterov <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> ---
> arch/arm64/include/asm/ptrace.h | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> index fce22c4..ce6998c 100644
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -25,6 +25,12 @@
> #define CurrentEL_EL1 (1 << 2)
> #define CurrentEL_EL2 (2 << 2)
>
> +/* PMR values used to mask/unmask interrupts */
> +#define GIC_PRIO_IRQON 0xf0
> +#define GIC_PRIO_STATUS_SHIFT 6
> +#define GIC_PRIO_STATUS_BIT (1 << GIC_PRIO_STATUS_SHIFT)
> +#define GIC_PRIO_IRQOFF (GIC_PRIO_IRQON ^ GIC_PRIO_STATUS_BIT)

Could you elaborate on the GIC priority logic a bit?

Are lower numbers higher priority?

Are there restrictions on valid PMR values?

IIUC GIC_PRIO_IRQOFF is 0xb0 (aka 0b10110000), which seems a little
surprising. I'd have expected that we'd use the most signficant bit.


Thanks,
Mark.

2018-11-29 18:41:43

by Mark Rutland

Subject: Re: [PATCH v6 08/24] arm64: Unmask PMR before going idle

On Mon, Nov 12, 2018 at 11:56:59AM +0000, Julien Thierry wrote:
> CPU does not received signals for interrupts with a priority masked by
> ICC_PMR_EL1. This means the CPU might not come back from a WFI
> instruction.
>
> Make sure ICC_PMR_EL1 does not mask interrupts when doing a WFI.
>
> Signed-off-by: Julien Thierry <[email protected]>
> Suggested-by: Daniel Thompson <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> ---
> arch/arm64/mm/proc.S | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index 2c75b0b..3c7064c 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -20,6 +20,7 @@
>
> #include <linux/init.h>
> #include <linux/linkage.h>
> +#include <linux/irqchip/arm-gic-v3.h>
> #include <asm/assembler.h>
> #include <asm/asm-offsets.h>
> #include <asm/hwcap.h>
> @@ -53,10 +54,27 @@
> * cpu_do_idle()
> *
> * Idle the processor (wait for interrupt).
> + *
> + * If the CPU supports priority masking we must do additional work to
> + * ensure that interrupts are not masked at the PMR (because the core will
> + * not wake up if we block the wake up signal in the interrupt controller).
> */
> ENTRY(cpu_do_idle)
> +alternative_if_not ARM64_HAS_IRQ_PRIO_MASKING
> + dsb sy // WFI may enter a low-power mode
> + wfi
> + ret
> +alternative_else
> + mrs x0, daif // save I bit
> + msr daifset, #2 // set I bit
> + mrs_s x1, SYS_ICC_PMR_EL1 // save PMR
> +alternative_endif
> + mov x2, #GIC_PRIO_IRQON
> + msr_s SYS_ICC_PMR_EL1, x2 // unmask PMR
> dsb sy // WFI may enter a low-power mode

Is the DSB SY sufficient and necessary to synchronise the update of
SYS_ICC_PMR_EL1? We don't need an ISB too?

> wfi
> + msr_s SYS_ICC_PMR_EL1, x1 // restore PMR

Likewise, we don't need any barriers here before we poke DAIF?

> + msr daif, x0 // restore I bit
> ret
> ENDPROC(cpu_do_idle)

If we build without CONFIG_ARM64_PSEUDO_NMI surely we don't want to emit
the alternative?

How about we move this to C, and have something like the below?

For the !CONFIG_ARM64_PSEUDO_NMI case it generates identical assembly to the
existing cpu_do_idle(). Note that I've assumed we don't need barriers, which
(as above) I'm not certain of.

Thanks,
Mark.

---->8----
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 7f1628effe6d..ccd2ad8c5e2f 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -73,6 +73,40 @@ EXPORT_SYMBOL_GPL(pm_power_off);

void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);

+static inline void __cpu_do_idle(void)
+{
+ /* WFI may enter a low-power mode */
+ dsb(sy);
+ wfi();
+}
+
+/*
+ * When using priority masking we need to take extra care, etc.
+ */
+static inline void __cpu_do_idle_irqprio(void)
+{
+ unsigned long flags = arch_local_irq_save();
+ unsigned long pmr = gic_read_pmr();
+
+ gic_write_pmr(GIC_PRIO_IRQON);
+
+ __cpu_do_idle();
+
+ gic_write_pmr(pmr);
+ arch_local_irq_enable();
+}
+
+/*
+ * Idle the processor (wait for interrupt).
+ */
+void cpu_do_idle(void)
+{
+ if (system_uses_irq_prio_masking())
+ __cpu_do_idle_irqprio();
+ else
+ __cpu_do_idle();
+}
+
/*
* This is our default idle handler.
*/
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 03646e6a2ef4..38c0171e52e2 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -49,17 +49,6 @@

#define MAIR(attr, mt) ((attr) << ((mt) * 8))

-/*
- * cpu_do_idle()
- *
- * Idle the processor (wait for interrupt).
- */
-ENTRY(cpu_do_idle)
- dsb sy // WFI may enter a low-power mode
- wfi
- ret
-ENDPROC(cpu_do_idle)
-
#ifdef CONFIG_CPU_PM
/**
* cpu_do_suspend - save CPU registers context

2018-11-30 08:56:29

by Julien Thierry

Subject: Re: [PATCH v6 06/24] arm64: ptrace: Provide definitions for PMR values

On 29/11/18 16:40, Mark Rutland wrote:
> On Mon, Nov 12, 2018 at 11:56:57AM +0000, Julien Thierry wrote:
>> Introduce fixed values for PMR that are going to be used to mask and
>> unmask interrupts by priority. These values are chosent in such a way
>
> Nit: s/chosent/chosen/
>
>> that a single bit (GIC_PMR_UNMASKED_BIT) encodes the information whether
>> interrupts are masked or not.
>
> There's no GIC_PMR_UNMASKED_BIT in this patch. Should that be
> GIC_PRIO_STATUS_BIT?
>

Yep, forgot to update the commit message when renaming, thanks.

>> Signed-off-by: Julien Thierry <[email protected]>
>> Suggested-by: Daniel Thompson <[email protected]>
>> Cc: Oleg Nesterov <[email protected]>
>> Cc: Catalin Marinas <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> ---
>> arch/arm64/include/asm/ptrace.h | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
>> index fce22c4..ce6998c 100644
>> --- a/arch/arm64/include/asm/ptrace.h
>> +++ b/arch/arm64/include/asm/ptrace.h
>> @@ -25,6 +25,12 @@
>> #define CurrentEL_EL1 (1 << 2)
>> #define CurrentEL_EL2 (2 << 2)
>>
>> +/* PMR values used to mask/unmask interrupts */
>> +#define GIC_PRIO_IRQON 0xf0
>> +#define GIC_PRIO_STATUS_SHIFT 6
>> +#define GIC_PRIO_STATUS_BIT (1 << GIC_PRIO_STATUS_SHIFT)
>> +#define GIC_PRIO_IRQOFF (GIC_PRIO_IRQON ^ GIC_PRIO_STATUS_BIT)
>
> Could you elaborate on the GIC priority logic a bit?
>

Yes, I'll give details below.

> Are lower numbers higher priority?
>

Yes, that is the case.

> Are there restrictions on valid PMR values?
>

Yes, there are at most 8 priority bits but implementations are free to
implement a number of priority bits:
- between 5 and 8 when GIC runs two security states (bits [7:3] always
being implemented and [2:0] being optional), but non-secure side is
always deprived or the lowest implemented bit
- between 4 and 8 when GIC runs only one security state (bits [7:4]
implemented, bits [3:0] optional)

This is detailed in section 4.8 "Interrupt prioritization" of the GICv3
architecture specification.

So Linux should always be able to see bits [7:4].

> IIUC GIC_PRIO_IRQOFF is 0xb0 (aka 0b10110000), which seems a little
> surprising. I'd have expected that we'd use the most signficant bit.
>

So, re-reading the GICv3 spec, I believe this sparked from a confusion...

The idea was that the GICv3 specification would recommend to keep
non-secure group-1 interrupts at a lower priority that group-0 (and
secure group-1 interrupts) interrupts, and to do so the idea was to
always keep bit[7] == 1 for non-secure group-1.

So, we would need to have priority bit[7] == 1 for both normal
interrupts and pseudo-NMIs, and using the most significant bit to mask
would mean masking pseudo-NMIs as well.

However, I only find mention of this in the notes of section 4.8.6
"Software accesses of interrupt priority". The section only applies to
GIC with two security states, and the recommendation of writing
non-secure group-1 priorities with bit[7] == 1 is only directed at
writes from the secure side. From the non-secure side, the GIC already
does some magic to enforce that the value kept in the distributor has
bit[7] == 1.

So, I believe that from the non-secure point of view, we could define
pseudo-NMI priority as e.g. 0x40 (which the GIC will convert to 0xa0)
and use the most significant bit of PMR to mask normal interrupts which
would be more intuitive.

Marc, as GIC expert do you agree with this? Or is there a reason we
should keep bit[7] == 1 for non-secure group-1 priorities?

Thanks,

--
Julien Thierry

2018-11-30 09:19:55

by Julien Thierry

Subject: Re: [PATCH v6 05/24] irqchip/gic-v3: Switch to PMR masking before calling IRQ handler

On 29/11/18 18:12, Mark Rutland wrote:
> On Mon, Nov 12, 2018 at 11:56:56AM +0000, Julien Thierry wrote:
>> Mask the IRQ priority through PMR and re-enable IRQs at CPU level,
>> allowing only higher priority interrupts to be received during interrupt
>> handling.
>>
>> Signed-off-by: Julien Thierry <[email protected]>
>> Cc: Catalin Marinas <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: Jason Cooper <[email protected]>
>> Cc: Marc Zyngier <[email protected]>
>> ---
>> arch/arm/include/asm/arch_gicv3.h | 17 +++++++++++++++++
>> arch/arm64/include/asm/arch_gicv3.h | 17 +++++++++++++++++
>> drivers/irqchip/irq-gic-v3.c | 10 ++++++++++
>> 3 files changed, 44 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/arch_gicv3.h b/arch/arm/include/asm/arch_gicv3.h
>> index bef0b5d..f6f485f 100644
>> --- a/arch/arm/include/asm/arch_gicv3.h
>> +++ b/arch/arm/include/asm/arch_gicv3.h
>> @@ -363,5 +363,22 @@ static inline void gits_write_vpendbaser(u64 val, void * __iomem addr)
>>
>> #define gits_read_vpendbaser(c) __gic_readq_nonatomic(c)
>>
>> +static inline bool gic_prio_masking_enabled(void)
>> +{
>> + return false;
>> +}
>> +
>> +static inline void gic_pmr_mask_irqs(void)
>> +{
>> + /* Should not get called. */
>> + WARN_ON_ONCE(true);
>> +}
>> +
>> +static inline void gic_arch_enable_irqs(void)
>> +{
>> + /* Should not get called. */
>> + WARN_ON_ONCE(true);
>> +}
>> +
>> #endif /* !__ASSEMBLY__ */
>> #endif /* !__ASM_ARCH_GICV3_H */
>> diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
>> index 37193e2..3f8d5f4 100644
>> --- a/arch/arm64/include/asm/arch_gicv3.h
>> +++ b/arch/arm64/include/asm/arch_gicv3.h
>> @@ -155,5 +155,22 @@ static inline u32 gic_read_rpr(void)
>> #define gits_write_vpendbaser(v, c) writeq_relaxed(v, c)
>> #define gits_read_vpendbaser(c) readq_relaxed(c)
>>
>> +static inline bool gic_prio_masking_enabled(void)
>> +{
>> + return system_supports_irq_prio_masking();
>> +}
>> +
>> +static inline void gic_pmr_mask_irqs(void)
>> +{
>> + /* Should not get called yet. */
>> + WARN_ON_ONCE(true);
>> +}
>> +
>> +static inline void gic_arch_enable_irqs(void)
>> +{
>> + /* Should not get called yet. */
>> + WARN_ON_ONCE(true);
>> +}
>> +
>> #endif /* __ASSEMBLY__ */
>> #endif /* __ASM_ARCH_GICV3_H */
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index 8f87f40..e5d8c14 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -353,6 +353,11 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
>> if (likely(irqnr > 15 && irqnr < 1020) || irqnr >= 8192) {
>> int err;
>>
>> + if (gic_prio_masking_enabled()) {
>> + gic_pmr_mask_irqs();
>> + gic_arch_enable_irqs();
>> + }
>
> IIUC, if we have two pNMIs, this will allow one to preempt another, e.g.
>
> < pNMI#1 asserted >
>
> < CPU takes IRQ exception for pNMI #1>
>
> irqnr = gic_read_iar(); // pNMI #1
>
> < pNMI#2 asserted >
>
> // masks IRQs at GIC, leaves other pNMIs unmasked
> gic_pmr_mask_irqs()
> gic_arch_enable_irqs();
>
> ...
>
> < CPU takes IRQ exception for pNMI #2 >
>
> ... or is that not a problem? Is the NMI code re-entrant?

At this patch stage, the GICv3 handling code is not supporting
pseudo-NMIs yet, only using interrupt priorities.

I introduce this in patch 18: irqchip/gic-v3: Handle pseudo-NMIs.

And yes, this would be an issue but only after gic_write_eoir. Once an
interrupt is running, its priority becomes the running priority and only
an interrupt with higher priority can preempt it. When we do the eoir,
the running priority is dropped meaning any interrupt can preempt (if
PSR.I is cleared and its priority is not masked by PMR).

What is done in the later patch for pseudo-NMIs is that we don't switch
to PMR masking and leave the I bit set since we don't want anything to
preempt it.

>
>> +
>> if (static_branch_likely(&supports_deactivate_key))
>> gic_write_eoir(irqnr);
>> else
>> @@ -371,6 +376,11 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
>> return;
>> }
>> if (irqnr < 16) {
>> + if (gic_prio_masking_enabled()) {
>> + gic_pmr_mask_irqs();
>> + gic_arch_enable_irqs();
>> + }
>
> Can we pull this above the two cases, or is there a problem with doing
> this for spurious IRQs?
>

So the reason I split this is to avoid doing it for NMIs. Otherwise we
would mask PMR, clear PSR.I and once we know we have an NMI set PSR.I again.

An alternative to it would be to check whether we have an NMI before we
know the type of interrupt we have, which would be unnecessary for IPIs
and of course spurious interrupts.

Maybe I can always mask PMR and clear PSR.I (after the interrupt ack) at
this stage and then do the necessary modifications once pseudo-NMI
handling code is added?

> Where is the corresponding unmask of the PMR, and disable of IRQs? It's
> difficult to follow the logic if that's in another patch.
>
In patch 7: arm64: Make PMR part of task context

The arch/arm64 code becomes responsible for saving/restoring the value
of PMR upon exception entry/return.

I'm not really sure whether it makes sense to merge both patches though.
Would explaining that PMR will get restored on irq return in the commit
message be sufficient?

Thanks,

--
Julien Thierry

2018-11-30 09:26:21

by Julien Thierry

Subject: Re: [PATCH v6 07/24] arm64: Make PMR part of task context

On 29/11/18 16:46, Mark Rutland wrote:
> On Mon, Nov 12, 2018 at 11:56:58AM +0000, Julien Thierry wrote:
>> If ICC_PMR_EL1 is used to mask interrupts, its value should be
>> saved/restored whenever a task is context switched out/in or
>> gets an exception.
>>
>> Add PMR to the registers to save in the pt_regs struct upon kernel entry,
>> and restore it before ERET. Also, initialize it to a sane value when
>> creating new tasks.
>
> Could you please elaborate on when this matters?
>
> Does this actually matter for context-switch? Can we do that in a
> pseudo-NMI handler?
>
> Or does this only matter for exception entry/return, and not
> context-switch?
>

Yes, PMR becomes an equivalent of PSR.I and in the same way it will need
to be saved/restored on exception entry/return (except this is not done
by the architecture for PMR).

It is also necessary when context switching to a task that was preempted
in exception context (el1_preempt, return from syscall, ...). In the
same way as spsr_el1.

I'll update the commit message.

Thanks,

Julien

>
>> Signed-off-by: Julien Thierry <[email protected]>
>> Cc: Catalin Marinas <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> Cc: Dave Martin <[email protected]>
>> ---
>> arch/arm64/include/asm/processor.h | 3 +++
>> arch/arm64/include/asm/ptrace.h | 14 +++++++++++---
>> arch/arm64/kernel/asm-offsets.c | 1 +
>> arch/arm64/kernel/entry.S | 13 +++++++++++++
>> arch/arm64/kernel/process.c | 6 ++++++
>> 5 files changed, 34 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
>> index 6b0d4df..b2315ef 100644
>> --- a/arch/arm64/include/asm/processor.h
>> +++ b/arch/arm64/include/asm/processor.h
>> @@ -168,6 +168,9 @@ static inline void start_thread_common(struct pt_regs *regs, unsigned long pc)
>> memset(regs, 0, sizeof(*regs));
>> forget_syscall(regs);
>> regs->pc = pc;
>> +
>> + if (system_supports_irq_prio_masking())
>> + regs->pmr_save = GIC_PRIO_IRQON;
>> }
>>
>> static inline void start_thread(struct pt_regs *regs, unsigned long pc,
>> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
>> index ce6998c..0ad46f5 100644
>> --- a/arch/arm64/include/asm/ptrace.h
>> +++ b/arch/arm64/include/asm/ptrace.h
>> @@ -19,6 +19,8 @@
>> #ifndef __ASM_PTRACE_H
>> #define __ASM_PTRACE_H
>>
>> +#include <asm/cpufeature.h>
>> +
>> #include <uapi/asm/ptrace.h>
>>
>> /* Current Exception Level values, as contained in CurrentEL */
>> @@ -173,7 +175,8 @@ struct pt_regs {
>> #endif
>>
>> u64 orig_addr_limit;
>> - u64 unused; // maintain 16 byte alignment
>> + /* Only valid when ARM64_HAS_IRQ_PRIO_MASKING is enabled. */
>> + u64 pmr_save;
>> u64 stackframe[2];
>> };
>>
>> @@ -208,8 +211,13 @@ static inline void forget_syscall(struct pt_regs *regs)
>> #define processor_mode(regs) \
>> ((regs)->pstate & PSR_MODE_MASK)
>>
>> -#define interrupts_enabled(regs) \
>> - (!((regs)->pstate & PSR_I_BIT))
>> +#define irqs_priority_unmasked(regs) \
>> + (system_supports_irq_prio_masking() ? \
>> + (regs)->pmr_save & GIC_PRIO_STATUS_BIT : \
>> + true)
>> +
>> +#define interrupts_enabled(regs) \
>> + (!((regs)->pstate & PSR_I_BIT) && irqs_priority_unmasked(regs))
>>
>> #define fast_interrupts_enabled(regs) \
>> (!((regs)->pstate & PSR_F_BIT))
>> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
>> index 323aeb5..bab4122 100644
>> --- a/arch/arm64/kernel/asm-offsets.c
>> +++ b/arch/arm64/kernel/asm-offsets.c
>> @@ -78,6 +78,7 @@ int main(void)
>> DEFINE(S_ORIG_X0, offsetof(struct pt_regs, orig_x0));
>> DEFINE(S_SYSCALLNO, offsetof(struct pt_regs, syscallno));
>> DEFINE(S_ORIG_ADDR_LIMIT, offsetof(struct pt_regs, orig_addr_limit));
>> + DEFINE(S_PMR_SAVE, offsetof(struct pt_regs, pmr_save));
>> DEFINE(S_STACKFRAME, offsetof(struct pt_regs, stackframe));
>> DEFINE(S_FRAME_SIZE, sizeof(struct pt_regs));
>> BLANK();
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index 039144e..eb8120e 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -249,6 +249,12 @@ alternative_else_nop_endif
>> msr sp_el0, tsk
>> .endif
>>
>> + /* Save pmr */
>> +alternative_if ARM64_HAS_IRQ_PRIO_MASKING
>> + mrs_s x20, SYS_ICC_PMR_EL1
>> + str x20, [sp, #S_PMR_SAVE]
>> +alternative_else_nop_endif
>> +
>> /*
>> * Registers that may be useful after this macro is invoked:
>> *
>> @@ -269,6 +275,13 @@ alternative_else_nop_endif
>> /* No need to restore UAO, it will be restored from SPSR_EL1 */
>> .endif
>>
>> + /* Restore pmr */
>> +alternative_if ARM64_HAS_IRQ_PRIO_MASKING
>> + ldr x20, [sp, #S_PMR_SAVE]
>> + msr_s SYS_ICC_PMR_EL1, x20
>> + dsb sy
>> +alternative_else_nop_endif
>> +
>> ldp x21, x22, [sp, #S_PC] // load ELR, SPSR
>> .if \el == 0
>> ct_user_enter
>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>> index d9a4c2d..71e8850 100644
>> --- a/arch/arm64/kernel/process.c
>> +++ b/arch/arm64/kernel/process.c
>> @@ -231,6 +231,9 @@ void __show_regs(struct pt_regs *regs)
>>
>> printk("sp : %016llx\n", sp);
>>
>> + if (system_supports_irq_prio_masking())
>> + printk("pmr_save: %08llx\n", regs->pmr_save);
>> +
>> i = top_reg;
>>
>> while (i >= 0) {
>> @@ -362,6 +365,9 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
>> if (arm64_get_ssbd_state() == ARM64_SSBD_FORCE_DISABLE)
>> childregs->pstate |= PSR_SSBS_BIT;
>>
>> + if (system_supports_irq_prio_masking())
>> + childregs->pmr_save = GIC_PRIO_IRQON;
>> +
>> p->thread.cpu_context.x19 = stack_start;
>> p->thread.cpu_context.x20 = stk_sz;
>> }
>> --
>> 1.9.1
>>

--
Julien Thierry

2018-11-30 10:40:15

by Daniel Thompson

Subject: Re: [PATCH v6 06/24] arm64: ptrace: Provide definitions for PMR values

On Fri, Nov 30, 2018 at 08:53:47AM +0000, Julien Thierry wrote:
>
>
> On 29/11/18 16:40, Mark Rutland wrote:
> > On Mon, Nov 12, 2018 at 11:56:57AM +0000, Julien Thierry wrote:
> >> Introduce fixed values for PMR that are going to be used to mask and
> >> unmask interrupts by priority. These values are chosent in such a way
> >
> > Nit: s/chosent/chosen/
> >
> >> that a single bit (GIC_PMR_UNMASKED_BIT) encodes the information whether
> >> interrupts are masked or not.
> >
> > There's no GIC_PMR_UNMASKED_BIT in this patch. Should that be
> > GIC_PRIO_STATUS_BIT?
> >
>
> Yep, forgot to update the commit message when renaming, thanks.
>
> >> Signed-off-by: Julien Thierry <[email protected]>
> >> Suggested-by: Daniel Thompson <[email protected]>
> >> Cc: Oleg Nesterov <[email protected]>
> >> Cc: Catalin Marinas <[email protected]>
> >> Cc: Will Deacon <[email protected]>
> >> ---
> >> arch/arm64/include/asm/ptrace.h | 6 ++++++
> >> 1 file changed, 6 insertions(+)
> >>
> >> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> >> index fce22c4..ce6998c 100644
> >> --- a/arch/arm64/include/asm/ptrace.h
> >> +++ b/arch/arm64/include/asm/ptrace.h
> >> @@ -25,6 +25,12 @@
> >> #define CurrentEL_EL1 (1 << 2)
> >> #define CurrentEL_EL2 (2 << 2)
> >>
> >> +/* PMR values used to mask/unmask interrupts */
> >> +#define GIC_PRIO_IRQON 0xf0
> >> +#define GIC_PRIO_STATUS_SHIFT 6
> >> +#define GIC_PRIO_STATUS_BIT (1 << GIC_PRIO_STATUS_SHIFT)
> >> +#define GIC_PRIO_IRQOFF (GIC_PRIO_IRQON ^ GIC_PRIO_STATUS_BIT)
> >
> > Could you elaborate on the GIC priority logic a bit?
> >
>
> Yes, I'll give details below.
>
> > Are lower numbers higher priority?
> >
>
> Yes, that is the case.
>
> > Are there restrictions on valid PMR values?
> >
>
> Yes, there are at most 8 priority bits but implementations are free to
> implement a number of priority bits:
> - between 5 and 8 when GIC runs two security states (bits [7:3] always
> being implemented and [2:0] being optional), but non-secure side is
> always deprived or the lowest implemented bit
> - between 4 and 8 when GIC runs only one security state (bits [7:4]
> implemented, bits [3:0] optional)
>
> This is detailed in section 4.8 "Interrupt prioritization" of the GICv3
> architecture specification.
>
> So Linux should always be able to see bits [7:4].
>
> > IIUC GIC_PRIO_IRQOFF is 0xb0 (aka 0b10110000), which seems a little
> > surprising. I'd have expected that we'd use the most signficant bit.
> >
>
> So, re-reading the GICv3 spec, I believe this sparked from a confusion...
>
> The idea was that the GICv3 specification would recommend to keep
> non-secure group-1 interrupts at a lower priority that group-0 (and
> secure group-1 interrupts) interrupts, and to do so the idea was to
> always keep bit[7] == 1 for non-secure group-1.
>
> So, we would need to have priority bit[7] == 1 for both normal
> interrupts and pseudo-NMIs, and using the most significant bit to mask
> would mean masking pseudo-NMIs as well.
>
> However, I only find mention of this in the notes of section 4.8.6
> "Software accesses of interrupt priority". The section only applies to
> GIC with two security states, and the recommendation of writing
> non-secure group-1 priorities with bit[7] == 1 is only directed at
> writes from the secure side. From the non-secure side, the GIC already
> does some magic to enforce that the value kept in the distributor has
> bit[7] == 1.
>
> So, I believe that from the non-secure point of view, we could define
> pseudo-NMI priority as e.g. 0x40 (which the GIC will convert to 0xa0)
> and use the most significant bit of PMR to mask normal interrupts which
> would be more intuitive.
>
> Marc, as GIC expert do you agree with this? Or is there a reason we
> should keep bit[7] == 1 for non-secure group-1 priorities?

I think selecting bit 6 dates back to when I was working on this.

I originally used bit 7 but switched due to problems on the FVP at the
time (my memory is a little hazy here but it felt like it wasn't
doing the magic shift properly when running in non-secure mode).

Once the patchset was running on real hardware I kept on with bit 6
figuring that, given the magic shift from non-secure mode is a little
odd, it would remain furtile soil for future silicon bugs (I was
watching a lot of patches go past on the ML working round bugs in
non-Arm GIC implementations and ended up feeling rather paranoid
about things like that).


Daniel.

2018-11-30 10:56:53

by Julien Thierry

Subject: Re: [PATCH v6 08/24] arm64: Unmask PMR before going idle

On 29/11/18 17:44, Mark Rutland wrote:
> On Mon, Nov 12, 2018 at 11:56:59AM +0000, Julien Thierry wrote:
>> CPU does not received signals for interrupts with a priority masked by
>> ICC_PMR_EL1. This means the CPU might not come back from a WFI
>> instruction.
>>
>> Make sure ICC_PMR_EL1 does not mask interrupts when doing a WFI.
>>
>> Signed-off-by: Julien Thierry <[email protected]>
>> Suggested-by: Daniel Thompson <[email protected]>
>> Cc: Catalin Marinas <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> ---
>> arch/arm64/mm/proc.S | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
>> index 2c75b0b..3c7064c 100644
>> --- a/arch/arm64/mm/proc.S
>> +++ b/arch/arm64/mm/proc.S
>> @@ -20,6 +20,7 @@
>>
>> #include <linux/init.h>
>> #include <linux/linkage.h>
>> +#include <linux/irqchip/arm-gic-v3.h>
>> #include <asm/assembler.h>
>> #include <asm/asm-offsets.h>
>> #include <asm/hwcap.h>
>> @@ -53,10 +54,27 @@
>> * cpu_do_idle()
>> *
>> * Idle the processor (wait for interrupt).
>> + *
>> + * If the CPU supports priority masking we must do additional work to
>> + * ensure that interrupts are not masked at the PMR (because the core will
>> + * not wake up if we block the wake up signal in the interrupt controller).
>> */
>> ENTRY(cpu_do_idle)
>> +alternative_if_not ARM64_HAS_IRQ_PRIO_MASKING
>> + dsb sy // WFI may enter a low-power mode
>> + wfi
>> + ret
>> +alternative_else
>> + mrs x0, daif // save I bit
>> + msr daifset, #2 // set I bit
>> + mrs_s x1, SYS_ICC_PMR_EL1 // save PMR
>> +alternative_endif
>> + mov x2, #GIC_PRIO_IRQON
>> + msr_s SYS_ICC_PMR_EL1, x2 // unmask PMR
>> dsb sy // WFI may enter a low-power mode
>
> Is the DSB SY sufficient and necessary to synchronise the update of
> SYS_ICC_PMR_EL1? We don't need an ISB too?
>

DSB SY is necessary when we unmask interrupts to make sure that the
redistributor sees the update to PMR before we do WFI. My understanding
is that the resdistributor is free to stop forwarding interrupts to the
CPU interface if from its point of view those interrupts don't have a
high enough priority.

As for the ISB, I don't think we need one because writes to PMR are
self-synchronizing, so the write to PMR should be seen before DSB SY and
wfi.


>> wfi
>> + msr_s SYS_ICC_PMR_EL1, x1 // restore PMR
>
> Likewise, we don't need any barriers here before we poke DAIF?

Here we don't need DSB SY because the value being restored is either:
- GIC_PRIO_IRQON which is the same as the current value, the
redistributor is already aware of it.
- GIC_PRIO_IRQOFF and the self-synchronization of PMR ensures that no
interrupts with priorities lower than the value of PMR can be taken
(this does not require to be seen by the redistributor).


For the ISB, I have this small doubt about whether it is needed between
WFI and MSR PMR. But there is this bit in the ARM ARM section D12.1.3
"General behavior of accesses to the AArch64 System registers",
subsection "Synchronization requirements for AArch64 System registers":

"Direct writes using the instructions in Table D11-2 on page D11-2660
require synchronization before software can rely on the effects of
changes to the System registers to affect instructions appearing in
program order after the direct write to the System register. Direct
writes to these registers are not allowed to affect any instructions
appearing in program order before the direct write."

ICC_PMR_EL1 is part of the mentioned table. And reordering the direct
write to PMR before the WFI would definitely affect the WFI instruction,
so my interpretation is that this would not be allowed by the
architecture. So I don't think we need the ISB either, but my
understanding could be wrong.

>
>> + msr daif, x0 // restore I bit
>> ret
>> ENDPROC(cpu_do_idle)
>
> If we build without CONFIG_ARM64_PSEUDO_NMI surely we don't want to emit
> the alternative?
>
> How about we move this to C, and have something like the below?
>
> For the !CONFIG_ARM64_PSEUDO_NMI case it generates identical assembly to the
> existing cpu_do_idle(). Note that I've assumed we don't need barriers, which
> (as above) I'm not certain of.
>
> Thanks,
> Mark.
>
> ---->8----
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 7f1628effe6d..ccd2ad8c5e2f 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -73,6 +73,40 @@ EXPORT_SYMBOL_GPL(pm_power_off);
>
> void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>
> +static inline void __cpu_do_idle(void)
> +{
> + /* WFI may enter a low-power mode */
> + dsb(sy);
> + wfi();
> +}
> +
> +/*
> + * When using priority masking we need to take extra care, etc.
> + */
> +static inline void __cpu_do_idle_irqprio(void)
> +{
> + unsigned long flags = arch_local_irq_save();

The issue with this is that in patch 10, arch_local_irq_* functions
toggle PMR rather than PSR.I.

I could use local_daif_mask but I don't think disabling debug and async
is good. Otherwise and can do a small bit of inline assembly and have
something like:

static inline void __cpu_do_idle_irqprio(void)
{
unsigned long flags = arch_local_irq_save();

// set PSR.I

gic_write_pmr(GIC_PRIO_IRQON);

__cpu_do_idle();

arch_local_irq_restore(flags); // restores PMR and PSR.I
}


Otherwise I agree, moving it to C makes it more readable and avoid
generating code that will never get called.

I'll do it for the next version.

Thanks,

> + unsigned long pmr = gic_read_pmr();
> +
> + gic_write_pmr(GIC_PRIO_IRQON);
> +
> + __cpu_do_idle();
> +
> + gic_write_pmr(pmr);
> + arch_local_irq_enable();
> +}
> +
> +/*
> + * Idle the processor (wait for interrupt).
> + */
> +void cpu_do_idle(void)
> +{
> + if (system_uses_irq_prio_masking())
> + __cpu_do_idle_irqprio();
> + else
> + __cpu_do_idle();
> +}
> +
> /*
> * This is our default idle handler.
> */
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index 03646e6a2ef4..38c0171e52e2 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -49,17 +49,6 @@
>
> #define MAIR(attr, mt) ((attr) << ((mt) * 8))
>
> -/*
> - * cpu_do_idle()
> - *
> - * Idle the processor (wait for interrupt).
> - */
> -ENTRY(cpu_do_idle)
> - dsb sy // WFI may enter a low-power mode
> - wfi
> - ret
> -ENDPROC(cpu_do_idle)
> -
> #ifdef CONFIG_CPU_PM
> /**
> * cpu_do_suspend - save CPU registers context
>
>

--
Julien Thierry

2018-11-30 11:05:47

by Julien Thierry

Subject: Re: [PATCH v6 06/24] arm64: ptrace: Provide definitions for PMR values

On 30/11/18 10:38, Daniel Thompson wrote:
> On Fri, Nov 30, 2018 at 08:53:47AM +0000, Julien Thierry wrote:
>>
>>
>> On 29/11/18 16:40, Mark Rutland wrote:
>>> On Mon, Nov 12, 2018 at 11:56:57AM +0000, Julien Thierry wrote:
>>>> Introduce fixed values for PMR that are going to be used to mask and
>>>> unmask interrupts by priority. These values are chosent in such a way
>>>
>>> Nit: s/chosent/chosen/
>>>
>>>> that a single bit (GIC_PMR_UNMASKED_BIT) encodes the information whether
>>>> interrupts are masked or not.
>>>
>>> There's no GIC_PMR_UNMASKED_BIT in this patch. Should that be
>>> GIC_PRIO_STATUS_BIT?
>>>
>>
>> Yep, forgot to update the commit message when renaming, thanks.
>>
>>>> Signed-off-by: Julien Thierry <[email protected]>
>>>> Suggested-by: Daniel Thompson <[email protected]>
>>>> Cc: Oleg Nesterov <[email protected]>
>>>> Cc: Catalin Marinas <[email protected]>
>>>> Cc: Will Deacon <[email protected]>
>>>> ---
>>>> arch/arm64/include/asm/ptrace.h | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
>>>> index fce22c4..ce6998c 100644
>>>> --- a/arch/arm64/include/asm/ptrace.h
>>>> +++ b/arch/arm64/include/asm/ptrace.h
>>>> @@ -25,6 +25,12 @@
>>>> #define CurrentEL_EL1 (1 << 2)
>>>> #define CurrentEL_EL2 (2 << 2)
>>>>
>>>> +/* PMR values used to mask/unmask interrupts */
>>>> +#define GIC_PRIO_IRQON 0xf0
>>>> +#define GIC_PRIO_STATUS_SHIFT 6
>>>> +#define GIC_PRIO_STATUS_BIT (1 << GIC_PRIO_STATUS_SHIFT)
>>>> +#define GIC_PRIO_IRQOFF (GIC_PRIO_IRQON ^ GIC_PRIO_STATUS_BIT)
>>>
>>> Could you elaborate on the GIC priority logic a bit?
>>>
>>
>> Yes, I'll give details below.
>>
>>> Are lower numbers higher priority?
>>>
>>
>> Yes, that is the case.
>>
>>> Are there restrictions on valid PMR values?
>>>
>>
>> Yes, there are at most 8 priority bits but implementations are free to
>> implement a number of priority bits:
>> - between 5 and 8 when GIC runs two security states (bits [7:3] always
>> being implemented and [2:0] being optional), but non-secure side is
>> always deprived or the lowest implemented bit
>> - between 4 and 8 when GIC runs only one security state (bits [7:4]
>> implemented, bits [3:0] optional)
>>
>> This is detailed in section 4.8 "Interrupt prioritization" of the GICv3
>> architecture specification.
>>
>> So Linux should always be able to see bits [7:4].
>>
>>> IIUC GIC_PRIO_IRQOFF is 0xb0 (aka 0b10110000), which seems a little
>>> surprising. I'd have expected that we'd use the most signficant bit.
>>>
>>
>> So, re-reading the GICv3 spec, I believe this sparked from a confusion...
>>
>> The idea was that the GICv3 specification would recommend to keep
>> non-secure group-1 interrupts at a lower priority that group-0 (and
>> secure group-1 interrupts) interrupts, and to do so the idea was to
>> always keep bit[7] == 1 for non-secure group-1.
>>
>> So, we would need to have priority bit[7] == 1 for both normal
>> interrupts and pseudo-NMIs, and using the most significant bit to mask
>> would mean masking pseudo-NMIs as well.
>>
>> However, I only find mention of this in the notes of section 4.8.6
>> "Software accesses of interrupt priority". The section only applies to
>> GIC with two security states, and the recommendation of writing
>> non-secure group-1 priorities with bit[7] == 1 is only directed at
>> writes from the secure side. From the non-secure side, the GIC already
>> does some magic to enforce that the value kept in the distributor has
>> bit[7] == 1.
>>
>> So, I believe that from the non-secure point of view, we could define
>> pseudo-NMI priority as e.g. 0x40 (which the GIC will convert to 0xa0)
>> and use the most significant bit of PMR to mask normal interrupts which
>> would be more intuitive.
>>
>> Marc, as GIC expert do you agree with this? Or is there a reason we
>> should keep bit[7] == 1 for non-secure group-1 priorities?
>
> I think selecting bit 6 dates back to when I was working on this.
>
> I originally used bit 7 but switched due to problems on the FVP at the
> time (my memory is a little hazy here but it felt like it wasn't
> doing the magic shift properly when running in non-secure mode).
>

If you were using boot-wrapper, that might have been the case as
SCR_EL3.FIQ is not getting set.

The fun bit is that under this configuration the magic bit still happens
for non-secure accesses to priorities configured in the
distributor/redistributor, but it disables the magic for non-secure PMR
and RPR accesses. So you can easily end up masking too much stuff when
writting to PMR when SCR_EL1.FIQ is cleared if you don't realize that
what non-secure sees in the distributor is not aligned with what will be
masked by PMR or presented in RPR.

> Once the patchset was running on real hardware I kept on with bit 6
> figuring that, given the magic shift from non-secure mode is a little
> odd, it would remain furtile soil for future silicon bugs (I was
> watching a lot of patches go past on the ML working round bugs in
> non-Arm GIC implementations and ended up feeling rather paranoid
> about things like that).
>
>
> Daniel.
>

--
Julien Thierry

2018-11-30 13:38:24

by Mark Rutland

Subject: Re: [PATCH v6 08/24] arm64: Unmask PMR before going idle

On Fri, Nov 30, 2018 at 10:55:47AM +0000, Julien Thierry wrote:
> On 29/11/18 17:44, Mark Rutland wrote:
> > On Mon, Nov 12, 2018 at 11:56:59AM +0000, Julien Thierry wrote:

> >> + mov x2, #GIC_PRIO_IRQON
> >> + msr_s SYS_ICC_PMR_EL1, x2 // unmask PMR
> >> dsb sy // WFI may enter a low-power mode
> >
> > Is the DSB SY sufficient and necessary to synchronise the update of
> > SYS_ICC_PMR_EL1? We don't need an ISB too?
>
> DSB SY is necessary when we unmask interrupts to make sure that the
> redistributor sees the update to PMR before we do WFI. My understanding
> is that the resdistributor is free to stop forwarding interrupts to the
> CPU interface if from its point of view those interrupts don't have a
> high enough priority.
>
> As for the ISB, I don't think we need one because writes to PMR are
> self-synchronizing, so the write to PMR should be seen before DSB SY and
> wfi.

Having looked at ARM IHI 0069D, 8.1.6 "Observability of the effects of
accesses to the GIC registers", I think I agree.

My specific concern was that a CPU might complete the DSB before the
MSR, but I think it's clear per the GIC spec it's clear that an ISB is
not expected between the MSR and DSB, even if that's unusual.

> >> wfi
> >> + msr_s SYS_ICC_PMR_EL1, x1 // restore PMR
> >
> > Likewise, we don't need any barriers here before we poke DAIF?
>
> Here we don't need DSB SY because the value being restored is either:
> - GIC_PRIO_IRQON which is the same as the current value, the
> redistributor is already aware of it.
> - GIC_PRIO_IRQOFF and the self-synchronization of PMR ensures that no
> interrupts with priorities lower than the value of PMR can be taken
> (this does not require to be seen by the redistributor).
>
> For the ISB, I have this small doubt about whether it is needed between
> WFI and MSR PMR. But there is this bit in the ARM ARM section D12.1.3
> "General behavior of accesses to the AArch64 System registers",
> subsection "Synchronization requirements for AArch64 System registers":
>
> "Direct writes using the instructions in Table D11-2 on page D11-2660
> require synchronization before software can rely on the effects of
> changes to the System registers to affect instructions appearing in
> program order after the direct write to the System register. Direct
> writes to these registers are not allowed to affect any instructions
> appearing in program order before the direct write."
>
> ICC_PMR_EL1 is part of the mentioned table.

I think that's a defect in the ARM ARM, given it disagrees with the GIC
spec.

> And reordering the direct write to PMR before the WFI would definitely
> affect the WFI instruction, so my interpretation is that this would
> not be allowed by the architecture. So I don't think we need the ISB
> either, but my understanding could be wrong.

We already assume that a DSB can't be re-ordered w.r.t. the WFI, so as
long as the DSB can't complete before the MSR, I think we're good.

> >
> >> + msr daif, x0 // restore I bit
> >> ret
> >> ENDPROC(cpu_do_idle)
> >
> > If we build without CONFIG_ARM64_PSEUDO_NMI surely we don't want to emit
> > the alternative?
> >
> > How about we move this to C, and have something like the below?
> >
> > For the !CONFIG_ARM64_PSEUDO_NMI case it generates identical assembly to the
> > existing cpu_do_idle(). Note that I've assumed we don't need barriers, which
> > (as above) I'm not certain of.
> >
> > Thanks,
> > Mark.
> >
> > ---->8----
> > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > index 7f1628effe6d..ccd2ad8c5e2f 100644
> > --- a/arch/arm64/kernel/process.c
> > +++ b/arch/arm64/kernel/process.c
> > @@ -73,6 +73,40 @@ EXPORT_SYMBOL_GPL(pm_power_off);
> >
> > void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
> >
> > +static inline void __cpu_do_idle(void)
> > +{
> > + /* WFI may enter a low-power mode */
> > + dsb(sy);
> > + wfi();
> > +}
> > +
> > +/*
> > + * When using priority masking we need to take extra care, etc.
> > + */
> > +static inline void __cpu_do_idle_irqprio(void)
> > +{
> > + unsigned long flags = arch_local_irq_save();
>
> The issue with this is that in patch 10, arch_local_irq_* functions
> toggle PMR rather than PSR.I.
>
> I could use local_daif_mask but I don't think disabling debug and async
> is good. Otherwise and can do a small bit of inline assembly and have
> something like:

Can we factor out the existing arch_local_irq_save() somehow?

Thanks,
Mark.

2018-11-30 18:04:14

by Catalin Marinas

Subject: Re: [PATCH v6 01/24] arm64: Remove unused daif related functions/macros

On Mon, Nov 12, 2018 at 11:56:52AM +0000, Julien Thierry wrote:
> There are some helpers to modify PSR.[DAIF] bits that are not referenced
> anywhere. The less these bits are available outside of local_irq_*
> functions the better.
>
> Get rid of those unused helpers.
>
> Signed-off-by: Julien Thierry <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: James Morse <[email protected]>

Acked-by: Catalin Marinas <[email protected]>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

2018-11-30 18:10:01

by Catalin Marinas

Subject: Re: [PATCH v6 02/24] arm64: cpufeature: Set SYSREG_GIC_CPUIF as a boot system feature

On Mon, Nov 12, 2018 at 11:56:53AM +0000, Julien Thierry wrote:
> It is not supported to have some CPUs using GICv3 sysreg CPU interface
> while some others do not.
>
> Once ICC_SRE_EL1.SRE is set on a CPU, the bit cannot be cleared. Since
> matching this feature require setting ICC_SRE_EL1.SRE, it cannot be
> turned off if found on a CPU.
>
> Set the feature as STRICT_BOOT, if boot CPU has it, all other CPUs are
> required to have it.
>
> Signed-off-by: Julien Thierry <[email protected]>
> Suggested-by: Daniel Thompson <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Marc Zyngier <[email protected]>

Acked-by: Catalin Marinas <[email protected]>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

2018-11-30 18:10:33

by Catalin Marinas

Subject: Re: [PATCH v6 03/24] arm64: cpufeature: Add cpufeature for IRQ priority masking

On Mon, Nov 12, 2018 at 11:56:54AM +0000, Julien Thierry wrote:
> Add a cpufeature indicating whether a cpu supports masking interrupts
> by priority.
>
> The feature will be properly enabled in a later patch.
>
> Signed-off-by: Julien Thierry <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>

Acked-by: Catalin Marinas <[email protected]>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

2018-11-30 18:11:04

by Catalin Marinas

Subject: Re: [PATCH v6 04/24] arm/arm64: gic-v3: Add PMR and RPR accessors

On Mon, Nov 12, 2018 at 11:56:55AM +0000, Julien Thierry wrote:
> Add helper functions to access system registers related to interrupt
> priorities: PMR and RPR.
>
> Signed-off-by: Julien Thierry <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Marc Zyngier <[email protected]>

Acked-by: Catalin Marinas <[email protected]>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

2018-12-03 10:35:42

by Julien Thierry

Subject: Re: [PATCH v6 03/24] arm64: cpufeature: Add cpufeature for IRQ priority masking

On 29/11/18 17:12, Mark Rutland wrote:
> On Mon, Nov 12, 2018 at 11:56:54AM +0000, Julien Thierry wrote:
>> Add a cpufeature indicating whether a cpu supports masking interrupts
>> by priority.
>>
>> The feature will be properly enabled in a later patch.
>>
>> Signed-off-by: Julien Thierry <[email protected]>
>> Cc: Catalin Marinas <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> Cc: Marc Zyngier <[email protected]>
>> Cc: Suzuki K Poulose <[email protected]>
>> ---
>> arch/arm64/include/asm/cpucaps.h | 3 ++-
>> arch/arm64/include/asm/cpufeature.h | 6 ++++++
>> arch/arm64/kernel/cpufeature.c | 23 +++++++++++++++++++++++
>> 3 files changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
>> index 6e2d254..f367e5c 100644
>> --- a/arch/arm64/include/asm/cpucaps.h
>> +++ b/arch/arm64/include/asm/cpucaps.h
>> @@ -54,7 +54,8 @@
>> #define ARM64_HAS_CRC3233
>> #define ARM64_SSBS34
>> #define ARM64_WORKAROUND_118887335
>> +#define ARM64_HAS_IRQ_PRIO_MASKING36
>>
>> -#define ARM64_NCAPS36
>> +#define ARM64_NCAPS37
>>
>> #endif /* __ASM_CPUCAPS_H */
>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>> index 7e2ec64..a6e063f 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -514,6 +514,12 @@ static inline bool system_supports_cnp(void)
>> cpus_have_const_cap(ARM64_HAS_CNP);
>> }
>>
>> +static inline bool system_supports_irq_prio_masking(void)
>> +{
>> +return IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) &&
>> + cpus_have_const_cap(ARM64_HAS_IRQ_PRIO_MASKING);
>> +}
>
> This should probably be s/supports/uses/.
>

Fixed locally.

> With that:
>
> Reviewed-by: Mark Rutland <[email protected]>
>

Thanks,

Julien

> Mark.
>
>> +
>> #define ARM64_SSBD_UNKNOWN-1
>> #define ARM64_SSBD_FORCE_DISABLE0
>> #define ARM64_SSBD_KERNEL1
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index 03a9d96..1b5b553 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -1145,6 +1145,14 @@ static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused)
>> }
>> #endif /* CONFIG_ARM64_RAS_EXTN */
>>
>> +#ifdef CONFIG_ARM64_PSEUDO_NMI
>> +static bool can_use_gic_priorities(const struct arm64_cpu_capabilities *entry,
>> + int scope)
>> +{
>> +return false;
>> +}
>> +#endif
>> +
>> static const struct arm64_cpu_capabilities arm64_features[] = {
>> {
>> .desc = "GIC system register CPU interface",
>> @@ -1368,6 +1376,21 @@ static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused)
>> .cpu_enable = cpu_enable_cnp,
>> },
>> #endif
>> +#ifdef CONFIG_ARM64_PSEUDO_NMI
>> +{
>> +/*
>> + * Depends on having GICv3
>> + */
>> +.desc = "IRQ priority masking",
>> +.capability = ARM64_HAS_IRQ_PRIO_MASKING,
>> +.type = ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE,
>> +.matches = can_use_gic_priorities,
>> +.sys_reg = SYS_ID_AA64PFR0_EL1,
>> +.field_pos = ID_AA64PFR0_GIC_SHIFT,
>> +.sign = FTR_UNSIGNED,
>> +.min_field_value = 1,
>> +},
>> +#endif
>> {},
>> };
>>
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
Julien Thierry
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

2018-12-03 10:39:16

by Julien Thierry

Subject: Re: [PATCH v6 08/24] arm64: Unmask PMR before going idle

On 30/11/18 13:37, Mark Rutland wrote:
> On Fri, Nov 30, 2018 at 10:55:47AM +0000, Julien Thierry wrote:
>> On 29/11/18 17:44, Mark Rutland wrote:
>>> On Mon, Nov 12, 2018 at 11:56:59AM +0000, Julien Thierry wrote:
>
>>>> + mov x2, #GIC_PRIO_IRQON
>>>> + msr_s SYS_ICC_PMR_EL1, x2 // unmask PMR
>>>> dsb sy // WFI may enter a low-power mode
>>>
>>> Is the DSB SY sufficient and necessary to synchronise the update of
>>> SYS_ICC_PMR_EL1? We don't need an ISB too?
>>
>> DSB SY is necessary when we unmask interrupts to make sure that the
>> redistributor sees the update to PMR before we do WFI. My understanding
>> is that the resdistributor is free to stop forwarding interrupts to the
>> CPU interface if from its point of view those interrupts don't have a
>> high enough priority.
>>
>> As for the ISB, I don't think we need one because writes to PMR are
>> self-synchronizing, so the write to PMR should be seen before DSB SY and
>> wfi.
>
> Having looked at ARM IHI 0069D, 8.1.6 "Observability of the effects of
> accesses to the GIC registers", I think I agree.
>
> My specific concern was that a CPU might complete the DSB before the
> MSR, but I think it's clear per the GIC spec it's clear that an ISB is
> not expected between the MSR and DSB, even if that's unusual.
>
>>>> wfi
>>>> + msr_s SYS_ICC_PMR_EL1, x1 // restore PMR
>>>
>>> Likewise, we don't need any barriers here before we poke DAIF?
>>
>> Here we don't need DSB SY because the value being restored is either:
>> - GIC_PRIO_IRQON which is the same as the current value, the
>> redistributor is already aware of it.
>> - GIC_PRIO_IRQOFF and the self-synchronization of PMR ensures that no
>> interrupts with priorities lower than the value of PMR can be taken
>> (this does not require to be seen by the redistributor).
>>
>> For the ISB, I have this small doubt about whether it is needed between
>> WFI and MSR PMR. But there is this bit in the ARM ARM section D12.1.3
>> "General behavior of accesses to the AArch64 System registers",
>> subsection "Synchronization requirements for AArch64 System registers":
>>
>> "Direct writes using the instructions in Table D11-2 on page D11-2660
>> require synchronization before software can rely on the effects of
>> changes to the System registers to affect instructions appearing in
>> program order after the direct write to the System register. Direct
>> writes to these registers are not allowed to affect any instructions
>> appearing in program order before the direct write."
>>
>> ICC_PMR_EL1 is part of the mentioned table.
>
> I think that's a defect in the ARM ARM, given it disagrees with the GIC
> spec.
>
>> And reordering the direct write to PMR before the WFI would definitely
>> affect the WFI instruction, so my interpretation is that this would
>> not be allowed by the architecture. So I don't think we need the ISB
>> either, but my understanding could be wrong.
>
> We already assume that a DSB can't be re-ordered w.r.t. the WFI, so as
> long as the DSB can't complete before the MSR, I think we're good.
>
>>>
>>>> + msr daif, x0 // restore I bit
>>>> ret
>>>> ENDPROC(cpu_do_idle)
>>>
>>> If we build without CONFIG_ARM64_PSEUDO_NMI surely we don't want to emit
>>> the alternative?
>>>
>>> How about we move this to C, and have something like the below?
>>>
>>> For the !CONFIG_ARM64_PSEUDO_NMI case it generates identical assembly to the
>>> existing cpu_do_idle(). Note that I've assumed we don't need barriers, which
>>> (as above) I'm not certain of.
>>>
>>> Thanks,
>>> Mark.
>>>
>>> ---->8----
>>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>>> index 7f1628effe6d..ccd2ad8c5e2f 100644
>>> --- a/arch/arm64/kernel/process.c
>>> +++ b/arch/arm64/kernel/process.c
>>> @@ -73,6 +73,40 @@ EXPORT_SYMBOL_GPL(pm_power_off);
>>>
>>> void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>>>
>>> +static inline void __cpu_do_idle(void)
>>> +{
>>> + /* WFI may enter a low-power mode */
>>> + dsb(sy);
>>> + wfi();
>>> +}
>>> +
>>> +/*
>>> + * When using priority masking we need to take extra care, etc.
>>> + */
>>> +static inline void __cpu_do_idle_irqprio(void)
>>> +{
>>> + unsigned long flags = arch_local_irq_save();
>>
>> The issue with this is that in patch 10, arch_local_irq_* functions
>> toggle PMR rather than PSR.I.
>>
>> I could use local_daif_mask but I don't think disabling debug and async
>> is good. Otherwise and can do a small bit of inline assembly and have
>> something like:
>
> Can we factor out the existing arch_local_irq_save() somehow?
>

I'm not sure I understand what you're suggesting. Using individual
accessors for PMR and PSR.I instead of arch_local_irq_save?

I am just a bit concerned about having too many functions to play with
either (especially since most of the time those functions end up being
single use).

Thanks,

--
Julien Thierry

2018-12-04 16:23:41

by Catalin Marinas

Subject: Re: [PATCH v6 05/24] irqchip/gic-v3: Switch to PMR masking before calling IRQ handler

On Mon, Nov 12, 2018 at 11:56:56AM +0000, Julien Thierry wrote:
> Mask the IRQ priority through PMR and re-enable IRQs at CPU level,
> allowing only higher priority interrupts to be received during interrupt
> handling.
>
> Signed-off-by: Julien Thierry <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> ---
> arch/arm/include/asm/arch_gicv3.h | 17 +++++++++++++++++
> arch/arm64/include/asm/arch_gicv3.h | 17 +++++++++++++++++
> drivers/irqchip/irq-gic-v3.c | 10 ++++++++++
> 3 files changed, 44 insertions(+)

For the arm64 bits:

Acked-by: Catalin Marinas <[email protected]>

(this time without the legal disclaimer ;))

2018-12-04 17:10:19

by Catalin Marinas

Subject: Re: [PATCH v6 07/24] arm64: Make PMR part of task context

On Mon, Nov 12, 2018 at 11:56:58AM +0000, Julien Thierry wrote:
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 039144e..eb8120e 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -249,6 +249,12 @@ alternative_else_nop_endif
> msr sp_el0, tsk
> .endif
>
> + /* Save pmr */
> +alternative_if ARM64_HAS_IRQ_PRIO_MASKING
> + mrs_s x20, SYS_ICC_PMR_EL1
> + str x20, [sp, #S_PMR_SAVE]
> +alternative_else_nop_endif
> +
> /*
> * Registers that may be useful after this macro is invoked:
> *
> @@ -269,6 +275,13 @@ alternative_else_nop_endif
> /* No need to restore UAO, it will be restored from SPSR_EL1 */
> .endif
>
> + /* Restore pmr */
> +alternative_if ARM64_HAS_IRQ_PRIO_MASKING
> + ldr x20, [sp, #S_PMR_SAVE]
> + msr_s SYS_ICC_PMR_EL1, x20
> + dsb sy
> +alternative_else_nop_endif

What's this DSB for? If it's needed, please add a comment.

I would have expected an ISB (or none at all as we are going to return
from an exception).

--
Catalin

2018-12-04 17:32:00

by Julien Thierry

Subject: Re: [PATCH v6 07/24] arm64: Make PMR part of task context

On 04/12/18 17:09, Catalin Marinas wrote:
> On Mon, Nov 12, 2018 at 11:56:58AM +0000, Julien Thierry wrote:
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index 039144e..eb8120e 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -249,6 +249,12 @@ alternative_else_nop_endif
>> msr sp_el0, tsk
>> .endif
>>
>> + /* Save pmr */
>> +alternative_if ARM64_HAS_IRQ_PRIO_MASKING
>> + mrs_s x20, SYS_ICC_PMR_EL1
>> + str x20, [sp, #S_PMR_SAVE]
>> +alternative_else_nop_endif
>> +
>> /*
>> * Registers that may be useful after this macro is invoked:
>> *
>> @@ -269,6 +275,13 @@ alternative_else_nop_endif
>> /* No need to restore UAO, it will be restored from SPSR_EL1 */
>> .endif
>>
>> + /* Restore pmr */
>> +alternative_if ARM64_HAS_IRQ_PRIO_MASKING
>> + ldr x20, [sp, #S_PMR_SAVE]
>> + msr_s SYS_ICC_PMR_EL1, x20
>> + dsb sy
>> +alternative_else_nop_endif
>
> What's this DSB for? If it's needed, please add a comment.
>

The DSB is to make sure that, in the case we are unmasking interrupt
priorities, the unmasking is seen by the redistributor. Without it the
redistributor might only start forwarding interrupts (if their
priorities are too low) to the CPU once it has seen that PMR was
modified, which could happen after the CPU has returned from the exception.

I'll add a comment.

> I would have expected an ISB (or none at all as we are going to return
> from an exception).

So the two reasons we don't need an ISB are:
- the only thing that matter is that PMR modification + DSB happens
before exception return
- writes to ICC_PMR_EL1 are self synchronizing so we don't need an ISB
before the DSB

Thanks,

--
Julien Thierry

2018-12-04 17:37:38

by Catalin Marinas

Subject: Re: [PATCH v6 10/24] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking

On Mon, Nov 12, 2018 at 11:57:01AM +0000, Julien Thierry wrote:
> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
> index 24692ed..e0a32e4 100644
> --- a/arch/arm64/include/asm/irqflags.h
> +++ b/arch/arm64/include/asm/irqflags.h
> @@ -18,7 +18,27 @@
>
> #ifdef __KERNEL__
>
> +#include <asm/alternative.h>
> +#include <asm/cpufeature.h>
> #include <asm/ptrace.h>
> +#include <asm/sysreg.h>
> +
> +
> +/*
> + * When ICC_PMR_EL1 is used for interrupt masking, only the bit indicating
> + * whether the normal interrupts are masked is kept along with the daif
> + * flags.
> + */
> +#define ARCH_FLAG_PMR_EN 0x1
> +
> +#define MAKE_ARCH_FLAGS(daif, pmr) \
> + ((daif) | (((pmr) >> GIC_PRIO_STATUS_SHIFT) & ARCH_FLAG_PMR_EN))
> +
> +#define ARCH_FLAGS_GET_PMR(flags) \
> + ((((flags) & ARCH_FLAG_PMR_EN) << GIC_PRIO_STATUS_SHIFT) \
> + | GIC_PRIO_IRQOFF)
> +
> +#define ARCH_FLAGS_GET_DAIF(flags) ((flags) & ~ARCH_FLAG_PMR_EN)

I wonder whether we could just use the PSR_I_BIT here to decide whether
to set the GIC_PRIO_IRQ{ON,OFF}. We could clear the PSR_I_BIT in
_restore_daif() with an alternative.

> +/*
> + * CPU interrupt mask handling.
> + */
> 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)

DSB needed here as well? I guess I'd have to read the GIC spec before
asking again ;).

--
Catalin

2018-12-04 17:54:34

by Catalin Marinas

Subject: Re: [PATCH v6 15/24] arm64: Switch to PMR masking when starting CPUs

On Mon, Nov 12, 2018 at 11:57:06AM +0000, Julien Thierry wrote:
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 8dc9dde..e495360 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -35,6 +35,7 @@
> #include <linux/smp.h>
> #include <linux/seq_file.h>
> #include <linux/irq.h>
> +#include <linux/irqchip/arm-gic-v3.h>
> #include <linux/percpu.h>
> #include <linux/clockchips.h>
> #include <linux/completion.h>
> @@ -175,6 +176,25 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
> return ret;
> }
>
> +static void init_gic_priority_masking(void)
> +{
> + u32 gic_sre = gic_read_sre();
> + u32 cpuflags;
> +
> + if (WARN_ON(!(gic_sre & ICC_SRE_EL1_SRE)))
> + return;
> +
> + WARN_ON(!irqs_disabled());
> +
> + gic_write_pmr(GIC_PRIO_IRQOFF);
> +
> + cpuflags = read_sysreg(daif);
> +
> + /* We can only unmask PSR.I if we can take aborts */
> + if (!(cpuflags & PSR_A_BIT))
> + write_sysreg(cpuflags & ~PSR_I_BIT, daif);

I don't understand this. If you don't switch off PSR_I_BIT here, where
does it happen? In which scenario do we actually have the A bit still
set? At a quick look, smp_prepare_boot_cpu() would have the A bit
cleared previously by setup_arch(). We have secondary_start_kernel()
where you call init_gic_priority_masking() before local_daif_restore().

So what happens if you always turn off PSR_I_BIT here?

--
Catalin

2018-12-04 18:11:32

by Catalin Marinas

Subject: Re: [PATCH v6 21/24] arm64: Handle serror in NMI context

On Mon, Nov 12, 2018 at 11:57:12AM +0000, Julien Thierry wrote:
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 5f4d9ac..66344cd 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -897,13 +897,17 @@ bool arm64_is_fatal_ras_serror(struct pt_regs *regs, unsigned int esr)
>
> asmlinkage void do_serror(struct pt_regs *regs, unsigned int esr)
> {
> - nmi_enter();
> + const bool was_in_nmi = in_nmi();
> +
> + if (!was_in_nmi)
> + nmi_enter();
>
> /* non-RAS errors are not containable */
> if (!arm64_is_ras_serror(esr) || arm64_is_fatal_ras_serror(regs, esr))
> arm64_serror_panic(regs, esr);
>
> - nmi_exit();
> + if (!was_in_nmi)
> + nmi_exit();
> }

Do we actually need nmi_enter/exit in the outer do_serror() function?
Could we just move it to arm64_serror_panic()?

--
Catalin

2018-12-04 18:13:04

by Julien Thierry

Subject: Re: [PATCH v6 15/24] arm64: Switch to PMR masking when starting CPUs

On 04/12/18 17:51, Catalin Marinas wrote:
> On Mon, Nov 12, 2018 at 11:57:06AM +0000, Julien Thierry wrote:
>> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
>> index 8dc9dde..e495360 100644
>> --- a/arch/arm64/kernel/smp.c
>> +++ b/arch/arm64/kernel/smp.c
>> @@ -35,6 +35,7 @@
>> #include <linux/smp.h>
>> #include <linux/seq_file.h>
>> #include <linux/irq.h>
>> +#include <linux/irqchip/arm-gic-v3.h>
>> #include <linux/percpu.h>
>> #include <linux/clockchips.h>
>> #include <linux/completion.h>
>> @@ -175,6 +176,25 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
>> return ret;
>> }
>>
>> +static void init_gic_priority_masking(void)
>> +{
>> + u32 gic_sre = gic_read_sre();
>> + u32 cpuflags;
>> +
>> + if (WARN_ON(!(gic_sre & ICC_SRE_EL1_SRE)))
>> + return;
>> +
>> + WARN_ON(!irqs_disabled());
>> +
>> + gic_write_pmr(GIC_PRIO_IRQOFF);
>> +
>> + cpuflags = read_sysreg(daif);
>> +
>> + /* We can only unmask PSR.I if we can take aborts */
>> + if (!(cpuflags & PSR_A_BIT))
>> + write_sysreg(cpuflags & ~PSR_I_BIT, daif);
>
> I don't understand this. If you don't switch off PSR_I_BIT here, where
> does it happen? In which scenario do we actually have the A bit still
> set? At a quick look, smp_prepare_boot_cpu() would have the A bit
> cleared previously by setup_arch(). We have secondary_start_kernel()
> where you call init_gic_priority_masking() before local_daif_restore().
>

So this is for secondary CPUs where PSR.A can be still set.

The thing is that the daifflags.h establishes the order for disabling
types of exceptions:
Debug > Abort > IRQ

The idea is that when introducing pseudo-NMIs this becomes:
Debug > Abort > pseudo-NMI > IRQ

Whenever aborts are disabled (maybe because we just took an abort) we
don't want to take an NMI.

> So what happens if you always turn off PSR_I_BIT here?
>

So semantically it would be saying "we can take a pseudo-NMI here".
Realistically, I think it depends on the state of the GIC redistributor
for this CPU:
- If the re-distributor was initialized, nothing bad could happen as no
NMI could have been configured for this CPU yet.
- If the re-distributor initialization is done between the call to
init_gic_priority_mask() and the local_daif_restore() then probably bad
things could happen

I can try to figure out if it is safe to just clear PSR.I always, but I
also find it easier to always play by the rule "if PSR.A is set, PSR.I
is set".

Thanks,

--
Julien Thierry

2018-12-05 13:03:40

by James Morse

Subject: Re: [PATCH v6 21/24] arm64: Handle serror in NMI context

Hi Catalin,

On 04/12/2018 18:09, Catalin Marinas wrote:
> On Mon, Nov 12, 2018 at 11:57:12AM +0000, Julien Thierry wrote:
>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> index 5f4d9ac..66344cd 100644
>> --- a/arch/arm64/kernel/traps.c
>> +++ b/arch/arm64/kernel/traps.c
>> @@ -897,13 +897,17 @@ bool arm64_is_fatal_ras_serror(struct pt_regs *regs, unsigned int esr)
>>
>> asmlinkage void do_serror(struct pt_regs *regs, unsigned int esr)
>> {
>> - nmi_enter();
>> + const bool was_in_nmi = in_nmi();
>> +
>> + if (!was_in_nmi)
>> + nmi_enter();
>>
>> /* non-RAS errors are not containable */
>> if (!arm64_is_ras_serror(esr) || arm64_is_fatal_ras_serror(regs, esr))
>> arm64_serror_panic(regs, esr);
>>
>> - nmi_exit();
>> + if (!was_in_nmi)
>> + nmi_exit();
>> }
>
> Do we actually need nmi_enter/exit in the outer do_serror() function?
> Could we just move it to arm64_serror_panic()?

They might need to be here in the future: if we support kernel-first we would
have extra calls in here that need to be in_nmi(), the same if we call out to
APEI to support APCI's NOTIFY_SEI.


Thanks,

James

2018-12-05 16:58:05

by Julien Thierry

Subject: Re: [PATCH v6 10/24] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking

On 04/12/18 17:36, Catalin Marinas wrote:
> On Mon, Nov 12, 2018 at 11:57:01AM +0000, Julien Thierry wrote:
>> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
>> index 24692ed..e0a32e4 100644
>> --- a/arch/arm64/include/asm/irqflags.h
>> +++ b/arch/arm64/include/asm/irqflags.h
>> @@ -18,7 +18,27 @@
>>
>> #ifdef __KERNEL__
>>
>> +#include <asm/alternative.h>
>> +#include <asm/cpufeature.h>
>> #include <asm/ptrace.h>
>> +#include <asm/sysreg.h>
>> +
>> +
>> +/*
>> + * When ICC_PMR_EL1 is used for interrupt masking, only the bit indicating
>> + * whether the normal interrupts are masked is kept along with the daif
>> + * flags.
>> + */
>> +#define ARCH_FLAG_PMR_EN 0x1
>> +
>> +#define MAKE_ARCH_FLAGS(daif, pmr) \
>> + ((daif) | (((pmr) >> GIC_PRIO_STATUS_SHIFT) & ARCH_FLAG_PMR_EN))
>> +
>> +#define ARCH_FLAGS_GET_PMR(flags) \
>> + ((((flags) & ARCH_FLAG_PMR_EN) << GIC_PRIO_STATUS_SHIFT) \
>> + | GIC_PRIO_IRQOFF)
>> +
>> +#define ARCH_FLAGS_GET_DAIF(flags) ((flags) & ~ARCH_FLAG_PMR_EN)
>
> I wonder whether we could just use the PSR_I_BIT here to decide whether
> to set the GIC_PRIO_IRQ{ON,OFF}. We could clear the PSR_I_BIT in
> _restore_daif() with an alternative.
>

So, the issue with it is that some contexts might be using PSR.I to
disable interrupts (any contexts with async errors or debug exceptions
disabled, kvm guest entry paths, pseudo-NMIs, ...).

If any of these contexts calls local_irq_save()/local_irq_restore() or
local_daif_save()/local_daif_restore(), by only relying on PSR_I_BIT to
represent the PMR status, we might end up clearing PSR.I when we shouldn't.

I'm not sure whether there are no callers of these functions in those
context. But if that is the case, we could simplify things, yes.

Thanks,


>> +/*
>> + * CPU interrupt mask handling.
>> + */
>> 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)
>
> DSB needed here as well? I guess I'd have to read the GIC spec before
> asking again ;).
>

--
Julien Thierry

2018-12-05 18:27:13

by Catalin Marinas

Subject: Re: [PATCH v6 10/24] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking

On Wed, Dec 05, 2018 at 04:55:54PM +0000, Julien Thierry wrote:
> On 04/12/18 17:36, Catalin Marinas wrote:
> > On Mon, Nov 12, 2018 at 11:57:01AM +0000, Julien Thierry wrote:
> >> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
> >> index 24692ed..e0a32e4 100644
> >> --- a/arch/arm64/include/asm/irqflags.h
> >> +++ b/arch/arm64/include/asm/irqflags.h
> >> @@ -18,7 +18,27 @@
> >>
> >> #ifdef __KERNEL__
> >>
> >> +#include <asm/alternative.h>
> >> +#include <asm/cpufeature.h>
> >> #include <asm/ptrace.h>
> >> +#include <asm/sysreg.h>
> >> +
> >> +
> >> +/*
> >> + * When ICC_PMR_EL1 is used for interrupt masking, only the bit indicating
> >> + * whether the normal interrupts are masked is kept along with the daif
> >> + * flags.
> >> + */
> >> +#define ARCH_FLAG_PMR_EN 0x1
> >> +
> >> +#define MAKE_ARCH_FLAGS(daif, pmr) \
> >> + ((daif) | (((pmr) >> GIC_PRIO_STATUS_SHIFT) & ARCH_FLAG_PMR_EN))
> >> +
> >> +#define ARCH_FLAGS_GET_PMR(flags) \
> >> + ((((flags) & ARCH_FLAG_PMR_EN) << GIC_PRIO_STATUS_SHIFT) \
> >> + | GIC_PRIO_IRQOFF)
> >> +
> >> +#define ARCH_FLAGS_GET_DAIF(flags) ((flags) & ~ARCH_FLAG_PMR_EN)
> >
> > I wonder whether we could just use the PSR_I_BIT here to decide whether
> > to set the GIC_PRIO_IRQ{ON,OFF}. We could clear the PSR_I_BIT in
> > _restore_daif() with an alternative.
>
> So, the issue with it is that some contexts might be using PSR.I to
> disable interrupts (any contexts with async errors or debug exceptions
> disabled, kvm guest entry paths, pseudo-NMIs, ...).
>
> If any of these contexts calls local_irq_save()/local_irq_restore() or
> local_daif_save()/local_daif_restore(), by only relying on PSR_I_BIT to
> represent the PMR status, we might end up clearing PSR.I when we shouldn't.
>
> I'm not sure whether there are no callers of these functions in those
> context. But if that is the case, we could simplify things, yes.

There are callers of local_daif_save() (3) and local_daif_mask() (7) but
do they all need to disable the pseudo-NMIs?

At a brief look at x86, it seems that they have something like
stop_nmi() and restart_nmi(). These don't have save/restore semantics,
so we could do something similar on arm64 that only deals with the
PSTATE.I bit directly and keep the software (flags) PSR.I as the PMR
bit. But we'd have to go through the 10 local_daif_* cases above to see
which actually need the stop_nmi() semantics.

--
Catalin

2018-12-06 09:51:51

by Julien Thierry

Subject: Re: [PATCH v6 10/24] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking

On 05/12/18 18:26, Catalin Marinas wrote:
> On Wed, Dec 05, 2018 at 04:55:54PM +0000, Julien Thierry wrote:
>> On 04/12/18 17:36, Catalin Marinas wrote:
>>> On Mon, Nov 12, 2018 at 11:57:01AM +0000, Julien Thierry wrote:
>>>> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
>>>> index 24692ed..e0a32e4 100644
>>>> --- a/arch/arm64/include/asm/irqflags.h
>>>> +++ b/arch/arm64/include/asm/irqflags.h
>>>> @@ -18,7 +18,27 @@
>>>>
>>>> #ifdef __KERNEL__
>>>>
>>>> +#include <asm/alternative.h>
>>>> +#include <asm/cpufeature.h>
>>>> #include <asm/ptrace.h>
>>>> +#include <asm/sysreg.h>
>>>> +
>>>> +
>>>> +/*
>>>> + * When ICC_PMR_EL1 is used for interrupt masking, only the bit indicating
>>>> + * whether the normal interrupts are masked is kept along with the daif
>>>> + * flags.
>>>> + */
>>>> +#define ARCH_FLAG_PMR_EN 0x1
>>>> +
>>>> +#define MAKE_ARCH_FLAGS(daif, pmr) \
>>>> + ((daif) | (((pmr) >> GIC_PRIO_STATUS_SHIFT) & ARCH_FLAG_PMR_EN))
>>>> +
>>>> +#define ARCH_FLAGS_GET_PMR(flags) \
>>>> + ((((flags) & ARCH_FLAG_PMR_EN) << GIC_PRIO_STATUS_SHIFT) \
>>>> + | GIC_PRIO_IRQOFF)
>>>> +
>>>> +#define ARCH_FLAGS_GET_DAIF(flags) ((flags) & ~ARCH_FLAG_PMR_EN)
>>>
>>> I wonder whether we could just use the PSR_I_BIT here to decide whether
>>> to set the GIC_PRIO_IRQ{ON,OFF}. We could clear the PSR_I_BIT in
>>> _restore_daif() with an alternative.
>>
>> So, the issue with it is that some contexts might be using PSR.I to
>> disable interrupts (any contexts with async errors or debug exceptions
>> disabled, kvm guest entry paths, pseudo-NMIs, ...).
>>
>> If any of these contexts calls local_irq_save()/local_irq_restore() or
>> local_daif_save()/local_daif_restore(), by only relying on PSR_I_BIT to
>> represent the PMR status, we might end up clearing PSR.I when we shouldn't.
>>
>> I'm not sure whether there are no callers of these functions in those
>> context. But if that is the case, we could simplify things, yes.
>
> There are callers of local_daif_save() (3) and local_daif_mask() (7) but
> do they all need to disable the pseudo-NMIs?
>

Hmmm, I really think that both of those should be disabling NMIs.
Otherwise, if we take an NMI, the first thing the el1_irq handler is
going to do is "enable_da_f()" which could lead to potential issues.

One thing that could be done is:
- local_daif_save() and local_daif_mask() both mask all daif bits
(taking care to represent PMR value in the I bit of the saved flags)
- local_daif_restore() restores da_f as expected and decides values to
put for PMR and PSR.I as follows:
* do the da_f restore
* if PSR.A bit is cleared in the saved flags, then we also do a start_nmi()

However, this would not work with a local_daif_save()/restore() on the
return path of an NMI because I think it is the only context with NMIs
"stopped" that can take aborts. I can add a WARN_ON(in_nmi()) for
local_daif_restore() if that doesn't affect performance too much.

Does that sound alright?

> At a brief look at x86, it seems that they have something like
> stop_nmi() and restart_nmi(). These don't have save/restore semantics,
> so we could do something similar on arm64 that only deals with the
> PSTATE.I bit directly and keep the software (flags) PSR.I as the PMR
> bit. But we'd have to go through the 10 local_daif_* cases above to see
> which actually need the stop_nmi() semantics.
>

Yes, having those could be useful to deal with the above and maybe some
other places.

Thanks,

--
Julien Thierry

2018-12-10 16:21:11

by Catalin Marinas

Subject: Re: [PATCH v6 10/24] arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking

On Thu, Dec 06, 2018 at 09:50:18AM +0000, Julien Thierry wrote:
> On 05/12/18 18:26, Catalin Marinas wrote:
> > On Wed, Dec 05, 2018 at 04:55:54PM +0000, Julien Thierry wrote:
> >> On 04/12/18 17:36, Catalin Marinas wrote:
> >>> On Mon, Nov 12, 2018 at 11:57:01AM +0000, Julien Thierry wrote:
> >>>> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
> >>>> index 24692ed..e0a32e4 100644
> >>>> --- a/arch/arm64/include/asm/irqflags.h
> >>>> +++ b/arch/arm64/include/asm/irqflags.h
> >>>> @@ -18,7 +18,27 @@
> >>>>
> >>>> #ifdef __KERNEL__
> >>>>
> >>>> +#include <asm/alternative.h>
> >>>> +#include <asm/cpufeature.h>
> >>>> #include <asm/ptrace.h>
> >>>> +#include <asm/sysreg.h>
> >>>> +
> >>>> +
> >>>> +/*
> >>>> + * When ICC_PMR_EL1 is used for interrupt masking, only the bit indicating
> >>>> + * whether the normal interrupts are masked is kept along with the daif
> >>>> + * flags.
> >>>> + */
> >>>> +#define ARCH_FLAG_PMR_EN 0x1
> >>>> +
> >>>> +#define MAKE_ARCH_FLAGS(daif, pmr) \
> >>>> + ((daif) | (((pmr) >> GIC_PRIO_STATUS_SHIFT) & ARCH_FLAG_PMR_EN))
> >>>> +
> >>>> +#define ARCH_FLAGS_GET_PMR(flags) \
> >>>> + ((((flags) & ARCH_FLAG_PMR_EN) << GIC_PRIO_STATUS_SHIFT) \
> >>>> + | GIC_PRIO_IRQOFF)
> >>>> +
> >>>> +#define ARCH_FLAGS_GET_DAIF(flags) ((flags) & ~ARCH_FLAG_PMR_EN)
> >>>
> >>> I wonder whether we could just use the PSR_I_BIT here to decide whether
> >>> to set the GIC_PRIO_IRQ{ON,OFF}. We could clear the PSR_I_BIT in
> >>> _restore_daif() with an alternative.
> >>
> >> So, the issue with it is that some contexts might be using PSR.I to
> >> disable interrupts (any contexts with async errors or debug exceptions
> >> disabled, kvm guest entry paths, pseudo-NMIs, ...).
> >>
> >> If any of these contexts calls local_irq_save()/local_irq_restore() or
> >> local_daif_save()/local_daif_restore(), by only relying on PSR_I_BIT to
> >> represent the PMR status, we might end up clearing PSR.I when we shouldn't.
> >>
> >> I'm not sure whether there are no callers of these functions in those
> >> context. But if that is the case, we could simplify things, yes.
> >
> > There are callers of local_daif_save() (3) and local_daif_mask() (7) but
> > do they all need to disable the pseudo-NMIs?
>
> Hmmm, I really think that both of those should be disabling NMIs.
> Otherwise, if we take an NMI, the first thing the el1_irq handler is
> going to do is "enable_da_f()" which could lead to potential issues.
>
> One thing that could be done is:
> - local_daif_save() and local_daif_mask() both mask all daif bits
> (taking care to represent PMR value in the I bit of the saved flags)
> - local_daif_restore() restores da_f as expected and decides values to
> put for PMR and PSR.I as follows:
> * do the da_f restore
> * if PSR.A bit is cleared in the saved flags, then we also do a start_nmi()
>
> However, this would not work with a local_daif_save()/restore() on the
> return path of an NMI because I think it is the only context with NMIs
> "stopped" that can take aborts. I can add a WARN_ON(in_nmi()) for
> local_daif_restore() if that doesn't affect performance too much.

FTR, as we discussed this in the office, the conclusion (IIUC) we got to
was: leave the *_daif_*() functions unchanged, touching all the
corresponding PSTATE bits, but change the arch_local_irq_*() macros to
only touch the PMR when the feature is enabled.

--
Catalin