Re: [PATCH] target-i386: check vcpu features before accessing MSR_TSC_AUX

[Paolo Bonzini]

On 14/12/2015 12:07, Haozhong Zhang wrote:
> This patch fix a bug that prevents VM rebooting on recent versions of
> KVM (from commit 9dbe6cf).
> 
> kvm_get_msrs() is called to save guest MSR_TSC_AUX and other MSRs across
> rebooting. It only checks whether KVM exposes MSR_TSC_AUX to userspace.
> However, if vcpu does not support rdtscp (e.g. kvm64), current KVM will
> fail the saving and thus all other MSRs following it will fail in
> kvm_get_msrs(). As a result, from KVM commit 9dbe6cf that exposes
> MSR_TSC_AUX, VM can not successfully reboot.
> 
> This patch fixes this bug by adding the missing rdtscp feature checks.

That commit is not in any released kernel.  It's better if we just check
msr_info->host_initiated in vmx_get_msr and vmx_set_msr. Can you prepare
a patch?

Thanks,

Paolo

> Signed-off-by: Haozhong Zhang 
> ---
>  target-i386/kvm.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 6dc9846..cc842c6 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -1414,7 +1414,8 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
>  if (has_msr_hsave_pa) {
>  kvm_msr_entry_set([n++], MSR_VM_HSAVE_PA, env->vm_hsave);
>  }
> -if (has_msr_tsc_aux) {
> +if (has_msr_tsc_aux &&
> +(env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_RDTSCP)) {
>  kvm_msr_entry_set([n++], MSR_TSC_AUX, env->tsc_aux);
>  }
>  if (has_msr_tsc_adjust) {
> @@ -1793,7 +1794,8 @@ static int kvm_get_msrs(X86CPU *cpu)
>  if (has_msr_hsave_pa) {
>  msrs[n++].index = MSR_VM_HSAVE_PA;
>  }
> -if (has_msr_tsc_aux) {
> +if (has_msr_tsc_aux &&
> +(env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_RDTSCP)) {
>  msrs[n++].index = MSR_TSC_AUX;
>  }
>  if (has_msr_tsc_adjust) {
> 

This commit is not in any released kernel.  It's better if we just check
msr_info->host_initiated in vmx_get_msr and vmx_set_msr.  Can you
prepare a patch?

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

[PATCH] target-i386: check vcpu features before accessing MSR_TSC_AUX

[Haozhong Zhang]

This patch fix a bug that prevents VM rebooting on recent versions of
KVM (from commit 9dbe6cf).

kvm_get_msrs() is called to save guest MSR_TSC_AUX and other MSRs across
rebooting. It only checks whether KVM exposes MSR_TSC_AUX to userspace.
However, if vcpu does not support rdtscp (e.g. kvm64), current KVM will
fail the saving and thus all other MSRs following it will fail in
kvm_get_msrs(). As a result, from KVM commit 9dbe6cf that exposes
MSR_TSC_AUX, VM can not successfully reboot.

This patch fixes this bug by adding the missing rdtscp feature checks.

Signed-off-by: Haozhong Zhang 
---
 target-i386/kvm.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 6dc9846..cc842c6 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1414,7 +1414,8 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
 if (has_msr_hsave_pa) {
 kvm_msr_entry_set([n++], MSR_VM_HSAVE_PA, env->vm_hsave);
 }
-if (has_msr_tsc_aux) {
+if (has_msr_tsc_aux &&
+(env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_RDTSCP)) {
 kvm_msr_entry_set([n++], MSR_TSC_AUX, env->tsc_aux);
 }
 if (has_msr_tsc_adjust) {
@@ -1793,7 +1794,8 @@ static int kvm_get_msrs(X86CPU *cpu)
 if (has_msr_hsave_pa) {
 msrs[n++].index = MSR_VM_HSAVE_PA;
 }
-if (has_msr_tsc_aux) {
+if (has_msr_tsc_aux &&
+(env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_RDTSCP)) {
 msrs[n++].index = MSR_TSC_AUX;
 }
 if (has_msr_tsc_adjust) {
-- 
2.4.8

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

Re: [PATCH] target-i386: check vcpu features before accessing MSR_TSC_AUX

[Haozhong Zhang]

On 12/14/15 12:51, Paolo Bonzini wrote:
> 
> 
> On 14/12/2015 12:07, Haozhong Zhang wrote:
> > This patch fix a bug that prevents VM rebooting on recent versions of
> > KVM (from commit 9dbe6cf).
> > 
> > kvm_get_msrs() is called to save guest MSR_TSC_AUX and other MSRs across
> > rebooting. It only checks whether KVM exposes MSR_TSC_AUX to userspace.
> > However, if vcpu does not support rdtscp (e.g. kvm64), current KVM will
> > fail the saving and thus all other MSRs following it will fail in
> > kvm_get_msrs(). As a result, from KVM commit 9dbe6cf that exposes
> > MSR_TSC_AUX, VM can not successfully reboot.
> > 
> > This patch fixes this bug by adding the missing rdtscp feature checks.
> 
> That commit is not in any released kernel.

Right, it's currently only in kvm next. But I assume it would finally come
into a released kernel.

> It's better if we just check
> msr_info->host_initiated in vmx_get_msr and vmx_set_msr. Can you prepare
> a patch?
>

Yes, I'll send a KVM patch later. And then this QEMU patch is not
needed any more.

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

Re: [PATCH] target-i386: check vcpu features before accessing MSR_TSC_AUX

[Paolo Bonzini]

On 14/12/2015 14:54, Haozhong Zhang wrote:
>> > That commit is not in any released kernel.
> Right, it's currently only in kvm next. But I assume it would finally come
> into a released kernel.

Yes, but until it is, it's easier (and better) to fix KVM instead of QEMU.

> > It's better if we just check
> > msr_info->host_initiated in vmx_get_msr and vmx_set_msr. Can you prepare
> > a patch?
>
> Yes, I'll send a KVM patch later. And then this QEMU patch is not
> needed any more.

Great, thanks.

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