[PATCH] KVM: svm: add support for RDTSCP

[PATCH] KVM: svm: add support for RDTSCP

[Paolo Bonzini]

RDTSCP was never supported for AMD CPUs, which nobody noticed because
Linux does not use it.  But exactly the fact that Linux does not
use it makes the implementation very simple; we can freely trash
MSR_TSC_AUX while running the guest.

Cc: Joerg Roedel <joro@8bytes.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/svm.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 83a1c64..c302614 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -86,6 +86,7 @@ static const u32 host_save_user_msrs[] = {
 	MSR_FS_BASE,
 #endif
 	MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP,
+	MSR_TSC_AUX,
 };
 
 #define NR_HOST_SAVE_USER_MSRS ARRAY_SIZE(host_save_user_msrs)
@@ -135,6 +136,7 @@ struct vcpu_svm {
 	uint64_t asid_generation;
 	uint64_t sysenter_esp;
 	uint64_t sysenter_eip;
+	uint64_t tsc_aux;
 
 	u64 next_rip;
 
@@ -1238,6 +1240,9 @@ static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 			wrmsrl(MSR_AMD64_TSC_RATIO, tsc_ratio);
 		}
 	}
+	/* This assumes that the kernel never uses MSR_TSC_AUX */
+	if (static_cpu_has(X86_FEATURE_RDTSCP))
+		wrmsrl(MSR_TSC_AUX, svm->tsc_aux);
 }
 
 static void svm_vcpu_put(struct kvm_vcpu *vcpu)
@@ -3024,6 +3029,11 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_SYSENTER_ESP:
 		msr_info->data = svm->sysenter_esp;
 		break;
+	case MSR_TSC_AUX:
+		if (!boot_cpu_has(X86_FEATURE_RDTSCP))
+			return 1;
+		msr_info->data = svm->tsc_aux;
+		break;
 	/*
 	 * Nobody will change the following 5 values in the VMCB so we can
 	 * safely return them on rdmsr. They will always be 0 until LBRV is
@@ -3145,6 +3155,18 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 		svm->sysenter_esp = data;
 		svm->vmcb->save.sysenter_esp = data;
 		break;
+	case MSR_TSC_AUX:
+		if (!boot_cpu_has(X86_FEATURE_RDTSCP))
+			return 1;
+
+		/*
+		 * This is rare, so we update the MSR here instead of using
+		 * direct_access_msrs.  Doing that would require a rdmsr in
+		 * svm_vcpu_put.
+		 */
+		svm->tsc_aux = data;
+		wrmsrl(MSR_TSC_AUX, svm->tsc_aux);
+		break;
 	case MSR_IA32_DEBUGCTLMSR:
 		if (!boot_cpu_has(X86_FEATURE_LBRV)) {
 			vcpu_unimpl(vcpu, "%s: MSR_IA32_DEBUGCTL 0x%llx, nop\n",
@@ -4037,7 +4059,7 @@ static int svm_get_lpage_level(void)
 
 static bool svm_rdtscp_supported(void)
 {
-	return false;
+	return boot_cpu_has(X86_FEATURE_RDTSCP);
 }
 
 static bool svm_invpcid_supported(void)
-- 
2.4.3

[PATCH] KVM: x86: expose MSR_TSC_AUX to userspace

[Paolo Bonzini]

If we do not do this, it is not properly saved and restored across
migration.  Windows notices due to its self-protection mechanisms,
and is very upset about it (blue screen of death).

Cc: Radim Krcmar <rkrcmar@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index aba7f95..377160d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -950,7 +950,7 @@ static u32 msrs_to_save[] = {
 	MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
 #endif
 	MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
-	MSR_IA32_FEATURE_CONTROL, MSR_IA32_BNDCFGS
+	MSR_IA32_FEATURE_CONTROL, MSR_IA32_BNDCFGS, MSR_TSC_AUX,
 };
 
 static unsigned num_msrs_to_save;
@@ -3979,16 +3979,17 @@ static void kvm_init_msr_list(void)
 
 		/*
 		 * Even MSRs that are valid in the host may not be exposed
-		 * to the guests in some cases.  We could work around this
-		 * in VMX with the generic MSR save/load machinery, but it
-		 * is not really worthwhile since it will really only
-		 * happen with nested virtualization.
+		 * to the guests in some cases.
 		 */
 		switch (msrs_to_save[i]) {
 		case MSR_IA32_BNDCFGS:
 			if (!kvm_x86_ops->mpx_supported())
 				continue;
 			break;
+		case MSR_TSC_AUX:
+			if (!kvm_x86_ops->rdtscp_supported())
+				continue;
+			break;
 		default:
 			break;
 		}
-- 
2.4.3

Re: [PATCH] KVM: svm: add support for RDTSCP

[Joerg Roedel]

Hi Paolo,

On Thu, Nov 12, 2015 at 02:49:16PM +0100, Paolo Bonzini wrote:
> RDTSCP was never supported for AMD CPUs, which nobody noticed because
> Linux does not use it.  But exactly the fact that Linux does not
> use it makes the implementation very simple; we can freely trash
> MSR_TSC_AUX while running the guest.
> 
> Cc: Joerg Roedel <joro@8bytes.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/svm.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 83a1c64..c302614 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -86,6 +86,7 @@ static const u32 host_save_user_msrs[] = {
>  	MSR_FS_BASE,
>  #endif
>  	MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP,
> +	MSR_TSC_AUX,
>  };
>  
>  #define NR_HOST_SAVE_USER_MSRS ARRAY_SIZE(host_save_user_msrs)
> @@ -135,6 +136,7 @@ struct vcpu_svm {
>  	uint64_t asid_generation;
>  	uint64_t sysenter_esp;
>  	uint64_t sysenter_eip;
> +	uint64_t tsc_aux;
>  
>  	u64 next_rip;
>  
> @@ -1238,6 +1240,9 @@ static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  			wrmsrl(MSR_AMD64_TSC_RATIO, tsc_ratio);
>  		}
>  	}
> +	/* This assumes that the kernel never uses MSR_TSC_AUX */
> +	if (static_cpu_has(X86_FEATURE_RDTSCP))
> +		wrmsrl(MSR_TSC_AUX, svm->tsc_aux);
>  }

