Re: [PATCH v4 27/38] KVM: x86/pmu: Handle PMU MSRs interception and event filtering

[Mi, Dapeng]

On 5/17/2025 4:54 AM, Sean Christopherson wrote:
> On Fri, May 16, 2025, Dapeng Mi wrote:
>> On 3/25/2025 1:31 AM, Mingwei Zhang wrote:
>>> +	if (kvm_mediated_pmu_enabled(pmu_to_vcpu(pmu))) {
>>> +		bool allowed = check_pmu_event_filter(pmc);
>>> +
>>> +		if (pmc_is_gp(pmc)) {
>>> +			if (allowed)
>>> +				pmc->eventsel_hw |= pmc->eventsel &
>>> +						    ARCH_PERFMON_EVENTSEL_ENABLE;
>>> +			else
>>> +				pmc->eventsel_hw &= ~ARCH_PERFMON_EVENTSEL_ENABLE;
>>> +		} else {
>>> +			int idx = pmc->idx - KVM_FIXED_PMC_BASE_IDX;
>>> +
>>> +			if (allowed)
>>> +				pmu->fixed_ctr_ctrl_hw = pmu->fixed_ctr_ctrl;
>> Sean, just found there is a potential bug here.  The
>> "pmu->fixed_ctr_ctrl_hw" should not be assigned to "pmu->fixed_ctr_ctrl"
>> here, otherwise the other filtered fixed counter (not this allowed fixed
>> counter) could be enabled accidentally.
>>
>> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
>> index ba9d336f1d1d..f32e5f66f73b 100644
>> --- a/arch/x86/kvm/pmu.c
>> +++ b/arch/x86/kvm/pmu.c
>> @@ -473,7 +473,8 @@ static int reprogram_counter(struct kvm_pmc *pmc)
>>                         int idx = pmc->idx - KVM_FIXED_PMC_BASE_IDX;
>>
>>                         if (allowed)
>> -                               pmu->fixed_ctr_ctrl_hw = pmu->fixed_ctr_ctrl;
>> +                               pmu->fixed_ctr_ctrl_hw |= pmu->fixed_ctr_ctrl &
>> +                                              
>> intel_fixed_bits_by_idx(idx, 0xf);
> Hmm, I think that's fine, since pmu->fixed_ctr_ctrl should have changed?  But I'd

Not exactly. Assume guest enables the fixed counter 0 and 1, then
pmu->fixed_ctr_ctrl would be 0xbb. At the first time, user disables the
fixed counter 0 by KVM event filter, then pmu->fixed_ctr_ctrl_hw would be
0xb0, secondly user disables the fixed counter 1 as well, so
pmu->fixed_ctr_ctrl_hw is 0x0. At the third time, user re-enables fixed
counter 1, so pmu->fixed_ctr_ctrl_hw is expected to be 0xb0, but it's not
actually. Although the 1st calling (for fixed counter 0) to
reprogram_counter() would disable it, but the 2nd calling (for fixed
counter 1) to reprogram_counter() accidentally enables it
(pmu->fixed_ctr_ctrl_hw becomes 0xbb eventually). This is incorrect.


> rather play it safe and do (completely untested):
>
> 	if (pmc_is_gp(pmc)) {
> 		pmc->eventsel_hw &= ~ARCH_PERFMON_EVENTSEL_ENABLE;
> 		if (allowed)
> 			pmc->eventsel_hw |= pmc->eventsel &
> 					    ARCH_PERFMON_EVENTSEL_ENABLE;
> 	} else {
> 		u64 mask = intel_fixed_bits_by_idx(pmc->idx - KVM_FIXED_PMC_BASE_IDX, 0xf);
>
> 		pmu->fixed_ctr_ctrl_hw &= ~mask;
> 		if (allowed)
> 			pmu->fixed_ctr_ctrl_hw |= pmu->fixed_ctr_ctrl & mask;
> 	}

The code looks good.