[PATCH] irqchip/gic-v3: Fix priority comparison when non-secure priorities are used

[PATCH] irqchip/gic-v3: Fix priority comparison when non-secure priorities are used

[Chen-Yu Tsai]

When non-secure priorities are used, compared to the raw priority set,
the value read back from RPR is also right-shifted by one and the
highest bit set.

Add a macro to do the modifications to the raw priority when doing the
comparison against the RPR value. This corrects the pseudo-NMI behavior
when non-secure priorities in the GIC are used. Tested on 5.10 with
the "IPI as pseudo-NMI" series [1] applied on MT8195.

[1] https://lore.kernel.org/linux-arm-kernel/1604317487-14543-1-git-send-email-sumit.garg@linaro.org/

Fixes: 336780590990 ("irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/irqchip/irq-gic-v3.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index e0f4debe64e1..e7a0b55413db 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -100,6 +100,15 @@ EXPORT_SYMBOL(gic_pmr_sync);
 DEFINE_STATIC_KEY_FALSE(gic_nonsecure_priorities);
 EXPORT_SYMBOL(gic_nonsecure_priorities);
 
+#define GICD_INT_RPR_PRI(priority)					\
+	({								\
+		u32 __priority = (priority);				\
+		if (static_branch_unlikely(&gic_nonsecure_priorities))	\
+			__priority = 0x80 | (__priority >> 1);		\
+									\
+		__priority;						\
+	})
+
 /* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */
 static refcount_t *ppi_nmi_refs;
 
@@ -687,7 +696,7 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
 		return;
 
 	if (gic_supports_nmi() &&
-	    unlikely(gic_read_rpr() == GICD_INT_NMI_PRI)) {
+	    unlikely(gic_read_rpr() == GICD_INT_RPR_PRI(GICD_INT_NMI_PRI))) {
 		gic_handle_nmi(irqnr, regs);
 		return;
 	}
-- 
2.32.0.605.g8dce9f2422-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqchip/gic-v3: Fix priority comparison when non-secure priorities are used

[Marc Zyngier]

+ Alex, who introduced this.

On Wed, 11 Aug 2021 18:15:05 +0100,
Chen-Yu Tsai <wenst@chromium.org> wrote:
> 
> When non-secure priorities are used, compared to the raw priority set,
> the value read back from RPR is also right-shifted by one and the
> highest bit set.
> 
> Add a macro to do the modifications to the raw priority when doing the
> comparison against the RPR value. This corrects the pseudo-NMI behavior
> when non-secure priorities in the GIC are used. Tested on 5.10 with
> the "IPI as pseudo-NMI" series [1] applied on MT8195.
> 
> [1] https://lore.kernel.org/linux-arm-kernel/1604317487-14543-1-git-send-email-sumit.garg@linaro.org/
> 
> Fixes: 336780590990 ("irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>  drivers/irqchip/irq-gic-v3.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index e0f4debe64e1..e7a0b55413db 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -100,6 +100,15 @@ EXPORT_SYMBOL(gic_pmr_sync);
>  DEFINE_STATIC_KEY_FALSE(gic_nonsecure_priorities);
>  EXPORT_SYMBOL(gic_nonsecure_priorities);
>  
> +#define GICD_INT_RPR_PRI(priority)					\
> +	({								\
> +		u32 __priority = (priority);				\
> +		if (static_branch_unlikely(&gic_nonsecure_priorities))	\
> +			__priority = 0x80 | (__priority >> 1);		\
> +									\
> +		__priority;						\

This doesn't reflect what the pseudocode says of a read of ICC_RPR_EL1
AFAICS. When the priority is activated, it is indeed shifted. But a
read of RPR does appear to shift things back (and you loose the lowest
bit in the process). Please see 'aarch64/support/ICC_RPR_EL1' in the
architecture spec.

Can you confirm that SCR_EL3.FIQ is set on your system?

Thanks,

	M.

> +	})
> +
>  /* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */
>  static refcount_t *ppi_nmi_refs;
>  
> @@ -687,7 +696,7 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
>  		return;
>  
>  	if (gic_supports_nmi() &&
> -	    unlikely(gic_read_rpr() == GICD_INT_NMI_PRI)) {
> +	    unlikely(gic_read_rpr() == GICD_INT_RPR_PRI(GICD_INT_NMI_PRI))) {
>  		gic_handle_nmi(irqnr, regs);
>  		return;
>  	}


-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqchip/gic-v3: Fix priority comparison when non-secure priorities are used

[Alexandru Elisei]

Hi,

After re-familiarizing myself with the spec, it starting to look to me like indeed
there's something not quite right (read as: totally broken) with my patch.

Arm IHI 0069F, the pseudocode for reading ICC_RPR_EL1 (page 11-797), says that the
priority returned is unchanged if SCTLR_EL3.FIQ == 0. This means that the
ICC_RPR_EL1 read will return the secure view (the value as it is stored by the
GIC) of the priority, so for pseudo-nmis it will return (GICD_INT_NMI_PRI >> 1) |
0x80, which definitely != GICD_INT_NMI_PRI. This is further confirmed by this
statement on page 4-67:

"When GICD_CTLR.DS == 0, [..] For Non-secure access to ICC_PMR_EL1 and ICC_RPR_EL1
when SCR_EL3.FIQ == 0:
The Secure, unshifted view applies."

I don't know how I missed that during testing.

Did a quick test on the model with PMU NMIs (GICD_CTRL.DS = 0, SCTLR_EL2.FIQ = 0),
gic_handle_nmi() was not being called at all, but when I changed the comparison to
gic_read_rpr() == ((GICD_INT_NMI_PRI >> 1) | 0x80), NMIs were being correctly
handled again.

Will try to run some more tests on real hardware and see if I can confirm this.

Thanks,

Alex

On 8/11/21 7:31 PM, Marc Zyngier wrote:
> + Alex, who introduced this.
>
> On Wed, 11 Aug 2021 18:15:05 +0100,
> Chen-Yu Tsai <wenst@chromium.org> wrote:
>> When non-secure priorities are used, compared to the raw priority set,
>> the value read back from RPR is also right-shifted by one and the
>> highest bit set.
>>
>> Add a macro to do the modifications to the raw priority when doing the
>> comparison against the RPR value. This corrects the pseudo-NMI behavior
>> when non-secure priorities in the GIC are used. Tested on 5.10 with
>> the "IPI as pseudo-NMI" series [1] applied on MT8195.
>>
>> [1] https://lore.kernel.org/linux-arm-kernel/1604317487-14543-1-git-send-email-sumit.garg@linaro.org/
>>
>> Fixes: 336780590990 ("irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0")
>> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
>> ---
>>  drivers/irqchip/irq-gic-v3.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index e0f4debe64e1..e7a0b55413db 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -100,6 +100,15 @@ EXPORT_SYMBOL(gic_pmr_sync);
>>  DEFINE_STATIC_KEY_FALSE(gic_nonsecure_priorities);
>>  EXPORT_SYMBOL(gic_nonsecure_priorities);
>>  
>> +#define GICD_INT_RPR_PRI(priority)					\
>> +	({								\
>> +		u32 __priority = (priority);				\
>> +		if (static_branch_unlikely(&gic_nonsecure_priorities))	\
>> +			__priority = 0x80 | (__priority >> 1);		\
>> +									\
>> +		__priority;						\
> This doesn't reflect what the pseudocode says of a read of ICC_RPR_EL1
> AFAICS. When the priority is activated, it is indeed shifted. But a
> read of RPR does appear to shift things back (and you loose the lowest
> bit in the process). Please see 'aarch64/support/ICC_RPR_EL1' in the
> architecture spec.
>
> Can you confirm that SCR_EL3.FIQ is set on your system?
>
> Thanks,
>
> 	M.
>
>> +	})
>> +
>>  /* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */
>>  static refcount_t *ppi_nmi_refs;
>>  
>> @@ -687,7 +696,7 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
>>  		return;
>>  
>>  	if (gic_supports_nmi() &&
>> -	    unlikely(gic_read_rpr() == GICD_INT_NMI_PRI)) {
>> +	    unlikely(gic_read_rpr() == GICD_INT_RPR_PRI(GICD_INT_NMI_PRI))) {
>>  		gic_handle_nmi(irqnr, regs);
>>  		return;
>>  	}
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqchip/gic-v3: Fix priority comparison when non-secure priorities are used

[Marc Zyngier]

On Thu, 12 Aug 2021 12:51:34 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi,
> 
> After re-familiarizing myself with the spec, it starting to look to
> me like indeed there's something not quite right (read as: totally
> broken) with my patch.
> 
> Arm IHI 0069F, the pseudocode for reading ICC_RPR_EL1 (page 11-797),
> says that the priority returned is unchanged if SCTLR_EL3.FIQ ==
> 0.

Sure, but look at what ICC_RPR_EL1 does for FIQ==1:

<quote>
if HaveEL(EL3) && !IsSecure() && SCR_EL3.FIQ == '1' then
    // A Non-secure GIC access and Group 0 inaccessible to Non-secure.
        if pPriority<7> == '0' then
	    // Priority is in Secure half and not visible to Non-secure
	    Priority = Zeros();
        elsif !IsOnes(pPriority) then
	    // Non-secure access and not idle, so physical priority must be shifted
            pPriority<7:0> = (pPriority AND PRIMask())<6:0>:'0';

return ZeroExtend(pPriority);
</quote>

See how the the priority is shifted *left* (bits [6:0] end up in
[7:1])?

> This means that the ICC_RPR_EL1 read will return the secure view
> (the value as it is stored by the GIC) of the priority, so for
> pseudo-nmis it will return (GICD_INT_NMI_PRI >> 1) | 0x80, which
> definitely != GICD_INT_NMI_PRI.

That's not my reading of the pseudocode.

> This is further confirmed by this statement on page 4-67:
> 
> "When GICD_CTLR.DS == 0, [..] For Non-secure access to ICC_PMR_EL1
> and ICC_RPR_EL1 when SCR_EL3.FIQ == 0: The Secure, unshifted view
> applies."
> 
> I don't know how I missed that during testing.
> 
> Did a quick test on the model with PMU NMIs (GICD_CTRL.DS = 0,
> SCTLR_EL2.FIQ = 0), gic_handle_nmi() was not being called at all,

0? Really????

> but when I changed the comparison to gic_read_rpr() ==
> ((GICD_INT_NMI_PRI >> 1) | 0x80), NMIs were being correctly handled
> again.

You have completely lost me. This contradicts what you have written
above.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqchip/gic-v3: Fix priority comparison when non-secure priorities are used

[Alexandru Elisei]

Hi Marc,

On 8/12/21 2:09 PM, Marc Zyngier wrote:
> On Thu, 12 Aug 2021 12:51:34 +0100,
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>> Hi,
>>
>> After re-familiarizing myself with the spec, it starting to look to
>> me like indeed there's something not quite right (read as: totally
>> broken) with my patch.
>>
>> Arm IHI 0069F, the pseudocode for reading ICC_RPR_EL1 (page 11-797),
>> says that the priority returned is unchanged if SCTLR_EL3.FIQ ==
>> 0.
> Sure, but look at what ICC_RPR_EL1 does for FIQ==1:
>
> <quote>
> if HaveEL(EL3) && !IsSecure() && SCR_EL3.FIQ == '1' then
>     // A Non-secure GIC access and Group 0 inaccessible to Non-secure.
>         if pPriority<7> == '0' then
> 	    // Priority is in Secure half and not visible to Non-secure
> 	    Priority = Zeros();
>         elsif !IsOnes(pPriority) then
> 	    // Non-secure access and not idle, so physical priority must be shifted
>             pPriority<7:0> = (pPriority AND PRIMask())<6:0>:'0';
>
> return ZeroExtend(pPriority);
> </quote>
>
> See how the the priority is shifted *left* (bits [6:0] end up in
> [7:1])?

