| Message ID | 20231018030007.1968317-1-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Independently update HDFGRTR_EL2 and HDFGWTR_EL2 | expand |
On Wed, 18 Oct 2023 04:00:07 +0100, Anshuman Khandual <anshuman.khandual@arm.com> 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 <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Oliver Upton <oliver.upton@linux.dev> > Cc: Marc Zyngier <maz@kernel.org> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > 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.
On Wed, Oct 18, 2023 at 01:40:37PM +0100, Marc Zyngier wrote: > On Wed, 18 Oct 2023 04:00:07 +0100, > Anshuman Khandual <anshuman.khandual@arm.com> 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.
On 10/18/23 18:10, Marc Zyngier wrote: > On Wed, 18 Oct 2023 04:00:07 +0100, > Anshuman Khandual <anshuman.khandual@arm.com> 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 <catalin.marinas@arm.com> >> Cc: Will Deacon <will@kernel.org> >> Cc: Oliver Upton <oliver.upton@linux.dev> >> Cc: Marc Zyngier <maz@kernel.org> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> 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 <anshuman.khandual@arm.com> wrote: > > > > On 10/18/23 18:10, Marc Zyngier wrote: > > On Wed, 18 Oct 2023 04:00:07 +0100, > > Anshuman Khandual <anshuman.khandual@arm.com> 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.
On 10/19/23 12:45, Marc Zyngier wrote: > On Thu, 19 Oct 2023 04:36:15 +0100, > Anshuman Khandual <anshuman.khandual@arm.com> wrote: >> >> >> >> On 10/18/23 18:10, Marc Zyngier wrote: >>> On Wed, 18 Oct 2023 04:00:07 +0100, >>> Anshuman Khandual <anshuman.khandual@arm.com> 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.
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
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 <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Oliver Upton <oliver.upton@linux.dev> Cc: Marc Zyngier <maz@kernel.org> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- 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(-)