| Message ID | 20241216040831.2448257-7-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/hw_breakpoint: Enable FEAT_Debugv8p9 | expand |
On Mon, Dec 16, 2024 at 09:38:30AM +0530, Anshuman Khandual wrote: > Fine grained trap control for MDSELR_EL1 register needs to be configured in > HDFGRTR2_EL2, and HDFGWTR2_EL2 registers when kernel enters at EL1, but EL2 > is also present. Shouldn't this be "when kernel enters at EL2, and EL3 is also present"? Though it is really "If EL3 set FGTEn2 and the kernel is entered in EL2, then FGT2 must be initialized for MDSELR_EL1." If it was me, I'd just plagarize what was written for prior FGT commits for this code. :) > This adds a new helper __init_el2_fgt2() initializing this > new FEAT_FGT2 based fine grained registers. "This adds" is the same as saying "This patch/commit adds" which is well documented to avoid. Use imperative "Add a new helper...". Though it is clear from the diff that is what you are doing... > MDCR_EL2.EBWE needs to be enabled for additional (beyond 16) breakpoint and > watchpoint exceptions when kernel enters at EL1, but EL2 is also present. > This updates __init_el2_debug() as required for FEAT_Debugv8p9. > > While here, also update booting.rst with MDCR_EL3 and SCR_EL3 requirements. > > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Jonathan Corbet <corbet@lwn.net> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Oliver Upton <oliver.upton@linux.dev> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-doc@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: kvmarm@lists.linux.dev > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > Changes in V3: > > - Dropped MDCR_EL3.TDA boot requirement from documentation (separate series) > - Dropped MDCR_EL2_EBWE definition as MDCR_EL2 is now defined in tools sysreg > > https://lore.kernel.org/all/20241211065425.1106683-1-anshuman.khandual@arm.com/ > > Documentation/arch/arm64/booting.rst | 18 ++++++++++++++++++ > arch/arm64/include/asm/el2_setup.h | 26 ++++++++++++++++++++++++++ > 2 files changed, 44 insertions(+) > > diff --git a/Documentation/arch/arm64/booting.rst b/Documentation/arch/arm64/booting.rst > index 3278fb4bf219..054cfe1cad18 100644 > --- a/Documentation/arch/arm64/booting.rst > +++ b/Documentation/arch/arm64/booting.rst > @@ -288,6 +288,12 @@ Before jumping into the kernel, the following conditions must be met: > > - SCR_EL3.FGTEn (bit 27) must be initialised to 0b1. > > + For CPUs with the Fine Grained Traps (FEAT_FGT2) extension present: > + > + - If EL3 is present and the kernel is entered at EL2: > + > + - SCR_EL3.FGTEn2 (bit 59) must be initialised to 0b1. > + > For CPUs with support for HCRX_EL2 (FEAT_HCX) present: > > - If EL3 is present and the kernel is entered at EL2: > @@ -322,6 +328,18 @@ Before jumping into the kernel, the following conditions must be met: > - ZCR_EL2.LEN must be initialised to the same value for all CPUs the > kernel will execute on. > > + For CPUs with FEAT_Debugv8p9 extension present: > + > + - If the kernel is entered at EL1 and EL2 is present: > + > + - HDFGRTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1 > + - HDFGWTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1 > + - MDCR_EL2.EBWE (bit 43) must be initialized to 0b1 > + > + - If EL3 is present: > + > + - MDCR_EL3.EBWE (bit 43) must be initialized to 0b1 > + > For CPUs with the Scalable Matrix Extension (FEAT_SME): > > - If EL3 is present: > diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h > index 4ef52d7245bb..2fbfe27d38b5 100644 > --- a/arch/arm64/include/asm/el2_setup.h > +++ b/arch/arm64/include/asm/el2_setup.h > @@ -105,6 +105,13 @@ > // to own it. > > .Lskip_trace_\@: > + mrs x1, id_aa64dfr0_el1 > + ubfx x1, x1, #ID_AA64DFR0_EL1_DebugVer_SHIFT, #4 > + cmp x1, #ID_AA64DFR0_EL1_DebugVer_V8P9 > + b.lt .Lskip_dbg_v8p9_\@ > + > + orr x2, x2, #MDCR_EL2_EBWE > +.Lskip_dbg_v8p9_\@: > msr mdcr_el2, x2 // Configure debug traps > .endm > > @@ -244,6 +251,24 @@ > .Lskip_gcs_\@: > .endm > > +.macro __init_el2_fgt2 > + mrs x1, id_aa64mmfr0_el1 > + ubfx x1, x1, #ID_AA64MMFR0_EL1_FGT_SHIFT, #4 > + cmp x1, #ID_AA64MMFR0_EL1_FGT_FGT2 We already read this field in __init_el2_fgt, shouldn't we leverage that and move all this there rather than read the feature reg twice. > + b.lt .Lskip_fgt2_\@ > + > + mrs x1, id_aa64dfr0_el1 > + ubfx x1, x1, #ID_AA64DFR0_EL1_DebugVer_SHIFT, #4 > + cmp x1, #ID_AA64DFR0_EL1_DebugVer_V8P9 > + b.lt .Lskip_dbg_v8p9_\@ > + > + mov_q x0, HDFGWTR2_EL2_nMDSELR_EL1 > + msr_s SYS_HDFGWTR2_EL2, x0 > + msr_s SYS_HDFGRTR2_EL2, x0 If Debug v8.9 is not present, but FGT2 is, shouldn't we write 0 to these registers? That is what we do for FGT. I just realized I forgot to add FGT2 setup for the PMUv3.9 features I already added in 6.12 and 6.13. So this really needs to land sooner rather than later to add that. Rob
On 12/17/24 05:12, Rob Herring wrote: > On Mon, Dec 16, 2024 at 09:38:30AM +0530, Anshuman Khandual wrote: >> Fine grained trap control for MDSELR_EL1 register needs to be configured in >> HDFGRTR2_EL2, and HDFGWTR2_EL2 registers when kernel enters at EL1, but EL2 >> is also present. > > Shouldn't this be "when kernel enters at EL2, and EL3 is also present"? AFAICT - HDFGRTR2_EL2 and HDFGWTR2_EL2 registers configure traps into EL2 when accessed from EL1/EL0, provided all required EL3 trap controls are in place as well. These EL2 based trap configs need to be set before the kernel finally enters EL1. Although there is an assumption about EL3 based trap configs being in place, do we need to mention that in commit message as well. Is not updating booting.rst takes care of all EL3 requirements. > Though it is really "If EL3 set FGTEn2 and the kernel is entered in > EL2, then FGT2 must be initialized for MDSELR_EL1." > > If it was me, I'd just plagarize what was written for prior FGT > commits for this code. :) There are many commits that changed __init_el2_fgt() with different description formats. Do you have particular one in mind which can be followed here ? :) > >> This adds a new helper __init_el2_fgt2() initializing this >> new FEAT_FGT2 based fine grained registers. > > "This adds" is the same as saying "This patch/commit adds" which is well > documented to avoid. Use imperative "Add a new helper...". Though it is > clear from the diff that is what you are doing... Sure, will fix it. > > >> MDCR_EL2.EBWE needs to be enabled for additional (beyond 16) breakpoint and >> watchpoint exceptions when kernel enters at EL1, but EL2 is also present. >> This updates __init_el2_debug() as required for FEAT_Debugv8p9. >> >> While here, also update booting.rst with MDCR_EL3 and SCR_EL3 requirements. >> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will@kernel.org> >> Cc: Jonathan Corbet <corbet@lwn.net> >> Cc: Marc Zyngier <maz@kernel.org> >> Cc: Oliver Upton <oliver.upton@linux.dev> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-doc@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> Cc: kvmarm@lists.linux.dev >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> Changes in V3: >> >> - Dropped MDCR_EL3.TDA boot requirement from documentation (separate series) >> - Dropped MDCR_EL2_EBWE definition as MDCR_EL2 is now defined in tools sysreg >> >> https://lore.kernel.org/all/20241211065425.1106683-1-anshuman.khandual@arm.com/ >> >> Documentation/arch/arm64/booting.rst | 18 ++++++++++++++++++ >> arch/arm64/include/asm/el2_setup.h | 26 ++++++++++++++++++++++++++ >> 2 files changed, 44 insertions(+) >> >> diff --git a/Documentation/arch/arm64/booting.rst b/Documentation/arch/arm64/booting.rst >> index 3278fb4bf219..054cfe1cad18 100644 >> --- a/Documentation/arch/arm64/booting.rst >> +++ b/Documentation/arch/arm64/booting.rst >> @@ -288,6 +288,12 @@ Before jumping into the kernel, the following conditions must be met: >> >> - SCR_EL3.FGTEn (bit 27) must be initialised to 0b1. >> >> + For CPUs with the Fine Grained Traps (FEAT_FGT2) extension present: >> + >> + - If EL3 is present and the kernel is entered at EL2: >> + >> + - SCR_EL3.FGTEn2 (bit 59) must be initialised to 0b1. >> + >> For CPUs with support for HCRX_EL2 (FEAT_HCX) present: >> >> - If EL3 is present and the kernel is entered at EL2: >> @@ -322,6 +328,18 @@ Before jumping into the kernel, the following conditions must be met: >> - ZCR_EL2.LEN must be initialised to the same value for all CPUs the >> kernel will execute on. >> >> + For CPUs with FEAT_Debugv8p9 extension present: >> + >> + - If the kernel is entered at EL1 and EL2 is present: >> + >> + - HDFGRTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1 >> + - HDFGWTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1 >> + - MDCR_EL2.EBWE (bit 43) must be initialized to 0b1 >> + >> + - If EL3 is present: >> + >> + - MDCR_EL3.EBWE (bit 43) must be initialized to 0b1 >> + >> For CPUs with the Scalable Matrix Extension (FEAT_SME): >> >> - If EL3 is present: >> diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h >> index 4ef52d7245bb..2fbfe27d38b5 100644 >> --- a/arch/arm64/include/asm/el2_setup.h >> +++ b/arch/arm64/include/asm/el2_setup.h >> @@ -105,6 +105,13 @@ >> // to own it. >> >> .Lskip_trace_\@: >> + mrs x1, id_aa64dfr0_el1 >> + ubfx x1, x1, #ID_AA64DFR0_EL1_DebugVer_SHIFT, #4 >> + cmp x1, #ID_AA64DFR0_EL1_DebugVer_V8P9 >> + b.lt .Lskip_dbg_v8p9_\@ >> + >> + orr x2, x2, #MDCR_EL2_EBWE >> +.Lskip_dbg_v8p9_\@: >> msr mdcr_el2, x2 // Configure debug traps >> .endm >> >> @@ -244,6 +251,24 @@ >> .Lskip_gcs_\@: >> .endm >> >> +.macro __init_el2_fgt2 >> + mrs x1, id_aa64mmfr0_el1 >> + ubfx x1, x1, #ID_AA64MMFR0_EL1_FGT_SHIFT, #4 >> + cmp x1, #ID_AA64MMFR0_EL1_FGT_FGT2 > > We already read this field in __init_el2_fgt, shouldn't we leverage that > and move all this there rather than read the feature reg twice. Should not __init_el2_fgt2() remain separate to contain all future FEAT_FGT2 related trap enabled/disable checks ? OTOH reading id_aa64mmfr0_el1 register should improve some performance as well. > >> + b.lt .Lskip_fgt2_\@ >> + >> + mrs x1, id_aa64dfr0_el1 >> + ubfx x1, x1, #ID_AA64DFR0_EL1_DebugVer_SHIFT, #4 >> + cmp x1, #ID_AA64DFR0_EL1_DebugVer_V8P9 >> + b.lt .Lskip_dbg_v8p9_\@ >> + >> + mov_q x0, HDFGWTR2_EL2_nMDSELR_EL1 >> + msr_s SYS_HDFGWTR2_EL2, x0 >> + msr_s SYS_HDFGRTR2_EL2, x0 > > If Debug v8.9 is not present, but FGT2 is, shouldn't we write 0 to these > registers? That is what we do for FGT. Just to summerize what's being done in __init_el2_fgt() .macro __init_el2_fgt ............... mov x0, xzr ............... .Lskip_spe_fgt_\@: msr_s SYS_HDFGRTR_EL2, x0 msr_s SYS_HDFGWTR_EL2, x0 ............... .Lset_fgt_\@: msr_s SYS_HFGRTR_EL2, x0 msr_s SYS_HFGWTR_EL2, x0 msr_s SYS_HFGITR_EL2, xzr ............... If the relevant features are not present and their traps need not be handled, all FEAT_FGT based trap control registers (SYS_HDFGRTR_EL2, SYS_HDFGWTR_EL2, SYS_HFGRTR_EL2, SYS_HFGWTR_EL2 and SYS_HFGITR_EL2) get cleared. Hence same needs to be done for all FEAT_FGT2 register as well. Fair enough, something like the following makes sense ? .macro __init_el2_fgt2 mrs x1, id_aa64mmfr0_el1 ubfx x1, x1, #ID_AA64MMFR0_EL1_FGT_SHIFT, #4 cmp x1, #ID_AA64MMFR0_EL1_FGT_FGT2 b.lt .Lskip_fgt2_\@ mov x0, xzr mrs x1, id_aa64dfr0_el1 ubfx x1, x1, #ID_AA64DFR0_EL1_DebugVer_SHIFT, #4 cmp x1, #ID_AA64DFR0_EL1_DebugVer_V8P9 b.lt .Lskip_dbg_v8p9_\@ orr x0, x0, #HDFGWTR2_EL2_nMDSELR_EL1 .Lskip_dbg_v8p9_\@: msr_s SYS_HDFGWTR2_EL2, x0 msr_s SYS_HDFGRTR2_EL2, x0 msr_s SYS_HFGRTR2_EL2, xzr msr_s SYS_HFGWTR2_EL2, xzr msr_s SYS_HFGITR2_EL2, xzr .Lskip_fgt2_\@: .endm Just wondering - following the same principle if clearing these FEAT_FGT2 trap registers would make sense in itself, even without FEAT_Debugv8p9 enablement ? Something like the commit commit 31c00d2aeaa2da89361f5b64a64ca831433be5fc Author: Mark Brown <broonie@kernel.org> Date: Thu Apr 1 19:09:40 2021 +0100 arm64: Disable fine grained traps on boot The arm64 FEAT_FGT extension introduces a set of traps to EL2 for accesses to small sets of registers and instructions from EL1 and EL0. Currently Linux makes no use of this feature, ensure that it is not active at boot by disabling the traps during EL2 setup. > > > I just realized I forgot to add FGT2 setup for the PMUv3.9 features I > already added in 6.12 and 6.13. So this really needs to land sooner > rather than later to add that. Not sure if I got this correctly. Are you suggesting to carve out __init_el2_fgt2() from the series and post separately with PMUv3.9 requirements and fallback clearing for all FEAT_FGT2 trap config registers as mentioned above ?
On Tue, Dec 17, 2024 at 2:48 AM Anshuman Khandual <anshuman.khandual@arm.com> wrote: > > On 12/17/24 05:12, Rob Herring wrote: > > On Mon, Dec 16, 2024 at 09:38:30AM +0530, Anshuman Khandual wrote: > >> Fine grained trap control for MDSELR_EL1 register needs to be configured in > >> HDFGRTR2_EL2, and HDFGWTR2_EL2 registers when kernel enters at EL1, but EL2 > >> is also present. > > > > Shouldn't this be "when kernel enters at EL2, and EL3 is also present"? > > AFAICT - HDFGRTR2_EL2 and HDFGWTR2_EL2 registers configure traps into EL2 when > accessed from EL1/EL0, provided all required EL3 trap controls are in place as > well. These EL2 based trap configs need to be set before the kernel finally > enters EL1. Although there is an assumption about EL3 based trap configs being > in place, do we need to mention that in commit message as well. Is not updating > booting.rst takes care of all EL3 requirements. My point is just I read 'kernel enters at EL1' as meaning the kernel booted at EL1 and EL2 is not accessible. Maybe should reworded as 'before/if the kernel drops to EL1' > > Though it is really "If EL3 set FGTEn2 and the kernel is entered in > > EL2, then FGT2 must be initialized for MDSELR_EL1." > > > > If it was me, I'd just plagarize what was written for prior FGT > > commits for this code. :) > > There are many commits that changed __init_el2_fgt() with different description > formats. Do you have particular one in mind which can be followed here ? :) > > > > >> This adds a new helper __init_el2_fgt2() initializing this > >> new FEAT_FGT2 based fine grained registers. > > > > "This adds" is the same as saying "This patch/commit adds" which is well > > documented to avoid. Use imperative "Add a new helper...". Though it is > > clear from the diff that is what you are doing... > > Sure, will fix it. > > > > > > >> MDCR_EL2.EBWE needs to be enabled for additional (beyond 16) breakpoint and > >> watchpoint exceptions when kernel enters at EL1, but EL2 is also present. > >> This updates __init_el2_debug() as required for FEAT_Debugv8p9. > >> > >> While here, also update booting.rst with MDCR_EL3 and SCR_EL3 requirements. > >> > >> Cc: Catalin Marinas <catalin.marinas@arm.com> > >> Cc: Will Deacon <will@kernel.org> > >> Cc: Jonathan Corbet <corbet@lwn.net> > >> Cc: Marc Zyngier <maz@kernel.org> > >> Cc: Oliver Upton <oliver.upton@linux.dev> > >> Cc: linux-arm-kernel@lists.infradead.org > >> Cc: linux-doc@vger.kernel.org > >> Cc: linux-kernel@vger.kernel.org > >> Cc: kvmarm@lists.linux.dev > >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > >> --- > >> Changes in V3: > >> > >> - Dropped MDCR_EL3.TDA boot requirement from documentation (separate series) > >> - Dropped MDCR_EL2_EBWE definition as MDCR_EL2 is now defined in tools sysreg > >> > >> https://lore.kernel.org/all/20241211065425.1106683-1-anshuman.khandual@arm.com/ > >> > >> Documentation/arch/arm64/booting.rst | 18 ++++++++++++++++++ > >> arch/arm64/include/asm/el2_setup.h | 26 ++++++++++++++++++++++++++ > >> 2 files changed, 44 insertions(+) > >> > >> diff --git a/Documentation/arch/arm64/booting.rst b/Documentation/arch/arm64/booting.rst > >> index 3278fb4bf219..054cfe1cad18 100644 > >> --- a/Documentation/arch/arm64/booting.rst > >> +++ b/Documentation/arch/arm64/booting.rst > >> @@ -288,6 +288,12 @@ Before jumping into the kernel, the following conditions must be met: > >> > >> - SCR_EL3.FGTEn (bit 27) must be initialised to 0b1. > >> > >> + For CPUs with the Fine Grained Traps (FEAT_FGT2) extension present: > >> + > >> + - If EL3 is present and the kernel is entered at EL2: > >> + > >> + - SCR_EL3.FGTEn2 (bit 59) must be initialised to 0b1. > >> + > >> For CPUs with support for HCRX_EL2 (FEAT_HCX) present: > >> > >> - If EL3 is present and the kernel is entered at EL2: > >> @@ -322,6 +328,18 @@ Before jumping into the kernel, the following conditions must be met: > >> - ZCR_EL2.LEN must be initialised to the same value for all CPUs the > >> kernel will execute on. > >> > >> + For CPUs with FEAT_Debugv8p9 extension present: > >> + > >> + - If the kernel is entered at EL1 and EL2 is present: > >> + > >> + - HDFGRTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1 > >> + - HDFGWTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1 > >> + - MDCR_EL2.EBWE (bit 43) must be initialized to 0b1 > >> + > >> + - If EL3 is present: > >> + > >> + - MDCR_EL3.EBWE (bit 43) must be initialized to 0b1 > >> + > >> For CPUs with the Scalable Matrix Extension (FEAT_SME): > >> > >> - If EL3 is present: > >> diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h > >> index 4ef52d7245bb..2fbfe27d38b5 100644 > >> --- a/arch/arm64/include/asm/el2_setup.h > >> +++ b/arch/arm64/include/asm/el2_setup.h > >> @@ -105,6 +105,13 @@ > >> // to own it. > >> > >> .Lskip_trace_\@: > >> + mrs x1, id_aa64dfr0_el1 > >> + ubfx x1, x1, #ID_AA64DFR0_EL1_DebugVer_SHIFT, #4 > >> + cmp x1, #ID_AA64DFR0_EL1_DebugVer_V8P9 > >> + b.lt .Lskip_dbg_v8p9_\@ > >> + > >> + orr x2, x2, #MDCR_EL2_EBWE > >> +.Lskip_dbg_v8p9_\@: > >> msr mdcr_el2, x2 // Configure debug traps > >> .endm > >> > >> @@ -244,6 +251,24 @@ > >> .Lskip_gcs_\@: > >> .endm > >> > >> +.macro __init_el2_fgt2 > >> + mrs x1, id_aa64mmfr0_el1 > >> + ubfx x1, x1, #ID_AA64MMFR0_EL1_FGT_SHIFT, #4 > >> + cmp x1, #ID_AA64MMFR0_EL1_FGT_FGT2 > > > > We already read this field in __init_el2_fgt, shouldn't we leverage that > > and move all this there rather than read the feature reg twice. > > Should not __init_el2_fgt2() remain separate to contain all future FEAT_FGT2 > related trap enabled/disable checks ? OTOH reading id_aa64mmfr0_el1 register > should improve some performance as well. That's the tradeoff. I'll defer to others whether a single id register read here is preferred. > >> + b.lt .Lskip_fgt2_\@ > >> + > >> + mrs x1, id_aa64dfr0_el1 > >> + ubfx x1, x1, #ID_AA64DFR0_EL1_DebugVer_SHIFT, #4 > >> + cmp x1, #ID_AA64DFR0_EL1_DebugVer_V8P9 > >> + b.lt .Lskip_dbg_v8p9_\@ > >> + > >> + mov_q x0, HDFGWTR2_EL2_nMDSELR_EL1 > >> + msr_s SYS_HDFGWTR2_EL2, x0 > >> + msr_s SYS_HDFGRTR2_EL2, x0 > > > > If Debug v8.9 is not present, but FGT2 is, shouldn't we write 0 to these > > registers? That is what we do for FGT. > > Just to summerize what's being done in __init_el2_fgt() > > .macro __init_el2_fgt > ............... > mov x0, xzr > ............... > .Lskip_spe_fgt_\@: > msr_s SYS_HDFGRTR_EL2, x0 > msr_s SYS_HDFGWTR_EL2, x0 > ............... > .Lset_fgt_\@: > msr_s SYS_HFGRTR_EL2, x0 > msr_s SYS_HFGWTR_EL2, x0 > msr_s SYS_HFGITR_EL2, xzr > ............... > > If the relevant features are not present and their traps need not be handled, > all FEAT_FGT based trap control registers (SYS_HDFGRTR_EL2, SYS_HDFGWTR_EL2, > SYS_HFGRTR_EL2, SYS_HFGWTR_EL2 and SYS_HFGITR_EL2) get cleared. Hence same > needs to be done for all FEAT_FGT2 register as well. Fair enough, something > like the following makes sense ? > > .macro __init_el2_fgt2 > mrs x1, id_aa64mmfr0_el1 > ubfx x1, x1, #ID_AA64MMFR0_EL1_FGT_SHIFT, #4 > cmp x1, #ID_AA64MMFR0_EL1_FGT_FGT2 > b.lt .Lskip_fgt2_\@ > > mov x0, xzr > mrs x1, id_aa64dfr0_el1 > ubfx x1, x1, #ID_AA64DFR0_EL1_DebugVer_SHIFT, #4 > cmp x1, #ID_AA64DFR0_EL1_DebugVer_V8P9 > b.lt .Lskip_dbg_v8p9_\@ > > orr x0, x0, #HDFGWTR2_EL2_nMDSELR_EL1 > .Lskip_dbg_v8p9_\@: > msr_s SYS_HDFGWTR2_EL2, x0 > msr_s SYS_HDFGRTR2_EL2, x0 > msr_s SYS_HFGRTR2_EL2, xzr > msr_s SYS_HFGWTR2_EL2, xzr > msr_s SYS_HFGITR2_EL2, xzr > .Lskip_fgt2_\@: > .endm Looks good. > > Just wondering - following the same principle if clearing these FEAT_FGT2 trap > registers would make sense in itself, even without FEAT_Debugv8p9 enablement ? Probably and there's a Jira for it. Though there was some debate about whether anything was needed before any users. But that's kind of a mute point anyway because I need it for PMU now, too. > Something like the commit > > commit 31c00d2aeaa2da89361f5b64a64ca831433be5fc > Author: Mark Brown <broonie@kernel.org> > Date: Thu Apr 1 19:09:40 2021 +0100 > > arm64: Disable fine grained traps on boot > > The arm64 FEAT_FGT extension introduces a set of traps to EL2 for accesses > to small sets of registers and instructions from EL1 and EL0. Currently > Linux makes no use of this feature, ensure that it is not active at boot by > disabling the traps during EL2 setup. > > > > > > > > I just realized I forgot to add FGT2 setup for the PMUv3.9 features I > > already added in 6.12 and 6.13. So this really needs to land sooner > > rather than later to add that. > Not sure if I got this correctly. Are you suggesting to carve out __init_el2_fgt2() > from the series and post separately with PMUv3.9 requirements and fallback clearing > for all FEAT_FGT2 trap config registers as mentioned above ? Yes, as it needs to not be held up by any of the debug issues Mark raised. Also, it may need to be back ported to 6.12. And for that we'd want the PMU parts, but not the Debug. I still have to figure out what needs to be done on the KVM side. Rob
[....] On 12/17/24 23:23, Rob Herring wrote: >> Something like the commit >> >> commit 31c00d2aeaa2da89361f5b64a64ca831433be5fc >> Author: Mark Brown <broonie@kernel.org> >> Date: Thu Apr 1 19:09:40 2021 +0100 >> >> arm64: Disable fine grained traps on boot >> >> The arm64 FEAT_FGT extension introduces a set of traps to EL2 for accesses >> to small sets of registers and instructions from EL1 and EL0. Currently >> Linux makes no use of this feature, ensure that it is not active at boot by >> disabling the traps during EL2 setup. >> >> >>> >>> I just realized I forgot to add FGT2 setup for the PMUv3.9 features I >>> already added in 6.12 and 6.13. So this really needs to land sooner >>> rather than later to add that. >> Not sure if I got this correctly. Are you suggesting to carve out __init_el2_fgt2() >> from the series and post separately with PMUv3.9 requirements and fallback clearing >> for all FEAT_FGT2 trap config registers as mentioned above ? > Yes, as it needs to not be held up by any of the debug issues Mark > raised. Also, it may need to be back ported to 6.12. And for that we'd > want the PMU parts, but not the Debug. I still have to figure out what > needs to be done on the KVM side. Hi Rob, I did go through all the five FEAT_FGT2 trap control registers and it seems like the following are the controls available for FEAT_PMUv3p9 based registers. Although PMZR_EL0 does not get used in kernel right now but still might be a good idea to include anyway. Please let me know, if I might have missed something else related to FEAT_PMUv3p9. HDFGRTR2_EL2_nPMUACR_EL1 (mrs PMUACR_EL1) HDFGWTR2_EL2_nPMUACR_EL1 (msr PMUACR_EL1) HDFGWTR2_EL2_nPMZR_EL0 (msr PMZR_EL0) Following will be the change required for __init_el2_fgt2() along with all the tools sysreg updates required for the mentioned registers here. --- a/arch/arm64/include/asm/el2_setup.h +++ b/arch/arm64/include/asm/el2_setup.h @@ -233,6 +233,31 @@ .Lskip_fgt_\@: .endm +.macro __init_el2_fgt2 + mrs x1, id_aa64mmfr0_el1 + ubfx x1, x1, #ID_AA64MMFR0_EL1_FGT_SHIFT, #4 + cmp x1, #ID_AA64MMFR0_EL1_FGT_FGT2 + b.lt .Lskip_fgt2_\@ + + mov x0, xzr + mov x2, xzr + mrs x1, id_aa64dfr0_el1 + ubfx x1, x1, #ID_AA64DFR0_EL1_PMUVer_SHIFT, #4 + cmp x1, #ID_AA64DFR0_EL1_PMUVer_V3P9 + b.lt .Lskip_pmuv3p9_\@ + + orr x0, x0, #HDFGRTR2_EL2_nPMUACR_EL1 + orr x2, x2, #HDFGWTR2_EL2_nPMUACR_EL1 + orr x2, x2, #HDFGWTR2_EL2_nPMZR_EL0 +.Lskip_pmuv3p9_\@: + msr_s SYS_HDFGRTR2_EL2, x0 + msr_s SYS_HDFGWTR2_EL2, x2 + msr_s SYS_HFGRTR2_EL2, xzr + msr_s SYS_HFGWTR2_EL2, xzr + msr_s SYS_HFGITR2_EL2, xzr +.Lskip_fgt2_\@: +.endm + .macro __init_el2_gcs mrs_s x1, SYS_ID_AA64PFR1_EL1 ubfx x1, x1, #ID_AA64PFR1_EL1_GCS_SHIFT, #4 @@ -283,6 +308,7 @@ __init_el2_nvhe_idregs __init_el2_cptr __init_el2_fgt + __init_el2_fgt2 __init_el2_gcs .endm
On Tue, 17 Dec 2024 17:53:41 +0000, Rob Herring <robh@kernel.org> wrote: > > On Tue, Dec 17, 2024 at 2:48 AM Anshuman Khandual > <anshuman.khandual@arm.com> wrote: > > > > On 12/17/24 05:12, Rob Herring wrote: > > > On Mon, Dec 16, 2024 at 09:38:30AM +0530, Anshuman Khandual wrote: > > >> Fine grained trap control for MDSELR_EL1 register needs to be configured in > > >> HDFGRTR2_EL2, and HDFGWTR2_EL2 registers when kernel enters at EL1, but EL2 > > >> is also present. > > > > > > Shouldn't this be "when kernel enters at EL2, and EL3 is also present"? > > > > AFAICT - HDFGRTR2_EL2 and HDFGWTR2_EL2 registers configure traps into EL2 when > > accessed from EL1/EL0, provided all required EL3 trap controls are in place as > > well. These EL2 based trap configs need to be set before the kernel finally > > enters EL1. Although there is an assumption about EL3 based trap configs being > > in place, do we need to mention that in commit message as well. Is not updating > > booting.rst takes care of all EL3 requirements. > > My point is just I read 'kernel enters at EL1' as meaning the kernel > booted at EL1 and EL2 is not accessible. Maybe should reworded as > 'before/if the kernel drops to EL1' The EL2->EL1 downgrade is internal to the kernel. If we get it wrong, that's our fault, and there isn't anything to document. This document is about booting the kernel at any NS EL, so only the "external" requirements matter. But I don't think we should be prescriptive about the state of these registers, as long as the potential traps are correctly handled. For the above, I'd rather have something like: "The MDSELR_EL1 register must be freely accessible when the kernel is entered at EL1 and that higher ELs are present in the system." On its own, that should be enough. You can subsequently add: "This includes HDFGRTR2_EL2, HDFGWTR2_EL2 and MDCR_EL2 when EL2 is present, as well as MDCR_EL3 when EL3 is present". but that's paraphrasing the architecture, and is definitely incomplete (see the MDSELR_EL1 pseudocode for reference). > > > > Though it is really "If EL3 set FGTEn2 and the kernel is entered in > > > EL2, then FGT2 must be initialized for MDSELR_EL1." > > > > > > If it was me, I'd just plagarize what was written for prior FGT > > > commits for this code. :) > > > > There are many commits that changed __init_el2_fgt() with different description > > formats. Do you have particular one in mind which can be followed here ? :) > > > > > > > >> This adds a new helper __init_el2_fgt2() initializing this > > >> new FEAT_FGT2 based fine grained registers. > > > > > > "This adds" is the same as saying "This patch/commit adds" which is well > > > documented to avoid. Use imperative "Add a new helper...". Though it is > > > clear from the diff that is what you are doing... > > > > Sure, will fix it. > > > > > > > > > > >> MDCR_EL2.EBWE needs to be enabled for additional (beyond 16) breakpoint and > > >> watchpoint exceptions when kernel enters at EL1, but EL2 is also present. > > >> This updates __init_el2_debug() as required for FEAT_Debugv8p9. > > >> > > >> While here, also update booting.rst with MDCR_EL3 and SCR_EL3 requirements. > > >> > > >> Cc: Catalin Marinas <catalin.marinas@arm.com> > > >> Cc: Will Deacon <will@kernel.org> > > >> Cc: Jonathan Corbet <corbet@lwn.net> > > >> Cc: Marc Zyngier <maz@kernel.org> > > >> Cc: Oliver Upton <oliver.upton@linux.dev> > > >> Cc: linux-arm-kernel@lists.infradead.org > > >> Cc: linux-doc@vger.kernel.org > > >> Cc: linux-kernel@vger.kernel.org > > >> Cc: kvmarm@lists.linux.dev > > >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > > >> --- > > >> Changes in V3: > > >> > > >> - Dropped MDCR_EL3.TDA boot requirement from documentation (separate series) > > >> - Dropped MDCR_EL2_EBWE definition as MDCR_EL2 is now defined in tools sysreg > > >> > > >> https://lore.kernel.org/all/20241211065425.1106683-1-anshuman.khandual@arm.com/ > > >> > > >> Documentation/arch/arm64/booting.rst | 18 ++++++++++++++++++ > > >> arch/arm64/include/asm/el2_setup.h | 26 ++++++++++++++++++++++++++ > > >> 2 files changed, 44 insertions(+) > > >> > > >> diff --git a/Documentation/arch/arm64/booting.rst b/Documentation/arch/arm64/booting.rst > > >> index 3278fb4bf219..054cfe1cad18 100644 > > >> --- a/Documentation/arch/arm64/booting.rst > > >> +++ b/Documentation/arch/arm64/booting.rst > > >> @@ -288,6 +288,12 @@ Before jumping into the kernel, the following conditions must be met: > > >> > > >> - SCR_EL3.FGTEn (bit 27) must be initialised to 0b1. > > >> > > >> + For CPUs with the Fine Grained Traps (FEAT_FGT2) extension present: > > >> + > > >> + - If EL3 is present and the kernel is entered at EL2: > > >> + > > >> + - SCR_EL3.FGTEn2 (bit 59) must be initialised to 0b1. > > >> + > > >> For CPUs with support for HCRX_EL2 (FEAT_HCX) present: > > >> > > >> - If EL3 is present and the kernel is entered at EL2: > > >> @@ -322,6 +328,18 @@ Before jumping into the kernel, the following conditions must be met: > > >> - ZCR_EL2.LEN must be initialised to the same value for all CPUs the > > >> kernel will execute on. > > >> > > >> + For CPUs with FEAT_Debugv8p9 extension present: > > >> + > > >> + - If the kernel is entered at EL1 and EL2 is present: > > >> + > > >> + - HDFGRTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1 > > >> + - HDFGWTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1 But then you don't say anything about SCR_EL3.FGTEn2. And what if the hypervisor wants to trap things in order to emulate on lazy switch things? The more I look at those, the more I think these requirements are not saying what we want to express, which is that some registers must be accessible one way or another. > > >> + - MDCR_EL2.EBWE (bit 43) must be initialized to 0b1 > > >> + > > >> + - If EL3 is present: > > >> + > > >> + - MDCR_EL3.EBWE (bit 43) must be initialized to 0b1 > > >> + > > >> For CPUs with the Scalable Matrix Extension (FEAT_SME): > > >> > > >> - If EL3 is present: > > >> diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h > > >> index 4ef52d7245bb..2fbfe27d38b5 100644 > > >> --- a/arch/arm64/include/asm/el2_setup.h > > >> +++ b/arch/arm64/include/asm/el2_setup.h > > >> @@ -105,6 +105,13 @@ > > >> // to own it. > > >> > > >> .Lskip_trace_\@: > > >> + mrs x1, id_aa64dfr0_el1 > > >> + ubfx x1, x1, #ID_AA64DFR0_EL1_DebugVer_SHIFT, #4 > > >> + cmp x1, #ID_AA64DFR0_EL1_DebugVer_V8P9 > > >> + b.lt .Lskip_dbg_v8p9_\@ > > >> + > > >> + orr x2, x2, #MDCR_EL2_EBWE > > >> +.Lskip_dbg_v8p9_\@: > > >> msr mdcr_el2, x2 // Configure debug traps > > >> .endm > > >> > > >> @@ -244,6 +251,24 @@ > > >> .Lskip_gcs_\@: > > >> .endm > > >> > > >> +.macro __init_el2_fgt2 > > >> + mrs x1, id_aa64mmfr0_el1 > > >> + ubfx x1, x1, #ID_AA64MMFR0_EL1_FGT_SHIFT, #4 > > >> + cmp x1, #ID_AA64MMFR0_EL1_FGT_FGT2 > > > > > > We already read this field in __init_el2_fgt, shouldn't we leverage that > > > and move all this there rather than read the feature reg twice. > > > > Should not __init_el2_fgt2() remain separate to contain all future FEAT_FGT2 > > related trap enabled/disable checks ? OTOH reading id_aa64mmfr0_el1 register > > should improve some performance as well. > > That's the tradeoff. I'll defer to others whether a single id register > read here is preferred. Are we *really* talking about *performance* here? For something that is executed once per CPU boot? Just keep the damn thing readable, if at all possible... M.
On Wed, Dec 18, 2024 at 09:10:51AM +0000, Marc Zyngier wrote: > But I don't think we should be prescriptive about the state of these > registers, as long as the potential traps are correctly handled. The rest of the document is written in terms of explicit register values, if we're changing approach we should probably update all the existing requirements to be written in the same way since having a mix of approaches tends to be a big red flag that there should be different handling. Consistency will make the document clearer and easier for people to work with. FWIW there is an explict note in the document about the fact that it's an as-if rule: | Where the values documented | disable traps it is permissible for these traps to be enabled so long as | those traps are handled transparently by higher exception levels as though | the values documented were set. from when I raised this before.
diff --git a/Documentation/arch/arm64/booting.rst b/Documentation/arch/arm64/booting.rst index 3278fb4bf219..054cfe1cad18 100644 --- a/Documentation/arch/arm64/booting.rst +++ b/Documentation/arch/arm64/booting.rst @@ -288,6 +288,12 @@ Before jumping into the kernel, the following conditions must be met: - SCR_EL3.FGTEn (bit 27) must be initialised to 0b1. + For CPUs with the Fine Grained Traps (FEAT_FGT2) extension present: + + - If EL3 is present and the kernel is entered at EL2: + + - SCR_EL3.FGTEn2 (bit 59) must be initialised to 0b1. + For CPUs with support for HCRX_EL2 (FEAT_HCX) present: - If EL3 is present and the kernel is entered at EL2: @@ -322,6 +328,18 @@ Before jumping into the kernel, the following conditions must be met: - ZCR_EL2.LEN must be initialised to the same value for all CPUs the kernel will execute on. + For CPUs with FEAT_Debugv8p9 extension present: + + - If the kernel is entered at EL1 and EL2 is present: + + - HDFGRTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1 + - HDFGWTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1 + - MDCR_EL2.EBWE (bit 43) must be initialized to 0b1 + + - If EL3 is present: + + - MDCR_EL3.EBWE (bit 43) must be initialized to 0b1 + For CPUs with the Scalable Matrix Extension (FEAT_SME): - If EL3 is present: diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h index 4ef52d7245bb..2fbfe27d38b5 100644 --- a/arch/arm64/include/asm/el2_setup.h +++ b/arch/arm64/include/asm/el2_setup.h @@ -105,6 +105,13 @@ // to own it. .Lskip_trace_\@: + mrs x1, id_aa64dfr0_el1 + ubfx x1, x1, #ID_AA64DFR0_EL1_DebugVer_SHIFT, #4 + cmp x1, #ID_AA64DFR0_EL1_DebugVer_V8P9 + b.lt .Lskip_dbg_v8p9_\@ + + orr x2, x2, #MDCR_EL2_EBWE +.Lskip_dbg_v8p9_\@: msr mdcr_el2, x2 // Configure debug traps .endm @@ -244,6 +251,24 @@ .Lskip_gcs_\@: .endm +.macro __init_el2_fgt2 + mrs x1, id_aa64mmfr0_el1 + ubfx x1, x1, #ID_AA64MMFR0_EL1_FGT_SHIFT, #4 + cmp x1, #ID_AA64MMFR0_EL1_FGT_FGT2 + b.lt .Lskip_fgt2_\@ + + mrs x1, id_aa64dfr0_el1 + ubfx x1, x1, #ID_AA64DFR0_EL1_DebugVer_SHIFT, #4 + cmp x1, #ID_AA64DFR0_EL1_DebugVer_V8P9 + b.lt .Lskip_dbg_v8p9_\@ + + mov_q x0, HDFGWTR2_EL2_nMDSELR_EL1 + msr_s SYS_HDFGWTR2_EL2, x0 + msr_s SYS_HDFGRTR2_EL2, x0 +.Lskip_dbg_v8p9_\@: +.Lskip_fgt2_\@: +.endm + .macro __init_el2_nvhe_prepare_eret mov x0, #INIT_PSTATE_EL1 msr spsr_el2, x0 @@ -283,6 +308,7 @@ __init_el2_nvhe_idregs __init_el2_cptr __init_el2_fgt + __init_el2_fgt2 __init_el2_gcs .endm
Fine grained trap control for MDSELR_EL1 register needs to be configured in HDFGRTR2_EL2, and HDFGWTR2_EL2 registers when kernel enters at EL1, but EL2 is also present. This adds a new helper __init_el2_fgt2() initializing this new FEAT_FGT2 based fine grained registers. MDCR_EL2.EBWE needs to be enabled for additional (beyond 16) breakpoint and watchpoint exceptions when kernel enters at EL1, but EL2 is also present. This updates __init_el2_debug() as required for FEAT_Debugv8p9. While here, also update booting.rst with MDCR_EL3 and SCR_EL3 requirements. Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Jonathan Corbet <corbet@lwn.net> Cc: Marc Zyngier <maz@kernel.org> Cc: Oliver Upton <oliver.upton@linux.dev> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-doc@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: kvmarm@lists.linux.dev Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- Changes in V3: - Dropped MDCR_EL3.TDA boot requirement from documentation (separate series) - Dropped MDCR_EL2_EBWE definition as MDCR_EL2 is now defined in tools sysreg https://lore.kernel.org/all/20241211065425.1106683-1-anshuman.khandual@arm.com/ Documentation/arch/arm64/booting.rst | 18 ++++++++++++++++++ arch/arm64/include/asm/el2_setup.h | 26 ++++++++++++++++++++++++++ 2 files changed, 44 insertions(+)