diff mbox series

[3/4] KVM: x86: hyper-v: Track Hyper-V TSC page status

Message ID 20210315143706.859293-4-vkuznets@redhat.com (mailing list archive)
State New, archived
Headers show
Series KVM: x86: hyper-v: TSC page fixes | expand

Commit Message

Vitaly Kuznetsov March 15, 2021, 2:37 p.m. UTC 
Create an infrastructure for tracking Hyper-V TSC page status, i.e. if it
was updated from guest/host side or if we've failed to set it up (because
e.g. guest wrote some garbage to HV_X64_MSR_REFERENCE_TSC) and there's no
need to retry.

Also, in a hypothetical situation when we are in 'always catchup' mode for
TSC we can now avoid contending 'hv->hv_lock' on every guest enter by
setting the state to HV_TSC_PAGE_BROKEN after compute_tsc_page_parameters()
returns false.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  9 +++++++++
 arch/x86/kvm/hyperv.c           | 23 +++++++++++++++++------
 2 files changed, 26 insertions(+), 6 deletions(-)

Comments

Sean Christopherson March 15, 2021, 3:15 p.m. UTC | #1 
On Mon, Mar 15, 2021, Vitaly Kuznetsov wrote:
> Create an infrastructure for tracking Hyper-V TSC page status, i.e. if it
> was updated from guest/host side or if we've failed to set it up (because
> e.g. guest wrote some garbage to HV_X64_MSR_REFERENCE_TSC) and there's no
> need to retry.
> 
> Also, in a hypothetical situation when we are in 'always catchup' mode for
> TSC we can now avoid contending 'hv->hv_lock' on every guest enter by
> setting the state to HV_TSC_PAGE_BROKEN after compute_tsc_page_parameters()
> returns false.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index eefb85b86fe8..2a8d078b16cb 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -1087,7 +1087,7 @@ void kvm_hv_setup_tsc_page(struct kvm *kvm,
>  	BUILD_BUG_ON(sizeof(tsc_seq) != sizeof(hv->tsc_ref.tsc_sequence));
>  	BUILD_BUG_ON(offsetof(struct ms_hyperv_tsc_page, tsc_sequence) != 0);
>  
> -	if (!(hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE))
> +	if (hv->hv_tsc_page_status == HV_TSC_PAGE_BROKEN)
>  		return;
>  
>  	mutex_lock(&hv->hv_lock);

...

> @@ -1133,6 +1133,12 @@ void kvm_hv_setup_tsc_page(struct kvm *kvm,
>  	hv->tsc_ref.tsc_sequence = tsc_seq;
>  	kvm_write_guest(kvm, gfn_to_gpa(gfn),
>  			&hv->tsc_ref, sizeof(hv->tsc_ref.tsc_sequence));
> +
> +	hv->hv_tsc_page_status = HV_TSC_PAGE_SET;
> +	goto out_unlock;
> +
> +out_err:
> +	hv->hv_tsc_page_status = HV_TSC_PAGE_BROKEN;
>  out_unlock:
>  	mutex_unlock(&hv->hv_lock);
>  }
> @@ -1193,8 +1199,13 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data,
>  	}
>  	case HV_X64_MSR_REFERENCE_TSC:
>  		hv->hv_tsc_page = data;
> -		if (hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE)
> +		if (hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE) {
> +			if (!host)
> +				hv->hv_tsc_page_status = HV_TSC_PAGE_GUEST_CHANGED;
> +			else
> +				hv->hv_tsc_page_status = HV_TSC_PAGE_HOST_CHANGED;

Writing the status without taking hv->hv_lock could cause the update to be lost,
e.g. if a different vCPU fails kvm_hv_setup_tsc_page() at the same time, its
write to set status to HV_TSC_PAGE_BROKEN would race with this write.

>  			kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
> +		}
>  		break;
>  	case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
>  		return kvm_hv_msr_set_crash_data(kvm,
> -- 
> 2.30.2
>
Vitaly Kuznetsov March 15, 2021, 3:34 p.m. UTC | #2 
Sean Christopherson <seanjc@google.com> writes:

> On Mon, Mar 15, 2021, Vitaly Kuznetsov wrote:
>> Create an infrastructure for tracking Hyper-V TSC page status, i.e. if it
>> was updated from guest/host side or if we've failed to set it up (because
>> e.g. guest wrote some garbage to HV_X64_MSR_REFERENCE_TSC) and there's no
>> need to retry.
>> 
>> Also, in a hypothetical situation when we are in 'always catchup' mode for
>> TSC we can now avoid contending 'hv->hv_lock' on every guest enter by
>> setting the state to HV_TSC_PAGE_BROKEN after compute_tsc_page_parameters()
>> returns false.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>> index eefb85b86fe8..2a8d078b16cb 100644
>> --- a/arch/x86/kvm/hyperv.c
>> +++ b/arch/x86/kvm/hyperv.c
>> @@ -1087,7 +1087,7 @@ void kvm_hv_setup_tsc_page(struct kvm *kvm,
>>  	BUILD_BUG_ON(sizeof(tsc_seq) != sizeof(hv->tsc_ref.tsc_sequence));
>>  	BUILD_BUG_ON(offsetof(struct ms_hyperv_tsc_page, tsc_sequence) != 0);
>>  
>> -	if (!(hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE))
>> +	if (hv->hv_tsc_page_status == HV_TSC_PAGE_BROKEN)
>>  		return;
>>  
>>  	mutex_lock(&hv->hv_lock);
>
> ...
>
>> @@ -1133,6 +1133,12 @@ void kvm_hv_setup_tsc_page(struct kvm *kvm,
>>  	hv->tsc_ref.tsc_sequence = tsc_seq;
>>  	kvm_write_guest(kvm, gfn_to_gpa(gfn),
>>  			&hv->tsc_ref, sizeof(hv->tsc_ref.tsc_sequence));
>> +
>> +	hv->hv_tsc_page_status = HV_TSC_PAGE_SET;
>> +	goto out_unlock;
>> +
>> +out_err:
>> +	hv->hv_tsc_page_status = HV_TSC_PAGE_BROKEN;
>>  out_unlock:
>>  	mutex_unlock(&hv->hv_lock);
>>  }
>> @@ -1193,8 +1199,13 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data,
>>  	}
>>  	case HV_X64_MSR_REFERENCE_TSC:
>>  		hv->hv_tsc_page = data;
>> -		if (hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE)
>> +		if (hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE) {
>> +			if (!host)
>> +				hv->hv_tsc_page_status = HV_TSC_PAGE_GUEST_CHANGED;
>> +			else
>> +				hv->hv_tsc_page_status = HV_TSC_PAGE_HOST_CHANGED;
>
> Writing the status without taking hv->hv_lock could cause the update to be lost,
> e.g. if a different vCPU fails kvm_hv_setup_tsc_page() at the same time, its
> write to set status to HV_TSC_PAGE_BROKEN would race with this write.
>

Oh, right you are, the lock was somewhere in my brain :-) Will do in v2.

