Gleb Natapov wrote on 2012-12-11:
> 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.
Ok. We can optimized it further if it really cause problem.
I will rebase the patch based on this.
>>
>>>> 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.
Best regards,
Yang
--
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
