Date: Tue, 25 Jun 2024 14:58:56 +0100
From: Marc Zyngier <maz@...nel.org>
To: Anshuman Khandual <anshuman.khandual@....com>
Cc: linux-arm-kernel@...ts.infradead.org,
	Oliver Upton <oliver.upton@...ux.dev>,
	James Morse <james.morse@....com>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>,
	Mark Brown <broonie@...nel.org>,
	kvmarm@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC 10/10] KVM: arm64: nv: Add new HDFGRTR2_GROUP & HDFGRTR2_GROUP based FGU handling

On Tue, 25 Jun 2024 10:03:29 +0100,
Anshuman Khandual <anshuman.khandual@....com> wrote:
> 
> >>>> +#define __HDFGRTR2_EL2_nMASK	~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK)
> >>>
> >>> Note the *nMASK* here. 'n' is for *negative*. Just look at how
> >>> __HDFGRTR_EL2_MASK and __HDFGRTR_EL2_nMASK are written.
> >>
> >> Still trying to understand what does these three masks represent
> >> for a given FGT register XXXX
> >>
> >> 	- __XXXX_RES0
> > 
> > RES0 bits.
> > 
> >> 	- __XXXX_MASK
> > 
> > Positive trap bits.
> > 
> >> 	- __XXXX_nMASK
> > 
> > Negative trap bits.
> 
> Right, figured that eventually but these were not very clear at
> first.

I keep hearing I'm not clear. But at this stage, I don't know what to
do to make it (or myself) clearer.

> 
> > 
> >>
> >> But from the mentioned example for HDFGRTR_EL2.
> >>
> >> #define __HDFGRTR_EL2_RES0      HDFGRTR_EL2_RES0
> >> #define __HDFGRTR_EL2_MASK      (BIT(63) | GENMASK(58, 50) | GENMASK(48, 43) | \
> >>                                  GENMASK(41, 40) | GENMASK(37, 22) | \
> >>                                  GENMASK(19, 9) | GENMASK(7, 0))
> >> #define __HDFGRTR_EL2_nMASK     ~(__HDFGRTR_EL2_RES0 | __HDFGRTR_EL2_MASK)
> >>
> >> Looking at HDFGRTR_EL2 definition inside arch/arm64/tools/sysreg
> >>
> >> HDFGRTR_EL2_RES0 = BIT(49) | GENMASK(39, 38) | GENMASK(21, 20) | BIT(8)
> >>
> >> is representing the entire mask in the register which are RES0. But then what
> >> does __HDFGRTR_EL2_MASK signify ? Positive trap bits mask ?
> >>
> >> The following bits belong in __HDFGRTR_EL2_MASK
> >>
> >> HDFGRTR_EL2.PMBIDR_EL1    (63)
> >> HDFGRTR_EL2.PMCEIDn_EL0   (58)
> >>
> >> Where as the following bits belong in __HDFGRTR_EL2_nMASK
> >>
> >> HDFGRTR_EL2.nPMSNEVFR_EL1 (61)
> >> HDFGRTR_EL2.nBRBCTL	  (60) 
> >>
> >> Reworking proposed HDFGRTR2_EL2 and HDFGWTR2_EL2. 
> >>
> >> #define __HDFGRTR2_EL2_RES0     HDFGRTR2_EL2_RES0
> >> #define __HDFGRTR2_EL2_MASK     0
> >> #define __HDFGRTR2_EL2_nMASK    ~(__HDFGRTR2_EL2_RES0 | __HDFGRTR2_EL2_MASK)
> >>
> >> #define __HDFGWTR2_EL2_RES0     HDFGWTR2_EL2_RES0
> >> #define __HDFGWTR2_EL2_MASK     0
> >> #define __HDFGWTR2_EL2_nMASK    ~(__HDFGWTR2_EL2_RES0 | __HDFGWTR2_EL2_MASK)
> >>
> >> Please note that all trap bits in both these registers are negative ones
> >> hence __HDFGRTR2_EL2_MASK/__HDFGWTR2_EL2_MASK should be 0.
> > 
> > That's because you're looking at the XML, and not the ARM ARM this was
> > written against.
> 
> Did not follow that. Both in ddi0601/2024-03 XML and ARM ARM DDI 0487K.a
> there are no positive trap bits, either in HDFGRTR2_EL2 or HDFGWTR2_EL2.
> OR did I miss something here.

From this very file:

<quote>
/*
 * FGT register definitions
 *
 * RES0 and polarity masks as of DDI0487J.a, to be updated as needed.
 * We're not using the generated masks as they are usually ahead of
 * the published ARM ARM, which we use as a reference.
 *
 * Once we get to a point where the two describe the same thing, we'll
 * merge the definitions. One day.
 */
</quote>

In case it wasn't written *CLEARLY* enough.