>>  			kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
>> +		}
>>  		break;
>>  	case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
>>  		return kvm_hv_msr_set_crash_data(kvm,
>> -- 
>> 2.30.2
>> 
>
Vitaly Kuznetsov March 16, 2021, 12:24 p.m. UTC | #3 
Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> Sean Christopherson <seanjc@google.com> writes:
>
>> On Mon, Mar 15, 2021, Vitaly Kuznetsov wrote:
>>> Create an infrastructure for tracking Hyper-V TSC page status, i.e. if it
>>> was updated from guest/host side or if we've failed to set it up (because
>>> e.g. guest wrote some garbage to HV_X64_MSR_REFERENCE_TSC) and there's no
>>> need to retry.
>>> 
>>> Also, in a hypothetical situation when we are in 'always catchup' mode for
>>> TSC we can now avoid contending 'hv->hv_lock' on every guest enter by
>>> setting the state to HV_TSC_PAGE_BROKEN after compute_tsc_page_parameters()
>>> returns false.
>>> 
>>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>>> ---
>>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>>> index eefb85b86fe8..2a8d078b16cb 100644
>>> --- a/arch/x86/kvm/hyperv.c
>>> +++ b/arch/x86/kvm/hyperv.c
>>> @@ -1087,7 +1087,7 @@ void kvm_hv_setup_tsc_page(struct kvm *kvm,
>>>  	BUILD_BUG_ON(sizeof(tsc_seq) != sizeof(hv->tsc_ref.tsc_sequence));
>>>  	BUILD_BUG_ON(offsetof(struct ms_hyperv_tsc_page, tsc_sequence) != 0);
>>>  
>>> -	if (!(hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE))
>>> +	if (hv->hv_tsc_page_status == HV_TSC_PAGE_BROKEN)
>>>  		return;
>>>  
>>>  	mutex_lock(&hv->hv_lock);
>>
>> ...
>>
>>> @@ -1133,6 +1133,12 @@ void kvm_hv_setup_tsc_page(struct kvm *kvm,
>>>  	hv->tsc_ref.tsc_sequence = tsc_seq;
>>>  	kvm_write_guest(kvm, gfn_to_gpa(gfn),
>>>  			&hv->tsc_ref, sizeof(hv->tsc_ref.tsc_sequence));
>>> +
>>> +	hv->hv_tsc_page_status = HV_TSC_PAGE_SET;
>>> +	goto out_unlock;
>>> +
>>> +out_err:
>>> +	hv->hv_tsc_page_status = HV_TSC_PAGE_BROKEN;
>>>  out_unlock:
>>>  	mutex_unlock(&hv->hv_lock);
>>>  }
>>> @@ -1193,8 +1199,13 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data,
>>>  	}
>>>  	case HV_X64_MSR_REFERENCE_TSC:
>>>  		hv->hv_tsc_page = data;
>>> -		if (hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE)
>>> +		if (hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE) {
>>> +			if (!host)
>>> +				hv->hv_tsc_page_status = HV_TSC_PAGE_GUEST_CHANGED;
>>> +			else
>>> +				hv->hv_tsc_page_status = HV_TSC_PAGE_HOST_CHANGED;
>>
>> Writing the status without taking hv->hv_lock could cause the update to be lost,
>> e.g. if a different vCPU fails kvm_hv_setup_tsc_page() at the same time, its
>> write to set status to HV_TSC_PAGE_BROKEN would race with this write.
>>
>
> Oh, right you are, the lock was somewhere in my brain :-) Will do in
> v2.

