Currently PMSNEVFR_EL1 system register read, and write access EL2 traps are
disabled, via setting the same bit (i.e 62) in HDFGRTR_EL2, and HDFGWTR_EL2
respectively. Although very similar, bit fields are not exact same in these
two EL2 trap configure registers particularly when it comes to read-only or
write-only accesses such as ready-only 'HDFGRTR_EL2.nBRBIDR' which needs to
be set while enabling BRBE on NVHE platforms. Using the exact same bit mask
fields for both these trap register risk writing into their RESERVED areas,
which is undesirable.
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Oliver Upton <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
This applies on v6.6-rc6.
I guess it should be okay to use 'x2' as it is in the clobbered register
list for init_el2_state() function. But please do let me know otherwise.
arch/arm64/include/asm/el2_setup.h | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
index 899b5c10f84c..c534afb1a30d 100644
--- a/arch/arm64/include/asm/el2_setup.h
+++ b/arch/arm64/include/asm/el2_setup.h
@@ -206,16 +206,19 @@
cbz x1, .Lskip_fgt_\@
mov x0, xzr
+ mov x2, xzr
mrs x1, id_aa64dfr0_el1
ubfx x1, x1, #ID_AA64DFR0_EL1_PMSVer_SHIFT, #4
cmp x1, #3
b.lt .Lset_debug_fgt_\@
+
/* Disable PMSNEVFR_EL1 read and write traps */
- orr x0, x0, #(1 << 62)
+ orr x0, x0, #HDFGRTR_EL2_nPMSNEVFR_EL1_MASK
+ orr x2, x2, #HDFGWTR_EL2_nPMSNEVFR_EL1_MASK
.Lset_debug_fgt_\@:
msr_s SYS_HDFGRTR_EL2, x0
- msr_s SYS_HDFGWTR_EL2, x0
+ msr_s SYS_HDFGWTR_EL2, x2
mov x0, xzr
mrs x1, id_aa64pfr1_el1
--
2.25.1
On Wed, 18 Oct 2023 04:00:07 +0100,
Anshuman Khandual <[email protected]> wrote:
>
> Currently PMSNEVFR_EL1 system register read, and write access EL2 traps are
> disabled, via setting the same bit (i.e 62) in HDFGRTR_EL2, and HDFGWTR_EL2
> respectively. Although very similar, bit fields are not exact same in these
> two EL2 trap configure registers particularly when it comes to read-only or
> write-only accesses such as ready-only 'HDFGRTR_EL2.nBRBIDR' which needs to
> be set while enabling BRBE on NVHE platforms. Using the exact same bit mask
> fields for both these trap register risk writing into their RESERVED areas,
> which is undesirable.
Sorry, I don't understand at all what you are describing. You seem to
imply that the read and write effects of the FGT doesn't apply the
same way. But my reading of the ARM ARM is that behave completely
symmetrically.
Also, what is nBRBIDR doing here? It is still set to 0. What
'RESERVED' state are you talking about?
>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Oliver Upton <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> This applies on v6.6-rc6.
>
> I guess it should be okay to use 'x2' as it is in the clobbered register
> list for init_el2_state() function. But please do let me know otherwise.
>
> arch/arm64/include/asm/el2_setup.h | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
> index 899b5c10f84c..c534afb1a30d 100644
> --- a/arch/arm64/include/asm/el2_setup.h
> +++ b/arch/arm64/include/asm/el2_setup.h
> @@ -206,16 +206,19 @@
> cbz x1, .Lskip_fgt_\@
>
> mov x0, xzr
> + mov x2, xzr
> mrs x1, id_aa64dfr0_el1
> ubfx x1, x1, #ID_AA64DFR0_EL1_PMSVer_SHIFT, #4
> cmp x1, #3
> b.lt .Lset_debug_fgt_\@
> +
> /* Disable PMSNEVFR_EL1 read and write traps */
> - orr x0, x0, #(1 << 62)
> + orr x0, x0, #HDFGRTR_EL2_nPMSNEVFR_EL1_MASK
> + orr x2, x2, #HDFGWTR_EL2_nPMSNEVFR_EL1_MASK
>
> .Lset_debug_fgt_\@:
> msr_s SYS_HDFGRTR_EL2, x0
> - msr_s SYS_HDFGWTR_EL2, x0
> + msr_s SYS_HDFGWTR_EL2, x2
So what has changed here, aside from clobbering an extra register? The
masks are the same, the initial values are the same... Is it in
preparation for some other work?
/me puzzled.
M.
--
Without deviation from the norm, progress is not possible.
On Wed, Oct 18, 2023 at 01:40:37PM +0100, Marc Zyngier wrote:
> On Wed, 18 Oct 2023 04:00:07 +0100,
> Anshuman Khandual <[email protected]> wrote:
> >
> > Currently PMSNEVFR_EL1 system register read, and write access EL2 traps are
> > disabled, via setting the same bit (i.e 62) in HDFGRTR_EL2, and HDFGWTR_EL2
> > respectively. Although very similar, bit fields are not exact same in these
> > two EL2 trap configure registers particularly when it comes to read-only or
> > write-only accesses such as ready-only 'HDFGRTR_EL2.nBRBIDR' which needs to
> > be set while enabling BRBE on NVHE platforms. Using the exact same bit mask
> > fields for both these trap register risk writing into their RESERVED areas,
> > which is undesirable.
>
> Sorry, I don't understand at all what you are describing. You seem to
> imply that the read and write effects of the FGT doesn't apply the
> same way. But my reading of the ARM ARM is that behave completely
> symmetrically.
nBRBIDR is an asymmetric bit (bit 59 of HDFGWTR_EL2 is RES0). While the
architecture *could* repurpose this WTR bit for something else, that
feels rather implementation and software hostile. I don't think there's
a practical issue here, especially since the architecture has already
allocated another pair of debug trap registers to make room for more
bits.
> So what has changed here, aside from clobbering an extra register? The
> masks are the same, the initial values are the same... Is it in
> preparation for some other work?
Yeah, it feels as though this patch is taken out of context. Without a
justifying functional change I don't see the value in fiddling with this
code.
--
Thanks,
Oliver
On 10/18/23 18:10, Marc Zyngier wrote:
> On Wed, 18 Oct 2023 04:00:07 +0100,
> Anshuman Khandual <[email protected]> wrote:
>>
>> Currently PMSNEVFR_EL1 system register read, and write access EL2 traps are
>> disabled, via setting the same bit (i.e 62) in HDFGRTR_EL2, and HDFGWTR_EL2
>> respectively. Although very similar, bit fields are not exact same in these
>> two EL2 trap configure registers particularly when it comes to read-only or
>> write-only accesses such as ready-only 'HDFGRTR_EL2.nBRBIDR' which needs to
>> be set while enabling BRBE on NVHE platforms. Using the exact same bit mask
>> fields for both these trap register risk writing into their RESERVED areas,
>> which is undesirable.
>
> Sorry, I don't understand at all what you are describing. You seem to
> imply that the read and write effects of the FGT doesn't apply the
> same way. But my reading of the ARM ARM is that behave completely
> symmetrically.
>
> Also, what is nBRBIDR doing here? It is still set to 0. What
> 'RESERVED' state are you talking about?
Let's observe the following example which includes the nBRBIDR problem,
mentioned earlier.
Read access trap configure
HDFGRTR_EL2[59] - nBRBIDR
HDFGRTR_EL2[58] - PMCEIDn_EL0
Write access trap configure
HDFGWTR_EL2[59:58] - RES0
Because BRBIDR_EL1 and PMCEID<N>_EL0 are read only registers they don't
have corresponding entries in HDFGWTR_EL2 for write trap configuration.
Using the exact same value contained in 'x0' both for HDFGRTR_EL2, and
HDFGWTR_EL2 will be problematic in case it contains bit fields that are
available only in one of the registers but not in the other.
If 'x0' contains nBRBIDR being set, it will be okay for HDFGRTR_EL2 but
might not be okay for HDFGWTR_EL2 where it will get into RESERVED areas.
>
>>
>> Cc: Catalin Marinas <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> Cc: Oliver Upton <[email protected]>
>> Cc: Marc Zyngier <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Anshuman Khandual <[email protected]>
>> ---
>> This applies on v6.6-rc6.
>>
>> I guess it should be okay to use 'x2' as it is in the clobbered register
>> list for init_el2_state() function. But please do let me know otherwise.
>>
>> arch/arm64/include/asm/el2_setup.h | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
>> index 899b5c10f84c..c534afb1a30d 100644
>> --- a/arch/arm64/include/asm/el2_setup.h
>> +++ b/arch/arm64/include/asm/el2_setup.h
>> @@ -206,16 +206,19 @@
>> cbz x1, .Lskip_fgt_\@
>>
>> mov x0, xzr
>> + mov x2, xzr
>> mrs x1, id_aa64dfr0_el1
>> ubfx x1, x1, #ID_AA64DFR0_EL1_PMSVer_SHIFT, #4
>> cmp x1, #3
>> b.lt .Lset_debug_fgt_\@
>> +
>> /* Disable PMSNEVFR_EL1 read and write traps */
>> - orr x0, x0, #(1 << 62)
>> + orr x0, x0, #HDFGRTR_EL2_nPMSNEVFR_EL1_MASK
>> + orr x2, x2, #HDFGWTR_EL2_nPMSNEVFR_EL1_MASK
>>
>> .Lset_debug_fgt_\@:
>> msr_s SYS_HDFGRTR_EL2, x0
>> - msr_s SYS_HDFGWTR_EL2, x0
>> + msr_s SYS_HDFGWTR_EL2, x2
>
> So what has changed here, aside from clobbering an extra register? The
> masks are the same, the initial values are the same... Is it in
> preparation for some other work?
Right, this is in preparation for the BRBE register and instructions trap
configuration. Planning to add the following change for BRBE enablement.
diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
index c534afb1a30d..128177465a9c 100644
--- a/arch/arm64/include/asm/el2_setup.h
+++ b/arch/arm64/include/asm/el2_setup.h
@@ -217,6 +217,32 @@
orr x2, x2, #HDFGWTR_EL2_nPMSNEVFR_EL1_MASK
.Lset_debug_fgt_\@:
+#ifdef CONFIG_ARM64_BRBE
+ mrs x1, id_aa64dfr0_el1
+ ubfx x1, x1, #ID_AA64DFR0_EL1_BRBE_SHIFT, #4
+ cbz x1, .Lskip_brbe_reg_fgt_\@
+
+ /*
+ * Disable BRBINF_EL1, BRBINFINJ_EL1, BRBSRC_EL1, BRBSRCINJ_EL1,
+ * BRBTGT_EL1, BRBTGTINJ_EL1, and BRBTS_EL1 read traps.
+ */
+ orr x0, x0, #HDFGRTR_EL2_nBRBDATA_MASK
+
+ /*
+ * Disable BRBINFINJ_EL1, BRBSRCINJ_EL1, BRBTGTINJ_EL1, and
+ * BRBTS_EL1 write traps.
+ */
+ orr x2, x2, #HDFGWTR_EL2_nBRBDATA_MASK
+
+ /* Disable BRBCR_EL1, BRBFCR_EL1 read and write traps */
+ orr x0, x0, #HDFGRTR_EL2_nBRBCTL_MASK
+ orr x2, x2, #HDFGWTR_EL2_nBRBCTL_MASK
+
+ /* Disable BRBIDR_EL1 read traps */
+ orr x0, x0, #HDFGRTR_EL2_nBRBIDR_MASK
+
+.Lskip_brbe_reg_fgt_\@:
+#endif
msr_s SYS_HDFGRTR_EL2, x0
msr_s SYS_HDFGWTR_EL2, x2
@@ -241,7 +267,22 @@
.Lset_fgt_\@:
msr_s SYS_HFGRTR_EL2, x0
msr_s SYS_HFGWTR_EL2, x0
- msr_s SYS_HFGITR_EL2, xzr
+
+ mov x0, xzr
+#ifdef CONFIG_ARM64_BRBE
+ mrs x1, id_aa64dfr0_el1
+ ubfx x1, x1, #ID_AA64DFR0_EL1_BRBE_SHIFT, #4
+ cbz x1, .Lskip_brbe_insn_fgt_\@
+
+ /* Disable trapping for BRBIALL instruction */
+ orr x0, x0, #HFGITR_EL2_nBRBIALL_MASK
+
+ /* Disable trapping for BRBINJ instruction */
+ orr x0, x0, #HFGITR_EL2_nBRBINJ_MASK
+
+.Lskip_brbe_insn_fgt_\@:
+#endif
+ msr_s SYS_HFGITR_EL2, x0
mrs x1, id_aa64pfr0_el1 // AMU traps UNDEF without AMU
ubfx x1, x1, #ID_AA64PFR0_EL1_AMU_SHIFT, #4
After the above change, HDFGRTR_EL2_nBRBIDR_MASK only gets set
inside HFGRTR_EL2 but not inside HFGWTR_EL2.
>
> /me puzzled.
I should have given more details in the commit message but hope
you have some context now, but please do let me know if there
is something still missing.
On Thu, 19 Oct 2023 04:36:15 +0100,
Anshuman Khandual <[email protected]> wrote:
>
>
>
> On 10/18/23 18:10, Marc Zyngier wrote:
> > On Wed, 18 Oct 2023 04:00:07 +0100,
> > Anshuman Khandual <[email protected]> wrote:
> >>
> >> Currently PMSNEVFR_EL1 system register read, and write access EL2 traps are
> >> disabled, via setting the same bit (i.e 62) in HDFGRTR_EL2, and HDFGWTR_EL2
> >> respectively. Although very similar, bit fields are not exact same in these
> >> two EL2 trap configure registers particularly when it comes to read-only or
> >> write-only accesses such as ready-only 'HDFGRTR_EL2.nBRBIDR' which needs to
> >> be set while enabling BRBE on NVHE platforms. Using the exact same bit mask
> >> fields for both these trap register risk writing into their RESERVED areas,
> >> which is undesirable.
> >
> > Sorry, I don't understand at all what you are describing. You seem to
> > imply that the read and write effects of the FGT doesn't apply the
> > same way. But my reading of the ARM ARM is that behave completely
> > symmetrically.
> >
> > Also, what is nBRBIDR doing here? It is still set to 0. What
> > 'RESERVED' state are you talking about?
>
> Let's observe the following example which includes the nBRBIDR problem,
> mentioned earlier.
>
> Read access trap configure
>
> HDFGRTR_EL2[59] - nBRBIDR
> HDFGRTR_EL2[58] - PMCEIDn_EL0
>
> Write access trap configure
>
> HDFGWTR_EL2[59:58] - RES0
>
> Because BRBIDR_EL1 and PMCEID<N>_EL0 are read only registers they don't
> have corresponding entries in HDFGWTR_EL2 for write trap configuration.
>
> Using the exact same value contained in 'x0' both for HDFGRTR_EL2, and
> HDFGWTR_EL2 will be problematic in case it contains bit fields that are
> available only in one of the registers but not in the other.
>
> If 'x0' contains nBRBIDR being set, it will be okay for HDFGRTR_EL2 but
> might not be okay for HDFGWTR_EL2 where it will get into RESERVED areas.
None of which matters for this patch. You keep arguing about something
that does not exist in the change you're proposing.
[...]
> I should have given more details in the commit message but hope
> you have some context now, but please do let me know if there
> is something still missing.
What is missing is a useful patch. This one just obfuscates things for
no particular purpose. If you have a useful change to contribute,
please send that instead (your BRBE change). We don't need an extra,
standalone and pointless patch such as this one.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On 10/19/23 12:45, Marc Zyngier wrote:
> On Thu, 19 Oct 2023 04:36:15 +0100,
> Anshuman Khandual <[email protected]> wrote:
>>
>>
>>
>> On 10/18/23 18:10, Marc Zyngier wrote:
>>> On Wed, 18 Oct 2023 04:00:07 +0100,
>>> Anshuman Khandual <[email protected]> wrote:
>>>>
>>>> Currently PMSNEVFR_EL1 system register read, and write access EL2 traps are
>>>> disabled, via setting the same bit (i.e 62) in HDFGRTR_EL2, and HDFGWTR_EL2
>>>> respectively. Although very similar, bit fields are not exact same in these
>>>> two EL2 trap configure registers particularly when it comes to read-only or
>>>> write-only accesses such as ready-only 'HDFGRTR_EL2.nBRBIDR' which needs to
>>>> be set while enabling BRBE on NVHE platforms. Using the exact same bit mask
>>>> fields for both these trap register risk writing into their RESERVED areas,
>>>> which is undesirable.
>>>
>>> Sorry, I don't understand at all what you are describing. You seem to
>>> imply that the read and write effects of the FGT doesn't apply the
>>> same way. But my reading of the ARM ARM is that behave completely
>>> symmetrically.
>>>
>>> Also, what is nBRBIDR doing here? It is still set to 0. What
>>> 'RESERVED' state are you talking about?
>>
>> Let's observe the following example which includes the nBRBIDR problem,
>> mentioned earlier.
>>
>> Read access trap configure
>>
>> HDFGRTR_EL2[59] - nBRBIDR
>> HDFGRTR_EL2[58] - PMCEIDn_EL0
>>
>> Write access trap configure
>>
>> HDFGWTR_EL2[59:58] - RES0
>>
>> Because BRBIDR_EL1 and PMCEID<N>_EL0 are read only registers they don't
>> have corresponding entries in HDFGWTR_EL2 for write trap configuration.
>>
>> Using the exact same value contained in 'x0' both for HDFGRTR_EL2, and
>> HDFGWTR_EL2 will be problematic in case it contains bit fields that are
>> available only in one of the registers but not in the other.
>>
>> If 'x0' contains nBRBIDR being set, it will be okay for HDFGRTR_EL2 but
>> might not be okay for HDFGWTR_EL2 where it will get into RESERVED areas.
>
> None of which matters for this patch. You keep arguing about something
> that does not exist in the change you're proposing.
>
> [...]
>
>> I should have given more details in the commit message but hope
>> you have some context now, but please do let me know if there
>> is something still missing.
>
> What is missing is a useful patch. This one just obfuscates things for
> no particular purpose. If you have a useful change to contribute,
> please send that instead (your BRBE change). We don't need an extra,
> standalone and pointless patch such as this one.
I will fold this patch with other BRBE changes as mentioned earlier but
thought that - separating out updates for HDFGRTR_EL2, and HDFGWTR_EL2
should be done as stand alone change in a preparatory patch. Seems like
that was an incorrect assumption.