On 11/20/2024 1:31 PM, Mi, Dapeng wrote:
> On 11/20/2024 12:32 AM, Sean Christopherson wrote:
>> On Thu, Aug 01, 2024, Mingwei Zhang wrote:
>>> Clear RDPMC_EXITING in vmcs when all counters on the host side are exposed
>>> to guest VM. This gives performance to passthrough PMU. However, when guest
>>> does not get all counters, intercept RDPMC to prevent access to unexposed
>>> counters. Make decision in vmx_vcpu_after_set_cpuid() when guest enables
>>> PMU and passthrough PMU is enabled.
>>>
>>> Co-developed-by: Xiong Zhang <xiong.y.zhang@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Xiong Zhang <xiong.y.zhang@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Mingwei Zhang <mizhang@xxxxxxxxxx>
>>> Signed-off-by: Dapeng Mi <dapeng1.mi@xxxxxxxxxxxxxxx>
>>> Tested-by: Yongwei Ma <yongwei.ma@xxxxxxxxx>
>>> ---
>>> arch/x86/kvm/pmu.c | 16 ++++++++++++++++
>>> arch/x86/kvm/pmu.h | 1 +
>>> arch/x86/kvm/vmx/vmx.c | 5 +++++
>>> 3 files changed, 22 insertions(+)
>>>
>>> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
>>> index e656f72fdace..19104e16a986 100644
>>> --- a/arch/x86/kvm/pmu.c
>>> +++ b/arch/x86/kvm/pmu.c
>>> @@ -96,6 +96,22 @@ void kvm_pmu_ops_update(const struct kvm_pmu_ops *pmu_ops)
>>> #undef __KVM_X86_PMU_OP
>>> }
>>>
>>> +bool kvm_pmu_check_rdpmc_passthrough(struct kvm_vcpu *vcpu)
>> As suggested earlier, kvm_rdpmc_in_guest().
> Sure.
>
>
>>> +{
>>> + struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
>>> +
>>> + if (is_passthrough_pmu_enabled(vcpu) &&
>>> + !enable_vmware_backdoor &&
>> Please add a comment about the VMware backdoor, I doubt most folks know about
>> VMware's tweaks to RDPMC behavior. It's somewhat obvious from the code and
>> comment in check_rdpmc(), but I think it's worth calling out here too.
> Sure.
>
>
>>> + pmu->nr_arch_gp_counters == kvm_pmu_cap.num_counters_gp &&
>>> + pmu->nr_arch_fixed_counters == kvm_pmu_cap.num_counters_fixed &&
>>> + pmu->counter_bitmask[KVM_PMC_GP] == (((u64)1 << kvm_pmu_cap.bit_width_gp) - 1) &&
>>> + pmu->counter_bitmask[KVM_PMC_FIXED] == (((u64)1 << kvm_pmu_cap.bit_width_fixed) - 1))
>> BIT_ULL? GENMASK_ULL?
> Sure.
>
>
>>> + return true;
>>> +
>>> + return false;
>> Do this:
>>
>>
>> return <true>;
>>
>> not:
>>
>> if (<true>)
>> return true;
>>
>> return false;
>>
>> Short-circuiting on certain cases is fine, and I would probably vote for that so
>> it's easier to add comments, but that's obviously not what's done here. E.g. either
>>
>> if (!enable_mediated_pmu)
>> return false;
>>
>> /* comment goes here */
>> if (enable_vmware_backdoor)
>> return false;
>>
>> return <counters checks>;
>>
>> or
>>
>> return <massive combined check>;
> Nice suggestion. Thanks.
>
>
>>> +}
>>> +EXPORT_SYMBOL_GPL(kvm_pmu_check_rdpmc_passthrough);
>> Maybe just make this an inline in a header? enable_vmware_backdoor is exported,
>> and presumably enable_mediated_pmu will be too.
> Sure.
>
>
>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>>> index 4d60a8cf2dd1..339742350b7a 100644
>>> --- a/arch/x86/kvm/vmx/vmx.c
>>> +++ b/arch/x86/kvm/vmx/vmx.c
>>> @@ -7911,6 +7911,11 @@ void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>>> vmx->msr_ia32_feature_control_valid_bits &=
>>> ~FEAT_CTL_SGX_LC_ENABLED;
>>>
>>> + if (kvm_pmu_check_rdpmc_passthrough(&vmx->vcpu))
>> No need to follow vmx->vcpu, @vcpu is readily available.
> Yes.
>
>
>>> + exec_controls_clearbit(vmx, CPU_BASED_RDPMC_EXITING);
>>> + else
>>> + exec_controls_setbit(vmx, CPU_BASED_RDPMC_EXITING);
>> I wonder if it makes sense to add a helper to change a bit. IIRC, the only reason
>> I didn't add one along with the set/clear helpers was because there weren't many
>> users and I couldn't think of good alternative to "set".
>>
>> I still don't have a good name, but I think we're reaching the point where it's
>> worth forcing the issue to avoid common goofs, e.g. handling only the "clear"
>> case and no the "set" case.
>>
>> Maybe changebit? E.g.
>>
>> static __always_inline void lname##_controls_changebit(struct vcpu_vmx *vmx, u##bits val, \
>> bool set) \
>> { \
>> if (set) \
>> lname##_controls_setbit(vmx, val); \
>> else \
>> lname##_controls_clearbit(vmx, val); \
>> }
>>
>>
>> and then vmx_refresh_apicv_exec_ctrl() can be:
>>
>> secondary_exec_controls_changebit(vmx,
>> SECONDARY_EXEC_APIC_REGISTER_VIRT |
>> SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY,
>> kvm_vcpu_apicv_active(vcpu));
>> tertiary_exec_controls_changebit(vmx, TERTIARY_EXEC_IPI_VIRT,
>> kvm_vcpu_apicv_active(vcpu) && enable_ipiv);
>>
>> and this can be:
>>
>> exec_controls_changebit(vmx, CPU_BASED_RDPMC_EXITING,
>> !kvm_rdpmc_in_guest(vcpu));
> Sure. would add a separate patch to add these helpers.
Hi Sean,
The upcoming v4 patches would include some code which you suggest in the
comments, like this one. I would like add a co-developed-by and
corresponding SoB tags for you in the patch if the patch includes you
suggested code. Is it ok? Or you more prefer the "suggested-by" tag? Thanks.
>
>
>
