2020-06-25 15:01:36

by Alexandru Elisei

[permalink] [raw]
Subject: [PATCH 2/2] irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0

The GIC's internal view of the priority mask register and the assigned
interrupt priorities are based on whether GIC security is enabled and
whether firmware routes Group 0 interrupts to EL3. At the moment, we
support priority masking when ICC_PMR_EL1 and interrupt priorities are
either both modified by the GIC, or both left unchanged.

Trusted Firmware-A's default interrupt routing model allows Group 0
interrupts to be delivered to the non-secure world (SCR_EL3.FIQ == 0).
Unfortunately, this is precisely the case that the GIC driver doesn't
support: ICC_PMR_EL1 remains unchanged, but the GIC's view of interrupt
priorities is different from the software programmed values.

Support pseudo-NMIs when SCR_EL3.FIQ == 0 by using a different value to
mask regular interrupts. All the other values remain the same.

Signed-off-by: Alexandru Elisei <[email protected]>
---
arch/arm64/include/asm/arch_gicv3.h | 8 ++++-
arch/arm64/include/asm/daifflags.h | 4 +--
arch/arm64/include/asm/irqflags.h | 14 +++++---
arch/arm64/include/asm/ptrace.h | 12 +++++++
arch/arm64/kernel/entry.S | 2 +-
arch/arm64/kvm/hyp/switch.c | 2 +-
drivers/irqchip/irq-gic-v3.c | 52 ++++++++++++++++++++++-------
7 files changed, 73 insertions(+), 21 deletions(-)

diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
index a358e97572c1..c2a67a81e39d 100644
--- a/arch/arm64/include/asm/arch_gicv3.h
+++ b/arch/arm64/include/asm/arch_gicv3.h
@@ -162,7 +162,13 @@ static inline void gic_pmr_mask_irqs(void)
* are applied to IRQ priorities
*/
BUILD_BUG_ON((0x80 | (GICD_INT_DEF_PRI >> 1)) >= GIC_PRIO_IRQON);
- gic_write_pmr(GIC_PRIO_IRQOFF);
+ /*
+ * Same situation as above, but now we make sure that we can mask
+ * regular interrupts.
+ */
+ BUILD_BUG_ON((0x80 | (GICD_INT_DEF_PRI >> 1)) < (GIC_PRIO_IRQOFF_NS |
+ GIC_PRIO_PSR_I_SET));
+ gic_write_pmr(gic_prio_irqoff());
}