> Sure, will add the following new registers in arch/arm64/tools/sysreg format
> except for the ones that require a formula based enumeration. Those will be
> added into arch/arm64/include/asm/sysreg.h directly e.g
> 
> +#define SYS_SPMEVCNTR_EL0(m)           sys_reg(2, 3, 14, (0 | (m >> 3)), (m & 7))
> +#define SYS_SPMEVTYPER_EL0(m)          sys_reg(2, 3, 14, (2 | (m >> 3)), (m & 7))
> +#define SYS_SPMEVFILTR_EL0(m)          sys_reg(2, 3, 14, (4 | (m >> 3)), (m & 7))
> +#define SYS_SPMEVFILT2R_EL0(m)         sys_reg(2, 3, 14, (6 | (m >> 3)), (m & 7))
> 
> 9d93fc432f1a arm64/sysreg: Add remaining debug registers affected by HDFGxTR2_EL2
> 56b8830f0a5e arm64/sysreg: Add register fields for SPMCGCR1_EL1
> ad674ae52178 arm64/sysreg: Add register fields for SPMCGCR0_EL1
> 8538a282d208 arm64/sysreg: Add register fields for PMZR_EL0
> c88f0b8d898e arm64/sysreg: Add register fields for PMSSCR_EL1
> 8788ad49b7b6 arm64/sysreg: Add register fields for SPMCFGR_EL1
> 142de2bc3d7b arm64/sysreg: Add register fields for SPMDEVARCH_EL1
> 3d903ba35e8c arm64/sysreg: Add register fields for SPMIIDR_EL1
> 1a4db0b8b100 arm64/sysreg: Add register fields for PMICNTSVR_EL1
> 6bee8f139ba5 arm64/sysreg: Add register fields for SPMSELR_EL0
> b208ab4cd54d arm64/sysreg: Add register fields for SPMCNTENSET_EL0
> 644abf522c8a arm64/sysreg: Add register fields for SPMCNTENCLR_EL0
> fad9b7751359 arm64/sysreg: Add register fields for SPMINTENSET_EL1
> c28299b8df76 arm64/sysreg: Add register fields for SPMINTENCLR_EL1
> b9c283b27980 arm64/sysreg: Add register fields for SPMOVSSET_EL0
> 03dd01a26c46 arm64/sysreg: Add register fields for SPMOVSCLR_EL0
> c8a2f1b688de arm64/sysreg: Add register fields for SPMCR_EL0
> 422ca4026aa7 arm64/sysreg: Add register fields for SPMACCESSR_EL1
> d790b2570461 arm64/sysreg: Add register fields for SPMSCR_EL1
> c2cdd0fecdcb arm64/sysreg: Add register fields for PMCCNTSVR_EL1
> 3b793f3f07b8 arm64/sysreg: Add register fields for PMUACR_EL1
> a1804742ee8c arm64/sysreg: Add register fields for PMICFILTR_EL0
> c88476b9c52c arm64/sysreg: Add register fields for PMICNTR_EL0
> 6d1520f3477b arm64/sysreg: Add register fields for PMECR_EL1
> 257ea3ec7950 arm64/sysreg: Add register fields for PMIAR_EL1
> a29f787102f0 arm64/sysreg: Add register fields for SPMDEVAFF_EL1
> 3b7c4d4cf0eb arm64/sysreg: Add register fields for PMSDSFR_EL1
> 2a14e5dc1903 arm64/sysreg: Add register fields for TRBMPAM_EL1

Have you done this by hand? Or have you used an automated generator
that parses the XML?  If it's the former, please do the latter and
compare the results.

[...]

> >
> >>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> >>>> index f921af014d0c..8029f408855d 100644
> >>>> --- a/arch/arm64/kvm/sys_regs.c
> >>>> +++ b/arch/arm64/kvm/sys_regs.c
> >>>> @@ -4110,6 +4110,51 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu)
> >>>>  		kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 |
> >>>>  						  HAFGRTR_EL2_RES1);
> >>>>  
> >>>> +	/* FEAT_TRBE_MPAM is not exposed to the guest */
> >>>> +	kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nTRBMPAM_EL1);
> >>>
> >>> Same thing about dynamic configuration.
> >>>
> >>> But more importantly, you are disabling anything *but* MPAM.  Does it
> >>> seem right to you?
> >>
> >> Did not get that, should the inverse ~ be dropped here and also for all
> >> other negative trap bits across the register ?
> > 
> > Look at the way FGU works. A bit set to 1 means that if we have
> > trapped because of this bit (as per the FGT table), we inject an
> > UNDEF.
> 
> Seems like positive and negative trap bits do not make any difference
> here, while setting respective bits in kvm->arch.fgu[HDFGRTR2_GROUP]. 
> In this case the inverse should be dropped for all bits.

Yup, that's what "A bit set to 1" means here. We don't need to follow
the convolutions of the HW here.

> 
> Should the lone trap bit (nPMZR_EL0) which is present in HDFGWTR2_EL2
> but not in HDFGRTR2_EL2, be set in kvm->arch.fgu[HDFGWTR2_GROUP] as
> well ?
> 
> if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, V3P9)) {
> 	kvm->arch.fgu[HDFGRTR2_GROUP] |= HDFGRTR2_EL2_nPMUACR_EL1;
> 	kvm->arch.fgu[HDFGWTR2_GROUP] |= HDFGWTR2_EL2_nPMZR_EL0;
> }

Possibly. At the time this code was written, ARMv8.9 wasn't in the ARM
ARM.

	M.

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

Powered by blists - more mailing lists