[PATCH] arm64: Do not trap PMSNEVFR_EL1

[PATCH] arm64: Do not trap PMSNEVFR_EL1

[Alexandru Elisei]

Commit 31c00d2aeaa2 ("arm64: Disable fine grained traps on boot") zeroed
the fine grained trap registers to prevent unwanted register traps from
occuring. However, for the PMSNEVFR_EL1 register, the corresponding
HDFGRTR_EL2.nPMSNEVFR_EL1 field must be 1 to disable trapping. Set the
field to 1 if FEAT_SPEv1p2 is detected.

Fixes: 31c00d2aeaa2 ("arm64: Disable fine grained traps on boot")
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
Based on v5.14-rc7. Also, we could write 1 << 62 to HDFGRTR_EL2 unconditionally
since the field is RAZ/WI if !FEAT_SPEv1p2. I don't have a strong preference for
either approaches, but I chose this implementation because it's clearer (even
though it's more verbose and it's one extra trap on NV).

Tested on the model, using boot-wrapper built from commit 5cd6238ec4ef
("aarch32: fix .globl replacement"). Without this patch, in NVHE mode, the model
freezes when I try to read PMSNEVFR_EL1. With this patch, the model doesn't hang
when I read the register, but it hangs when I write to it. I've gone throught
the pseudocode for reading and writing to PMSNEVFR_EL1 and from what I can tell
nothing should be trapping the accesses. On top of that, this is what I tried on
the model with this patch applied:

1. VHE mode, I can read and write to PMSNEVFR_EL1 without any issues, so the
hang is not caused by an incorrect EL3 configuration.

2. NVHE mode, I can read and write just fine to *PMSEVFR_EL1*, so the hang is
not caused by an EL2 trap that affects the rest of the profiling control
registers. I have tried printing the HDFGRTR_EL2 value in this situation using
semihosting, the value is what it is programmed by __init_el2_fgt (that is,
1 << 62).

At this point, I am inclined to think it's a model bug because reading works,
but writing causes a hang and that looks very suspicious to me. I'm going to
open a model bug internally and see what comes of it.

 arch/arm64/include/asm/el2_setup.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
index b83fb24954b7..8a9adb2039fd 100644
--- a/arch/arm64/include/asm/el2_setup.h
+++ b/arch/arm64/include/asm/el2_setup.h
@@ -149,7 +149,16 @@
 	ubfx	x1, x1, #ID_AA64MMFR0_FGT_SHIFT, #4
 	cbz	x1, .Lskip_fgt_\@
 
-	msr_s	SYS_HDFGRTR_EL2, xzr
+	mov	x0, xzr
+	mrs	x1, id_aa64dfr0_el1
+	ubfx	x1, x1, #ID_AA64DFR0_PMSVER_SHIFT, #4
+	cmp	x1, #3
+	b.lt	.Lset_fgt_\@
+	/* Set HDFGRTR_EL2.nPMSNEVFR_EL1 to disable the register trap */
+	orr	x0, x0, #(1 << 62)
+
+.Lset_fgt_\@:
+	msr_s	SYS_HDFGRTR_EL2, x0
 	msr_s	SYS_HDFGWTR_EL2, xzr
 	msr_s	SYS_HFGRTR_EL2, xzr
 	msr_s	SYS_HFGWTR_EL2, xzr
-- 
2.33.0


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

Re: [PATCH] arm64: Do not trap PMSNEVFR_EL1

[Alexandru Elisei]

Errr... somehow I forgot to add the arm64 maintainers. Fixing that.

On 8/24/21 2:24 PM, Alexandru Elisei wrote:
> Commit 31c00d2aeaa2 ("arm64: Disable fine grained traps on boot") zeroed
> the fine grained trap registers to prevent unwanted register traps from
> occuring. However, for the PMSNEVFR_EL1 register, the corresponding
> HDFGRTR_EL2.nPMSNEVFR_EL1 field must be 1 to disable trapping. Set the
> field to 1 if FEAT_SPEv1p2 is detected.
>
> Fixes: 31c00d2aeaa2 ("arm64: Disable fine grained traps on boot")
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> Based on v5.14-rc7. Also, we could write 1 << 62 to HDFGRTR_EL2 unconditionally
> since the field is RAZ/WI if !FEAT_SPEv1p2. I don't have a strong preference for
> either approaches, but I chose this implementation because it's clearer (even
> though it's more verbose and it's one extra trap on NV).
>
> Tested on the model, using boot-wrapper built from commit 5cd6238ec4ef
> ("aarch32: fix .globl replacement"). Without this patch, in NVHE mode, the model
> freezes when I try to read PMSNEVFR_EL1. With this patch, the model doesn't hang
> when I read the register, but it hangs when I write to it. I've gone throught
> the pseudocode for reading and writing to PMSNEVFR_EL1 and from what I can tell
> nothing should be trapping the accesses. On top of that, this is what I tried on
> the model with this patch applied:
>
> 1. VHE mode, I can read and write to PMSNEVFR_EL1 without any issues, so the
> hang is not caused by an incorrect EL3 configuration.
>
> 2. NVHE mode, I can read and write just fine to *PMSEVFR_EL1*, so the hang is
> not caused by an EL2 trap that affects the rest of the profiling control
> registers. I have tried printing the HDFGRTR_EL2 value in this situation using
> semihosting, the value is what it is programmed by __init_el2_fgt (that is,
> 1 << 62).
>
> At this point, I am inclined to think it's a model bug because reading works,
> but writing causes a hang and that looks very suspicious to me. I'm going to
> open a model bug internally and see what comes of it.
>
>  arch/arm64/include/asm/el2_setup.h | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
> index b83fb24954b7..8a9adb2039fd 100644
> --- a/arch/arm64/include/asm/el2_setup.h
> +++ b/arch/arm64/include/asm/el2_setup.h
> @@ -149,7 +149,16 @@
>  	ubfx	x1, x1, #ID_AA64MMFR0_FGT_SHIFT, #4
>  	cbz	x1, .Lskip_fgt_\@
>  
> -	msr_s	SYS_HDFGRTR_EL2, xzr
> +	mov	x0, xzr
> +	mrs	x1, id_aa64dfr0_el1
> +	ubfx	x1, x1, #ID_AA64DFR0_PMSVER_SHIFT, #4
> +	cmp	x1, #3
> +	b.lt	.Lset_fgt_\@
> +	/* Set HDFGRTR_EL2.nPMSNEVFR_EL1 to disable the register trap */
> +	orr	x0, x0, #(1 << 62)
> +
> +.Lset_fgt_\@:
> +	msr_s	SYS_HDFGRTR_EL2, x0
>  	msr_s	SYS_HDFGWTR_EL2, xzr
>  	msr_s	SYS_HFGRTR_EL2, xzr
>  	msr_s	SYS_HFGWTR_EL2, xzr

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

