[PATCH v2 0/4] x86/kvm/hyper-v: More fixes for TSC page clocksource for Hyper-V on KVM

[PATCH v2 0/4] x86/kvm/hyper-v: More fixes for TSC page clocksource for Hyper-V on KVM

[Vitaly Kuznetsov]

Changes since v1:
- Drop 'polling' bit check for now as we don't support this mode
  [Roman Kagan].
- Add a comment explaining "!host && !masked" in synic_set_sint()
- Add Roman's R-b to PATCH1 and PATCH2.

Previously (see commit 72c139bacf) it was found that Hyper-V frequency
MSRs are required to make Hyper-V on KVM pass through TSC page as stable
clocksource to its guests. However, to make things work this is not
enough. Hyper-V tries to prove that TSC frequency underneath it won't
change (e.g. when it's migrated), there are two mechanisms:

1) Invariant TSC (CPUID.80000007H:EDX[8]). If Hyper-V sees this bit it will
 treat TSC as stable. We, however, don't want to pass it as it makes
 migration hard (e.g. Qemu adds a migration blocker when 'invtsc' flag is
 passed. Genuine Hyper-V running in L0 doesn't pass it either.

2) Hyper-V Reenlightenment (CPUID.40000003H:EAX[13]).

This patch series add rudimentary support for Hyper-V reenlightenment
notifications to KVM ('producer' part; we already implemented
reenlightenment 'consumer' for KVM-on Hyper-V some time ago) and fixes
bugs I found during testing. Fully fledged reenlightenment  implementation
will be added later when we learn to migrate nested workloads in KVM making
testing possible.

Qemu patches are also required, I'll post them separately.

Vitaly Kuznetsov (3):
  x86/kvm/hyper-v: add reenlightenment MSRs support
  x86/kvm/hyper-v: remove stale entries from vec_bitmap/auto_eoi_bitmap
    on vector change
  x86/kvm/hyper-v: inject #GP only when invalid SINTx vector is unmasked

 arch/x86/include/asm/kvm_host.h    |  4 +++
 arch/x86/include/uapi/asm/hyperv.h |  2 ++
 arch/x86/kvm/hyperv.c              | 61 +++++++++++++++++++++++++++++++-------
 arch/x86/kvm/x86.c                 | 12 +++++++-
 4 files changed, 68 insertions(+), 11 deletions(-)

-- 
2.14.3

[PATCH v2 1/3] x86/kvm/hyper-v: add reenlightenment MSRs support

[Vitaly Kuznetsov]

Nested Hyper-V/Windows guest running on top of KVM will use TSC page
clocksource in two cases:
- L0 exposes invariant TSC (CPUID.80000007H:EDX[8]).
- L0 provides Hyper-V Reenlightenment support (CPUID.40000003H:EAX[13]).

Exposing invariant TSC effectively blocks migration to hosts with different
TSC frequencies, providing reenlightenment support will be needed when we
start migrating nested workloads.

Implement rudimentary support for reenlightenment MSRs. For now, these are
just read/write MSRs with no effect.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
---
 arch/x86/include/asm/kvm_host.h |  4 ++++
 arch/x86/kvm/hyperv.c           | 21 +++++++++++++++++++++
 arch/x86/kvm/x86.c              | 12 +++++++++++-
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 516798431328..eff8aa880333 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -752,6 +752,10 @@ struct kvm_hv {
 	u64 hv_crash_ctl;
 
 	HV_REFERENCE_TSC_PAGE tsc_ref;
+
+	u64 hv_reenlightenment_control;
+	u64 hv_tsc_emulation_control;
+	u64 hv_tsc_emulation_status;
 };
 
 enum kvm_irqchip_mode {
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index dc97f2544b6f..05f414525538 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -736,6 +736,9 @@ static bool kvm_hv_msr_partition_wide(u32 msr)
 	case HV_X64_MSR_CRASH_CTL:
 	case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
 	case HV_X64_MSR_RESET:
+	case HV_X64_MSR_REENLIGHTENMENT_CONTROL:
+	case HV_X64_MSR_TSC_EMULATION_CONTROL:
+	case HV_X64_MSR_TSC_EMULATION_STATUS:
 		r = true;
 		break;
 	}