Yes, when SCR_EL3.FIQ=1, but gic_nonsecure_priorities is enabled when
SCR_EL3.FIQ=0 (gic_has_group0()). In that case ICC_RPR_EL1 returns (what I assume
to be) the highest priority interrupt from ICC_AP0R_EL1, ICC_AP1R_EL1NS and
ICC_AP1R_EL1S. Isn't that the secure view (or Distributor value) of the priority?

>
>> This means that the ICC_RPR_EL1 read will return the secure view
>> (the value as it is stored by the GIC) of the priority, so for
>> pseudo-nmis it will return (GICD_INT_NMI_PRI >> 1) | 0x80, which
>> definitely != GICD_INT_NMI_PRI.
> That's not my reading of the pseudocode.
>
>> This is further confirmed by this statement on page 4-67:
>>
>> "When GICD_CTLR.DS == 0, [..] For Non-secure access to ICC_PMR_EL1
>> and ICC_RPR_EL1 when SCR_EL3.FIQ == 0: The Secure, unshifted view
>> applies."
>>
>> I don't know how I missed that during testing.
>>
>> Did a quick test on the model with PMU NMIs (GICD_CTRL.DS = 0,
>> SCTLR_EL2.FIQ = 0), gic_handle_nmi() was not being called at all,
> 0? Really????

