Re: [PATCH] Enabling IA32_TSC_ADJUST for guest VM

[Avi Kivity <avi@redhat.com>]

On 09/19/2012 08:44 PM, Auld, Will wrote:
> From 9982bb73460b05c1328068aae047b14b2294e2da Mon Sep 17 00:00:00 2001
> From: Will Auld <will.auld@intel.com>
> Date: Wed, 12 Sep 2012 18:10:56 -0700
> Subject: [PATCH] Enabling IA32_TSC_ADJUST for guest VM
> 
> CPUID.7.0.EBX[1]=1 indicates IA32_TSC_ADJUST MSR 0x3b is supported
> 
> Basic design is to emulate the MSR by allowing reads and writes to a guest vcpu specific location to store the value of the emulated MSR while adding the value to the vmcs tsc_offset. In this way the IA32_TSC_ADJUST value will be included in all reads to the TSC MSR whether through rdmsr or rdtsc. This is of course as long as the "use TSC counter offsetting" VM-execution control is enabled as well as the IA32_TSC_ADJUST control.
> 
> However, because hardware will only return the TSC + IA32_TSC_ADJUST + vmsc tsc_offset for a guest process when it does and rdtsc (with the correct settings) the value of our virtualized IA32_TSC_ADJUST must be stored in one of these three locations. The argument against storing it in the actual MSR is performance. This is likely to be seldom used while the save/restore is required on every transition. IA32_TSC_ADJUST was created as a way to solve some issues with writing TSC itself so that is not an option either. The remaining option, defined above as our solution has the problem of returning incorrect vmcs tsc_offset values (unless we intercept and fix, not done here) as mentioned above. However, more problematic is that storing the data in vmcs tsc_offset will have a different semant
 ic effect on the system than does using the actual MSR. This is illustrated in the following example: The hypervisor set the IA32_TSC_ADJUST, then the guest sets it and a guest process perf!
 or!
>  ms a rdtsc. In this case the guest process will get TSC + IA32_TSC_ADJUST_hyperviser + vmsc tsc_offset including IA32_TSC_ADJUST_guest. While the total system semantics changed the semantics as seen by the guest do not and hence this will not cause a problem.
> +++ b/arch/x86/kvm/cpuid.c
> @@ -248,8 +248,8 @@ static int do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  
>  	/* cpuid 7.0.ebx */
>  	const u32 kvm_supported_word9_x86_features =
> -		F(FSGSBASE) | F(BMI1) | F(HLE) | F(AVX2) | F(SMEP) |
> -		F(BMI2) | F(ERMS) | f_invpcid | F(RTM);
> +		F(FSGSBASE) | F(TSC_ADJUST) | F(BMI1) | F(HLE) |
> +		F(AVX2) | F(SMEP) | F(BMI2) | F(ERMS) | f_invpcid | F(RTM);
>  

You're exposing this feature unconditionally, but part of the
implementation is in vmx.c.  This means that if an AMD processor arrives
that implements the feature, we will expose the feature even though we
lack some of the implementation.

So we need to mask the feature here based on a callback from kvm_x86_ops.

>  	/* all calls to cpuid_count() should be made on the same cpu */
>  	get_cpu();
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c00f03d..35d11b3 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2173,6 +2173,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
>  	case MSR_IA32_SYSENTER_ESP:
>  		data = vmcs_readl(GUEST_SYSENTER_ESP);
>  		break;
> +	case MSR_TSC_ADJUST:
> +		data = (u64)vcpu->arch.tsc_adjust;
> +		break;

Can be moved to common code.

>  	case MSR_TSC_AUX:
>  		if (!to_vmx(vcpu)->rdtscp_enabled)
>  			return 1;
> @@ -2241,6 +2244,13 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
>  		}
>  		ret = kvm_set_msr_common(vcpu, msr_index, data);
>  		break;
> +	case MSR_TSC_ADJUST:
> +#define DUMMY 1

What is this?

> +		vmx_adjust_tsc_offset(vcpu,
> +				(s64)(data-vcpu->arch.tsc_adjust),

Cast unneeded; space between operands please.

> +				(bool)DUMMY);
> +		vcpu->arch.tsc_adjust = (s64)data;

Cast is unneeded.

> +		break;
>  	case MSR_TSC_AUX:
>  		if (!vmx->rdtscp_enabled)
>  			return 1;
> @@ -3931,6 +3941,8 @@ static int vmx_vcpu_reset(struct kvm_vcpu *vcpu)
>  
>  	vcpu->arch.regs_avail = ~((1 << VCPU_REGS_RIP) | (1 << VCPU_REGS_RSP));
>  
> +	vcpu->arch.tsc_adjust = 0x0;
> +

Can be moved to common code.

>  	vmx->rmode.vm86_active = 0;
>  

-- 
error compiling committee.c: too many arguments to function