static inline void gic_arch_enable_irqs(void)
diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
index ec213b4a1650..3efa240a6c48 100644
--- a/arch/arm64/include/asm/daifflags.h
+++ b/arch/arm64/include/asm/daifflags.h
@@ -22,7 +22,7 @@
static inline void local_daif_mask(void)
{
WARN_ON(system_has_prio_mask_debugging() &&
- (read_sysreg_s(SYS_ICC_PMR_EL1) == (GIC_PRIO_IRQOFF |
+ (read_sysreg_s(SYS_ICC_PMR_EL1) == (gic_prio_irqoff() |
GIC_PRIO_PSR_I_SET)));

asm volatile(
@@ -87,7 +87,7 @@ static inline void local_daif_restore(unsigned long flags)
* asynchronous errors, we can take NMIs
*/
flags &= ~PSR_I_BIT;
- pmr = GIC_PRIO_IRQOFF;
+ pmr = gic_prio_irqoff();
} else {
pmr = GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET;
}
diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
index aa4b6521ef14..dc68e11c63a1 100644
--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h
@@ -28,10 +28,13 @@
*/
static inline void arch_local_irq_enable(void)
{
+ u32 pmr_irqon = GIC_PRIO_IRQON;
+
if (system_has_prio_mask_debugging()) {
u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
+ u32 pmr_irqoff = gic_prio_irqoff();

- WARN_ON_ONCE(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
+ WARN_ON_ONCE(pmr != pmr_irqon && pmr != pmr_irqoff);
}

asm volatile(ALTERNATIVE(
@@ -39,7 +42,7 @@ static inline void arch_local_irq_enable(void)
__msr_s(SYS_ICC_PMR_EL1, "%0"),
ARM64_HAS_IRQ_PRIO_MASKING)
:
- : "r" ((unsigned long) GIC_PRIO_IRQON)
+ : "r" (pmr_irqon)
: "memory");

pmr_sync();
@@ -47,10 +50,13 @@ static inline void arch_local_irq_enable(void)

static inline void arch_local_irq_disable(void)
{
+ u32 pmr_irqoff = gic_prio_irqoff();
+
if (system_has_prio_mask_debugging()) {
u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
+ u32 pmr_irqon = GIC_PRIO_IRQON;

- WARN_ON_ONCE(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
+ WARN_ON_ONCE(pmr != pmr_irqon && pmr != pmr_irqoff);
}

asm volatile(ALTERNATIVE(
@@ -58,7 +64,7 @@ static inline void arch_local_irq_disable(void)
__msr_s(SYS_ICC_PMR_EL1, "%0"),
ARM64_HAS_IRQ_PRIO_MASKING)
:
- : "r" ((unsigned long) GIC_PRIO_IRQOFF)
+ : "r" (pmr_irqoff)
: "memory");
}

diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 953b6a1ce549..ad58f05544a1 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -32,6 +32,7 @@
*/
#define GIC_PRIO_IRQON 0xe0
#define GIC_PRIO_IRQOFF (GIC_PRIO_IRQON & ~0x80)
+#define GIC_PRIO_IRQOFF_NS 0xa0
#define GIC_PRIO_PSR_I_SET (1 << 4)

/* Additional SPSR bits not exposed in the UABI */
@@ -129,6 +130,17 @@
#define compat_sp_fiq regs[29]
#define compat_lr_fiq regs[30]

+#define gic_prio_irqoff() \
+ ({ \
+ extern struct static_key_false gic_nonsecure_priorities;\
+ u8 __prio = GIC_PRIO_IRQOFF; \
+ \
+ if (static_branch_unlikely(&gic_nonsecure_priorities)) \
+ __prio = GIC_PRIO_IRQOFF_NS; \
+ \
+ __prio; \
+ })
+
static inline unsigned long compat_psr_to_pstate(const unsigned long psr)
{
unsigned long pstate;
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 5304d193c79d..73654234f454 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -643,7 +643,7 @@ alternative_else_nop_endif
#ifdef CONFIG_ARM64_PSEUDO_NMI
/*
* When using IRQ priority masking, we can get spurious interrupts while
- * PMR is set to GIC_PRIO_IRQOFF. An NMI might also have occurred in a
+ * PMR is set to mask interrupts. An NMI might also have occurred in a
* section with interrupts disabled. Skip tracing in those cases.
*/
test_irqs_unmasked res=x0, pmr=x20
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index db1c4487d95d..40d1041ab46d 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -870,7 +870,7 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)

/* Returning to host will clear PSR.I, remask PMR if needed */
if (system_uses_irq_prio_masking())
- gic_write_pmr(GIC_PRIO_IRQOFF);
+ gic_write_pmr(gic_prio_irqoff());

return exit_code;
}
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 83103277d2a9..012ff8819313 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -75,16 +75,14 @@ static DEFINE_STATIC_KEY_TRUE(supports_deactivate_key);
*
* 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.
+ * from the (re)distributor when security is enabled. When SCR_EL3.FIQ == 0,
+ * these values are unchanched by the GIC.
*
* 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);

@@ -97,6 +95,9 @@ static DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis);
DEFINE_STATIC_KEY_FALSE(gic_pmr_sync);
EXPORT_SYMBOL(gic_pmr_sync);

+DEFINE_STATIC_KEY_FALSE(gic_nonsecure_priorities);
+EXPORT_SYMBOL(gic_nonsecure_priorities);
+
/* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */
static refcount_t *ppi_nmi_refs;

@@ -932,14 +933,16 @@ 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 {
+ } else if (gic_supports_nmi()) {
/*
* Mismatch configuration with boot CPU, the system is likely
* to die as interrupt masking will not work properly on all
* CPUs
*/
- WARN_ON(gic_supports_nmi() && group0 &&
- !gic_dist_security_disabled());
+ if (static_branch_unlikely(&gic_nonsecure_priorities))
+ WARN_ON(!group0 || gic_dist_security_disabled());
+ else
+ WARN_ON(group0 && !gic_dist_security_disabled());
}

/*
@@ -1544,11 +1547,6 @@ static void gic_enable_nmi_support(void)
if (!gic_prio_masking_enabled())
return;

- if (gic_has_group0() && !gic_dist_security_disabled()) {
- pr_warn("SCR_EL3.FIQ is cleared, cannot enable use of pseudo-NMIs\n");
- return;
- }
-
ppi_nmi_refs = kcalloc(gic_data.ppi_nr, sizeof(*ppi_nmi_refs), GFP_KERNEL);
if (!ppi_nmi_refs)
return;
@@ -1567,6 +1565,36 @@ static void gic_enable_nmi_support(void)
pr_info("Pseudo-NMIs enabled using %s ICC_PMR_EL1 synchronisation\n",
static_branch_unlikely(&gic_pmr_sync) ? "forced" : "relaxed");

+ /*
+ * How priority values are used by the GIC depends on two things:
+ * the security state of the GIC (controlled by the GICD_CTRL.DS bit)
+ * and if Group 0 interrupts can be delivered to Linux in the non-secure
+ * world as FIQs (controlled by the SCR_EL3.FIQ bit). These affect the
+ * the ICC_PMR_EL1 register and the priority that software assigns to
+ * interrupts:
+ *
+ * GICD_CTRL.DS | SCR_EL3.FIQ | ICC_PMR_EL1 | Group 1 priority
+ * -----------------------------------------------------------
+ * 1 | - | unchanged | unchanged
+ * -----------------------------------------------------------
+ * 0 | 1 | non-secure | non-secure
+ * -----------------------------------------------------------
+ * 0 | 0 | unchanged | non-secure
+ *
+ * where non-secure means that the value is right-shifted by one and the
+ * MSB bit set, to make it fit in the non-secure priority range.
+ *
+ * In the first two cases, where ICC_PMR_EL1 and the interrupt priority
+ * are both either modified, or unchanged, we can use the same set of
+ * priorities.
+ *
+ * In the last case, where only the interrupt priorities are modified to
+ * be in the non-secure range, we use a different PMR value to mask IRQs
+ * and the rest of the values that we use remain unchanged.
+ */
+ if (gic_has_group0() && !gic_dist_security_disabled())
+ static_branch_enable(&gic_nonsecure_priorities);
+
static_branch_enable(&supports_pseudo_nmis);

if (static_branch_likely(&supports_deactivate_key))
--
2.27.0


2020-06-26 04:38:49

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH 2/2] irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0

Hi Alexandru,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on tip/irq/core v5.8-rc2 next-20200625]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Alexandru-Elisei/irqchip-gic-v3-Support-pseudo-NMIs-when-SCR_EL3-FIQ-0/20200625-230144
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: arm64-randconfig-r025-20200624 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 8911a35180c6777188fefe0954a2451a2b91deaf)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