@@ -981,6 +984,15 @@ static int kvm_hv_set_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data,
 			kvm_make_request(KVM_REQ_HV_RESET, vcpu);
 		}
 		break;
+	case HV_X64_MSR_REENLIGHTENMENT_CONTROL:
+		hv->hv_reenlightenment_control = data;
+		break;
+	case HV_X64_MSR_TSC_EMULATION_CONTROL:
+		hv->hv_tsc_emulation_control = data;
+		break;
+	case HV_X64_MSR_TSC_EMULATION_STATUS:
+		hv->hv_tsc_emulation_status = data;
+		break;
 	default:
 		vcpu_unimpl(vcpu, "Hyper-V uhandled wrmsr: 0x%x data 0x%llx\n",
 			    msr, data);
@@ -1105,6 +1117,15 @@ static int kvm_hv_get_msr_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 	case HV_X64_MSR_RESET:
 		data = 0;
 		break;
+	case HV_X64_MSR_REENLIGHTENMENT_CONTROL:
+		data = hv->hv_reenlightenment_control;
+		break;
+	case HV_X64_MSR_TSC_EMULATION_CONTROL:
+		data = hv->hv_tsc_emulation_control;
+		break;
+	case HV_X64_MSR_TSC_EMULATION_STATUS:
+		data = hv->hv_tsc_emulation_status;
+		break;
 	default:
 		vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr);
 		return 1;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c53298dfbf50..d97ab134b2d9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1026,7 +1026,11 @@ static u32 emulated_msrs[] = {
 	HV_X64_MSR_VP_RUNTIME,
 	HV_X64_MSR_SCONTROL,
 	HV_X64_MSR_STIMER0_CONFIG,
-	HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME,
+	HV_X64_MSR_APIC_ASSIST_PAGE,
+	HV_X64_MSR_REENLIGHTENMENT_CONTROL, HV_X64_MSR_TSC_EMULATION_CONTROL,
+	HV_X64_MSR_TSC_EMULATION_STATUS,
+
+	MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME,
 	MSR_KVM_PV_EOI_EN,
 
 	MSR_IA32_TSC_ADJUST,
@@ -2325,6 +2329,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
 	case HV_X64_MSR_CRASH_CTL:
 	case HV_X64_MSR_STIMER0_CONFIG ... HV_X64_MSR_STIMER3_COUNT:
+	case HV_X64_MSR_REENLIGHTENMENT_CONTROL:
+	case HV_X64_MSR_TSC_EMULATION_CONTROL:
+	case HV_X64_MSR_TSC_EMULATION_STATUS:
 		return kvm_hv_set_msr_common(vcpu, msr, data,
 					     msr_info->host_initiated);
 	case MSR_IA32_BBL_CR_CTL3:
@@ -2551,6 +2558,9 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case HV_X64_MSR_CRASH_P0 ... HV_X64_MSR_CRASH_P4:
 	case HV_X64_MSR_CRASH_CTL:
 	case HV_X64_MSR_STIMER0_CONFIG ... HV_X64_MSR_STIMER3_COUNT:
+	case HV_X64_MSR_REENLIGHTENMENT_CONTROL:
+	case HV_X64_MSR_TSC_EMULATION_CONTROL:
+	case HV_X64_MSR_TSC_EMULATION_STATUS:
 		return kvm_hv_get_msr_common(vcpu,
 					     msr_info->index, &msr_info->data);
 		break;
-- 
2.14.3

[PATCH v2 2/3] x86/kvm/hyper-v: remove stale entries from vec_bitmap/auto_eoi_bitmap on vector change

[Vitaly Kuznetsov]

When a new vector is written to SINx we update vec_bitmap/auto_eoi_bitmap
but we forget to remove old vector from these masks (in case it is not
present in some other SINTx).

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
---
 arch/x86/include/uapi/asm/hyperv.h |  2 ++
 arch/x86/kvm/hyperv.c              | 32 ++++++++++++++++++++++----------
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h
index 197c2e6c7376..62c778a303a1 100644
--- a/arch/x86/include/uapi/asm/hyperv.h
+++ b/arch/x86/include/uapi/asm/hyperv.h
@@ -318,6 +318,8 @@ typedef struct _HV_REFERENCE_TSC_PAGE {
 #define HV_SYNIC_SINT_COUNT		(16)
 /* Define the expected SynIC version. */
 #define HV_SYNIC_VERSION_1		(0x1)
+/* Valid SynIC vectors are 16-255. */
+#define HV_SYNIC_FIRST_VALID_VECTOR	(16)
 
 #define HV_SYNIC_CONTROL_ENABLE		(1ULL << 0)
 #define HV_SYNIC_SIMP_ENABLE		(1ULL << 0)
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 05f414525538..6d14f808145d 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -74,13 +74,30 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic,
 	return false;
 }
 
+static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
+				int vector)
+{
+	if (vector < HV_SYNIC_FIRST_VALID_VECTOR)
+		return;
+
+	if (synic_has_vector_connected(synic, vector))
+		__set_bit(vector, synic->vec_bitmap);
+	else
+		__clear_bit(vector, synic->vec_bitmap);
+
+	if (synic_has_vector_auto_eoi(synic, vector))
+		__set_bit(vector, synic->auto_eoi_bitmap);
+	else
+		__clear_bit(vector, synic->auto_eoi_bitmap);
+}
+
 static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
 			  u64 data, bool host)
 {
-	int vector;
+	int vector, old_vector;
 
 	vector = data & HV_SYNIC_SINT_VECTOR_MASK;
-	if (vector < 16 && !host)
+	if (vector < HV_SYNIC_FIRST_VALID_VECTOR && !host)
 		return 1;
 	/*
 	 * Guest may configure multiple SINTs to use the same vector, so
@@ -88,18 +105,13 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
 	 * bitmap of vectors with auto-eoi behavior.  The bitmaps are
 	 * updated here, and atomically queried on fast paths.
 	 */
+	old_vector = synic_read_sint(synic, sint) & HV_SYNIC_SINT_VECTOR_MASK;
 
 	atomic64_set(&synic->sint[sint], data);
 
-	if (synic_has_vector_connected(synic, vector))
-		__set_bit(vector, synic->vec_bitmap);
-	else
-		__clear_bit(vector, synic->vec_bitmap);
+	synic_update_vector(synic, old_vector);
 
-	if (synic_has_vector_auto_eoi(synic, vector))
-		__set_bit(vector, synic->auto_eoi_bitmap);
-	else
-		__clear_bit(vector, synic->auto_eoi_bitmap);
+	synic_update_vector(synic, vector);
 
 	/* Load SynIC vectors into EOI exit bitmap */
 	kvm_make_request(KVM_REQ_SCAN_IOAPIC, synic_to_vcpu(synic));
-- 
2.14.3

[PATCH v2 3/3] x86/kvm/hyper-v: inject #GP only when invalid SINTx vector is unmasked

[Vitaly Kuznetsov]

Hyper-V 2016 on KVM with SynIC enabled doesn't boot with the following
trace:

    kvm_entry:            vcpu 0
    kvm_exit:             reason MSR_WRITE rip 0xfffff8000131c1e5 info 0 0
    kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000090 data 0x10000 host 0
    kvm_msr:              msr_write 40000090 = 0x10000 (#GP)
    kvm_inj_exception:    #GP (0x0)

KVM acts according to the following statement from TLFS:

"
11.8.4 SINTx Registers
...
Valid values for vector are 16-255 inclusive. Specifying an invalid
vector number results in #GP.
"

However, I checked and genuine Hyper-V doesn't #GP when we write 0x10000
to SINTx. I checked with Microsoft and they confirmed that if either the
Masked bit (bit 16) or the Polling bit (bit 18) is set to 1, then they
ignore the value of Vector. Make KVM act accordingly.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
Changes since v1:
- Drop 'polling' bit check for now as we don't support this mode. We'll
  need to bring some form of this check back when polling mode is
  implemented [Roman Kagan].
- Add a comment explaining "!host && !masked" in synic_set_sint()
---
 arch/x86/kvm/hyperv.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 6d14f808145d..b390a4d9ea14 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -95,9 +95,17 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
 			  u64 data, bool host)
 {
 	int vector, old_vector;
+	bool masked;
 
 	vector = data & HV_SYNIC_SINT_VECTOR_MASK;
-	if (vector < HV_SYNIC_FIRST_VALID_VECTOR && !host)
+	masked = data & HV_SYNIC_SINT_MASKED;
+
+	/*
+	 * Valid vectors are 16-255, however, nested Hyper-V attempts to write
+	 * default '0x10000' value on boot and this should not #GP. We need to
+	 * allow zero-initing the register from host as well.
+	 */
+	if (vector < HV_SYNIC_FIRST_VALID_VECTOR && !host && !masked)
 		return 1;
 	/*
 	 * Guest may configure multiple SINTs to use the same vector, so
-- 
2.14.3

Re: [PATCH v2 3/3] x86/kvm/hyper-v: inject #GP only when invalid SINTx vector is unmasked

[Roman Kagan]

On Thu, Mar 01, 2018 at 03:15:14PM +0100, Vitaly Kuznetsov wrote:
> Hyper-V 2016 on KVM with SynIC enabled doesn't boot with the following
> trace:
> 
>     kvm_entry:            vcpu 0
>     kvm_exit:             reason MSR_WRITE rip 0xfffff8000131c1e5 info 0 0
>     kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000090 data 0x10000 host 0
>     kvm_msr:              msr_write 40000090 = 0x10000 (#GP)
>     kvm_inj_exception:    #GP (0x0)
> 
> KVM acts according to the following statement from TLFS:
> 
> "
> 11.8.4 SINTx Registers
> ...
> Valid values for vector are 16-255 inclusive. Specifying an invalid
> vector number results in #GP.
> "
> 
> However, I checked and genuine Hyper-V doesn't #GP when we write 0x10000
> to SINTx. I checked with Microsoft and they confirmed that if either the
> Masked bit (bit 16) or the Polling bit (bit 18) is set to 1, then they
> ignore the value of Vector. Make KVM act accordingly.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> Changes since v1:
> - Drop 'polling' bit check for now as we don't support this mode. We'll
>   need to bring some form of this check back when polling mode is
>   implemented [Roman Kagan].
> - Add a comment explaining "!host && !masked" in synic_set_sint()
> ---
>  arch/x86/kvm/hyperv.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)

Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>

Re: [PATCH v2 2/3] x86/kvm/hyper-v: remove stale entries from vec_bitmap/auto_eoi_bitmap on vector change

[Radim Krčmář]

2018-03-01 15:15+0100, Vitaly Kuznetsov:
> When a new vector is written to SINx we update vec_bitmap/auto_eoi_bitmap
> but we forget to remove old vector from these masks (in case it is not
> present in some other SINTx).
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
>  arch/x86/include/uapi/asm/hyperv.h |  2 ++
>  arch/x86/kvm/hyperv.c              | 32 ++++++++++++++++++++++----------
>  2 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h
> index 197c2e6c7376..62c778a303a1 100644
> --- a/arch/x86/include/uapi/asm/hyperv.h
> +++ b/arch/x86/include/uapi/asm/hyperv.h
> @@ -318,6 +318,8 @@ typedef struct _HV_REFERENCE_TSC_PAGE {
>  #define HV_SYNIC_SINT_COUNT		(16)
>  /* Define the expected SynIC version. */
>  #define HV_SYNIC_VERSION_1		(0x1)
> +/* Valid SynIC vectors are 16-255. */
> +#define HV_SYNIC_FIRST_VALID_VECTOR	(16)
>  
>  #define HV_SYNIC_CONTROL_ENABLE		(1ULL << 0)
>  #define HV_SYNIC_SIMP_ENABLE		(1ULL << 0)
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 05f414525538..6d14f808145d 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -74,13 +74,30 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic,
>  	return false;
>  }
>  
> +static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> +				int vector)
> +{
> +	if (vector < HV_SYNIC_FIRST_VALID_VECTOR)
> +		return;
> +
> +	if (synic_has_vector_connected(synic, vector))
> +		__set_bit(vector, synic->vec_bitmap);
> +	else
> +		__clear_bit(vector, synic->vec_bitmap);
> +
> +	if (synic_has_vector_auto_eoi(synic, vector))
> +		__set_bit(vector, synic->auto_eoi_bitmap);
> +	else
> +		__clear_bit(vector, synic->auto_eoi_bitmap);
> +}
> +
>  static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
>  			  u64 data, bool host)
>  {
> -	int vector;
> +	int vector, old_vector;
>  
>  	vector = data & HV_SYNIC_SINT_VECTOR_MASK;
> -	if (vector < 16 && !host)
> +	if (vector < HV_SYNIC_FIRST_VALID_VECTOR && !host)
>  		return 1;
>  	/*
>  	 * Guest may configure multiple SINTs to use the same vector, so
> @@ -88,18 +105,13 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
>  	 * bitmap of vectors with auto-eoi behavior.  The bitmaps are
>  	 * updated here, and atomically queried on fast paths.
>  	 */
> +	old_vector = synic_read_sint(synic, sint) & HV_SYNIC_SINT_VECTOR_MASK;
>  
>  	atomic64_set(&synic->sint[sint], data);
>  
> -	if (synic_has_vector_connected(synic, vector))
> -		__set_bit(vector, synic->vec_bitmap);
> -	else
> -		__clear_bit(vector, synic->vec_bitmap);
> +	synic_update_vector(synic, old_vector);
> 
> -	if (synic_has_vector_auto_eoi(synic, vector))
> -		__set_bit(vector, synic->auto_eoi_bitmap);
> -	else
> -		__clear_bit(vector, synic->auto_eoi_bitmap);
> +	synic_update_vector(synic, vector);

This looks like it solves the problem when we get two SINTs with the
same vector back-to-back , but shouldn't these bits really be cleared on
EOI (either auto or manual)?

Thanks.

Re: [PATCH v2 2/3] x86/kvm/hyper-v: remove stale entries from vec_bitmap/auto_eoi_bitmap on vector change

[Vitaly Kuznetsov]

Radim Krčmář <rkrcmar@redhat.com> writes:

> 2018-03-01 15:15+0100, Vitaly Kuznetsov:
>> When a new vector is written to SINx we update vec_bitmap/auto_eoi_bitmap
>> but we forget to remove old vector from these masks (in case it is not
>> present in some other SINTx).
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
>> ---
>>  arch/x86/include/uapi/asm/hyperv.h |  2 ++
>>  arch/x86/kvm/hyperv.c              | 32 ++++++++++++++++++++++----------
>>  2 files changed, 24 insertions(+), 10 deletions(-)
>> 
>> diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h
>> index 197c2e6c7376..62c778a303a1 100644
>> --- a/arch/x86/include/uapi/asm/hyperv.h
>> +++ b/arch/x86/include/uapi/asm/hyperv.h
>> @@ -318,6 +318,8 @@ typedef struct _HV_REFERENCE_TSC_PAGE {
>>  #define HV_SYNIC_SINT_COUNT		(16)
>>  /* Define the expected SynIC version. */
>>  #define HV_SYNIC_VERSION_1		(0x1)
>> +/* Valid SynIC vectors are 16-255. */
>> +#define HV_SYNIC_FIRST_VALID_VECTOR	(16)
>>  
>>  #define HV_SYNIC_CONTROL_ENABLE		(1ULL << 0)
>>  #define HV_SYNIC_SIMP_ENABLE		(1ULL << 0)
>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>> index 05f414525538..6d14f808145d 100644
>> --- a/arch/x86/kvm/hyperv.c
>> +++ b/arch/x86/kvm/hyperv.c
>> @@ -74,13 +74,30 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic,
>>  	return false;
>>  }
>>  
>> +static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
>> +				int vector)
>> +{
>> +	if (vector < HV_SYNIC_FIRST_VALID_VECTOR)
>> +		return;
>> +
>> +	if (synic_has_vector_connected(synic, vector))
>> +		__set_bit(vector, synic->vec_bitmap);
>> +	else
>> +		__clear_bit(vector, synic->vec_bitmap);
>> +
>> +	if (synic_has_vector_auto_eoi(synic, vector))
>> +		__set_bit(vector, synic->auto_eoi_bitmap);
>> +	else
>> +		__clear_bit(vector, synic->auto_eoi_bitmap);
>> +}
>> +
>>  static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
>>  			  u64 data, bool host)
>>  {
>> -	int vector;
>> +	int vector, old_vector;
>>  
>>  	vector = data & HV_SYNIC_SINT_VECTOR_MASK;
>> -	if (vector < 16 && !host)
>> +	if (vector < HV_SYNIC_FIRST_VALID_VECTOR && !host)
>>  		return 1;
>>  	/*
>>  	 * Guest may configure multiple SINTs to use the same vector, so
>> @@ -88,18 +105,13 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
>>  	 * bitmap of vectors with auto-eoi behavior.  The bitmaps are
>>  	 * updated here, and atomically queried on fast paths.
>>  	 */
>> +	old_vector = synic_read_sint(synic, sint) & HV_SYNIC_SINT_VECTOR_MASK;
>>  
>>  	atomic64_set(&synic->sint[sint], data);
>>  
>> -	if (synic_has_vector_connected(synic, vector))
>> -		__set_bit(vector, synic->vec_bitmap);
>> -	else
>> -		__clear_bit(vector, synic->vec_bitmap);
>> +	synic_update_vector(synic, old_vector);
>> 
>> -	if (synic_has_vector_auto_eoi(synic, vector))
>> -		__set_bit(vector, synic->auto_eoi_bitmap);
>> -	else
>> -		__clear_bit(vector, synic->auto_eoi_bitmap);
>> +	synic_update_vector(synic, vector);
>
> This looks like it solves the problem when we get two SINTs with the
> same vector back-to-back , but shouldn't these bits really be cleared on
> EOI (either auto or manual)?

Hmm,

I was trying to address the following issue: guest programs SynIC's
SINTx with some vector but later re-programs it with a different
one. Without the patch synic->vec_bitmap and synic->auto_eoi_bitmap keep
stale data. If there's no concurrent interrupt than we're safe, but what
happens if there is one...

kvm_hv_synic_send_eoi() already goes through all SINTx but we already
updated vector so it won't find any. We could've added something like
'old_vector' but what if the request with this vector came _after_ we
re-programed SynIC (and, so, it wasn't meant to be serviced by SynIC?)?

-- 
  Vitaly

Re: [PATCH v2 2/3] x86/kvm/hyper-v: remove stale entries from vec_bitmap/auto_eoi_bitmap on vector change

[Radim Krčmář]

2018-03-09 16:21+0100, Vitaly Kuznetsov:
> Radim Krčmář <rkrcmar@redhat.com> writes:
> > This looks like it solves the problem when we get two SINTs with the
> > same vector back-to-back , but shouldn't these bits really be cleared on
> > EOI (either auto or manual)?
> 
> Hmm,
> 
> I was trying to address the following issue: guest programs SynIC's
> SINTx with some vector but later re-programs it with a different
> one. Without the patch synic->vec_bitmap and synic->auto_eoi_bitmap keep
> stale data. If there's no concurrent interrupt than we're safe, but what
> happens if there is one...
> 
> kvm_hv_synic_send_eoi() already goes through all SINTx but we already
> updated vector so it won't find any. We could've added something like
> 'old_vector' but what if the request with this vector came _after_ we
> re-programed SynIC (and, so, it wasn't meant to be serviced by SynIC?)?

I now read that TLFS puts the responsitiblity on guest OS when toggling
auto-EOI, so let's assume that the OS is to blame for disabling/changing
vectors with pending interrupts as well.

Applied all, thanks.