Yes, I was also very surprised myself, I was certain that I tested my patch on the
model with PMU NMIs. Here's what I did to get this result, I would very much
appreciate anyone pointing out what I did wrong here.

$ git diff
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index e0f4debe64e1..749748df2466 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -623,6 +623,8 @@ static inline void gic_handle_nmi(u32 irqnr, struct pt_regs *regs)
        bool irqs_enabled = interrupts_enabled(regs);
        int err;
 
+       panic("Pseudo-NMI");
+
        if (irqs_enabled)
                nmi_enter();
 
@@ -1654,8 +1656,10 @@ static void gic_enable_nmi_support(void)
         * 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())
+       if (gic_has_group0() && !gic_dist_security_disabled()) {
+               pr_info("enabling gic_nonsecure_priorities");
                static_branch_enable(&gic_nonsecure_priorities);
+       }
 
        static_branch_enable(&supports_pseudo_nmis);
 
And a truncated log for FVP:

Boot-wrapper v0.2

[..]
[    0.000000] Kernel command line: earlycon=pl011,0x1c090000 console=ttyAMA0
kpti=off root=/dev/vda irqchip.gicv3_pseudo_nmi=1
[..]
[    0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
[    0.000000] GICv3: GIC: Using split EOI/Deactivate mode
[    0.000000] GICv3: 224 SPIs implemented
[    0.000000] GICv3: 0 Extended SPIs implemented
[    0.000000] GICv3: Distributor has no Range Selector support
[    0.000000] Root IRQ handler: gic_handle_irq
[    0.000000] GICv3: 16 PPIs implemented
[    0.000000] GICv3: CPU0: found redistributor 0 region 0:0x000000002f100000
[    0.000000] GICv3: Pseudo-NMIs enabled using relaxed ICC_PMR_EL1 synchronisation
[    0.000000] GICv3: enabling gic_nonsecure_priorities
[..]
[    0.037784] armv8-pmu pmu: hw perfevents: no interrupt-affinity property, guessing.
[    0.037901] hw perfevents: enabled with armv8_pmuv3 PMU driver, 9 counters
available, using NMIs
[..]
# cat /proc/interrupts
           CPU0       CPU1       CPU2       CPU3      
  9:          0          0          0          0     GICv3  25 Level     vgic
 11:          0          0          0          0     GICv3  30 Level     kvm guest
ptimer
 12:          0          0          0          0     GICv3  27 Level     kvm guest
vtimer
 13:         83         77         69         88     GICv3  26 Level     arch_timer
 16:          0          0          0          0     GICv3  92 Level     arm-pmu
 17:          0          0          0          0     GICv3  93 Level     arm-pmu
 18:          0          0          0          0     GICv3  94 Level     arm-pmu
 19:          0          0          0          0     GICv3  95 Level     arm-pmu
 22:          0          0          0          0     GICv3  47 Level     eth0
 24:         33          0          0          0     GICv3  37 Level     uart-pl011
 29:          0          0          0          0     GICv3  36 Level     rtc-pl031
 31:        138          0          0          0     GICv3  74 Level     virtio0
IPI0:       251        334        252        172       Rescheduling interrupts
IPI1:        26         23         10         20       Function call interrupts
IPI2:         0          0          0          0       CPU stop interrupts
IPI3:         0          0          0          0       CPU stop (for crash dump)
interrupts
IPI4:         0          0          0          0       Timer broadcast interrupts
IPI5:         0          0          0          0       IRQ work interrupts
IPI6:         0          0          0          0       CPU wake-up interrupts
Err:          0
# perf record -a -- sleep 5

^C[ perf record: Woken up 1 times to write data ]
[    6.908820] random: perf: uninitialized urandom read (6 bytes read)

[    7.596139] random: perf: uninitialized urandom read (6 bytes read)
[ perf record: Captured and wrote 0.013 MB perf.data (24 samples) ]

#
# cat /proc/interrupts
           CPU0       CPU1       CPU2       CPU3      
  9:          0          0          0          0     GICv3  25 Level     vgic
 11:          0          0          0          0     GICv3  30 Level     kvm guest
ptimer
 12:          0          0          0          0     GICv3  27 Level     kvm guest
vtimer
 13:        546        115         82        132     GICv3  26 Level     arch_timer
 16:          6          0          0          0     GICv3  92 Level     arm-pmu
 17:          0         10          0          0     GICv3  93 Level     arm-pmu
 18:          0          0          4          0     GICv3  94 Level     arm-pmu
 19:          0          0          0          4     GICv3  95 Level     arm-pmu
 22:          0          0          0          0     GICv3  47 Level     eth0
 24:         80          0          0          0     GICv3  37 Level     uart-pl011
 29:          0          0          0          0     GICv3  36 Level     rtc-pl031
 31:        195          0          0          0     GICv3  74 Level     virtio0
IPI0:       259        367        263        224       Rescheduling interrupts
IPI1:        27         51         16         24       Function call interrupts
IPI2:         0          0          0          0       CPU stop interrupts
IPI3:         0          0          0          0       CPU stop (for crash dump)
interrupts
IPI4:         0          0          0          0       Timer broadcast interrupts
IPI5:         0          0          0          0       IRQ work interrupts
IPI6:         0          0          0          0       CPU wake-up interrupts
Err:          0
#

With this change to the driver:

$ git diff
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index e0f4debe64e1..e58e62ab64fe 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -623,6 +623,8 @@ static inline void gic_handle_nmi(u32 irqnr, struct pt_regs *regs)
        bool irqs_enabled = interrupts_enabled(regs);
        int err;
 
+       panic("Pseudo-NMI");
+
        if (irqs_enabled)
                nmi_enter();
 
@@ -687,7 +689,7 @@ static asmlinkage void __exception_irq_entry
gic_handle_irq(struct pt_regs *regs
                return;
 
        if (gic_supports_nmi() &&
-           unlikely(gic_read_rpr() == GICD_INT_NMI_PRI)) {
+           unlikely(gic_read_rpr() == ((GICD_INT_NMI_PRI >> 1) | 0x80))) {
                gic_handle_nmi(irqnr, regs);
                return;
        }
@@ -1654,8 +1656,10 @@ static void gic_enable_nmi_support(void)
         * 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())
+       if (gic_has_group0() && !gic_dist_security_disabled()) {
+               pr_info("enabling gic_nonsecure_priorities");
                static_branch_enable(&gic_nonsecure_priorities);
+       }
 
        static_branch_enable(&supports_pseudo_nmis);
 

This is what I get from FVP (also truncated):

Boot-wrapper v0.2

[..]
[    0.000000] Kernel command line: earlycon=pl011,0x1c090000 console=ttyAMA0
kpti=off root=/dev/vda irqchip.gicv3_pseudo_nmi=1
[..]
[    0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
[    0.000000] GICv3: GIC: Using split EOI/Deactivate mode
[    0.000000] GICv3: 224 SPIs implemented
[    0.000000] GICv3: 0 Extended SPIs implemented
[    0.000000] GICv3: Distributor has no Range Selector support
[    0.000000] Root IRQ handler: gic_handle_irq
[    0.000000] GICv3: 16 PPIs implemented
[    0.000000] GICv3: CPU0: found redistributor 0 region 0:0x000000002f100000
[    0.000000] GICv3: Pseudo-NMIs enabled using relaxed ICC_PMR_EL1 synchronisation
[    0.000000] GICv3: enabling gic_nonsecure_priorities
[..]
[    0.037749] armv8-pmu pmu: hw perfevents: no interrupt-affinity property, guessing.
[    0.037860] hw perfevents: enabled with armv8_pmuv3 PMU driver, 9 counters
available, using NMIs
[..]
# perf record -a -- sleep 10
[    3.556669] Kernel panic - not syncing: Pseudo-NMI
[    3.556676] CPU: 0 PID: 145 Comm: perf Not tainted 5.14.0-rc5-dirty #23
[    3.556690] Hardware name: FVP Base (DT)
[    3.556692] Call trace:
[    3.556700]  dump_backtrace+0x0/0x19c
[    3.556710]  show_stack+0x1c/0x70
[    3.556725]  dump_stack_lvl+0x68/0x84
[    3.556729]  dump_stack+0x1c/0x38
[    3.556741]  panic+0x16c/0x340
[    3.556750]  gic_handle_irq+0x178/0x1a4
[    3.556760]  call_on_irq_stack+0x2c/0x58
[    3.556776]  do_interrupt_handler+0x54/0x60
[    3.556791]  el1_interrupt+0x30/0xa0
[    3.556800]  el1h_64_irq_handler+0x1c/0x2c
[    3.556809]  el1h_64_irq+0x78/0x7c
[    3.556819]  do_vfs_ioctl+0x90/0xd30
[    3.556830]  __arm64_sys_ioctl+0x88/0xf0
[    3.556842]  invoke_syscall+0x48/0x114
[    3.556849]  el0_svc_common+0x48/0x17c
[    3.556859]  do_el0_svc+0x2c/0x94
[    3.556873]  el0_svc+0x2c/0x5c
[    3.556879]  el0t_64_sync_handler+0xa8/0x130
[    3.556889]  el0t_64_sync+0x198/0x19c
[    3.556907] SMP: stopping secondary CPUs
[    3.556919] Kernel Offset: 0x57c5cb480000 from 0xffff800010000000
[    3.556922] PHYS_OFFSET: 0xffffebb580000000
[    3.556930] CPU features: 0x000000c1,ff3d4eef
[    3.556940] Memory Limit: none
[    3.556949] ---[ end Kernel panic - not syncing: Pseudo-NMI ]---

>
>> but when I changed the comparison to gic_read_rpr() ==
>> ((GICD_INT_NMI_PRI >> 1) | 0x80), NMIs were being correctly handled
>> again.
> You have completely lost me. This contradicts what you have written
> above.

I don't see how that is the case - ICC_RPR_EL1 contains the priority value as seen
by the Distributor, and non-secure priorities get right-shifted when
SCR_EL3.FIQ=0, meaning that GICD_INT_NMI_PRI becomes (GICD_INT_NMI_PRI >> 1) |
0x80 in the Distributor. Can you elaborate where I'm contradicting myself?

Thanks,

Alex


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqchip/gic-v3: Fix priority comparison when non-secure priorities are used

[Marc Zyngier]

Hi Alex,

On Thu, 12 Aug 2021 15:24:03 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi Marc,
> 
> On 8/12/21 2:09 PM, Marc Zyngier wrote:
> > On Thu, 12 Aug 2021 12:51:34 +0100,
> > Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> >> Hi,
> >>
> >> After re-familiarizing myself with the spec, it starting to look to
> >> me like indeed there's something not quite right (read as: totally
> >> broken) with my patch.
> >>
> >> Arm IHI 0069F, the pseudocode for reading ICC_RPR_EL1 (page 11-797),
> >> says that the priority returned is unchanged if SCTLR_EL3.FIQ ==
> >> 0.
> > Sure, but look at what ICC_RPR_EL1 does for FIQ==1:
> >
> > <quote>
> > if HaveEL(EL3) && !IsSecure() && SCR_EL3.FIQ == '1' then
> >     // A Non-secure GIC access and Group 0 inaccessible to Non-secure.
> >         if pPriority<7> == '0' then
> > 	    // Priority is in Secure half and not visible to Non-secure
> > 	    Priority = Zeros();
> >         elsif !IsOnes(pPriority) then
> > 	    // Non-secure access and not idle, so physical priority must be shifted
> >             pPriority<7:0> = (pPriority AND PRIMask())<6:0>:'0';
> >
> > return ZeroExtend(pPriority);
> > </quote>
> >
> > See how the the priority is shifted *left* (bits [6:0] end up in
> > [7:1])?
> 
> Yes, when SCR_EL3.FIQ=1, but gic_nonsecure_priorities is enabled
> when SCR_EL3.FIQ=0 (gic_has_group0()). In that case ICC_RPR_EL1
> returns (what I assume to be) the highest priority interrupt from
> ICC_AP0R_EL1, ICC_AP1R_EL1NS and ICC_AP1R_EL1S. Isn't that the
> secure view (or Distributor value) of the priority?

Yup. I guess I got confused with what "non-secure" priorities mean in
this context.

[...]

> I don't see how that is the case - ICC_RPR_EL1 contains the priority
> value as seen by the Distributor, and non-secure priorities get
> right-shifted when SCR_EL3.FIQ=0, meaning that GICD_INT_NMI_PRI
> becomes (GICD_INT_NMI_PRI >> 1) | 0x80 in the Distributor. Can you
> elaborate where I'm contradicting myself?

I think I know why I confused myself. When FIQ==0, G0 is NS. On the
face of it, this should mean that no shift occurs. However, G1S is
still in the picture, and we get the extra shift to preserve the
ordering with G1S.

This is a different configuration from that of a guest, where G0 is
also NS, but there is no shift at all, as there is no G1S.

The GIC strikes back. Again.

I run some more tests with this patch, and merge it of nothing breaks.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqchip/gic-v3: Fix priority comparison when non-secure priorities are used

[Chen-Yu Tsai]

Hi,

On Thu, Aug 12, 2021 at 2:31 AM Marc Zyngier <maz@kernel.org> wrote:
>
> + Alex, who introduced this.
>
> On Wed, 11 Aug 2021 18:15:05 +0100,
> Chen-Yu Tsai <wenst@chromium.org> wrote:
> >
> > When non-secure priorities are used, compared to the raw priority set,
> > the value read back from RPR is also right-shifted by one and the
> > highest bit set.
> >
> > Add a macro to do the modifications to the raw priority when doing the
> > comparison against the RPR value. This corrects the pseudo-NMI behavior
> > when non-secure priorities in the GIC are used. Tested on 5.10 with
> > the "IPI as pseudo-NMI" series [1] applied on MT8195.
> >
> > [1] https://lore.kernel.org/linux-arm-kernel/1604317487-14543-1-git-send-email-sumit.garg@linaro.org/
> >
> > Fixes: 336780590990 ("irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0")
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> >  drivers/irqchip/irq-gic-v3.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index e0f4debe64e1..e7a0b55413db 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -100,6 +100,15 @@ EXPORT_SYMBOL(gic_pmr_sync);
> >  DEFINE_STATIC_KEY_FALSE(gic_nonsecure_priorities);
> >  EXPORT_SYMBOL(gic_nonsecure_priorities);
> >
> > +#define GICD_INT_RPR_PRI(priority)                                   \
> > +     ({                                                              \
> > +             u32 __priority = (priority);                            \
> > +             if (static_branch_unlikely(&gic_nonsecure_priorities))  \
> > +                     __priority = 0x80 | (__priority >> 1);          \
> > +                                                                     \
> > +             __priority;                                             \
>
> This doesn't reflect what the pseudocode says of a read of ICC_RPR_EL1
> AFAICS. When the priority is activated, it is indeed shifted. But a
> read of RPR does appear to shift things back (and you loose the lowest
> bit in the process). Please see 'aarch64/support/ICC_RPR_EL1' in the
> architecture spec.
>
> Can you confirm that SCR_EL3.FIQ is set on your system?

I gave this a test with mainline on an ROC-RK3399-PC. I figure this is
more available and stable than the MT8195 [1].

My board is running:

- ATF v2.4-561-g465af20ce (based on git commit 8078b5c5a) with console and
  some erratas enabled, but otherwise standard rk3399 config
  "-dirty" was due to the SCR_EL3 line.

NOTICE:  BL31: v2.4(debug):v2.4-561-g465af20ce-dirty
NOTICE:  BL31: Built : 22:59:42, Aug 16 2021
INFO:    GICv3 with legacy support detected.
INFO:    ARM GICv3 driver initialized in EL3
INFO:    Maximum SPI INTID supported: 287
INFO:    plat_rockchip_pmu_init(1628): pd status 3e
INFO:    BL31: Initializing runtime services
INFO:    BL31: cortex_a53: CPU workaround for 855873 was applied
INFO:    BL31: cortex_a53: CPU workaround for 1530924 was applied
INFO:    BL31: Preparing for EL3 exit to normal world
INFO:    BL31: SCR_EL3=0x238, FIQ not set
INFO:    Entry point address = 0x200000
INFO:    SPSR = 0x3c9

- kernel next-20210813 + "NMI as IPI" series + debug printks scattered
  in the GICv3 driver

GICv3: GIC: Using split EOI/Deactivate mode
GICv3: 256 SPIs implemented
GICv3: 0 Extended SPIs implemented
GICv3: Distributor has no Range Selector support
Root IRQ handler: gic_handle_irq
GICv3: 16 PPIs implemented
GICv3: CPU0: found redistributor 0 region 0:0x00000000fef00000
GICv3: Pseudo-NMIs enabled using relaxed ICC_PMR_EL1 synchronisation
GICv3: Priority bits (gic_get_pribits()): 5
GICv3: Group0 available (gic_has_group0())
GICv3: Distributor security enabled (gic_dist_security_disabled()
GICv3: non-secure priorities used (gic_nonsecure_priorities)
GICv3: gic_irq_nmi_setup start: irq 7
GICv3: interrupt 7 priority a0
GICv3: interrupt 7 priority 20

So the RK3399 actually works correctly _without_ my fix. The NMI backtrace
triggers going through `gic_handle_nmi()`. If my fix is applied, it goes
through `gic_handle_irq()` instead.


However the MT8195 needs this to work correctly.

 === ATF ===
NOTICE:  MT8195 bl31_setup
NOTICE:  BL31: v2.5(debug):
NOTICE:  BL31: Built : Wed Jul  7 11:17:50 UTC 2021
INFO:    GICv3 without legacy support detected.
INFO:    ARM GICv3 driver initialized in EL3
INFO:    Maximum SPI INTID supported: 895
NOTICE:  MT8195 spm_boot_init
INFO:    BL31: Initializing runtime services
INFO:    BL31: cortex_a55: CPU workaround for 1530923 was applied
INFO:    SPM: enable CPC mode
INFO:    mcdi ready for mcusys-off-idle and system suspend
INFO:    BL31: Preparing for EL3 exit to normal world
INFO:    Entry point address = 0x80000000
INFO:    SPSR = 0x8

 === kernel GICv3 ===
GICv3: GIC: Using split EOI/Deactivate mode
GICv3: 864 SPIs implemented
GICv3: 0 Extended SPIs implemented
GICv3: Distributor has no Range Selector support
GICv3: 16 PPIs implemented
GICv3: CPU0: found redistributor 0 region 0:0x000000000c040000
GICv3: Pseudo-NMIs enabled using relaxed ICC_PMR_EL1 synchronisation
GICv3: Priority bits: 5
GICv3: Group0 available
GICv3: Distributor security enabled
GICv3: non-secure priorities used
GICv3: gic_irq_nmi_setup start: irq 7
GICv3: interrupt 7 priority a0
GICv3: interrupt 7 priority 20

I will dig through our ATF code base and try to see what's different.


ChenYu


[1] On a side note, the latest Chrome OS kernel 5.10 for the MT8195 gives
    a spinlock recursion during boot and hangs if pseudo-NMIs are enabled.
    I had to dig through my reflog to find the early version I developed
    on.


> Thanks,
>
>         M.
>
> > +     })
> > +
> >  /* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */
> >  static refcount_t *ppi_nmi_refs;
> >
> > @@ -687,7 +696,7 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
> >               return;
> >
> >       if (gic_supports_nmi() &&
> > -         unlikely(gic_read_rpr() == GICD_INT_NMI_PRI)) {
> > +         unlikely(gic_read_rpr() == GICD_INT_RPR_PRI(GICD_INT_NMI_PRI))) {
> >               gic_handle_nmi(irqnr, regs);
> >               return;
> >       }
>
>
> --
> Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqchip/gic-v3: Fix priority comparison when non-secure priorities are used

[Alexandru Elisei]

Hello,

Pending Marc's testing (I realized I don't have any hardware to test this on at
the moment), this patch looks correct to me. One comment below.

On 8/11/21 6:15 PM, Chen-Yu Tsai wrote:
> When non-secure priorities are used, compared to the raw priority set,
> the value read back from RPR is also right-shifted by one and the
> highest bit set.
>
> Add a macro to do the modifications to the raw priority when doing the
> comparison against the RPR value. This corrects the pseudo-NMI behavior
> when non-secure priorities in the GIC are used. Tested on 5.10 with
> the "IPI as pseudo-NMI" series [1] applied on MT8195.
>
> [1] https://lore.kernel.org/linux-arm-kernel/1604317487-14543-1-git-send-email-sumit.garg@linaro.org/
>
> Fixes: 336780590990 ("irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>  drivers/irqchip/irq-gic-v3.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index e0f4debe64e1..e7a0b55413db 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -100,6 +100,15 @@ EXPORT_SYMBOL(gic_pmr_sync);
>  DEFINE_STATIC_KEY_FALSE(gic_nonsecure_priorities);
>  EXPORT_SYMBOL(gic_nonsecure_priorities);
>  
> +#define GICD_INT_RPR_PRI(priority)					\
> +	({								\
> +		u32 __priority = (priority);				\
> +		if (static_branch_unlikely(&gic_nonsecure_priorities))	\
> +			__priority = 0x80 | (__priority >> 1);		\
> +									\
> +		__priority;						\
> +	})

Would you mind adding a comment to the macro explaining why it's needed? This
behaviour is rather subtle and I'm hoping it will save someone's time at some
point in the future. I'm thinking something like this (please ignore it if you can
think of something better):

When the Non-secure world has access to group 0 interrupts (SCR_EL3.FIQ = 0),
reading the ICC_RPR_EL1 register will return the Distributor's view of the
interrupt priority. When GIC security is enabled (GICD_CTLR.DS = 0), the interrupt
priority written by software is moved to the Non-secure range by the Distributor.
If both are true (which is the situation where gic_nonsecure_priorities gets
enabled), then we need to shift down the priority programmed by software if we
want match it against the value returned from ICC_RPR_EL1.

With a comment added:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,

Alex

> +
>  /* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */
>  static refcount_t *ppi_nmi_refs;
>  
> @@ -687,7 +696,7 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
>  		return;
>  
>  	if (gic_supports_nmi() &&
> -	    unlikely(gic_read_rpr() == GICD_INT_NMI_PRI)) {
> +	    unlikely(gic_read_rpr() == GICD_INT_RPR_PRI(GICD_INT_NMI_PRI))) {
>  		gic_handle_nmi(irqnr, regs);
>  		return;
>  	}

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqchip/gic-v3: Fix priority comparison when non-secure priorities are used

[Marc Zyngier]

On Fri, 20 Aug 2021 14:31:39 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hello,
> 
> Pending Marc's testing (I realized I don't have any hardware to test
> this on at the moment), this patch looks correct to me. One comment
> below.

