Re: [RFC 10/10] KVM: arm64: nv: Add new HDFGRTR2_GROUP & HDFGRTR2_GROUP based FGU handling

[Marc Zyngier <maz@kernel.org>]

On Fri, 02 Aug 2024 10:25:44 +0100,
Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> 
> On 8/1/24 21:33, Marc Zyngier wrote:
> > On Thu, 01 Aug 2024 11:46:22 +0100,
> > Anshuman Khandual <anshuman.khandual@arm.com> wrote:

[...]

> >> +	SR_FGT(SYS_SPMACCESSR_EL1,	HDFGRTR2, nSPMACCESSR_EL1, 0),
> > 
> > This (and I take it most of the stuff here) is also gated by
> > MDCR_EL2.SPM, which is a coarse grained trap. That needs to be
> > described as well. For every new register that you add here.
> 
> I did not find a SPM field in MDCR_EL2 either in latest ARM ARM or in
> the latest XML. But as per current HDFGRTR2_EL2 description the field
> nSPMACCESSR_EL1 is gated by FEAT_SPMU feature, which is being checked
> via ID_AA64DFR1_EL1.PMU when required. So could you please give some
> more details.

I misspelled it. It is MDCR_EL2.EnSPM.

And you are completely missing the point. It is not about
HDFGRTR2_EL2, but about SPMACCESSR_EL1 (and all its little friends).

To convince yourself, just look at the pseudocode for SPMACCESSR_EL1,
limited to an EL1 access:

elsif PSTATE.EL == EL1 then
    if HaveEL(EL3) && EL3SDDUndefPriority() && MDCR_EL3.EnPM2 == '0' then
        UNDEFINED;
    elsif EL2Enabled() && IsFeatureImplemented(FEAT_FGT2) && ((HaveEL(EL3) && SCR_EL3.FGTEn2 == '0') || HDFGRTR2_EL2.nSPMACCESSR_EL1 == '0') then
        AArch64.SystemAccessTrap(EL2, 0x18);
    elsif EL2Enabled() && MDCR_EL2.EnSPM == '0' then
        AArch64.SystemAccessTrap(EL2, 0x18);
    elsif HaveEL(EL3) && MDCR_EL3.EnPM2 == '0' then
        if EL3SDDUndef() then
            UNDEFINED;
        else
            AArch64.SystemAccessTrap(EL3, 0x18);
    elsif EffectiveHCR_EL2_NVx() IN {'111'} then
        X[t, 64] = NVMem[0x8E8];
    else
        X[t, 64] = SPMACCESSR_EL1;


Can you spot the *TWO* conditions where we take an exception to EL2
with 0x18 as the EC?

- One is when HDFGxTR2_EL2.nSPMACCESSR_EL1 == '0': that's a fine
  grained trap.
- The other is when MDCR_EL2.EnSPM == '0': that's a coarse grained
  trap.

Both conditions need to be captured in the various tables in this
file, for each and every register that you describe.

[...]

> > Now, the main issues are that:
> > 
> > - you're missing the coarse grained trapping for all the stuff you
> >   have just added. It's not a huge amount of work, but you need, for
> >   each register, to describe what traps apply to it. The fine grained
> >   stuff is most, but not all of it. There should be enough of it
> >   already to guide you through it.
> 
> Coarse grained trapping for FEAT_FGT2 based fine grained registers ?

Not for FEAT_FGT2. For the registers that FEAT_FGT2 traps. Can you see
the difference?

> Afraid, did not understand this. Could you please give some pointers
> on similar existing code.

See above. And if you want some example, just took at the file you are
patching already. Look at how MDCR_EL2 conditions the trapping of all
the debug, PMU, AMU registers, for example. There is no shortage of
them.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.