On Tue, Dec 11, 2012 at 12:05:39PM +0000, Zhang, Yang Z wrote:
> 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 <kevin.tian@xxxxxxxxx>
> >> Signed-off-by: Yang Zhang <yang.z.zhang@xxxxxxxxx>
> >> ---
> >> 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.
http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/102159.
> >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.
We try to follow hardware behavior as close as possible and when we
fail to do so we often regret it. Furthermore we do not want to have
different behaviour with and without vid. kvm_cpu_has_interrupt() can
continue checking apic before ExtINT since this will not change function
return value, but without evidence of performance degradation I prefer
kvm_cpu_has_interrupt() and kvm_cpu_get_interrupt() to have similar
logic. kvm_cpu_has_interrupt() is called relatively rare (only when
KVM_REQ_EVENT is set) and the check is very light and can be optimized
further.
>
> >> 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
>
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