In file included from arch/arm64/kernel/asm-offsets.c:10:
In file included from include/linux/arm_sdei.h:8:
In file included from include/acpi/ghes.h:5:
In file included from include/acpi/apei.h:9:
In file included from include/linux/acpi.h:13:
In file included from include/linux/irqdomain.h:35:
In file included from include/linux/of.h:17:
In file included from include/linux/kobject.h:20:
In file included from include/linux/sysfs.h:16:
In file included from include/linux/kernfs.h:13:
In file included from include/linux/idr.h:15:
In file included from include/linux/radix-tree.h:15:
In file included from include/linux/rcupdate.h:26:
In file included from include/linux/irqflags.h:16:
>> arch/arm64/include/asm/irqflags.h:45:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
: "r" (pmr_irqon)
^
arch/arm64/include/asm/irqflags.h:42:29: note: use constraint modifier "w"
__msr_s(SYS_ICC_PMR_EL1, "%0"),
^
arch/arm64/include/asm/irqflags.h:67:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
: "r" (pmr_irqoff)
^
arch/arm64/include/asm/irqflags.h:64:29: note: use constraint modifier "w"
__msr_s(SYS_ICC_PMR_EL1, "%0"),
^
2 warnings generated.
--
In file included from drivers/power/supply/ltc2941-battery-gauge.c:12:
In file included from include/linux/module.h:13:
In file included from include/linux/stat.h:6:
In file included from arch/arm64/include/asm/stat.h:12:
In file included from include/linux/time.h:6:
In file included from include/linux/seqlock.h:36:
In file included from include/linux/spinlock.h:54:
In file included from include/linux/irqflags.h:16:
>> arch/arm64/include/asm/irqflags.h:45:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
: "r" (pmr_irqon)
^
arch/arm64/include/asm/irqflags.h:42:29: note: use constraint modifier "w"
__msr_s(SYS_ICC_PMR_EL1, "%0"),
^
arch/arm64/include/asm/irqflags.h:67:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
: "r" (pmr_irqoff)
^
arch/arm64/include/asm/irqflags.h:64:29: note: use constraint modifier "w"
__msr_s(SYS_ICC_PMR_EL1, "%0"),
^
drivers/power/supply/ltc2941-battery-gauge.c:476:13: warning: cast to smaller integer type 'enum ltc294x_id' from 'const void *' [-Wvoid-pointer-to-enum-cast]
info->id = (enum ltc294x_id)of_device_get_match_data(&client->dev);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 warnings generated.
--
In file included from drivers/power/supply/goldfish_battery.c:11:
In file included from include/linux/module.h:13:
In file included from include/linux/stat.h:6:
In file included from arch/arm64/include/asm/stat.h:12:
In file included from include/linux/time.h:6:
In file included from include/linux/seqlock.h:36:
In file included from include/linux/spinlock.h:54:
In file included from include/linux/irqflags.h:16:
>> arch/arm64/include/asm/irqflags.h:45:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
: "r" (pmr_irqon)
^
arch/arm64/include/asm/irqflags.h:42:29: note: use constraint modifier "w"
__msr_s(SYS_ICC_PMR_EL1, "%0"),
^
arch/arm64/include/asm/irqflags.h:67:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
: "r" (pmr_irqoff)
^
arch/arm64/include/asm/irqflags.h:64:29: note: use constraint modifier "w"
__msr_s(SYS_ICC_PMR_EL1, "%0"),
^
drivers/power/supply/goldfish_battery.c:269:36: warning: unused variable 'goldfish_battery_acpi_match' [-Wunused-const-variable]
static const struct acpi_device_id goldfish_battery_acpi_match[] = {
^
3 warnings generated.
--
In file included from drivers/power/supply/bq25890_charger.c:8:
In file included from include/linux/module.h:13:
In file included from include/linux/stat.h:6:
In file included from arch/arm64/include/asm/stat.h:12:
In file included from include/linux/time.h:6:
In file included from include/linux/seqlock.h:36:
In file included from include/linux/spinlock.h:54:
In file included from include/linux/irqflags.h:16:
>> arch/arm64/include/asm/irqflags.h:45:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
: "r" (pmr_irqon)
^
arch/arm64/include/asm/irqflags.h:42:29: note: use constraint modifier "w"
__msr_s(SYS_ICC_PMR_EL1, "%0"),
^
arch/arm64/include/asm/irqflags.h:67:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
: "r" (pmr_irqoff)
^
arch/arm64/include/asm/irqflags.h:64:29: note: use constraint modifier "w"
__msr_s(SYS_ICC_PMR_EL1, "%0"),
^
drivers/power/supply/bq25890_charger.c:1060:36: warning: unused variable 'bq25890_acpi_match' [-Wunused-const-variable]
static const struct acpi_device_id bq25890_acpi_match[] = {
^
3 warnings generated.
--
In file included from drivers/power/reset/vexpress-poweroff.c:8:
In file included from include/linux/notifier.h:15:
In file included from include/linux/rwsem.h:16:
In file included from include/linux/spinlock.h:54:
In file included from include/linux/irqflags.h:16:
>> arch/arm64/include/asm/irqflags.h:45:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
: "r" (pmr_irqon)
^
arch/arm64/include/asm/irqflags.h:42:29: note: use constraint modifier "w"
__msr_s(SYS_ICC_PMR_EL1, "%0"),
^
arch/arm64/include/asm/irqflags.h:67:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
: "r" (pmr_irqoff)
^
arch/arm64/include/asm/irqflags.h:64:29: note: use constraint modifier "w"
__msr_s(SYS_ICC_PMR_EL1, "%0"),
^
drivers/power/reset/vexpress-poweroff.c:124:10: warning: cast to smaller integer type 'enum vexpress_reset_func' from 'const void *' [-Wvoid-pointer-to-enum-cast]
switch ((enum vexpress_reset_func)match->data) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 warnings generated.
--
In file included from arch/arm64/kernel/asm-offsets.c:10:
In file included from include/linux/arm_sdei.h:8:
In file included from include/acpi/ghes.h:5:
In file included from include/acpi/apei.h:9:
In file included from include/linux/acpi.h:13:
In file included from include/linux/irqdomain.h:35:
In file included from include/linux/of.h:17:
In file included from include/linux/kobject.h:20:
In file included from include/linux/sysfs.h:16:
In file included from include/linux/kernfs.h:13:
In file included from include/linux/idr.h:15:
In file included from include/linux/radix-tree.h:15:
In file included from include/linux/rcupdate.h:26:
In file included from include/linux/irqflags.h:16:
>> arch/arm64/include/asm/irqflags.h:45:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
: "r" (pmr_irqon)
^
arch/arm64/include/asm/irqflags.h:42:29: note: use constraint modifier "w"
__msr_s(SYS_ICC_PMR_EL1, "%0"),
^
arch/arm64/include/asm/irqflags.h:67:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
: "r" (pmr_irqoff)
^
arch/arm64/include/asm/irqflags.h:64:29: note: use constraint modifier "w"
__msr_s(SYS_ICC_PMR_EL1, "%0"),
^
2 warnings generated.
arch/arm64/kernel/vdso/vgettimeofday.c:9:5: warning: no previous prototype for function '__kernel_clock_gettime' [-Wmissing-prototypes]
int __kernel_clock_gettime(clockid_t clock,
^
arch/arm64/kernel/vdso/vgettimeofday.c:9:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int __kernel_clock_gettime(clockid_t clock,
^
static
arch/arm64/kernel/vdso/vgettimeofday.c:15:5: warning: no previous prototype for function '__kernel_gettimeofday' [-Wmissing-prototypes]
int __kernel_gettimeofday(struct __kernel_old_timeval *tv,
^
arch/arm64/kernel/vdso/vgettimeofday.c:15:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int __kernel_gettimeofday(struct __kernel_old_timeval *tv,
^
static
arch/arm64/kernel/vdso/vgettimeofday.c:21:5: warning: no previous prototype for function '__kernel_clock_getres' [-Wmissing-prototypes]
int __kernel_clock_getres(clockid_t clock_id,
^
arch/arm64/kernel/vdso/vgettimeofday.c:21:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int __kernel_clock_getres(clockid_t clock_id,
^
static
3 warnings generated.

vim +45 arch/arm64/include/asm/irqflags.h

12
13 /*
14 * Aarch64 has flags for masking: Debug, Asynchronous (serror), Interrupts and
15 * FIQ exceptions, in the 'daif' register. We mask and unmask them in 'dai'
16 * order:
17 * Masking debug exceptions causes all other exceptions to be masked too/
18 * Masking SError masks irq, but not debug exceptions. Masking irqs has no
19 * side effects for other flags. Keeping to this order makes it easier for
20 * entry.S to know which exceptions should be unmasked.
21 *
22 * FIQ is never expected, but we mask it when we disable debug exceptions, and
23 * unmask it at all other times.
24 */
25
26 /*
27 * CPU interrupt mask handling.
28 */
29 static inline void arch_local_irq_enable(void)
30 {
31 u32 pmr_irqon = GIC_PRIO_IRQON;
32
33 if (system_has_prio_mask_debugging()) {
34 u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
35 u32 pmr_irqoff = gic_prio_irqoff();
36
37 WARN_ON_ONCE(pmr != pmr_irqon && pmr != pmr_irqoff);
38 }
39
40 asm volatile(ALTERNATIVE(
41 "msr daifclr, #2 // arch_local_irq_enable",
42 __msr_s(SYS_ICC_PMR_EL1, "%0"),
43 ARM64_HAS_IRQ_PRIO_MASKING)
44 :
> 45 : "r" (pmr_irqon)
46 : "memory");
47
48 pmr_sync();
49 }
50

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]


Attachments:
(No filename) (13.12 kB)
.config.gz (30.82 kB)
Download all attachments

2020-06-26 11:40:40

by Alexandru Elisei

[permalink] [raw]
Subject: Re: [PATCH 2/2] irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0

Hi,

On 6/26/20 2:51 AM, kernel test robot wrote:
> Hi Alexandru,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on arm64/for-next/core]
> [also build test WARNING on tip/irq/core v5.8-rc2 next-20200625]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url: https://github.com/0day-ci/linux/commits/Alexandru-Elisei/irqchip-gic-v3-Support-pseudo-NMIs-when-SCR_EL3-FIQ-0/20200625-230144
> base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
> config: arm64-randconfig-r025-20200624 (attached as .config)
> compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 8911a35180c6777188fefe0954a2451a2b91deaf)
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # install arm64 cross compiling tool for clang build
> # apt-get install binutils-aarch64-linux-gnu
> # save the attached .config to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64

My mistake, I'll start compiling the kernel with clang too.

The register width for ICC_PMR_EL1 in the kernel is rather inconsistent: in
arch_gicv3.h, the accessors use 32 bits for the PMR value which gets casted to 64
bit by the {read,write}_sysreg_s macros anyway, in struct pt_regs the register is
64-bit, in __cpu_do_idle_irqprio it's declared as an unsigned long,
arch_local_irqs_{disable,enable} declares it as u32 and casts it to an unsigned
long when used by the inline assembly, the gicv3 irqchip driver uses it as a 32
bit variable.

I think the confusion stems from the fact that originally it was a 32 bit
register, but was changed to 64 bits in Arm IHI 0069E (January 2019).

I could cast it to an unsigned long in the inline assembly, but IMO that looks a
bit awkward. Before sending the patches I was considering changing it everywhere
to 64 bits, but Mark Rutland had a different idea. Mark, would you mind explaining
why keeping it 32 bit wide makes more sense?

Thanks,
Alex
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
>
> All warnings (new ones prefixed by >>):
>
> In file included from arch/arm64/kernel/asm-offsets.c:10:
> In file included from include/linux/arm_sdei.h:8:
> In file included from include/acpi/ghes.h:5:
> In file included from include/acpi/apei.h:9:
> In file included from include/linux/acpi.h:13:
> In file included from include/linux/irqdomain.h:35:
> In file included from include/linux/of.h:17:
> In file included from include/linux/kobject.h:20:
> In file included from include/linux/sysfs.h:16:
> In file included from include/linux/kernfs.h:13:
> In file included from include/linux/idr.h:15:
> In file included from include/linux/radix-tree.h:15:
> In file included from include/linux/rcupdate.h:26:
> In file included from include/linux/irqflags.h:16:
>>> arch/arm64/include/asm/irqflags.h:45:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
> : "r" (pmr_irqon)
> ^
> arch/arm64/include/asm/irqflags.h:42:29: note: use constraint modifier "w"
> __msr_s(SYS_ICC_PMR_EL1, "%0"),
> ^
> arch/arm64/include/asm/irqflags.h:67:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
> : "r" (pmr_irqoff)
> ^
> arch/arm64/include/asm/irqflags.h:64:29: note: use constraint modifier "w"
> __msr_s(SYS_ICC_PMR_EL1, "%0"),
> ^
> 2 warnings generated.
> --
> In file included from drivers/power/supply/ltc2941-battery-gauge.c:12:
> In file included from include/linux/module.h:13:
> In file included from include/linux/stat.h:6:
> In file included from arch/arm64/include/asm/stat.h:12:
> In file included from include/linux/time.h:6:
> In file included from include/linux/seqlock.h:36:
> In file included from include/linux/spinlock.h:54:
> In file included from include/linux/irqflags.h:16:
>>> arch/arm64/include/asm/irqflags.h:45:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
> : "r" (pmr_irqon)
> ^
> arch/arm64/include/asm/irqflags.h:42:29: note: use constraint modifier "w"
> __msr_s(SYS_ICC_PMR_EL1, "%0"),
> ^
> arch/arm64/include/asm/irqflags.h:67:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
> : "r" (pmr_irqoff)
> ^
> arch/arm64/include/asm/irqflags.h:64:29: note: use constraint modifier "w"
> __msr_s(SYS_ICC_PMR_EL1, "%0"),
> ^
> drivers/power/supply/ltc2941-battery-gauge.c:476:13: warning: cast to smaller integer type 'enum ltc294x_id' from 'const void *' [-Wvoid-pointer-to-enum-cast]
> info->id = (enum ltc294x_id)of_device_get_match_data(&client->dev);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 3 warnings generated.
> --
> In file included from drivers/power/supply/goldfish_battery.c:11:
> In file included from include/linux/module.h:13:
> In file included from include/linux/stat.h:6:
> In file included from arch/arm64/include/asm/stat.h:12:
> In file included from include/linux/time.h:6:
> In file included from include/linux/seqlock.h:36:
> In file included from include/linux/spinlock.h:54:
> In file included from include/linux/irqflags.h:16:
>>> arch/arm64/include/asm/irqflags.h:45:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
> : "r" (pmr_irqon)
> ^
> arch/arm64/include/asm/irqflags.h:42:29: note: use constraint modifier "w"
> __msr_s(SYS_ICC_PMR_EL1, "%0"),
> ^
> arch/arm64/include/asm/irqflags.h:67:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
> : "r" (pmr_irqoff)
> ^
> arch/arm64/include/asm/irqflags.h:64:29: note: use constraint modifier "w"
> __msr_s(SYS_ICC_PMR_EL1, "%0"),
> ^
> drivers/power/supply/goldfish_battery.c:269:36: warning: unused variable 'goldfish_battery_acpi_match' [-Wunused-const-variable]
> static const struct acpi_device_id goldfish_battery_acpi_match[] = {
> ^
> 3 warnings generated.
> --
> In file included from drivers/power/supply/bq25890_charger.c:8:
> In file included from include/linux/module.h:13:
> In file included from include/linux/stat.h:6:
> In file included from arch/arm64/include/asm/stat.h:12:
> In file included from include/linux/time.h:6:
> In file included from include/linux/seqlock.h:36:
> In file included from include/linux/spinlock.h:54:
> In file included from include/linux/irqflags.h:16:
>>> arch/arm64/include/asm/irqflags.h:45:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
> : "r" (pmr_irqon)
> ^
> arch/arm64/include/asm/irqflags.h:42:29: note: use constraint modifier "w"
> __msr_s(SYS_ICC_PMR_EL1, "%0"),
> ^
> arch/arm64/include/asm/irqflags.h:67:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
> : "r" (pmr_irqoff)
> ^
> arch/arm64/include/asm/irqflags.h:64:29: note: use constraint modifier "w"
> __msr_s(SYS_ICC_PMR_EL1, "%0"),
> ^
> drivers/power/supply/bq25890_charger.c:1060:36: warning: unused variable 'bq25890_acpi_match' [-Wunused-const-variable]
> static const struct acpi_device_id bq25890_acpi_match[] = {
> ^
> 3 warnings generated.
> --
> In file included from drivers/power/reset/vexpress-poweroff.c:8:
> In file included from include/linux/notifier.h:15:
> In file included from include/linux/rwsem.h:16:
> In file included from include/linux/spinlock.h:54:
> In file included from include/linux/irqflags.h:16:
>>> arch/arm64/include/asm/irqflags.h:45:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
> : "r" (pmr_irqon)
> ^
> arch/arm64/include/asm/irqflags.h:42:29: note: use constraint modifier "w"
> __msr_s(SYS_ICC_PMR_EL1, "%0"),
> ^
> arch/arm64/include/asm/irqflags.h:67:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
> : "r" (pmr_irqoff)
> ^
> arch/arm64/include/asm/irqflags.h:64:29: note: use constraint modifier "w"
> __msr_s(SYS_ICC_PMR_EL1, "%0"),
> ^
> drivers/power/reset/vexpress-poweroff.c:124:10: warning: cast to smaller integer type 'enum vexpress_reset_func' from 'const void *' [-Wvoid-pointer-to-enum-cast]
> switch ((enum vexpress_reset_func)match->data) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 3 warnings generated.
> --
> In file included from arch/arm64/kernel/asm-offsets.c:10:
> In file included from include/linux/arm_sdei.h:8:
> In file included from include/acpi/ghes.h:5:
> In file included from include/acpi/apei.h:9:
> In file included from include/linux/acpi.h:13:
> In file included from include/linux/irqdomain.h:35:
> In file included from include/linux/of.h:17:
> In file included from include/linux/kobject.h:20:
> In file included from include/linux/sysfs.h:16:
> In file included from include/linux/kernfs.h:13:
> In file included from include/linux/idr.h:15:
> In file included from include/linux/radix-tree.h:15:
> In file included from include/linux/rcupdate.h:26:
> In file included from include/linux/irqflags.h:16:
>>> arch/arm64/include/asm/irqflags.h:45:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
> : "r" (pmr_irqon)
> ^
> arch/arm64/include/asm/irqflags.h:42:29: note: use constraint modifier "w"
> __msr_s(SYS_ICC_PMR_EL1, "%0"),
> ^
> arch/arm64/include/asm/irqflags.h:67:10: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
> : "r" (pmr_irqoff)
> ^
> arch/arm64/include/asm/irqflags.h:64:29: note: use constraint modifier "w"
> __msr_s(SYS_ICC_PMR_EL1, "%0"),
> ^
> 2 warnings generated.
> arch/arm64/kernel/vdso/vgettimeofday.c:9:5: warning: no previous prototype for function '__kernel_clock_gettime' [-Wmissing-prototypes]
> int __kernel_clock_gettime(clockid_t clock,
> ^
> arch/arm64/kernel/vdso/vgettimeofday.c:9:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
> int __kernel_clock_gettime(clockid_t clock,
> ^
> static
> arch/arm64/kernel/vdso/vgettimeofday.c:15:5: warning: no previous prototype for function '__kernel_gettimeofday' [-Wmissing-prototypes]
> int __kernel_gettimeofday(struct __kernel_old_timeval *tv,
> ^
> arch/arm64/kernel/vdso/vgettimeofday.c:15:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
> int __kernel_gettimeofday(struct __kernel_old_timeval *tv,
> ^
> static
> arch/arm64/kernel/vdso/vgettimeofday.c:21:5: warning: no previous prototype for function '__kernel_clock_getres' [-Wmissing-prototypes]
> int __kernel_clock_getres(clockid_t clock_id,
> ^
> arch/arm64/kernel/vdso/vgettimeofday.c:21:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
> int __kernel_clock_getres(clockid_t clock_id,
> ^
> static
> 3 warnings generated.
>
> vim +45 arch/arm64/include/asm/irqflags.h
>
> 12
> 13 /*
> 14 * Aarch64 has flags for masking: Debug, Asynchronous (serror), Interrupts and
> 15 * FIQ exceptions, in the 'daif' register. We mask and unmask them in 'dai'
> 16 * order:
> 17 * Masking debug exceptions causes all other exceptions to be masked too/
> 18 * Masking SError masks irq, but not debug exceptions. Masking irqs has no
> 19 * side effects for other flags. Keeping to this order makes it easier for
> 20 * entry.S to know which exceptions should be unmasked.
> 21 *
> 22 * FIQ is never expected, but we mask it when we disable debug exceptions, and
> 23 * unmask it at all other times.
> 24 */
> 25
> 26 /*
> 27 * CPU interrupt mask handling.
> 28 */
> 29 static inline void arch_local_irq_enable(void)
> 30 {
> 31 u32 pmr_irqon = GIC_PRIO_IRQON;
> 32
> 33 if (system_has_prio_mask_debugging()) {
> 34 u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
> 35 u32 pmr_irqoff = gic_prio_irqoff();
> 36
> 37 WARN_ON_ONCE(pmr != pmr_irqon && pmr != pmr_irqoff);
> 38 }
> 39
> 40 asm volatile(ALTERNATIVE(
> 41 "msr daifclr, #2 // arch_local_irq_enable",
> 42 __msr_s(SYS_ICC_PMR_EL1, "%0"),
> 43 ARM64_HAS_IRQ_PRIO_MASKING)
> 44 :
> > 45 : "r" (pmr_irqon)
> 46 : "memory");
> 47
> 48 pmr_sync();
> 49 }
> 50
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/[email protected]