On Fri, 27 Jun 2025 21:45:57 +0100, Colton Lewis coltonlewis@google.com wrote:
Marc Zyngier maz@kernel.org writes:
On Thu, 26 Jun 2025 21:04:46 +0100, Colton Lewis coltonlewis@google.com wrote:
+static inline void __activate_pmu_fgt(struct kvm_vcpu *vcpu) +{
- struct kvm_cpu_context *hctxt = host_data_ptr(host_ctxt);
- struct kvm *kvm = kern_hyp_va(vcpu->kvm);
- u64 set;
- u64 clr;
- set = HDFGRTR_EL2_PMOVS
| HDFGRTR_EL2_PMCCFILTR_EL0| HDFGRTR_EL2_PMEVTYPERn_EL0;- clr = HDFGRTR_EL2_PMUSERENR_EL0
| HDFGRTR_EL2_PMSELR_EL0| HDFGRTR_EL2_PMINTEN| HDFGRTR_EL2_PMCNTEN| HDFGRTR_EL2_PMCCNTR_EL0| HDFGRTR_EL2_PMEVCNTRn_EL0;- update_fgt_traps_cs(hctxt, vcpu, kvm, HDFGRTR_EL2, clr, set);
- set = HDFGWTR_EL2_PMOVS
| HDFGWTR_EL2_PMCCFILTR_EL0| HDFGWTR_EL2_PMEVTYPERn_EL0;- clr = HDFGWTR_EL2_PMUSERENR_EL0
| HDFGWTR_EL2_PMCR_EL0| HDFGWTR_EL2_PMSELR_EL0| HDFGWTR_EL2_PMINTEN| HDFGWTR_EL2_PMCNTEN| HDFGWTR_EL2_PMCCNTR_EL0| HDFGWTR_EL2_PMEVCNTRn_EL0;- update_fgt_traps_cs(hctxt, vcpu, kvm, HDFGWTR_EL2, clr, set);
- if (!cpus_have_final_cap(ARM64_HAS_FGT2))
return;- set = HDFGRTR2_EL2_nPMICFILTR_EL0
| HDFGRTR2_EL2_nPMICNTR_EL0;- clr = 0;
- update_fgt_traps_cs(hctxt, vcpu, kvm, HDFGRTR2_EL2, clr, set);
- set = HDFGWTR2_EL2_nPMICFILTR_EL0
| HDFGWTR2_EL2_nPMICNTR_EL0;- clr = 0;
- update_fgt_traps_cs(hctxt, vcpu, kvm, HDFGWTR2_EL2, clr, set);
This feels wrong. There should be one place to populate the FGTs that apply to a guest as set from the host, not two or more.
There is such a construct in the SME series, and maybe you could have a look at it, specially if the trap configuration is this static.
M.
-- Without deviation from the norm, progress is not possible.
I'm assuming you are referring to Mark Brown's series [1], specifically patches 5 and 18 and I see what you mean.
You are probably thinking configuration should happen from sys_regs.c:kvm_calculate_traps or thereabout and should be setting bits in the existing kvm->arch.fgt array.
Correct me if I'm mistaken.
I'm saying there should be exactly one place where we write to the individual trap registers, and that the source of these settings should be equally unique when they are immutable in the lifetime of the guest.
That's the existing pattern for most trap configuration, including HCR_EL2, ICH_HCR_EL2, HCRX_EL2, and the FGU configuration that trickles into the actual trap registers, and I want to stick with it if at all possible.
The way it is done in the SME series may be reasonable, but I haven't reviewed this series at all. I'm merely pointing out that similar constructs exist for other features.
M.