Re: [PATCH v2 21/26] KVM: arm64: nv: Add trap forwarding for HDFGxTR_EL2

[Marc Zyngier <maz@kernel.org>]

Hi Eric,

On Mon, 07 Aug 2023 18:19:16 +0100,
Eric Auger <eric.auger@redhat.com> wrote:
> 
> Hi Marc,
> On 7/28/23 10:29, Marc Zyngier wrote:
> > ... and finally, the Debug version of FGT, with its *enormous*
> > list of trapped registers.
> >
> > Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Hi Marc, I think you mixed up with the R-b's sent on
> [PATCH v2 06/26] arm64: Add debug registers affected by HDFGxTR_EL2

Most probably. Really sorry about that. I'll fix that right away.

> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/include/asm/kvm_arm.h |  11 +
> >  arch/arm64/kvm/emulate-nested.c  | 460 +++++++++++++++++++++++++++++++

[...]

> > +	SR_FGT(SYS_TRCIMSPEC0, 		HDFGRTR, TRCIMSPECn, 1),
> why not SYS_TRCIMSPEC(0)?

Yup, it's been mentioned a couple of time already, now fixed.

> > +	SR_FGT(SYS_TRCIMSPEC(1), 	HDFGRTR, TRCIMSPECn, 1),
> > +	SR_FGT(SYS_TRCIMSPEC(2), 	HDFGRTR, TRCIMSPECn, 1),
> > +	SR_FGT(SYS_TRCIMSPEC(3), 	HDFGRTR, TRCIMSPECn, 1),
> > +	SR_FGT(SYS_TRCIMSPEC(4), 	HDFGRTR, TRCIMSPECn, 1),
> > +	SR_FGT(SYS_TRCIMSPEC(5), 	HDFGRTR, TRCIMSPECn, 1),
> > +	SR_FGT(SYS_TRCIMSPEC(6), 	HDFGRTR, TRCIMSPECn, 1),
> > +	SR_FGT(SYS_TRCIMSPEC(7), 	HDFGRTR, TRCIMSPECn, 1),
> > +	SR_FGT(SYS_TRCDEVARCH, 		HDFGRTR, TRCID, 1),
> > +	SR_FGT(SYS_TRCDEVID, 		HDFGRTR, TRCID, 1),
> what about all of the TRCIDR regs refered to in the spec?

Ah, nothing escapes you! Yup totally missed it, now added.

[...]

> > +	SR_FGT(SYS_TRCRSCTLR(2), 	HDFGRTR, TRC, 1),y
> > +	SR_FGT(SYS_TRCRSCTLR(3), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(4), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(5), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(6), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(7), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(8), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(9), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(10), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(11), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(12), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(13), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(14), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(15), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(16), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(17), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(18), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(19), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(20), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(21), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(22), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(23), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(24), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(25), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(26), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(27), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(28), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(29), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(30), 	HDFGRTR, TRC, 1),
> > +	SR_FGT(SYS_TRCRSCTLR(31), 	HDFGRTR, TRC, 1),y
> > +	SR_FGT(SYS_TRCQCTLR, 		HDFGRTR, TRC, 1),
> nit: maybe put this one before the
> 
> SYS_TRCRSCTLR(n) series to follow the spec order

Done.

[...]

> > +	/*
> > +	 * HDFGWTR_EL2
> > +	 *
> > +	 * Although HDFGRTR_EL2 and HDFGWTR_EL2 registers largely
> > +	 * overlap in their bit assignment, there are a number of bits
> > +	 * that are RES0 on one side, and an actual trap bit on the
> > +	 * other.  The policy chosen here is to describe all the
> > +	 * read-side mappings, and only the write-side mappings that
> > +	 * differ from the write side, and the trap handler will pick
> differ from the read side?

Well spotted! Now fixed.

Thanks again!

	M.

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