[PATCH v2 0/2] x86/kvm/hyper-v: Direct Mode stimers fixes

[PATCH v2 0/2] x86/kvm/hyper-v: Direct Mode stimers fixes

[Vitaly Kuznetsov]

Changes in v2:
- PATCH2 added [Roman Kagan]

This is a follow-up to my "x86/kvm/hyper-v: direct mode for synthetic
timers" series. Roman identified a couple of issues with the patchset:
we don't check that the supplied APIC vector is valid and we care about
EOI for nothing. These can be merged with the original patch implementing
direct mode stimers or just put on top.

Vitaly Kuznetsov (2):
  x86/hyper-v: Stop caring about EOI for direct stimers
  x86/kvm/hyper-v: disallow setting illegal vectors for direct mode
    stimers

 arch/x86/kvm/hyperv.c | 41 ++++++++---------------------------------
 1 file changed, 8 insertions(+), 33 deletions(-)

-- 
2.19.2

[PATCH v2 1/2] x86/hyper-v: Stop caring about EOI for direct stimers

[Vitaly Kuznetsov]

Turns out we over-engineered Direct Mode for stimers a bit: unlike
traditional stimers where we may want to try to re-inject the message upon
EOI, Direct Mode stimers just set the irq in APIC and kvm_apic_set_irq()
fails only when APIC is disabled (see APIC_DM_FIXED case in
__apic_accept_irq()). Remove the redundant part.

Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/hyperv.c | 36 +++---------------------------------
 1 file changed, 3 insertions(+), 33 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index e6a2a085644a..0a16a77e6ac3 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -56,21 +56,8 @@ static inline int synic_get_sint_vector(u64 sint_value)
 static bool synic_has_vector_connected(struct kvm_vcpu_hv_synic *synic,
 				      int vector)
 {
-	struct kvm_vcpu *vcpu = synic_to_vcpu(synic);
-	struct kvm_vcpu_hv *hv_vcpu = vcpu_to_hv_vcpu(vcpu);
-	struct kvm_vcpu_hv_stimer *stimer;
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(hv_vcpu->stimer); i++) {
-		stimer = &hv_vcpu->stimer[i];
-		if (stimer->config.enable && stimer->config.direct_mode &&
-		    stimer->config.apic_vector == vector)
-			return true;
-	}
-
-	if (vector < HV_SYNIC_FIRST_VALID_VECTOR)
-		return false;
-
 	for (i = 0; i < ARRAY_SIZE(synic->sint); i++) {
 		if (synic_get_sint_vector(synic_read_sint(synic, i)) == vector)
 			return true;
@@ -96,14 +83,14 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic,
 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 (vector < HV_SYNIC_FIRST_VALID_VECTOR)
-		return;
-
 	if (synic_has_vector_auto_eoi(synic, vector))
 		__set_bit(vector, synic->auto_eoi_bitmap);
 	else
@@ -382,9 +369,7 @@ int kvm_hv_synic_set_irq(struct kvm *kvm, u32 vpidx, u32 sint)
 
 void kvm_hv_synic_send_eoi(struct kvm_vcpu *vcpu, int vector)
 {
-	struct kvm_vcpu_hv *hv_vcpu = vcpu_to_hv_vcpu(vcpu);
 	struct kvm_vcpu_hv_synic *synic = vcpu_to_synic(vcpu);
-	struct kvm_vcpu_hv_stimer *stimer;
 	int i;
 
 	trace_kvm_hv_synic_send_eoi(vcpu->vcpu_id, vector);
@@ -392,14 +377,6 @@ void kvm_hv_synic_send_eoi(struct kvm_vcpu *vcpu, int vector)
 	for (i = 0; i < ARRAY_SIZE(synic->sint); i++)
 		if (synic_get_sint_vector(synic_read_sint(synic, i)) == vector)
 			kvm_hv_notify_acked_sint(vcpu, i);
-
-	for (i = 0; i < ARRAY_SIZE(hv_vcpu->stimer); i++) {
-		stimer = &hv_vcpu->stimer[i];
-		if (stimer->msg_pending && stimer->config.enable &&
-		    stimer->config.direct_mode &&
-		    stimer->config.apic_vector == vector)
-			stimer_mark_pending(stimer, false);
-	}
 }
 
 static int kvm_hv_set_sint_gsi(struct kvm *kvm, u32 vpidx, u32 sint, int gsi)
@@ -566,8 +543,6 @@ static int stimer_start(struct kvm_vcpu_hv_stimer *stimer)
 static int stimer_set_config(struct kvm_vcpu_hv_stimer *stimer, u64 config,
 			     bool host)
 {
-	struct kvm_vcpu *vcpu = stimer_to_vcpu(stimer);
-	struct kvm_vcpu_hv *hv_vcpu = vcpu_to_hv_vcpu(vcpu);
 	union hv_stimer_config new_config = {.as_uint64 = config},
 		old_config = {.as_uint64 = stimer->config.as_uint64};
 
@@ -580,11 +555,6 @@ static int stimer_set_config(struct kvm_vcpu_hv_stimer *stimer, u64 config,
 		new_config.enable = 0;
 	stimer->config.as_uint64 = new_config.as_uint64;
 
-	if (old_config.direct_mode)
-		synic_update_vector(&hv_vcpu->synic, old_config.apic_vector);
-	if (new_config.direct_mode)
-		synic_update_vector(&hv_vcpu->synic, new_config.apic_vector);
-
 	stimer_mark_pending(stimer, false);
 	return 0;
 }
-- 
2.19.2

Re: [PATCH v2 1/2] x86/hyper-v: Stop caring about EOI for direct stimers

[Roman Kagan]

On Wed, Dec 12, 2018 at 05:50:16PM +0100, Vitaly Kuznetsov wrote:
> Turns out we over-engineered Direct Mode for stimers a bit: unlike
> traditional stimers where we may want to try to re-inject the message upon
> EOI, Direct Mode stimers just set the irq in APIC and kvm_apic_set_irq()
> fails only when APIC is disabled (see APIC_DM_FIXED case in
> __apic_accept_irq()). Remove the redundant part.
> 
> Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/kvm/hyperv.c | 36 +++---------------------------------
>  1 file changed, 3 insertions(+), 33 deletions(-)

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

[PATCH v2 2/2] x86/kvm/hyper-v: disallow setting illegal vectors for direct mode stimers

[Vitaly Kuznetsov]

APIC vectors used for direct mode stimers should be valid for lAPIC and
just like genuine Hyper-V we should #GP when an illegal one is specified.

Add the appropriate check to stimer_set_config()

Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/hyperv.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 0a16a77e6ac3..8723a802e9b7 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -549,6 +549,11 @@ static int stimer_set_config(struct kvm_vcpu_hv_stimer *stimer, u64 config,
 	trace_kvm_hv_stimer_set_config(stimer_to_vcpu(stimer)->vcpu_id,
 				       stimer->index, config, host);
 
+	/* Valid vectors for Direct Mode are 16..255. */
+	if (new_config.enable && new_config.direct_mode &&
+	    new_config.apic_vector < HV_SYNIC_FIRST_VALID_VECTOR)
+		return 1;
+
 	stimer_cleanup(stimer);
 	if (old_config.enable &&
 	    !new_config.direct_mode && new_config.sintx == 0)
-- 
2.19.2

Re: [PATCH v2 2/2] x86/kvm/hyper-v: disallow setting illegal vectors for direct mode stimers

[Roman Kagan]

On Wed, Dec 12, 2018 at 05:50:17PM +0100, Vitaly Kuznetsov wrote:
> APIC vectors used for direct mode stimers should be valid for lAPIC and
> just like genuine Hyper-V we should #GP when an illegal one is specified.
> 
> Add the appropriate check to stimer_set_config()
> 
> Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/kvm/hyperv.c | 5 +++++
>  1 file changed, 5 insertions(+)

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

Re: [PATCH v2 0/2] x86/kvm/hyper-v: Direct Mode stimers fixes

[Paolo Bonzini]

On 12/12/18 17:50, Vitaly Kuznetsov wrote:
> Changes in v2:
> - PATCH2 added [Roman Kagan]
> 
> This is a follow-up to my "x86/kvm/hyper-v: direct mode for synthetic
> timers" series. Roman identified a couple of issues with the patchset:
> we don't check that the supplied APIC vector is valid and we care about
> EOI for nothing. These can be merged with the original patch implementing
> direct mode stimers or just put on top.
> 
> Vitaly Kuznetsov (2):
>   x86/hyper-v: Stop caring about EOI for direct stimers
>   x86/kvm/hyper-v: disallow setting illegal vectors for direct mode
>     stimers
> 
>  arch/x86/kvm/hyperv.c | 41 ++++++++---------------------------------
>  1 file changed, 8 insertions(+), 33 deletions(-)
> 

Queued, thanks.

Paolo