kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Fix MSR_IA32_PERF_STATUS
@ 2010-09-01  7:34 Avi Kivity
  2010-09-01  7:34 ` [PATCH 1/2] KVM: Don't save/restore MSR_IA32_PERF_STATUS Avi Kivity
  2010-09-01  7:34 ` [PATCH 2/2] KVM: Fix bogus value returned for MSR_IA32_PERF_STATUS Avi Kivity
  0 siblings, 2 replies; 6+ messages in thread
From: Avi Kivity @ 2010-09-01  7:34 UTC (permalink / raw)
  To: Marcelo Tosatti, kvm, Alexander Graf

MSR_IA32_PERF_STATUS is read-only, yet exposed for save/restore.  This
generates annoying printk()s when userspace attempts to initialize it.

The value returned is also bogus, the comments appear to refer to another msr.

Fix both issues.  Alex, please test with a guest Mac.

Avi Kivity (2):
  KVM: Don't save/restore MSR_IA32_PERF_STATUS
  KVM: Fix bogus value returned for MSR_IA32_PERF_STATUS

 arch/x86/kvm/x86.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 1/2] KVM: Don't save/restore MSR_IA32_PERF_STATUS
  2010-09-01  7:34 [PATCH 0/2] Fix MSR_IA32_PERF_STATUS Avi Kivity
@ 2010-09-01  7:34 ` Avi Kivity
  2010-09-01  7:34 ` [PATCH 2/2] KVM: Fix bogus value returned for MSR_IA32_PERF_STATUS Avi Kivity
  1 sibling, 0 replies; 6+ messages in thread
From: Avi Kivity @ 2010-09-01  7:34 UTC (permalink / raw)
  To: Marcelo Tosatti, kvm, Alexander Graf

It is read/only; restoring it only results in annoying messages.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/x86.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cc6d6cd..1cbf168 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -739,7 +739,7 @@ static u32 msrs_to_save[] = {
 #ifdef CONFIG_X86_64
 	MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
 #endif
-	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA
+	MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA
 };
 
 static unsigned num_msrs_to_save;
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2] KVM: Fix bogus value returned for MSR_IA32_PERF_STATUS
  2010-09-01  7:34 [PATCH 0/2] Fix MSR_IA32_PERF_STATUS Avi Kivity
  2010-09-01  7:34 ` [PATCH 1/2] KVM: Don't save/restore MSR_IA32_PERF_STATUS Avi Kivity
@ 2010-09-01  7:34 ` Avi Kivity
  2010-09-01  8:49   ` Alexander Graf
  1 sibling, 1 reply; 6+ messages in thread
From: Avi Kivity @ 2010-09-01  7:34 UTC (permalink / raw)
  To: Marcelo Tosatti, kvm, Alexander Graf

The returned value is completely bogus, and sets reserved bits.
Return zero instead.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/x86.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1cbf168..a2c03f1 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1641,10 +1641,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 		data = vcpu->arch.ia32_misc_enable_msr;
 		break;
 	case MSR_IA32_PERF_STATUS:
-		/* TSC increment by tick */
-		data = 1000ULL;
-		/* CPU multiplier */
-		data |= (((uint64_t)4ULL) << 40);
+		data = 0;
 		break;
 	case MSR_EFER:
 		data = vcpu->arch.efer;
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] KVM: Fix bogus value returned for MSR_IA32_PERF_STATUS
  2010-09-01  7:34 ` [PATCH 2/2] KVM: Fix bogus value returned for MSR_IA32_PERF_STATUS Avi Kivity
@ 2010-09-01  8:49   ` Alexander Graf
  2010-09-01  8:55     ` Avi Kivity
  0 siblings, 1 reply; 6+ messages in thread
From: Alexander Graf @ 2010-09-01  8:49 UTC (permalink / raw)
  To: Avi Kivity; +Cc: Marcelo Tosatti, kvm, Alexander Graf


On 01.09.2010, at 09:34, Avi Kivity wrote:

