diff mbox

qemu-kvm bugfix for IA32_FEATURE_CONTROL

Message ID DE8DF0795D48FD4CA783C40EC8292335013398F3@SHSMSX101.ccr.corp.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Liu, Jinsong Aug. 19, 2013, 2:31 p.m. UTC 
Paolo Bonzini wrote:
> 
> The patch looks good.  Please repost it with checkpatch.pl failures
> fixed. 
> 
> Paolo

Thanks Stefan and Paolo! Updated patch attached.

Regards,
Jinsong

Comments

Andreas Färber Aug. 19, 2013, 2:59 p.m. UTC | #1 
Am 19.08.2013 16:31, schrieb Liu, Jinsong:
> Paolo Bonzini wrote:
>>
>> The patch looks good.  Please repost it with checkpatch.pl failures
>> fixed. 
>>
>> Paolo
> 
> Thanks Stefan and Paolo! Updated patch attached.
> 
> Regards,
> Jinsong
> 
> ===============
> From a0ddf948d40e42de862543157a5668a1c12faae6 Mon Sep 17 00:00:00 2001
> From: Liu Jinsong <jinsong.liu@intel.com>
> Date: Mon, 19 Aug 2013 09:33:30 +0800
> Subject: [PATCH] qemu-kvm bugfix for IA32_FEATURE_CONTROL
> 
> This patch is to fix the bug https://bugs.launchpad.net/qemu-kvm/+bug/1207623
> 
> IA32_FEATURE_CONTROL is pointless if not expose VMX or SMX bits to
> cpuid.1.ecx of vcpu. Current qemu-kvm will error return when kvm_put_msrs
> or kvm_get_msrs.
> 
> Signed-off-by: Liu Jinsong <jinsong.liu@intel.com>

Jinsong, if this is for upstream QEMU, then the commit message needs
some small improvements:

qemu-kvm is no longer maintained since 1.3 so it should not be occurring
any more.

Please use a prefix of "target-i386: " (the directory name) to signal
where you are changing code, i.e. x86 only.

"bugfix" is not a very telling description of what a patch is doing.

(Up to Paolo and Gleb whether they'll fix it or whether they require a
resend.)

Also please use git-send-email to submit patches and use "PATCH v2" etc.
for submission as top-level patch:
http://wiki.qemu.org/Contribute/SubmitAPatch

One question inline...

