Re: [PATCH 16/27] KVM: arm64: nv: Add trap forwarding for HCR_EL2

[Marc Zyngier <maz@kernel.org>]

Hey Eric,

Thanks for looking into this, much appreciated given how tedious it
is.

On Thu, 13 Jul 2023 15:05:33 +0100,
Eric Auger <eric.auger@redhat.com> wrote:
> 
> Hi Marc,
> 
> On 7/12/23 16:57, Marc Zyngier wrote:
> > Describe the HCR_EL2 register, and associate it with all the sysregs
> > it allows to trap.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/emulate-nested.c | 475 ++++++++++++++++++++++++++++++++
> >  1 file changed, 475 insertions(+)
> >
> > diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
> > index 5bab2e85d70c..51901e85e43d 100644
> > --- a/arch/arm64/kvm/emulate-nested.c
> > +++ b/arch/arm64/kvm/emulate-nested.c

[...]

> > +	[CGT_HCR_TPC] = {
> modern revisions now refer to TPCP, maybe worth a comment?

Absolutely.

[...]

> > +	SR_RANGE_TRAP(SYS_ID_PFR0_EL1,
> > +		      sys_reg(3, 0, 0, 7, 7), CGT_HCR_TID3),
> in the spec I see this upper limit in the FEAT_FGT section. Out of
> curiosity how were you able to convert the sys reg names into this Op0,
> Op1, CRn, CRm, Op2. Is there any ordering logic documented somewhere for
> those group3 regs?

If you look at the sysreg encoding described on page D18-6308 if
version J.a of the ARM ARM, you will find a block of 56 contiguous
encodings ranging from (3, 0, 0, 1, 0), which happens to be
ID_PFR0_EL1, all the way to a reserved range ending in (3, 0, 0, 7,
7).

This is the block of register that is controlled by TID3.

> I checked Table D18-2 and this looks good but I wonder if there isn't
> any more efficient way to review this.

Not that I know of, unfortunately. Even the pseudocode isn't enough
for this as it doesn't described the trapping of unallocated regions.

> > +	SR_TRAP(SYS_ICC_SGI0R_EL1,	CGT_HCR_IMO_FMO),
> > +	SR_TRAP(SYS_ICC_ASGI1R_EL1,	CGT_HCR_IMO_FMO),
> > +	SR_TRAP(SYS_ICC_SGI1R_EL1,	CGT_HCR_IMO_FMO),
> > +	SR_RANGE_TRAP(sys_reg(3, 0, 11, 0, 0),
> > +		      sys_reg(3, 0, 11, 15, 7), CGT_HCR_TIDCP),
> > +	SR_RANGE_TRAP(sys_reg(3, 1, 11, 0, 0),
> > +		      sys_reg(3, 1, 11, 15, 7), CGT_HCR_TIDCP),
> > +	SR_RANGE_TRAP(sys_reg(3, 2, 11, 0, 0),
> > +		      sys_reg(3, 2, 11, 15, 7), CGT_HCR_TIDCP),
> > +	SR_RANGE_TRAP(sys_reg(3, 3, 11, 0, 0),
> > +		      sys_reg(3, 3, 11, 15, 7), CGT_HCR_TIDCP),
> > +	SR_RANGE_TRAP(sys_reg(3, 4, 11, 0, 0),
> > +		      sys_reg(3, 4, 11, 15, 7), CGT_HCR_TIDCP),
> > +	SR_RANGE_TRAP(sys_reg(3, 5, 11, 0, 0),
> > +		      sys_reg(3, 5, 11, 15, 7), CGT_HCR_TIDCP),
> > +	SR_RANGE_TRAP(sys_reg(3, 6, 11, 0, 0),
> > +		      sys_reg(3, 6, 11, 15, 7), CGT_HCR_TIDCP),
> > +	SR_RANGE_TRAP(sys_reg(3, 7, 11, 0, 0),
> > +		      sys_reg(3, 7, 11, 15, 7), CGT_HCR_TIDCP),
> > +	SR_RANGE_TRAP(sys_reg(3, 0, 15, 0, 0),
> > +		      sys_reg(3, 0, 15, 15, 7), CGT_HCR_TIDCP),
> > +	SR_RANGE_TRAP(sys_reg(3, 1, 15, 0, 0),
> > +		      sys_reg(3, 1, 15, 15, 7), CGT_HCR_TIDCP),
> > +	SR_RANGE_TRAP(sys_reg(3, 2, 15, 0, 0),
> > +		      sys_reg(3, 2, 15, 15, 7), CGT_HCR_TIDCP),
> > +	SR_RANGE_TRAP(sys_reg(3, 3, 15, 0, 0),
> > +		      sys_reg(3, 3, 15, 15, 7), CGT_HCR_TIDCP),
> > +	SR_RANGE_TRAP(sys_reg(3, 4, 15, 0, 0),
> > +		      sys_reg(3, 4, 15, 15, 7), CGT_HCR_TIDCP),
> > +	SR_RANGE_TRAP(sys_reg(3, 5, 15, 0, 0),
> > +		      sys_reg(3, 5, 15, 15, 7), CGT_HCR_TIDCP),
> > +	SR_RANGE_TRAP(sys_reg(3, 6, 15, 0, 0),
> > +		      sys_reg(3, 6, 15, 15, 7), CGT_HCR_TIDCP),
> > +	SR_RANGE_TRAP(sys_reg(3, 7, 15, 0, 0),
> > +		      sys_reg(3, 7, 15, 15, 7), CGT_HCR_TIDCP),
> > +	SR_TRAP(SYS_ACTLR_EL1,		CGT_HCR_TACR),
> > +	SR_TRAP(SYS_DC_ISW,		CGT_HCR_TSW),
> > +	SR_TRAP(SYS_DC_CSW,		CGT_HCR_TSW),
> > +	SR_TRAP(SYS_DC_CISW,		CGT_HCR_TSW),
> > +	SR_TRAP(SYS_DC_IGSW,		CGT_HCR_TSW),
> > +	SR_TRAP(SYS_DC_IGDSW,		CGT_HCR_TSW),
> > +	SR_TRAP(SYS_DC_CGSW,		CGT_HCR_TSW),
> > +	SR_TRAP(SYS_DC_CGDSW,		CGT_HCR_TSW),
> > +	SR_TRAP(SYS_DC_CIGSW,		CGT_HCR_TSW),
> > +	SR_TRAP(SYS_DC_CIGDSW,		CGT_HCR_TSW),
> > +	SR_TRAP(SYS_DC_CIVAC,		CGT_HCR_TPC),
> I don't see CVADP?

Me neither! Good catch!

[...]

> > +	SR_TRAP(SYS_SCTLR_EL1,		CGT_HCR_TVM_TRVM),
> > +	SR_TRAP(SYS_TTBR0_EL1,		CGT_HCR_TVM_TRVM),
> > +	SR_TRAP(SYS_TTBR1_EL1,		CGT_HCR_TVM_TRVM),
> > +	SR_TRAP(SYS_TCR_EL1,		CGT_HCR_TVM_TRVM),
> > +	SR_TRAP(SYS_ESR_EL1,		CGT_HCR_TVM_TRVM),
> > +	SR_TRAP(SYS_FAR_EL1,		CGT_HCR_TVM_TRVM),
> > +	SR_TRAP(SYS_AFSR0_EL1,		CGT_HCR_TVM_TRVM),*
> Looking at the SFSR0_EL1 MRS/MSR pseudo code I understand TRVM is tested
> on read and
> TVM is tested on write. However CGT_HCR_TVM has FORWARD_ANY behaviour
> while TRVM looks good as FORWARD_READ? Do I miss something.

You're not missing anything. For some reason, I had in my head that
TVM was trapping both reads and writes, while the spec is clear that
it only traps writes.

> 
> > +	SR_TRAP(SYS_AFSR1_EL1,		CGT_HCR_TVM_TRVM),*
> same here and below

Yup, I need to fix the TVM encoding like this:

@@ -176,7 +176,7 @@ static const struct trap_bits coarse_trap_bits[] = {
 		.index		= HCR_EL2,
 		.value		= HCR_TVM,
 		.mask		= HCR_TVM,
-		.behaviour	= BEHAVE_FORWARD_ANY,
+		.behaviour	= BEHAVE_FORWARD_WRITE,
 	},
 	[CGT_HCR_TDZ] = {
 		.index		= HCR_EL2,

[...]

> > +	/* All _EL2 registers */
> > +	SR_RANGE_TRAP(sys_reg(3, 4, 0, 0, 0),
> > +		      sys_reg(3, 4, 10, 15, 7), CGT_HCR_NV),
> > +	SR_RANGE_TRAP(sys_reg(3, 4, 12, 0, 0),
> > +		      sys_reg(3, 4, 14, 15, 7), CGT_HCR_NV),
> > +	/* All _EL02, _EL12 registers */
> > +	SR_RANGE_TRAP(sys_reg(3, 5, 0, 0, 0),
> > +		      sys_reg(3, 5, 10, 15, 7), CGT_HCR_NV),
> > +	SR_RANGE_TRAP(sys_reg(3, 5, 12, 0, 0),
> > +		      sys_reg(3, 5, 14, 15, 7), CGT_HCR_NV),
> same question as bove, where in the ARM ARM do you find those
> ranges?

I went over the encoding with a fine comb, and realised that all the
(3, 4, ...) encodings are EL2, and all the (3, 5, ...) ones are EL02
and EL12.

I appreciate that this is taking a massive bet on the future, but
there is no such rule in the ARM ARM as such...

> > +	SR_TRAP(SYS_SP_EL1,		CGT_HCR_NV),*
> > +	SR_TRAP(OP_AT_S1E2R,		CGT_HCR_NV),*
> > +	SR_TRAP(OP_AT_S1E2W,		CGT_HCR_NV),*
> > +	SR_TRAP(OP_AT_S12E1R,		CGT_HCR_NV),*
> > +	SR_TRAP(OP_AT_S12E1W,		CGT_HCR_NV),*
> > +	SR_TRAP(OP_AT_S12E0R,		CGT_HCR_NV),*
> according to the pseudo code NV2 is not checked
> shouldn't we have a separate CGT? Question also valid for a bunch of ops
> below

Hmmm. Yes, this is wrong. Well spotted. I guess I need a
CGT_HCR_NV_nNV2 for the cases that want that particular condition (NV1
probably needs a similar fix).

[...]

> CIGDPAE?
> CIPAE?

These two are part of RME (well, technically MEC, but that an RME
thing), and I have no plan to support this with NV -- yet.

> CFP/CPP/DVP RCTX?

These are definitely missing. I'll add them.

Thanks again for going through this list, this is awesome work!

	M.

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