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

["Andreas Färber" <afaerber@suse.de>]

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


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg