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

[Oliver Upton]

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