Seems good so far. I tested it both in a VM, on a FIQ==1 host, and on
D05, which runs with FIQ==0. Can't be more broken than it was... ;-)

> 
> On 8/11/21 6:15 PM, Chen-Yu Tsai wrote:
> > When non-secure priorities are used, compared to the raw priority set,
> > the value read back from RPR is also right-shifted by one and the
> > highest bit set.
> >
> > Add a macro to do the modifications to the raw priority when doing the
> > comparison against the RPR value. This corrects the pseudo-NMI behavior
> > when non-secure priorities in the GIC are used. Tested on 5.10 with
> > the "IPI as pseudo-NMI" series [1] applied on MT8195.
> >
> > [1] https://lore.kernel.org/linux-arm-kernel/1604317487-14543-1-git-send-email-sumit.garg@linaro.org/
> >
> > Fixes: 336780590990 ("irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0")
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> >  drivers/irqchip/irq-gic-v3.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index e0f4debe64e1..e7a0b55413db 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -100,6 +100,15 @@ EXPORT_SYMBOL(gic_pmr_sync);
> >  DEFINE_STATIC_KEY_FALSE(gic_nonsecure_priorities);
> >  EXPORT_SYMBOL(gic_nonsecure_priorities);
> >  
> > +#define GICD_INT_RPR_PRI(priority)					\
> > +	({								\
> > +		u32 __priority = (priority);				\
> > +		if (static_branch_unlikely(&gic_nonsecure_priorities))	\
> > +			__priority = 0x80 | (__priority >> 1);		\
> > +									\
> > +		__priority;						\
> > +	})
> 
> Would you mind adding a comment to the macro explaining why it's
> needed? This behaviour is rather subtle and I'm hoping it will save
> someone's time at some point in the future. I'm thinking something
> like this (please ignore it if you can think of something better):
> 
> When the Non-secure world has access to group 0 interrupts
> (SCR_EL3.FIQ = 0), reading the ICC_RPR_EL1 register will return the
> Distributor's view of the interrupt priority. When GIC security is
> enabled (GICD_CTLR.DS = 0), the interrupt priority written by
> software is moved to the Non-secure range by the Distributor.  If
> both are true (which is the situation where gic_nonsecure_priorities
> gets enabled), then we need to shift down the priority programmed by
> software if we want match it against the value returned from
> ICC_RPR_EL1.
> 
> With a comment added:
> 
> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Let me fold this into the commit and push it out again.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel