Gleb Natapov wrote on 2012-12-11: > Looks very good overall. Are you testing this with vid disabled with > Linux/Windows guests? Small comments below. Yes. I tested rhel6u3, rhel5u4, winxp and win7. All of them work well with and without vid enabled.
> On Mon, Dec 10, 2012 at 03:20:39PM +0800, Yang Zhang wrote: >> Virtual interrupt delivery avoids KVM to inject vAPIC interrupts >> manually, which is fully taken care of by the hardware. This needs >> some special awareness into existing interrupr injection path: >> >> - for pending interrupt, instead of direct injection, we may need >> update architecture specific indicators before resuming to guest. >> - A pending interrupt, which is masked by ISR, should be also >> considered in above update action, since hardware will decide >> when to inject it at right time. Current has_interrupt and >> get_interrupt only returns a valid vector from injection p.o.v. >> Signed-off-by: Kevin Tian <[email protected]> >> Signed-off-by: Yang Zhang <[email protected]> >> --- >> arch/ia64/kvm/lapic.h | 6 ++ >> arch/x86/include/asm/kvm_host.h | 5 ++ arch/x86/include/asm/vmx.h >> | 11 ++++ arch/x86/kvm/irq.c | 76 >> ++++++++++++++++++++++------ arch/x86/kvm/lapic.c | 99 >> +++++++++++++++++++++++++++++++++--- arch/x86/kvm/lapic.h | >> 9 +++ arch/x86/kvm/svm.c | 25 +++++++++ >> arch/x86/kvm/vmx.c | 104 >> +++++++++++++++++++++++++++++++++++++-- arch/x86/kvm/x86.c >> | 18 ++++--- include/linux/kvm_host.h | 2 + >> virt/kvm/ioapic.c | 35 +++++++++++++ virt/kvm/ioapic.h >> | 1 + virt/kvm/irq_comm.c | 18 +++++++ >> 13 files changed, 372 insertions(+), 37 deletions(-) >> diff --git a/arch/ia64/kvm/lapic.h b/arch/ia64/kvm/lapic.h >> index c5f92a9..cb59eb4 100644 >> --- a/arch/ia64/kvm/lapic.h >> +++ b/arch/ia64/kvm/lapic.h >> @@ -27,4 +27,10 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct > kvm_lapic_irq *irq); >> #define kvm_apic_present(x) (true) >> #define kvm_lapic_enabled(x) (true) >> +static inline void kvm_update_eoi_exitmap(struct kvm *kvm, >> + struct kvm_lapic_irq *irq) >> +{ >> + /* IA64 has no apicv supporting, do nothing here */ >> +} >> + >> #endif >> diff --git a/arch/x86/include/asm/kvm_host.h >> b/arch/x86/include/asm/kvm_host.h index dc87b65..d797ade 100644 --- >> a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h >> @@ -697,6 +697,10 @@ struct kvm_x86_ops { >> void (*enable_nmi_window)(struct kvm_vcpu *vcpu); >> void (*enable_irq_window)(struct kvm_vcpu *vcpu); >> void (*update_cr8_intercept)(struct kvm_vcpu *vcpu, int tpr, int irr); >> + int (*has_virtual_interrupt_delivery)(struct kvm_vcpu *vcpu); >> + void (*update_irq)(struct kvm_vcpu *vcpu, int max_irr); > Lets call it update_apic_irq since this is what is does. Ok. >> +/* >> * Read pending interrupt vector and intack. >> */ >> int kvm_cpu_get_interrupt(struct kvm_vcpu *v) { - struct kvm_pic *s; >> int vector; >> >> if (!irqchip_in_kernel(v->kvm)) >> return v->arch.interrupt.nr; >> - vector = kvm_get_apic_interrupt(v); /* APIC */ >> - if (vector == -1) { >> - if (kvm_apic_accept_pic_intr(v)) { >> - s = pic_irqchip(v->kvm); >> - s->output = 0; /* PIC */ >> - vector = kvm_pic_read_irq(v->kvm); >> - } >> + if (kvm_apic_vid_enabled(v)) >> + vector = kvm_cpu_get_extint(v); /* non-APIC */ >> + else { >> + vector = kvm_get_apic_interrupt(v); /* APIC */ >> + if (vector == -1) >> + vector = kvm_cpu_get_extint(v); /* non-APIC */ >> } > I've send the patch to fix ExtINT handling. Can you review it and rebase on > top of it? Sorry. I missed it. >From performance point, I thought this is not friendly. As we known, Extint >interrupt is used rarely(it may only exist when in virtual wire mode). When >guest boot up, it is in apic mode. So most of time, interrupts are went to >apic not pic. And it seems check extint first is unnecessary. From my point, >if there is no correctness issue, virtualization isn't force to follow the >hardware's behavior. >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 3bdaf29..060f36b 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -5534,12 +5534,10 @@ static void inject_pending_event(struct kvm_vcpu > *vcpu) >> vcpu->arch.nmi_injected = true; >> kvm_x86_ops->set_nmi(vcpu); >> } >> - } else if (kvm_cpu_has_interrupt(vcpu)) { >> - if (kvm_x86_ops->interrupt_allowed(vcpu)) { >> - kvm_queue_interrupt(vcpu, kvm_cpu_get_interrupt(vcpu), >> - false); >> - kvm_x86_ops->set_irq(vcpu); >> - } >> + } else if (kvm_cpu_has_injectable_intr(vcpu) && >> + kvm_x86_ops->interrupt_allowed(vcpu)) { >> + kvm_queue_interrupt(vcpu, kvm_cpu_get_interrupt(vcpu), false); >> + kvm_x86_ops->set_irq(vcpu); > Please do not change indentation unnecessary. Now this block has > different one from previous. Ok. > >> } >> } >> @@ -5655,6 +5653,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) >> kvm_handle_pmu_event(vcpu); >> if (kvm_check_request(KVM_REQ_PMI, vcpu)) >> kvm_deliver_pmi(vcpu); >> + if (kvm_check_request(KVM_REQ_EOIBITMAP, vcpu)) >> + kvm_x86_ops->load_eoi_exitmap(vcpu); >> } >> >> if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) { >> @@ -5663,10 +5663,14 @@ static int vcpu_enter_guest(struct kvm_vcpu > *vcpu) >> /* enable NMI/IRQ window open exits if needed */ >> if (vcpu->arch.nmi_pending) >> kvm_x86_ops->enable_nmi_window(vcpu); >> - else if (kvm_cpu_has_interrupt(vcpu) || req_int_win) >> + else if (kvm_cpu_has_injectable_intr(vcpu) || req_int_win) >> kvm_x86_ops->enable_irq_window(vcpu); >> >> if (kvm_lapic_enabled(vcpu)) { >> + /* update archtecture specific hints for APIC >> + * virtual interrupt delivery */ >> + kvm_x86_ops->update_irq(vcpu, >> + kvm_lapic_find_highest_irr(vcpu)); > Hmm, OK so now we scan IRR even if vid is not enabled. Yeah, I know I > suggested it :). So lets do it similarly to cr8_update callback. If vid is > disabled set kvm_x86_ops->update_irq to NULL. Here do > if (kvm_x86_ops->update_apic_irq) > kvm_x86_ops->update_apic_irq(vcpu, kvm_lapic_find_highest_irr(vcpu)); > Or write small helper function like update_cr8_intercept() below. Ok. >> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c >> index cfb7e4d..e214ea4 100644 >> --- a/virt/kvm/ioapic.c >> +++ b/virt/kvm/ioapic.c >> @@ -115,6 +115,40 @@ static void update_handled_vectors(struct kvm_ioapic > *ioapic) >> smp_wmb(); >> } >> +static void ioapic_update_eoi_exitmap_one(struct kvm_ioapic *ioapic, int >> pin) >> +{ >> + union kvm_ioapic_redirect_entry *e; >> + >> + e = &ioapic->redirtbl[pin]; >> + >> + /* PIT is a special case: which is edge trig but have EOI hook. >> + * Always set the eoi exit bitmap for PIT interrupt*/ > Drop the comment. > >> + if (!e->fields.mask && > Drop this from here. You have it in the caller now. Ok. >> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c >> index 2eb58af..93ecd2a 100644 >> --- a/virt/kvm/irq_comm.c >> +++ b/virt/kvm/irq_comm.c >> @@ -178,6 +178,24 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, > u32 irq, int level) >> return ret; >> } >> +bool kvm_irq_has_notifier(struct kvm *kvm, unsigned irqchip, unsigned pin) >> +{ >> + struct kvm_irq_ack_notifier *kian; >> + struct hlist_node *n; >> + int gsi; >> + >> + rcu_read_lock(); >> + gsi = rcu_dereference(kvm->irq_routing)->chip[irqchip][pin]; >> + if (gsi != -1) >> + hlist_for_each_entry_rcu(kian, n, &kvm->irq_ack_notifier_list, >> + link) >> + if (kian->gsi == gsi) >> + return true; >> + rcu_read_unlock(); >> + >> + return false; >> +} >> + > Shouldn't you call ioapic_update_eoi_exitmap() in > kvm_(un)register_irq_ack_notifier() too? Right. Will add it in next patch. Best regards, Yang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