> ---
>  target-i386/kvm.c |   17 +++++++++++++++--
>  1 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 84ac00a..5adeb03 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -65,6 +65,7 @@ static bool has_msr_star;
>  static bool has_msr_hsave_pa;
>  static bool has_msr_tsc_adjust;
>  static bool has_msr_tsc_deadline;
> +static bool has_msr_feature_control;
>  static bool has_msr_async_pf_en;
>  static bool has_msr_pv_eoi_en;
>  static bool has_msr_misc_enable;
> @@ -644,6 +645,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
>  
>      qemu_add_vm_change_state_handler(cpu_update_state, env);
>  
> +    c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);
> +    if (c) {
> +        has_msr_feature_control = !!(c->ecx & CPUID_EXT_VMX) ||
> +                                  !!(c->ecx & CPUID_EXT_SMX);
> +    }
> +
>      cpuid_data.cpuid.padding = 0;
>      r = kvm_vcpu_ioctl(cs, KVM_SET_CPUID2, &cpuid_data);
>      if (r) {
> @@ -1121,7 +1128,10 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
>          if (hyperv_vapic_recommended()) {
>              kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_APIC_ASSIST_PAGE, 0);
>          }
> -        kvm_msr_entry_set(&msrs[n++], MSR_IA32_FEATURE_CONTROL, env->msr_ia32_feature_control);
> +        if (has_msr_feature_control) {
> +            kvm_msr_entry_set(&msrs[n++], MSR_IA32_FEATURE_CONTROL,
> +                              env->msr_ia32_feature_control);
> +        }
>      }
>      if (env->mcg_cap) {
>          int i;
> @@ -1346,7 +1356,9 @@ static int kvm_get_msrs(X86CPU *cpu)
>      if (has_msr_misc_enable) {
>          msrs[n++].index = MSR_IA32_MISC_ENABLE;
>      }
> -    msrs[n++].index = MSR_IA32_FEATURE_CONTROL;
> +    if (has_msr_feature_control) {
> +        msrs[n++].index = MSR_IA32_FEATURE_CONTROL;
> +    }
>  
>      if (!env->tsc_valid) {
>          msrs[n++].index = MSR_IA32_TSC;
> @@ -1447,6 +1459,7 @@ static int kvm_get_msrs(X86CPU *cpu)
>              break;
>          case MSR_IA32_FEATURE_CONTROL:
>              env->msr_ia32_feature_control = msrs[i].data;
> +            break;

Was the fallthrough previously intended? Or is this a second,
unmentioned bugfix?

Regards,
Andreas

>          default:
>              if (msrs[i].index >= MSR_MC0_CTL &&
>                  msrs[i].index < MSR_MC0_CTL + (env->mcg_cap & 0xff) * 4) {
>
Paolo Bonzini Aug. 19, 2013, 3:20 p.m. UTC | #2 
Il 19/08/2013 16:59, Andreas Färber ha scritto:
> qemu-kvm is no longer maintained since 1.3 so it should not be occurring
> any more.
> 
> Please use a prefix of "target-i386: " (the directory name) to signal
> where you are changing code, i.e. x86 only.
> 
> "bugfix" is not a very telling description of what a patch is doing.
> 
> (Up to Paolo and Gleb whether they'll fix it or whether they require a
> resend.)

No, not this time at least. :)

Paolo

> Also please use git-send-email to submit patches and use "PATCH v2" etc.
> for submission as top-level patch:
> http://wiki.qemu.org/Contribute/SubmitAPatch

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Liu, Jinsong Aug. 20, 2013, 3:33 a.m. UTC | #3 
Andreas Färber wrote:
> Am 19.08.2013 16:31, schrieb Liu, Jinsong:
>> Paolo Bonzini wrote:
>>> 
>>> The patch looks good.  Please repost it with checkpatch.pl failures
>>> fixed. 
>>> 
>>> Paolo
>> 
>> Thanks Stefan and Paolo! Updated patch attached.
>> 
>> Regards,
>> Jinsong
>> 
>> ===============
>> From a0ddf948d40e42de862543157a5668a1c12faae6 Mon Sep 17 00:00:00
>> 2001 
>> From: Liu Jinsong <jinsong.liu@intel.com>
>> Date: Mon, 19 Aug 2013 09:33:30 +0800
>> Subject: [PATCH] qemu-kvm bugfix for IA32_FEATURE_CONTROL
>> 
>> This patch is to fix the bug
>> https://bugs.launchpad.net/qemu-kvm/+bug/1207623 
>> 
>> IA32_FEATURE_CONTROL is pointless if not expose VMX or SMX bits to
>> cpuid.1.ecx of vcpu. Current qemu-kvm will error return when
>> kvm_put_msrs 
>> or kvm_get_msrs.
>> 
>> Signed-off-by: Liu Jinsong <jinsong.liu@intel.com>
> 
> Jinsong, if this is for upstream QEMU, then the commit message needs
> some small improvements:
> 
> qemu-kvm is no longer maintained since 1.3 so it should not be
> occurring any more.

Thanks Andreas!

This patch is for qemu-kvm.
Per my understanding, there are some patches firstly checked in qemu-kvm uq/master branch.
This patch is to fix c/s 0779caeb1a17f4d3ed14e2925b36ba09b084fb7b of qemu-kvm uq/master branch
(which is to co-work w/ kvm IA32_FEATURE_CONTROL, and currently not yet in upstream qemu).

This patch is used to fix the bug introduced by 0779caeb1a17f4d3ed14e2925b36ba09b084fb7b of qemu-kvm uq/master branch. The bug is reported as
https://bugs.launchpad.net/qemu-kvm/+bug/1207623
https://bugs.launchpad.net/qemu/+bug/1213797

Anything I misunderstand, for upstream qemu and qemu-kvm?

> 
> Please use a prefix of "target-i386: " (the directory name) to signal
> where you are changing code, i.e. x86 only.
> 
> "bugfix" is not a very telling description of what a patch is doing.
> 
> (Up to Paolo and Gleb whether they'll fix it or whether they require a
> resend.)
> 
> Also please use git-send-email to submit patches and use "PATCH v2"
> etc. for submission as top-level patch:
> http://wiki.qemu.org/Contribute/SubmitAPatch

Thanks, will update per your comments :)

> 
> One question inline...
> 
>> ---
>>  target-i386/kvm.c |   17 +++++++++++++++--
>>  1 files changed, 15 insertions(+), 2 deletions(-)
>> 
>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>> index 84ac00a..5adeb03 100644
>> --- a/target-i386/kvm.c
>> +++ b/target-i386/kvm.c
>> @@ -65,6 +65,7 @@ static bool has_msr_star;
>>  static bool has_msr_hsave_pa;
>>  static bool has_msr_tsc_adjust;
>>  static bool has_msr_tsc_deadline;
>> +static bool has_msr_feature_control;
>>  static bool has_msr_async_pf_en;
>>  static bool has_msr_pv_eoi_en;
>>  static bool has_msr_misc_enable;
>> @@ -644,6 +645,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
>> 
>>      qemu_add_vm_change_state_handler(cpu_update_state, env);
>> 
>> +    c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0); +    if (c) {
>> +        has_msr_feature_control = !!(c->ecx & CPUID_EXT_VMX) ||
>> +                                  !!(c->ecx & CPUID_EXT_SMX); +    }
>> +
>>      cpuid_data.cpuid.padding = 0;
>>      r = kvm_vcpu_ioctl(cs, KVM_SET_CPUID2, &cpuid_data);      if
>> (r) { @@ -1121,7 +1128,10 @@ static int kvm_put_msrs(X86CPU *cpu,
>>          int level) if (hyperv_vapic_recommended()) {
>>              kvm_msr_entry_set(&msrs[n++],
>> HV_X64_MSR_APIC_ASSIST_PAGE, 0);          } -       
>> kvm_msr_entry_set(&msrs[n++], MSR_IA32_FEATURE_CONTROL,
>> env->msr_ia32_feature_control); +        if
>> (has_msr_feature_control) { +           
>>      kvm_msr_entry_set(&msrs[n++], MSR_IA32_FEATURE_CONTROL, +      
>>      env->msr_ia32_feature_control); +        } } if (env->mcg_cap) {
>>          int i;
>> @@ -1346,7 +1356,9 @@ static int kvm_get_msrs(X86CPU *cpu)
>>      if (has_msr_misc_enable) {
>>          msrs[n++].index = MSR_IA32_MISC_ENABLE;
>>      }
>> -    msrs[n++].index = MSR_IA32_FEATURE_CONTROL;
>> +    if (has_msr_feature_control) {
>> +        msrs[n++].index = MSR_IA32_FEATURE_CONTROL; +    }
>> 
>>      if (!env->tsc_valid) {
>>          msrs[n++].index = MSR_IA32_TSC;
>> @@ -1447,6 +1459,7 @@ static int kvm_get_msrs(X86CPU *cpu)          
>>          break; case MSR_IA32_FEATURE_CONTROL:
>>              env->msr_ia32_feature_control = msrs[i].data;
>> +            break;
> 
> Was the fallthrough previously intended? Or is this a second,
> unmentioned bugfix?

Hmm, it just add 'break' I think patch 0779caeb1a17f4d3ed14e2925b36ba09b084fb7b forget.

Thanks,
Jinsong

> 
> Regards,
> Andreas
> 
>>          default:
>>              if (msrs[i].index >= MSR_MC0_CTL &&
>>                  msrs[i].index < MSR_MC0_CTL + (env->mcg_cap & 0xff)
>> * 4) { 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Liu, Jinsong Aug. 20, 2013, 6:10 a.m. UTC | #4 
Paolo Bonzini wrote:
> Il 19/08/2013 16:59, Andreas Färber ha scritto:
>> qemu-kvm is no longer maintained since 1.3 so it should not be
>> occurring any more. 
>> 
>> Please use a prefix of "target-i386: " (the directory name) to signal
>> where you are changing code, i.e. x86 only.
>> 
>> "bugfix" is not a very telling description of what a patch is doing.
>> 
>> (Up to Paolo and Gleb whether they'll fix it or whether they require
>> a resend.)
> 
> No, not this time at least. :)
> 
> Paolo

Thanks Paolo, and Andreas's comments is also good, so I update commit message and will send out later.

Regards,
Jinsong

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paolo Bonzini Aug. 20, 2013, 4:43 p.m. UTC | #5 
Il 20/08/2013 05:33, Liu, Jinsong ha scritto:
> Thanks Andreas!
> 
> This patch is for qemu-kvm.

Even though the repository is still called qemu-kvm, the uq/master
branch is the only active one and patches there will end up in upstream
QEMU.

There are no qemu-kvm releases anymore.

I applied the patch to uq/master, thanks.

Paolo

> Per my understanding, there are some patches firstly checked in qemu-kvm uq/master branch.
> This patch is to fix c/s 0779caeb1a17f4d3ed14e2925b36ba09b084fb7b of qemu-kvm uq/master branch
> (which is to co-work w/ kvm IA32_FEATURE_CONTROL, and currently not yet in upstream qemu).
> 
> This patch is used to fix the bug introduced by 0779caeb1a17f4d3ed14e2925b36ba09b084fb7b of qemu-kvm uq/master branch. The bug is reported as
> https://bugs.launchpad.net/qemu-kvm/+bug/1207623
> https://bugs.launchpad.net/qemu/+bug/1213797
> 
> Anything I misunderstand, for upstream qemu and qemu-kvm?

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

===============
From a0ddf948d40e42de862543157a5668a1c12faae6 Mon Sep 17 00:00:00 2001
From: Liu Jinsong <jinsong.liu@intel.com>
Date: Mon, 19 Aug 2013 09:33:30 +0800
Subject: [PATCH] qemu-kvm bugfix for IA32_FEATURE_CONTROL

This patch is to fix the bug https://bugs.launchpad.net/qemu-kvm/+bug/1207623

IA32_FEATURE_CONTROL is pointless if not expose VMX or SMX bits to
cpuid.1.ecx of vcpu. Current qemu-kvm will error return when kvm_put_msrs
or kvm_get_msrs.

Signed-off-by: Liu Jinsong <jinsong.liu@intel.com>
---
 target-i386/kvm.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 84ac00a..5adeb03 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -65,6 +65,7 @@  static bool has_msr_star;
 static bool has_msr_hsave_pa;
 static bool has_msr_tsc_adjust;
 static bool has_msr_tsc_deadline;
+static bool has_msr_feature_control;
 static bool has_msr_async_pf_en;
 static bool has_msr_pv_eoi_en;
 static bool has_msr_misc_enable;
@@ -644,6 +645,12 @@  int kvm_arch_init_vcpu(CPUState *cs)
 
     qemu_add_vm_change_state_handler(cpu_update_state, env);
 
+    c = cpuid_find_entry(&cpuid_data.cpuid, 1, 0);
+    if (c) {
+        has_msr_feature_control = !!(c->ecx & CPUID_EXT_VMX) ||
+                                  !!(c->ecx & CPUID_EXT_SMX);
+    }
+
     cpuid_data.cpuid.padding = 0;
     r = kvm_vcpu_ioctl(cs, KVM_SET_CPUID2, &cpuid_data);
     if (r) {
@@ -1121,7 +1128,10 @@  static int kvm_put_msrs(X86CPU *cpu, int level)
         if (hyperv_vapic_recommended()) {
             kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_APIC_ASSIST_PAGE, 0);
         }
-        kvm_msr_entry_set(&msrs[n++], MSR_IA32_FEATURE_CONTROL, env->msr_ia32_feature_control);
+        if (has_msr_feature_control) {
+            kvm_msr_entry_set(&msrs[n++], MSR_IA32_FEATURE_CONTROL,
+                              env->msr_ia32_feature_control);
+        }
     }
     if (env->mcg_cap) {
         int i;
@@ -1346,7 +1356,9 @@  static int kvm_get_msrs(X86CPU *cpu)
     if (has_msr_misc_enable) {
         msrs[n++].index = MSR_IA32_MISC_ENABLE;
     }
-    msrs[n++].index = MSR_IA32_FEATURE_CONTROL;
+    if (has_msr_feature_control) {
+        msrs[n++].index = MSR_IA32_FEATURE_CONTROL;
+    }
 
     if (!env->tsc_valid) {
         msrs[n++].index = MSR_IA32_TSC;
@@ -1447,6 +1459,7 @@  static int kvm_get_msrs(X86CPU *cpu)
             break;
         case MSR_IA32_FEATURE_CONTROL:
             env->msr_ia32_feature_control = msrs[i].data;
+            break;
         default:
             if (msrs[i].index >= MSR_MC0_CTL &&
                 msrs[i].index < MSR_MC0_CTL + (env->mcg_cap & 0xff) * 4) {