[PATCH v2 06/25] KVM: arm64: nv: Add sanitising to VNCR-backed HCRX_EL2

[Marc Zyngier]

On Fri, 02 Feb 2024 08:52:32 +0000,
Oliver Upton <oliver.upton at linux.dev> wrote:
> 
> On Tue, Jan 30, 2024 at 08:45:13PM +0000, Marc Zyngier wrote:
> > Just like its little friends, HCRX_EL2 gets the feature set treatment
> > when backed by VNCR.
> > 
> > Signed-off-by: Marc Zyngier <maz at kernel.org>
> > ---
> >  arch/arm64/kvm/nested.c | 42 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> > 
> > diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> > index cdeef3259193..72db632b115a 100644
> > --- a/arch/arm64/kvm/nested.c
> > +++ b/arch/arm64/kvm/nested.c
> > @@ -263,6 +263,48 @@ int kvm_init_nv_sysregs(struct kvm *kvm)
> >  		res1 |= HCR_E2H;
> >  	set_sysreg_masks(kvm, HCR_EL2, res0, res1);
> >  
> > +	/* HCRX_EL2 */
> > +	res0 = HCRX_EL2_RES0;
> > +	res1 = HCRX_EL2_RES1;
> 
> I'm a bit worried that we're depending on the meaning of these generated
> RES0/RES1 bitmasks not changing behind our backs.
> 
> Not like people read anything, but do you think it'd make sense to add a
> warning comment to the sysreg file that adding new encodings can have a
> functional change on KVM?

No amount of warnings will do, because people don't give a damn.

I'm actually in favour of something far more radical: we snapshot the
raw value of all the used RES0/1, and put BUILD_BUG_ON()s that fire if
any value has changed. They will have to touch the KVM code to fix
that, and we catch them red-handed.

> 
> > +	if (!kvm_has_feat(kvm, ID_AA64ISAR3_EL1, PACM, TRIVIAL_IMP))
> > +		res0 |= HCRX_EL2_PACMEn;
> > +	if (!kvm_has_feat(kvm, ID_AA64PFR2_EL1, FPMR, IMP))
> > +		res0 |= HCRX_EL2_EnFPM;
> > +	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, GCS, IMP))
> > +		res0 |= HCRX_EL2_GCSEn;
> > +	if (!kvm_has_feat(kvm, ID_AA64ISAR2_EL1, SYSREG_128, IMP))
> > +		res0 |= HCRX_EL2_EnIDCP128;
> > +	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, ADERR, DEV_ASYNC))
> > +		res0 |= (HCRX_EL2_EnSDERR | HCRX_EL2_EnSNERR);
> > +	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, DF2, IMP))
> > +		res0 |= HCRX_EL2_TMEA;
> > +	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, D128, IMP))
> > +		res0 |= HCRX_EL2_D128En;
> > +	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, THE, IMP))
> > +		res0 |= HCRX_EL2_PTTWI;
> 
> Ok, not fair. The latest public version of the ARM ARM doesn't have any
> of this. Where are you getting it from?

The bloody XML, from which all of this should, in theory, be extracted
without any human intervention. Sadly, it seems that we are not
allowed to do so, from what I have been told.

The ARM ARM is, unfortunately, being quickly rendered obsolete by the
lack of updates (FEAT_THE is part of the v9.4 architecture, released
in 2022).

I'll make sure to add a link to the XML in the cover letter when I
repost the series.

Thanks,

	M.

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