Hmm, you seem to still intercept MSR_TSC_AUX, is that intentional?
Loading the guests value into the real cpu msr only makes sense to me
when the MSR is no longer intercepted.


	Joerg

Re: [PATCH] KVM: svm: add support for RDTSCP

[Paolo Bonzini]

On 12/11/2015 15:45, Joerg Roedel wrote:
> Hi Paolo,
> 
> On Thu, Nov 12, 2015 at 02:49:16PM +0100, Paolo Bonzini wrote:
>> RDTSCP was never supported for AMD CPUs, which nobody noticed because
>> Linux does not use it.  But exactly the fact that Linux does not
>> use it makes the implementation very simple; we can freely trash
>> MSR_TSC_AUX while running the guest.
>>
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  arch/x86/kvm/svm.c | 24 +++++++++++++++++++++++-
>>  1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 83a1c64..c302614 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -86,6 +86,7 @@ static const u32 host_save_user_msrs[] = {
>>  	MSR_FS_BASE,
>>  #endif
>>  	MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP,
>> +	MSR_TSC_AUX,
>>  };
>>  
>>  #define NR_HOST_SAVE_USER_MSRS ARRAY_SIZE(host_save_user_msrs)
>> @@ -135,6 +136,7 @@ struct vcpu_svm {
>>  	uint64_t asid_generation;
>>  	uint64_t sysenter_esp;
>>  	uint64_t sysenter_eip;
>> +	uint64_t tsc_aux;
>>  
>>  	u64 next_rip;
>>  
>> @@ -1238,6 +1240,9 @@ static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>  			wrmsrl(MSR_AMD64_TSC_RATIO, tsc_ratio);
>>  		}
>>  	}
>> +	/* This assumes that the kernel never uses MSR_TSC_AUX */
>> +	if (static_cpu_has(X86_FEATURE_RDTSCP))
>> +		wrmsrl(MSR_TSC_AUX, svm->tsc_aux);
>>  }
> 
> Hmm, you seem to still intercept MSR_TSC_AUX, is that intentional?

