Gleb Natapov wrote on 2012-12-12:
> On Wed, Dec 12, 2012 at 12:56:47PM +0800, Yang Zhang wrote:
>> From: Yang Zhang <yang.z.zhang@xxxxxxxxx>
>>
>> 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>
>> ---
>> diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
>> index ebd98d0..537ce4b 100644
>> --- a/arch/x86/kvm/irq.c
>> +++ b/arch/x86/kvm/irq.c
>> @@ -38,37 +38,81 @@ int kvm_cpu_has_pending_timer(struct kvm_vcpu
> *vcpu)
>> EXPORT_SYMBOL(kvm_cpu_has_pending_timer);
>>
>> /*
>> + * check if there is pending interrupt from
>> + * non-APIC source without intack.
>> + */
>> +static int kvm_cpu_has_extint(struct kvm_vcpu *v)
>> +{
>> + if (kvm_apic_accept_pic_intr(v))
>> + return pic_irqchip(v->kvm)->output; /* PIC */
>> + else
>> + return 0;
>> +}
>> +
>> +/*
>> + * check if there is injectable interrupt:
>> + * when virtual interrupt delivery enabled,
>> + * interrupt from apic will handled by hardware,
>> + * we don't need to check it here.
>> + */
>> +int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v)
>> +{
>> + if (!irqchip_in_kernel(v->kvm))
>> + return v->arch.interrupt.pending;
>> +
>> + if (kvm_cpu_has_extint(v))
>> + return 1;
>> + else if (!kvm_apic_vid_enabled(v))
>> + return kvm_apic_has_interrupt(v) != -1; /* LAPIC */
>> +
> I think:
> if (kvm_cpu_has_extint(v))
> return 1;
> if(kvm_apic_vid_enabled(v)) return 0; return
> kvm_apic_has_interrupt(v) != -1; /* LAPIC */
> is clearer.
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;
>> - if (kvm_apic_accept_pic_intr(v) && pic_irqchip(v->kvm)->output)
>> - return kvm_pic_read_irq(v->kvm); /* PIC */
>> + vector = kvm_cpu_get_extint(v);
>> +
>> + if (kvm_apic_vid_enabled(v))
>> + return vector; /* PIC */
>> + else if (vector == -1)
>> + vector = kvm_get_apic_interrupt(v); /* APIC */
>>
> No need "else" here:
> if (kvm_apic_vid_enabled(v) || vector != -1)
> return vector;
> return kvm_get_apic_interrupt(v);
Ok.
>> - return kvm_get_apic_interrupt(v); /* APIC */
>> + return vector;
>> }
>> EXPORT_SYMBOL_GPL(kvm_cpu_get_interrupt);
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 0664c13..0dfbd47 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -236,12 +236,14 @@ static inline void kvm_apic_set_id(struct kvm_lapic
> *apic, u8 id)
>> { apic_set_reg(apic, APIC_ID, id << 24);
>> recalculate_apic_map(apic->vcpu->kvm);
>> + ioapic_update_eoi_exitmap(apic->vcpu->kvm); }
>>
>> static inline void kvm_apic_set_ldr(struct kvm_lapic *apic, u32 id) {
>> apic_set_reg(apic, APIC_LDR, id);
>> recalculate_apic_map(apic->vcpu->kvm);
>> + ioapic_update_eoi_exitmap(apic->vcpu->kvm); }
>>
>> static inline int apic_lvt_enabled(struct kvm_lapic *apic, int lvt_type)
>> @@ -577,6 +579,63 @@ int kvm_apic_match_dest(struct kvm_vcpu *vcpu,
> struct kvm_lapic *source,
>> return result;
>> }
>> +static void kvm_apic_update_eoi_exitmap(struct kvm_vcpu *vcpu,
>> + u32 vector, bool set)
>> +{
>> + kvm_x86_ops->update_eoi_exitmap(vcpu, vector, set);
>> +}
>> +
>> +void kvm_update_eoi_exitmap(struct kvm *kvm, struct kvm_lapic_irq *irq)
>> +{
> We probably should move the whole function into vmx code, not just
> bitmap update logic. Sorry for not mentioning it earlier.
Sure.
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index dcb7952..b501d5a 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -3573,6 +3573,27 @@ static void update_cr8_intercept(struct kvm_vcpu
> *vcpu, int tpr, int irr)
>> set_cr_intercept(svm, INTERCEPT_CR8_WRITE);
>> }
>> +static int svm_has_virtual_interrupt_delivery(struct kvm_vcpu *vcpu)
>> +{
>> + return 0;
>> +}
>> +
>> +static void svm_update_apic_irq(struct kvm_vcpu *vcpu, int max_irr)
>> +{
>> + return ;
>> +}
> You do not need this function any more since caller checks for NULL
> pointer.
>> + .has_virtual_interrupt_delivery = svm_has_virtual_interrupt_delivery,
>> + .update_apic_irq = svm_update_apic_irq;
> Should not be set.
Sure.
>> + status = vmcs_read16(GUEST_INTR_STATUS);
>> + old = (u8)status & 0xff;
>> + if ((u8)vector != old) {
>> + status &= ~0xff;
>> + status |= (u8)vector;
>> + vmcs_write16(GUEST_INTR_STATUS, status);
>> + }
>> +}
>> +
>> +static void vmx_update_apic_irq(struct kvm_vcpu *vcpu, int max_irr)
>> +{
>> + if (!enable_apicv_reg_vid || max_irr == -1)
> No need to check enable_apicv_reg_vid since the function will not be
> called if vid is disabled.
Ok.
>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
>> index cfb7e4d..7c8d9e0 100644
>> --- a/virt/kvm/ioapic.c
>> +++ b/virt/kvm/ioapic.c
>> @@ -115,6 +115,37 @@ 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];
>> +
>> + if ((e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
>> + kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC, pin))) {
>> + struct kvm_lapic_irq irqe;
>> +
>> + irqe.dest_id = e->fields.dest_id;
>> + irqe.vector = e->fields.vector;
>> + irqe.dest_mode = e->fields.dest_mode;
>> + irqe.delivery_mode = e->fields.delivery_mode << 8;
>> + kvm_update_eoi_exitmap(ioapic->kvm, &irqe);
>> + }
>> +}
>> +
> There is a bugs in exitbitmap recalculation that I've missed.
> We need to zero all the exit bitmaps for all vcpus before we are
> starting to recalculate them to get rid of stale bits.
Right. I forget it too. So if consider to clear it, we should call ioapic_update_eoi_exitmap instead ioapic_update_eoi_exitmap_one when programming one program ioapic entry.
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