> The returned value is completely bogus, and sets reserved bits.
> Return zero instead.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
> arch/x86/kvm/x86.c |    5 +----
> 1 files changed, 1 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1cbf168..a2c03f1 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1641,10 +1641,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
> 		data = vcpu->arch.ia32_misc_enable_msr;
> 		break;
> 	case MSR_IA32_PERF_STATUS:
> -		/* TSC increment by tick */
> -		data = 1000ULL;
> -		/* CPU multiplier */
> -		data |= (((uint64_t)4ULL) << 40);
> +		data = 0;
> 		break;
> 	case MSR_EFER:
> 		data = vcpu->arch.efer;


This is the respective code snippet from xnu:

        /*
         * Get the TSC increment.  The TSC is incremented by this
         * on every bus tick.  Calculate the TSC conversion factors
         * to and from nano-seconds.
         */
        if (cpuid_info()->cpuid_family == CPU_FAMILY_PENTIUM_M) {
                uint64_t        prfsts;

                prfsts = rdmsr64(IA32_PERF_STS);
                tscGranularity = (uint32_t)bitfield(prfsts, 44, 40);
                N_by_2_bus_ratio = prfsts & bit(46);

        } else {
                panic("rtclock_init: unknown CPU family: 0x%X\n",
                        cpuid_info()->cpuid_family);
        }

        if (N_by_2_bus_ratio)
                tscFCvtt2n = busFCvtt2n * 2 / (uint64_t)tscGranularity;
        else
                tscFCvtt2n = busFCvtt2n / (uint64_t)tscGranularity;

        tscFreq = ((1 * Giga)  << 32) / tscFCvtt2n;
        tscFCvtn2t = 0xFFFFFFFFFFFFFFFFULL / tscFCvtt2n;


So by passing in 0 here, you effectively make that code divide something by 0 which results in a panic.


Alex


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] KVM: Fix bogus value returned for MSR_IA32_PERF_STATUS
  2010-09-01  8:49   ` Alexander Graf
@ 2010-09-01  8:55     ` Avi Kivity
  2010-09-01  8:56       ` Alexander Graf
  0 siblings, 1 reply; 6+ messages in thread
From: Avi Kivity @ 2010-09-01  8:55 UTC (permalink / raw)
  To: Alexander Graf; +Cc: Marcelo Tosatti, kvm, Alexander Graf

  On 09/01/2010 11:49 AM, Alexander Graf wrote:
> On 01.09.2010, at 09:34, Avi Kivity wrote:
>
>> The returned value is completely bogus, and sets reserved bits.
>> Return zero instead.
>>
>> Signed-off-by: Avi Kivity<avi@redhat.com>
>> ---
>> arch/x86/kvm/x86.c |    5 +----
>> 1 files changed, 1 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 1cbf168..a2c03f1 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -1641,10 +1641,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
>> 		data = vcpu->arch.ia32_misc_enable_msr;
>> 		break;
>> 	case MSR_IA32_PERF_STATUS:
>> -		/* TSC increment by tick */
>> -		data = 1000ULL;
>> -		/* CPU multiplier */
>> -		data |= (((uint64_t)4ULL)<<  40);
>> +		data = 0;
>> 		break;
>> 	case MSR_EFER:
>> 		data = vcpu->arch.efer;
>
> This is the respective code snippet from xnu:
>
>          /*
>           * Get the TSC increment.  The TSC is incremented by this
>           * on every bus tick.  Calculate the TSC conversion factors
>           * to and from nano-seconds.
>           */
>          if (cpuid_info()->cpuid_family == CPU_FAMILY_PENTIUM_M) {
>                  uint64_t        prfsts;
>
>                  prfsts = rdmsr64(IA32_PERF_STS);
>                  tscGranularity = (uint32_t)bitfield(prfsts, 44, 40);
>                  N_by_2_bus_ratio = prfsts&  bit(46);
>
>          } else {
>                  panic("rtclock_init: unknown CPU family: 0x%X\n",
>                          cpuid_info()->cpuid_family);
>          }
>
>          if (N_by_2_bus_ratio)
>                  tscFCvtt2n = busFCvtt2n * 2 / (uint64_t)tscGranularity;
>          else
>                  tscFCvtt2n = busFCvtt2n / (uint64_t)tscGranularity;
>
>          tscFreq = ((1 * Giga)<<  32) / tscFCvtt2n;
>          tscFCvtn2t = 0xFFFFFFFFFFFFFFFFULL / tscFCvtt2n;
>
>
> So by passing in 0 here, you effectively make that code divide something by 0 which results in a panic.
>
>

Right.  Searching again I find that this is actually documented, (under 
MSR_PERF_STATU
s instead of IA32_PERF_STATUS).  I'll just drop this patch.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] KVM: Fix bogus value returned for MSR_IA32_PERF_STATUS
  2010-09-01  8:55     ` Avi Kivity