Yes.  If I didn't intercept MSR_TSC_AUX, I would have to read it into
svm->tsc_aux on every svm_vcpu_put.  Because writing MSR_TSC_AUX is a
rare operation, I intercept the write, and update both svm->tsc_aux and
the processor MSR_TSC_AUX in svm_set_msr.

This is different from other host_save_msrs because the processor does
not save MSR_TSC_AUX automatically in the VMCB.  As remarked above, it
only works because the kernel never uses RDTSCP.

> Loading the guests value into the real cpu msr only makes sense to me
> when the MSR is no longer intercepted.

It's necessary for the correct operation of RDTSCP.  Since we don't
intercept RDTSCP, we need to load MSR_TSC_AUX.

Paolo

Re: [PATCH] KVM: x86: expose MSR_TSC_AUX to userspace

[Paolo Bonzini]

> Paolo, under what circumstances (which versions of Windows? Anything
> special running in the guest?) has this failure happened? I'd like to repro
> this, I'm not sure if we've observed it before.

We saw it with migration under Windows 10, nothing special running in the
guest.  It's very hard to reproduce, we've only seen it once but the BSOD
parameters provided surprisingly good evidence:

----------------------------------
CRITICAL_STRUCTURE_CORRUPTION (109)
This bugcheck is generated when the kernel detects that critical kernel code or
data have been corrupted. There are generally three causes for a corruption:
1) A driver has inadvertently or deliberately modified critical kernel code
 or data. See http://www.microsoft.com/whdc/driver/kernel/64bitPatching.mspx
2) A developer attempted to set a normal kernel breakpoint using a kernel
 debugger that was not attached when the system was booted. Normal breakpoints,
 "bp", can only be set if the debugger is attached at boot time. Hardware
 breakpoints, "ba", can be set at any time.
3) A hardware corruption occurred, e.g. failing RAM holding kernel code or data.
Arguments:
Arg1: a3a01f58a88e3638, Reserved
Arg2: b3b72bdefb0f076f, Reserved
Arg3: 00000001c0000103, Failure type dependent information
Arg4: 0000000000000007, Type of corrupted region, can be
	...
	7   : Critical MSR modification
----------------------------------

Argument 1 and 2 might be related to the old and new value (perhaps some
kind of hash).

Argument 3 is not documented either, but the low 32 bits look a lot like
the MSR_TSC_AUX index. :)

Paolo

Re: [PATCH] KVM: x86: expose MSR_TSC_AUX to userspace

[Peter Hornyack]

On Fri, Nov 13, 2015 at 1:42 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>> Paolo, under what circumstances (which versions of Windows? Anything
>> special running in the guest?) has this failure happened? I'd like to repro
>> this, I'm not sure if we've observed it before.
>
> We saw it with migration under Windows 10, nothing special running in the
> guest.  It's very hard to reproduce, we've only seen it once but the BSOD
> parameters provided surprisingly good evidence:

Great, thanks for that information and for the patch. I'll let you
know if I successfully reproduce the issue here.

> ----------------------------------
> CRITICAL_STRUCTURE_CORRUPTION (109)
> This bugcheck is generated when the kernel detects that critical kernel code or
> data have been corrupted. There are generally three causes for a corruption:
> 1) A driver has inadvertently or deliberately modified critical kernel code
>  or data. See http://www.microsoft.com/whdc/driver/kernel/64bitPatching.mspx
> 2) A developer attempted to set a normal kernel breakpoint using a kernel
>  debugger that was not attached when the system was booted. Normal breakpoints,
>  "bp", can only be set if the debugger is attached at boot time. Hardware
>  breakpoints, "ba", can be set at any time.
> 3) A hardware corruption occurred, e.g. failing RAM holding kernel code or data.
> Arguments:
> Arg1: a3a01f58a88e3638, Reserved
> Arg2: b3b72bdefb0f076f, Reserved
> Arg3: 00000001c0000103, Failure type dependent information
> Arg4: 0000000000000007, Type of corrupted region, can be
>         ...
>         7   : Critical MSR modification
> ----------------------------------
>
> Argument 1 and 2 might be related to the old and new value (perhaps some
> kind of hash).
>
> Argument 3 is not documented either, but the low 32 bits look a lot like
> the MSR_TSC_AUX index. :)
>
> Paolo

Peter