Actually no, kvm_hv_set_msr_pw() is only called from
kvm_hv_set_msr_common() with hv->hv_lock held so we're already
synchronized.

... and of course I figured that our by putting another
mutex_lock()/mutex_unlock() here and then wondering why everything hangs
:-)

>
>>>  			kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
>>> +		}
>>>  		break;
>>>  	case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
>>>  		return kvm_hv_msr_set_crash_data(kvm,
>>> -- 
>>> 2.30.2
>>> 
>>
Sean Christopherson March 16, 2021, 3:20 p.m. UTC | #4 
On Tue, Mar 16, 2021, Vitaly Kuznetsov wrote:
> Vitaly Kuznetsov <vkuznets@redhat.com> writes:
> 
> > Sean Christopherson <seanjc@google.com> writes:
> >
> >> On Mon, Mar 15, 2021, Vitaly Kuznetsov wrote:
> >>> @@ -1193,8 +1199,13 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data,
> >>>  	}
> >>>  	case HV_X64_MSR_REFERENCE_TSC:
> >>>  		hv->hv_tsc_page = data;
> >>> -		if (hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE)
> >>> +		if (hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE) {
> >>> +			if (!host)
> >>> +				hv->hv_tsc_page_status = HV_TSC_PAGE_GUEST_CHANGED;
> >>> +			else
> >>> +				hv->hv_tsc_page_status = HV_TSC_PAGE_HOST_CHANGED;
> >>
> >> Writing the status without taking hv->hv_lock could cause the update to be lost,
> >> e.g. if a different vCPU fails kvm_hv_setup_tsc_page() at the same time, its
> >> write to set status to HV_TSC_PAGE_BROKEN would race with this write.
> >>
> >
> > Oh, right you are, the lock was somewhere in my brain :-) Will do in
> > v2.
> 
> Actually no, kvm_hv_set_msr_pw() is only called from
> kvm_hv_set_msr_common() with hv->hv_lock held so we're already
> synchronized.
> 
> ... and of course I figured that our by putting another
> mutex_lock()/mutex_unlock() here and then wondering why everything hangs
> :-)

