Re: [PATCH] x86/hyper-v: Stop caring about EOI for direct stimers
On 05/12/18 16:36, 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 > Signed-off-by: Vitaly Kuznetsov > --- > 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; > } > Queued, thanks. Paolo
Re: [PATCH] x86/hyper-v: Stop caring about EOI for direct stimers
Roman Kagan writes: > [ Sorry, missed this one ] > > On Wed, Dec 05, 2018 at 04:36:21PM +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 >> Signed-off-by: Vitaly Kuznetsov >> --- >> 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; >> } > > As discussed in another thread, it seems worth while to make > stimer_set_config reject vectors 0..15. > > Besides I'd rather sqwash this patch into the one that introduced direct > timers, before it reached Linus' tree. I'm fine either way, I'm going to send v2 with a new patch adding the check to stimer_set_config() and leave it up to Paolo if he would want to squash them all together before sending to Linus.
Re: [PATCH] x86/hyper-v: Stop caring about EOI for direct stimers
[ Sorry, missed this one ] On Wed, Dec 05, 2018 at 04:36:21PM +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 > Signed-off-by: Vitaly Kuznetsov > --- > 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; > } As discussed in another thread, it seems worth while to make stimer_set_config reject vectors 0..15. Besides I'd rather sqwash this patch into the one that introduced direct timers, before it reached Linus' tree. Roman.