On 26/11/2024 4:23 pm, Oliver Upton wrote:
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.
I sent another version, it did come out much simpler and still does all the same things as before.
I didn't manage to make a single enable/disable knob though. The thing is the filtering is set on the source side of the driver and trbe is a sink thing. I would have to couple them together and add knowledge of the sink type to the source to make it work.
That would then open up the possibility for anyone adding a new source to get the trbe bit wrong in the future. Having KVM override the filter setting when trbe is in use seems a lot safer and easier to understand.