Doh, sorry :-/
diff mbox series

Patch

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 9bc091ecaaeb..87448e9e6b28 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -884,12 +884,21 @@  struct kvm_hv_syndbg {
 	u64 options;
 };
 
+enum hv_tsc_page_status {
+	HV_TSC_PAGE_UNSET = 0,
+	HV_TSC_PAGE_GUEST_CHANGED,
+	HV_TSC_PAGE_HOST_CHANGED,
+	HV_TSC_PAGE_SET,
+	HV_TSC_PAGE_BROKEN,
+};
+
 /* Hyper-V emulation context */
 struct kvm_hv {
 	struct mutex hv_lock;
 	u64 hv_guest_os_id;
 	u64 hv_hypercall;
 	u64 hv_tsc_page;
+	enum hv_tsc_page_status hv_tsc_page_status;
 
 	/* Hyper-v based guest crash (NT kernel bugcheck) parameters */
 	u64 hv_crash_param[HV_X64_MSR_CRASH_PARAMS];
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index eefb85b86fe8..2a8d078b16cb 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1087,7 +1087,7 @@  void kvm_hv_setup_tsc_page(struct kvm *kvm,
 	BUILD_BUG_ON(sizeof(tsc_seq) != sizeof(hv->tsc_ref.tsc_sequence));
 	BUILD_BUG_ON(offsetof(struct ms_hyperv_tsc_page, tsc_sequence) != 0);
 
-	if (!(hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE))
+	if (hv->hv_tsc_page_status == HV_TSC_PAGE_BROKEN)
 		return;
 
 	mutex_lock(&hv->hv_lock);
@@ -1101,7 +1101,7 @@  void kvm_hv_setup_tsc_page(struct kvm *kvm,
 	 */
 	if (unlikely(kvm_read_guest(kvm, gfn_to_gpa(gfn),
 				    &tsc_seq, sizeof(tsc_seq))))
-		goto out_unlock;
+		goto out_err;
 
 	/*
 	 * While we're computing and writing the parameters, force the
@@ -1110,15 +1110,15 @@  void kvm_hv_setup_tsc_page(struct kvm *kvm,
 	hv->tsc_ref.tsc_sequence = 0;
 	if (kvm_write_guest(kvm, gfn_to_gpa(gfn),
 			    &hv->tsc_ref, sizeof(hv->tsc_ref.tsc_sequence)))
-		goto out_unlock;
+		goto out_err;
 
 	if (!compute_tsc_page_parameters(hv_clock, &hv->tsc_ref))
-		goto out_unlock;
+		goto out_err;
 
 	/* Ensure sequence is zero before writing the rest of the struct.  */
 	smp_wmb();
 	if (kvm_write_guest(kvm, gfn_to_gpa(gfn), &hv->tsc_ref, sizeof(hv->tsc_ref)))
-		goto out_unlock;
+		goto out_err;
 
 	/*
 	 * Now switch to the TSC page mechanism by writing the sequence.
@@ -1133,6 +1133,12 @@  void kvm_hv_setup_tsc_page(struct kvm *kvm,
 	hv->tsc_ref.tsc_sequence = tsc_seq;
 	kvm_write_guest(kvm, gfn_to_gpa(gfn),
 			&hv->tsc_ref, sizeof(hv->tsc_ref.tsc_sequence));
+
+	hv->hv_tsc_page_status = HV_TSC_PAGE_SET;
+	goto out_unlock;
+
+out_err:
+	hv->hv_tsc_page_status = HV_TSC_PAGE_BROKEN;
 out_unlock:
 	mutex_unlock(&hv->hv_lock);
 }
@@ -1193,8 +1199,13 @@  static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data,
 	}
 	case HV_X64_MSR_REFERENCE_TSC:
 		hv->hv_tsc_page = data;
-		if (hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE)
+		if (hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE) {
+			if (!host)
+				hv->hv_tsc_page_status = HV_TSC_PAGE_GUEST_CHANGED;
+			else
+				hv->hv_tsc_page_status = HV_TSC_PAGE_HOST_CHANGED;
 			kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
+		}
 		break;
 	case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
 		return kvm_hv_msr_set_crash_data(kvm,