linux-kernel - Re: [PATCH 1/3] KVM: x86: Refresh PMU after writes to MSR_IA32_PERF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YuKqyTvbVx2UyP2w@google.com>
Date:   Thu, 28 Jul 2022 15:27:05 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Like Xu <like.xu.linux@...il.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [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@...il.com>
> > Signed-off-by: Sean Christopherson <seanjc@...gle.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.