Re: [PATCH v6 2/5] KVM: arm64: Add newly allocated ID registers to register descriptions

From: Marc Zyngier <maz@kernel.org>
To: Mark Brown <broonie@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Oliver Upton <oliver.upton@linux.dev>,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Jonathan Corbet <corbet@lwn.net>, Shuah Khan <shuah@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Dave Martin <Dave.Martin@arm.com>,
	kvmarm@lists.linux.dev, linux-doc@vger.kernel.org,
	linux-kselftest@vger.kernel.org
Subject: Re: [PATCH v6 2/5] KVM: arm64: Add newly allocated ID registers to register descriptions
Date: Wed, 10 Apr 2024 11:32:02 +0100	[thread overview]
Message-ID: <86ttk9se3h.wl-maz@kernel.org> (raw)
In-Reply-To: <73c6012f-adb0-470b-bd47-6093d28aea97@sirena.org.uk>

On Tue, 02 Apr 2024 18:21:55 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> On Sun, Mar 31, 2024 at 11:59:06AM +0100, Marc Zyngier wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> 
> > > The 2023 architecture extensions have allocated some new ID registers, add
> > > them to the KVM system register descriptions so that they are visible to
> > > guests.
> 
> > > We make the newly introduced dpISA features writeable, as well as
> > > allowing writes to ID_AA64ISAR3_EL1.CPA for FEAT_CPA which only
> > > introduces straigforward new instructions with no additional
> > > architectural state or traps.
> 
> > FPMR actively gets trapped by HCRX_EL2.
> 
> Sure, I'm not clear what you're trying to say here?

I'm saying (and not trying to say) that there are traps implied by the
features that you are adding.

> The "no additional" bit is referring to FEAT_CPA.

Well, that wasn't clear to me.

And when it comes to CPA, there are additional controls in SCTLR2_ELx,
which doesn't even gets context switched for EL1. What could possibly
go wrong?

> 
> > > -	ID_UNALLOCATED(6,3),
> > > +	ID_WRITABLE(ID_AA64ISAR3_EL1, ~(ID_AA64ISAR2_EL1_RES0 |
> > > +					ID_AA64ISAR3_EL1_PACM |
> > > +					ID_AA64ISAR3_EL1_TLBIW)),
> > >  	ID_UNALLOCATED(6,4),
> > >  	ID_UNALLOCATED(6,5),
> > >  	ID_UNALLOCATED(6,6),
> 
> > Where is the code that enforces the lack of support for MTEFAR,
> > MTESTOREONLY, and MTEPERM for SCTLR_ELx, EnPACM and EnFPM in HCRX_EL2?
> 
> Could you please be more explicit regarding what you're expecting to see
> here?

I'm expecting you to add all the required masking and fine-grained
disabling of features that are not explicitly advertised to the guest.

This should translate into additional init code in kvm_init_sysreg(),
kvm_init_nv_sysregs() and limit_nv_id_reg(). You also should update
the exception triaging infrastructure in emulate-nested.c.

> Other than the writeability mask for the ID register I would have
> expected to need explicit code to enable new features rather than
> explicit code to keep currently unsupported features unsupported.  I'm
> sure what you're referencing will be obvious once I see it but I'm
> drawing a blank.
> 
> > And I haven't checked whether TLBI VMALLWS2 can be trapped.
> 
> I didn't see anything but I might not be aware of where to look, there
> doesn't seem to be anything for that specifically in HFGITR_EL2 or
> HFGITR2_EL2 which would be the main places I'd expect to find
> something.

That's a really odd place to look. This is a S2 invalidation
primitive, which by definition is under the sole control of EL2, and
therefore cannot be trapped by any of the FGT registers, as they only
affect lesser-privileged ELs.

The instruction is described in the XML:

https://developer.arm.com/documentation/ddi0601/2024-03/AArch64-Instructions/TLBI-VMALLWS2E1--TLB-Invalidate-stage-2-dirty-state-by-VMID--EL1-0

	M.

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