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

From: Marc Zyngier <maz@kernel.org>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Joey Gouly <joey.gouly@arm.com>,
	Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH v2 06/25] KVM: arm64: nv: Add sanitising to VNCR-backed HCRX_EL2
Date: Fri, 02 Feb 2024 14:56:50 +0000	[thread overview]
Message-ID: <86h6iq7w3x.wl-maz@kernel.org> (raw)
In-Reply-To: <ZbytUKPFYiP2cdd8@linux.dev>

On Fri, 02 Feb 2024 08:52:32 +0000,
Oliver Upton <oliver.upton@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@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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel