Re: [PATCH v3 04/10] arm64/boot: Enable EL2 requirements for SPE_FEAT_FDS

[James Clark <james.clark@xxxxxxxxxx>]

On 15/07/2025 2:10 pm, James Clark wrote:
> On 15/07/2025 1:57 pm, Will Deacon wrote:
> > On Tue, Jul 15, 2025 at 01:48:03PM +0100, James Clark wrote:
> > > On 14/07/2025 2:54 pm, Will Deacon wrote:
> > > > On Thu, Jun 05, 2025 at 11:49:02AM +0100, James Clark wrote:
> > > > > SPE data source filtering (optional from Armv8.8) requires that 
> > > > > traps to
> > > > > the filter register PMSDSFR be disabled. Document the requirements and
> > > > > disable the traps if the feature is present.
> > > > >
> > > > > Tested-by: Leo Yan <leo.yan@xxxxxxx>
> > > > > Signed-off-by: James Clark <james.clark@xxxxxxxxxx>
> > > > > ---
> > > > >    Documentation/arch/arm64/booting.rst | 11 +++++++++++
> > > > >    arch/arm64/include/asm/el2_setup.h   | 14 ++++++++++++++
> > > > >    2 files changed, 25 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/arch/arm64/booting.rst b/Documentation/ 
> > > > > arch/arm64/booting.rst
> > > > > index dee7b6de864f..abd75085a239 100644
> > > > > --- a/Documentation/arch/arm64/booting.rst
> > > > > +++ b/Documentation/arch/arm64/booting.rst
> > > > > @@ -404,6 +404,17 @@ Before jumping into the kernel, the following 
> > > > > conditions must be met:
> > > > >        - HDFGWTR2_EL2.nPMICFILTR_EL0 (bit 3) must be initialised to 
> > > > > 0b1.
> > > > >        - HDFGWTR2_EL2.nPMUACR_EL1 (bit 4) must be initialised to 0b1.
> > > > > +  For CPUs with SPE data source filtering (FEAT_SPE_FDS):
> > > > > +
> > > > > +  - If EL3 is present:
> > > > > +
> > > > > +    - MDCR_EL3.EnPMS3 (bit 42) must be initialised to 0b1.
> > > > > +
> > > > > +  - If the kernel is entered at EL1 and EL2 is present:
> > > > > +
> > > > > +    - HDFGRTR2_EL2.nPMSDSFR_EL1 (bit 19) must be initialised to 0b1.
> > > > > +    - HDFGWTR2_EL2.nPMSDSFR_EL1 (bit 19) must be initialised to 0b1.
> > > > > +
> > > > >      For CPUs with Memory Copy and Memory Set instructions 
> > > > > (FEAT_MOPS):
> > > > >      - If the kernel is entered at EL1 and EL2 is present:
> > > > > diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/ 
> > > > > include/asm/el2_setup.h
> > > > > index 1e7c7475e43f..02b4a7fc016e 100644
> > > > > --- a/arch/arm64/include/asm/el2_setup.h
> > > > > +++ b/arch/arm64/include/asm/el2_setup.h
> > > > > @@ -279,6 +279,20 @@
> > > > >        orr    x0, x0, #HDFGRTR2_EL2_nPMICFILTR_EL0
> > > > >        orr    x0, x0, #HDFGRTR2_EL2_nPMUACR_EL1
> > > > >    .Lskip_pmuv3p9_\@:
> > > > > +    mrs    x1, id_aa64dfr0_el1
> > > > > +    ubfx    x1, x1, #ID_AA64DFR0_EL1_PMSVer_SHIFT, #4
> > > > > +    /* If SPE is implemented, */
> > > > > +    cmp    x1, #ID_AA64DFR0_EL1_PMSVer_IMP
> > > > > +    b.lt    .Lskip_spefds_\@
> > > > > +    /* we can read PMSIDR and */
> > > > > +    mrs_s    x1, SYS_PMSIDR_EL1
> > > > > +    and    x1, x1,  #PMSIDR_EL1_FDS
> > > > > +    /* if FEAT_SPE_FDS is implemented, */
> > > > > +    cbz    x1, .Lskip_spefds_\@
> > > > > +    /* disable traps to PMSDSFR. */
> > > > > +    orr    x0, x0, #HDFGRTR2_EL2_nPMSDSFR_EL1
> > > >
> > > > Why is this being done here rather than alongside the existing SPE
> > > > configuration of HDFGRTR_EL2 and HDFGWTR_EL2 near the start of
> > > > __init_el2_fgt?
> > > I thought everything was separated by which trap configs it writes to,
> > > rather than the feature. This SPE feature is in HDFGRTR2 so I put it in
> > > __init_el2_fgt2 rather than __init_el2_fgt.
> >
> > That's fair; __init_el2_fgt isn't the right place. But the redundancy of
> > re-reading PMSVer from DFR0 is a little jarring.
> >
> > > I suppose we could have a single __init_el2_spe that writes to both 
> > > HDFGRTR
> > > and HDFGRTR2 but we'd have to be careful to not overwrite what was 
> > > already
> > > done in the other sections.
> >
> > Right, perhaps it would be clearer to have trap-preserving macros for
> > features in a specific ID register rather than per-trap configuration
> > register macros.
> >
> > In other words, we have something like __init_fgt_aa64dfr0 which would
> > configure the FGT and FGT2 registers based on features in aa64dfr0. I
> > think you'd need to have a play to see how it ends up looking but the
> > main thing to avoid is having duplicate ID register parsing code for
> > setting up FGT and FGT2 traps.
> >
> > Will
>
> I'll give it a go but that could end up being fragile to something that 
> is dependent on two different ID registers in the future. Then we'd end 
> up in the same situation for a different reason.

I think I've run into it already. Wouldn't checking for FGT and FGT2 
have to be repeated when doing each ID register? Now we only do that 
once at the start of __init_el2_fgt and __init_el2_fgt2, even if we 
might sometimes check a different ID register twice. But if we flipped 
it we'd always have to repeat those.