Re: [PATCH] KVM: arm64: Fix TRFCR_EL1/PMSCR_EL1 access in hVHE mode

From: Marc Zyngier <maz@kernel.org>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	kvm@vger.kernel.org, James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>,
	James Clark <james.clark@arm.com>,
	Anshuman Khandual <anshuman.khandual@arm.com>
Subject: Re: [PATCH] KVM: arm64: Fix TRFCR_EL1/PMSCR_EL1 access in hVHE mode
Date: Thu, 29 Feb 2024 18:24:37 +0000	[thread overview]
Message-ID: <864jdr2knu.wl-maz@kernel.org> (raw)
In-Reply-To: <ZeDAxL9nr_qmYGS9@linux.dev>

On Thu, 29 Feb 2024 17:37:08 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Hey,
> 
> On Thu, Feb 29, 2024 at 02:54:17PM +0000, Marc Zyngier wrote:
> > When running in hVHE mode, EL1 accesses are performed with the EL12
> > accessor, as we run with HCR_EL2.E2H=1.
> > 
> > Unfortunately, both PMSCR_EL1 and TRFCR_EL1 are used with the
> > EL1 accessor, meaning that we actually affect the EL2 state. Duh.
> > 
> > Switch to using the {read,write}_sysreg_el1() helpers that will do
> > the right thing in all circumstances.
> 
> I was wondering if there was a way to surface these screw-ups at compile
> time, but there's nothing elegant that comes to mind. Guess we need to
> be very careful reviewing "nVHE" changes going forward.

My take on this is that there should hardly be any read_sysreg_s() in
the KVM code at all. We should always use read_sysreg_el*() so that
there is no ambiguity about the state we're dealing with (that's, of
course, only valid for registers that have both an EL1 and an EL2
counterpart -- registers that are shared across ELs must still use the
read_sysreg_s() accessor).

It would also free the drive-by hacker from having to understand the
subtleties of the E2H redirection. The macros do the right thing
everywhere (they are context aware), and they should be the first port
of call.

> 
> > Note that the 'Fixes:' tag doesn't represent the point where the bug
> > was introduced (there is no such point), but the first practical point
> > where the hVHE feature is usable.
> > 
> > Cc: James Clark <james.clark@arm.com>
> > Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> > Fixes: 38cba55008e5 ("KVM: arm64: Force HCR_E2H in guest context when ARM64_KVM_HVHE is set")
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> 
> Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

Thanks. What should we do about it? Fix for 6.8, or part of the 6.9
drop? hVHE+tracing is a pretty niche thing, and I don't have any other
fix for the time being...

	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