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.
