| Message ID | 20200127212256.194310-1-ehankland@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Fix perfctr WRMSR for running counters | expand |
On Mon, Jan 27, 2020 at 1:23 PM Eric Hankland <ehankland@google.com> wrote: > > Correct the logic in intel_pmu_set_msr() for fixed and general purpose > counters. This was recently changed to set pmc->counter without taking > in to account the value of pmc_read_counter() which will be incorrect if > the counter is currently running and non-zero; this changes back to the > old logic which accounted for the value of currently running counters. > > Signed-off-by: Eric Hankland <ehankland@google.com> Fixes: 2924b52117b2 ("KVM: x86/pmu: do not mask the value that is written to fixed PMUs") Reviewed-by: Jim Mattson <jmattson@google.com>
On 27/01/20 22:22, Eric Hankland wrote: > Correct the logic in intel_pmu_set_msr() for fixed and general purpose > counters. This was recently changed to set pmc->counter without taking > in to account the value of pmc_read_counter() which will be incorrect if > the counter is currently running and non-zero; this changes back to the > old logic which accounted for the value of currently running counters. > > Signed-off-by: Eric Hankland <ehankland@google.com> > --- > arch/x86/kvm/vmx/pmu_intel.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c > index 34a3a17bb6d7..9bdbe05b599c 100644 > --- a/arch/x86/kvm/vmx/pmu_intel.c > +++ b/arch/x86/kvm/vmx/pmu_intel.c > @@ -264,9 +264,10 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > pmc->counter = data; > else > pmc->counter = (s32)data; > + pmc->counter += pmc->counter - pmc_read_counter(pmc); I think this best written as it was before commit 2924b52117: if (!msr_info->host_initiated) data = (s64)(s32)data; pmc->counter += data - pmc_read_counter(pmc); Do you have a testcase? Paolo > return 0; > } else if ((pmc = get_fixed_pmc(pmu, msr))) { > - pmc->counter = data; > + pmc->counter += data - pmc_read_counter(pmc); > return 0; > } else if ((pmc = get_gp_pmc(pmu, msr, MSR_P6_EVNTSEL0))) { > if (data == pmc->eventsel) >
Sorry - I forgot to switch to plain text mode on my first reply. > I think this best written as it was before commit 2924b52117: > > if (!msr_info->host_initiated) > data = (s64)(s32)data; > pmc->counter += data - pmc_read_counter(pmc); Sounds good to me. > Do you have a testcase? I added a testcase to kvm-unit-tests/x86/pmu.c that fails without this patch and passes with it. Should I send out that patch now?
On 30/01/20 02:09, Eric Hankland wrote: > Sorry - I forgot to switch to plain text mode on my first reply. > >> I think this best written as it was before commit 2924b52117: >> >> if (!msr_info->host_initiated) >> data = (s64)(s32)data; >> pmc->counter += data - pmc_read_counter(pmc); > Sounds good to me. > >> Do you have a testcase? > I added a testcase to kvm-unit-tests/x86/pmu.c that fails without this > patch and passes with it. Should I send out that patch now? Yes, please send it! Thanks, Paolo
> Yes, please send it! Thanks,
I sent out the test a couple days ago and you queued it (commit
b9624f3f34bd "Test WRMSR on a running counter").
Are there any other changes that I should make?
Thanks,
Eric
On 07/02/20 23:15, Eric Hankland wrote: >> Yes, please send it! Thanks, > > I sent out the test a couple days ago and you queued it (commit > b9624f3f34bd "Test WRMSR on a running counter"). > Are there any other changes that I should make? Nope, thanks! Paolo
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c index 34a3a17bb6d7..9bdbe05b599c 100644 --- a/arch/x86/kvm/vmx/pmu_intel.c +++ b/arch/x86/kvm/vmx/pmu_intel.c @@ -264,9 +264,10 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) pmc->counter = data; else pmc->counter = (s32)data; + pmc->counter += pmc->counter - pmc_read_counter(pmc); return 0; } else if ((pmc = get_fixed_pmc(pmu, msr))) { - pmc->counter = data; + pmc->counter += data - pmc_read_counter(pmc); return 0; } else if ((pmc = get_gp_pmc(pmu, msr, MSR_P6_EVNTSEL0))) { if (data == pmc->eventsel)
Correct the logic in intel_pmu_set_msr() for fixed and general purpose counters. This was recently changed to set pmc->counter without taking in to account the value of pmc_read_counter() which will be incorrect if the counter is currently running and non-zero; this changes back to the old logic which accounted for the value of currently running counters. Signed-off-by: Eric Hankland <ehankland@google.com> --- arch/x86/kvm/vmx/pmu_intel.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)