LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [PATCH 2/2] KVM: x86/pmu: do not mask the value that is written to fixed PMUs
Date: Mon, 20 May 2019 12:01:58 -0700	[thread overview]
Message-ID: <20190520190158.GE28482@linux.intel.com> (raw)
In-Reply-To: <1558366951-19259-3-git-send-email-pbonzini@redhat.com>

On Mon, May 20, 2019 at 05:42:31PM +0200, Paolo Bonzini wrote:
> According to the SDM, for MSR_IA32_PERFCTR0/1 "the lower-order 32 bits of
> each MSR may be written with any value, and the high-order 8 bits are
> sign-extended according to the value of bit 31", but the fixed counters
> in real hardware appear to be limited to the width of the fixed counters.
> Fix KVM to do the same.

The section of the SDM you're quoting relates to P6 behavior, which
predates the architectural perfmons.  Section 18.2.1.1 "Architectural
Performance Monitoring Version 1 Facilities" has a more relevant blurb
for the MSR_IA32_PERFCTRx change (slightly modified to eliminate
embarassing typos in the SDM):

  The bit width of an IA32_PMCx MSR is reported using CPUID.0AH:EAXH[23:16].
  This is the number of valid bits for read operation.  On write operations,
  the lower-order 32-bits of the MSR may be written with any value, and the
  high-order bits are sign-extended from the value of bit 31.

And for the fixed counters, section 18.2.2 "Architectural Performance
Monitoring Version 2":

  The facilities provided by architectural performance monitoring version 2
  can be queried from CPUID leaf 0AH by examinng the content of register EDX:

    - Bits 5 through 12 of CPUID.0AH.EDX indicates the bit-width of fixed-
      function performance counters.  Bits beyond the width of the fixed-
      function counter are reserved and must be written as zeros.

> 
> Reported-by: Nadav Amit <nadav.amit@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/vmx/pmu_intel.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> index b6f5157445fe..a99613a060dd 100644
> --- a/arch/x86/kvm/vmx/pmu_intel.c
> +++ b/arch/x86/kvm/vmx/pmu_intel.c
> @@ -240,11 +240,14 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		}
>  		break;
>  	default:
> -		if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)) ||
> -		    (pmc = get_fixed_pmc(pmu, msr))) {
> -			if (!msr_info->host_initiated)
> -				data = (s64)(s32)data;
> -			pmc->counter += data - pmc_read_counter(pmc);
> +		if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0))) {
> +			if (msr_info->host_initiated)
> +				pmc->counter = data;
> +			else
> +				pmc->counter = (s32)data;
> +			return 0;
> +		} else if ((pmc = get_fixed_pmc(pmu, msr))) {
> +			pmc->counter = data;

Would it make sense to inject a #GP if the guest attempts to set bits that
are reserved to be zero, e.g. based on guest CPUID?

>  			return 0;
>  		} else if ((pmc = get_gp_pmc(pmu, msr, MSR_P6_EVNTSEL0))) {
>  			if (data == pmc->eventsel)
> -- 
> 1.8.3.1
> 

  reply	other threads:[~2019-05-20 19:02 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-20 15:42 [PATCH 0/2] " Paolo Bonzini
2019-05-20 15:42 ` [PATCH 1/2] KVM: x86/pmu: mask the result of rdpmc according to the width of the counters Paolo Bonzini
2019-05-20 15:42 ` [PATCH 2/2] KVM: x86/pmu: do not mask the value that is written to fixed PMUs Paolo Bonzini
2019-05-20 19:01   ` Sean Christopherson [this message]
2019-05-22 10:27     ` Paolo Bonzini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190520190158.GE28482@linux.intel.com \
    --to=sean.j.christopherson@intel.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --subject='Re: [PATCH 2/2] KVM: x86/pmu: do not mask the value that is written to fixed PMUs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).