lkml.org 
[lkml]   [2022]   [Jul]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/3] KVM: x86: Refresh PMU after writes to MSR_IA32_PERF_CAPABILITIES
    On Thu, Jul 28, 2022, Like Xu wrote:
    > On 28/7/2022 7:34 am, Sean Christopherson wrote:
    > > Refresh the PMU if userspace modifies MSR_IA32_PERF_CAPABILITIES. KVM
    > > consumes the vCPU's PERF_CAPABILITIES when enumerating PEBS support, but
    > > relies on CPUID updates to refresh the PMU. I.e. KVM will do the wrong
    > > thing if userspace stuffs PERF_CAPABILITIES _after_ setting guest CPUID.
    >
    > Unwise userspace should reap its consequences if it does not break KVM or host.

    I don't think this is a case of userspace being weird or unwise. IMO, setting
    CPUID before MSRs is perfectly logical and intuitive.

    > When a guest feature can be defined/controlled by multiple KVM APIs entries,
    > (such as SET_CPUID2, msr_feature, KVM_CAP, module_para), should KVM
    > define the priority of these APIs (e.g. whether they can override each other) ?

    KVM does have "rules" in the sense that it has an established ABI for things
    like KVM_CAP and module params, though documentation may be lacking in some cases.
    The CPUID and MSR ioctls don't have a prescribe ordering though.

    > Removing this ambiguity ensures consistency in the architecture and behavior
    > of all KVM features.

    Agreed, but the CPUID and MSR ioctls (among many others) have existed for quite
    some time. KVM likely can't retroactively force a specific order without breaking
    one userspace or another.

    > Any further performance optimizations can be based on these finalized values
    > as you do.
    >
    > >
    > > Opportunistically fix a curly-brace indentation.
    > >
    > > Fixes: c59a1f106f5c ("KVM: x86/pmu: Add IA32_PEBS_ENABLE MSR emulation for extended PEBS")
    > > Cc: Like Xu <like.xu.linux@gmail.com>
    > > Signed-off-by: Sean Christopherson <seanjc@google.com>
    > > ---
    > > arch/x86/kvm/x86.c | 4 ++--
    > > 1 file changed, 2 insertions(+), 2 deletions(-)
    > >
    > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
    > > index 5366f884e9a7..362c538285db 100644
    > > --- a/arch/x86/kvm/x86.c
    > > +++ b/arch/x86/kvm/x86.c
    > > @@ -3543,9 +3543,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
    > > return 1;
    > > vcpu->arch.perf_capabilities = data;
    > > -
    > > + kvm_pmu_refresh(vcpu);
    >
    > I had proposed this diff but was met with silence.

    My apologies, I either missed it or didn't connect the dots.

    \
     
     \ /
      Last update: 2022-07-28 17:29    [W:4.478 / U:0.764 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site