Re: [PATCH] arm64: Do not trap PMSNEVFR_EL1

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1467 bytes --]

On Tue, Aug 24, 2021 at 02:24:59PM +0100, Alexandru Elisei wrote:

> Commit 31c00d2aeaa2 ("arm64: Disable fine grained traps on boot") zeroed
> the fine grained trap registers to prevent unwanted register traps from
> occuring. However, for the PMSNEVFR_EL1 register, the corresponding
> HDFGRTR_EL2.nPMSNEVFR_EL1 field must be 1 to disable trapping. Set the
> field to 1 if FEAT_SPEv1p2 is detected.

Oh, that's a shame :/  I wonder why this feature is different to the
others, I just had a quick check and didn't see any other issues but...

> Based on v5.14-rc7. Also, we could write 1 << 62 to HDFGRTR_EL2 unconditionally
> since the field is RAZ/WI if !FEAT_SPEv1p2. I don't have a strong preference for
> either approaches, but I chose this implementation because it's clearer (even
> though it's more verbose and it's one extra trap on NV).

Yes, the explicit feature check is both clearer and more conservative -
it's unlikely to have a practical impact but 

> At this point, I am inclined to think it's a model bug because reading works,
> but writing causes a hang and that looks very suspicious to me. I'm going to
> open a model bug internally and see what comes of it.

...are you sure this isn't that the same issue also exists with the
equivalent field HDFGWTR_EL2 - glancing at the XML it appears to have
the same issue?  One of the fine grained aspects of fine grained traps
is that there's separate read and write traps!

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH] arm64: Do not trap PMSNEVFR_EL1

[Alexandru Elisei]

Hi Mark,

On 8/24/21 4:10 PM, Mark Brown wrote:
> On Tue, Aug 24, 2021 at 02:24:59PM +0100, Alexandru Elisei wrote:
>
>> Commit 31c00d2aeaa2 ("arm64: Disable fine grained traps on boot") zeroed
>> the fine grained trap registers to prevent unwanted register traps from
>> occuring. However, for the PMSNEVFR_EL1 register, the corresponding
>> HDFGRTR_EL2.nPMSNEVFR_EL1 field must be 1 to disable trapping. Set the
>> field to 1 if FEAT_SPEv1p2 is detected.
> Oh, that's a shame :/  I wonder why this feature is different to the
> others, I just had a quick check and didn't see any other issues but...
>
>> Based on v5.14-rc7. Also, we could write 1 << 62 to HDFGRTR_EL2 unconditionally
>> since the field is RAZ/WI if !FEAT_SPEv1p2. I don't have a strong preference for
>> either approaches, but I chose this implementation because it's clearer (even
>> though it's more verbose and it's one extra trap on NV).
> Yes, the explicit feature check is both clearer and more conservative -
> it's unlikely to have a practical impact but 
>
>> At this point, I am inclined to think it's a model bug because reading works,
>> but writing causes a hang and that looks very suspicious to me. I'm going to
>> open a model bug internally and see what comes of it.
> ...are you sure this isn't that the same issue also exists with the
> equivalent field HDFGWTR_EL2 - glancing at the XML it appears to have
> the same issue?  One of the fine grained aspects of fine grained traps
> is that there's separate read and write traps!

Yes, you are right, when I read the MSR PMSNEVFR_EL1, <Xt> pseudocode I missed the
fact that for writes the trap is controlled by HDFG*W*TR_EL1, not by
HDFG*R*TR_EL1. I set  the nPMSNEVFR_EL1 bit in HDFGWRTR_EL1, and Linux doesn't
hang anymore when I try to write to the register. Will send v2 shortly.

Thanks,

Alex


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