linux-kernel - Re: [PATCH v7 11/12] KVM: arm64: Swap TRFCR on guest switch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z0X2BBBaDejFfATp@linux.dev>
Date: Tue, 26 Nov 2024 08:23:32 -0800
From: Oliver Upton <oliver.upton@...ux.dev>
To: James Clark <james.clark@...aro.org>
Cc: suzuki.poulose@....com, coresight@...ts.linaro.org,
	kvmarm@...ts.linux.dev, Marc Zyngier <maz@...nel.org>,
	Joey Gouly <joey.gouly@....com>, Zenghui Yu <yuzenghui@...wei.com>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, Mike Leach <mike.leach@...aro.org>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Mark Rutland <mark.rutland@....com>,
	Anshuman Khandual <anshuman.khandual@....com>,
	"Rob Herring (Arm)" <robh@...nel.org>,
	Shiqi Liu <shiqiliu@...t.edu.cn>, Fuad Tabba <tabba@...gle.com>,
	James Morse <james.morse@....com>, Mark Brown <broonie@...nel.org>,
	Raghavendra Rao Ananta <rananta@...gle.com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 11/12] KVM: arm64: Swap TRFCR on guest switch

On Thu, Nov 21, 2024 at 12:50:10PM +0000, James Clark wrote:
> 
> 
> On 20/11/2024 5:31 pm, Oliver Upton wrote:
> > On Tue, Nov 12, 2024 at 10:37:10AM +0000, James Clark wrote:
> > > +void kvm_set_trfcr(u64 host_trfcr, u64 guest_trfcr)
> > > +{
> > > +	if (kvm_arm_skip_trace_state())
> > > +		return;
> > > +
> > > +	if (has_vhe())
> > > +		write_sysreg_s(guest_trfcr, SYS_TRFCR_EL12);
> > > +	else
> > > +		if (host_trfcr != guest_trfcr) {
> > > +			*host_data_ptr(host_debug_state.trfcr_el1) = guest_trfcr;
> > 
> > Huh? That's going into host_debug_state, which is the dumping grounds
> > for *host* context when entering a guest.
> > 
> > Not sure why we'd stick a *guest* value in there...
> > 
> 
> Only to save a 3rd storage place for trfcr when just the register and one
> place is technically enough. But yes if it's more readable to have
> guest_trfcr_el1 separately then that makes sense.

Yeah, since this is all per-cpu data at this point rather than per-vCPU,
it isn't the end of the world to use a few extra bytes.

> That works, it would be nice to have it consistent and have it that way for
> filtering, like kvm_set_guest_trace_filters(bool kernel, bool user). But I
> suppose we can justify not doing it there because we're not really
> interpreting the TRFCR value just writing it whole.

Agreed, the biggest thing I'd want to see in the exported interfaces
like this is to have enable/disable helpers to tell KVM when a driver
wants KVM to start/stop managing a piece of state while in a guest.

Then the hypervisor code can blindly save/restore some opaque values to
whatever registers it needs to update.

> > What if trace is disabled in the guest or in the host? Do we need to
> > synchronize when transitioning from an enabled -> disabled state like we
> > do today?
> > 
> 
> By synchronize do you mean the tsb_csync()? I can only see it being
> necessary for the TRBE case because then writing to the buffer is fatal.
> Without TRBE the trace sinks still work and the boundary of when exactly
> tracing is disabled in the kernel isn't critical.

Ack, I had the blinders on that we cared only about TRBE here.

> > I took a stab at this, completely untested of course && punts on
> > protected mode. But this is _generally_ how I'd like to see everything
> > fit together.
> > 
> 
> Would you expect to see the protected mode stuff ignored if I sent another
> version more like yours below? Or was that just skipped to keep the example
> shorter?

Skipped since I slapped this together in a hurry.

> I think I'm a bit uncertain on that one because removing HAS_TRBE means you
> can't check if TRBE is enabled or not in protected mode and it will go wrong
> if it is.

The protected mode hypervisor will need two bits of information.
Detecting that the feature is present can be done in the kernel so long
as the corresponding static key / cpucap is toggled before we drop
privileges.

Whether or not it is programmable + enabled is a decision that must be
made by observing hardware state from the hypervisor before entering a
guest.

[...]

> > +void kvm_enable_trbe(u64 guest_trfcr)
> > +{
> > +	if (WARN_ON_ONCE(preemptible()))
> > +		return;
> > +
> > +	if (has_vhe()) {
> > +		write_sysreg_s(guest_trfcr, SYS_TRFCR_EL12);
> > +		return;
> > +	}
> > +
> > +	*host_data_ptr(guest_trfcr_el1) = guest_trfcr;
> > +	host_data_set_flag(HOST_TRBE_ENABLED);
> 
> FWIW TRBE and TRF are separate features, so this wouldn't do the filtering
> correctly if TRBE wasn't in use, but I can split it out into
> separate kvm_enable_trbe(void) and kvm_set_guest_filters(u64 guest_trfcr).

KVM manages the same piece of state (TRFCR_EL1) either way though right?

The expectation I had is that KVM is informed any time a trace session
(TRBE or otherwise) is enabled/disabled on a CPU, likely with a TRFCR_EL1
of 0 if guest mode is excluded.

The function names might need massaging, but I was hoping to have a
single set of enable/disable knobs to cover all bases here.

-- 
Thanks,
Oliver