Re: [Qemu-devel] [PATCH] qemu-kvm bugfix for IA32_FEATURE_CONTROL

[Liu, Jinsong]

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 <address@hidden>
>> 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 <address@hidden>
> 
> 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) {