@ 2010-09-01  8:56       ` Alexander Graf
  0 siblings, 0 replies; 6+ messages in thread
From: Alexander Graf @ 2010-09-01  8:56 UTC (permalink / raw)
  To: Avi Kivity; +Cc: Marcelo Tosatti, KVM list


On 01.09.2010, at 10:55, Avi Kivity wrote:

> On 09/01/2010 11:49 AM, Alexander Graf wrote:
>> On 01.09.2010, at 09:34, Avi Kivity wrote:
>> 
>>> The returned value is completely bogus, and sets reserved bits.
>>> Return zero instead.
>>> 
>>> Signed-off-by: Avi Kivity<avi@redhat.com>
>>> ---
>>> arch/x86/kvm/x86.c |    5 +----
>>> 1 files changed, 1 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 1cbf168..a2c03f1 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -1641,10 +1641,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
>>> 		data = vcpu->arch.ia32_misc_enable_msr;
>>> 		break;
>>> 	case MSR_IA32_PERF_STATUS:
>>> -		/* TSC increment by tick */
>>> -		data = 1000ULL;
>>> -		/* CPU multiplier */
>>> -		data |= (((uint64_t)4ULL)<<  40);
>>> +		data = 0;
>>> 		break;
>>> 	case MSR_EFER:
>>> 		data = vcpu->arch.efer;
>> 
>> This is the respective code snippet from xnu:
>> 
>>         /*
>>          * Get the TSC increment.  The TSC is incremented by this
>>          * on every bus tick.  Calculate the TSC conversion factors
>>          * to and from nano-seconds.
>>          */
>>         if (cpuid_info()->cpuid_family == CPU_FAMILY_PENTIUM_M) {
>>                 uint64_t        prfsts;
>> 
>>                 prfsts = rdmsr64(IA32_PERF_STS);
>>                 tscGranularity = (uint32_t)bitfield(prfsts, 44, 40);
>>                 N_by_2_bus_ratio = prfsts&  bit(46);
>> 
>>         } else {
>>                 panic("rtclock_init: unknown CPU family: 0x%X\n",
>>                         cpuid_info()->cpuid_family);
>>         }
>> 
>>         if (N_by_2_bus_ratio)
>>                 tscFCvtt2n = busFCvtt2n * 2 / (uint64_t)tscGranularity;
>>         else
>>                 tscFCvtt2n = busFCvtt2n / (uint64_t)tscGranularity;
>> 
>>         tscFreq = ((1 * Giga)<<  32) / tscFCvtt2n;
>>         tscFCvtn2t = 0xFFFFFFFFFFFFFFFFULL / tscFCvtt2n;
>> 
>> 
>> So by passing in 0 here, you effectively make that code divide something by 0 which results in a panic.
>> 
>> 
> 
> Right.  Searching again I find that this is actually documented, (under MSR_PERF_STATU
> s instead of IA32_PERF_STATUS).  I'll just drop this patch.

Sounds good. The other one seems sane to me. I don't have a working setup for Mac guests atm. That was on the TODO list once the AHCI stuff gets upstream :).


Alex


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-09-01  8:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-01  7:34 [PATCH 0/2] Fix MSR_IA32_PERF_STATUS Avi Kivity
2010-09-01  7:34 ` [PATCH 1/2] KVM: Don't save/restore MSR_IA32_PERF_STATUS Avi Kivity
2010-09-01  7:34 ` [PATCH 2/2] KVM: Fix bogus value returned for MSR_IA32_PERF_STATUS Avi Kivity
2010-09-01  8:49   ` Alexander Graf
2010-09-01  8:55     ` Avi Kivity
2010-09-01  8:56       ` Alexander Graf

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).