Re: [PATCH v3 10/22] KVM: arm64: Set up FGT for Partitioned PMU

[Marc Zyngier <maz@kernel.org>]

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.

-- 
Jazz isn't dead. It just smells funny.