Gleb Natapov wrote on 2013-01-07:
> On Mon, Jan 07, 2013 at 10:02:36AM +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>
>> ---
>> arch/ia64/kvm/lapic.h | 6 ++
>> arch/x86/include/asm/kvm_host.h | 8 ++ arch/x86/include/asm/vmx.h
>> | 11 +++ arch/x86/kvm/irq.c | 56 +++++++++++-
>> arch/x86/kvm/lapic.c | 87 +++++++++++-------
>> arch/x86/kvm/lapic.h | 29 +++++- arch/x86/kvm/svm.c
>> | 36 ++++++++ arch/x86/kvm/vmx.c | 190
>> ++++++++++++++++++++++++++++++++++++++- arch/x86/kvm/x86.c
>> | 11 ++- include/linux/kvm_host.h | 2 + virt/kvm/ioapic.c
>> | 41 +++++++++ virt/kvm/ioapic.h | 1
>> + virt/kvm/irq_comm.c | 20 ++++ 13 files changed, 451
>> insertions(+), 47 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 c431b33..135603f 100644 ---
>> a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -697,6 +697,13 @@ 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_apic_irq)(struct kvm_vcpu *vcpu, int max_irr);
>> + void (*update_eoi_exitmap)(struct kvm *kvm, struct kvm_lapic_irq *irq);
>> + void (*update_exitmap_start)(struct kvm_vcpu *vcpu);
>> + void (*update_exitmap_end)(struct kvm_vcpu *vcpu);
>> + void (*load_eoi_exitmap)(struct kvm_vcpu *vcpu);
> The amount of callbacks to update exit bitmap start to become insane.
As your suggestion below, if using global lock, then three callbacks is enough
>> + void (*restore_rvi)(struct kvm_vcpu *vcpu);
> rvi? Call it set_svi() and make it do just that - set svi.
Typo.
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 0664c13..e1baf37 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -133,6 +133,12 @@ static inline int apic_enabled(struct kvm_lapic *apic)
>> return kvm_apic_sw_enabled(apic) && kvm_apic_hw_enabled(apic);
>> }
>> +bool kvm_apic_present(struct kvm_vcpu *vcpu) +{ + return
>> kvm_vcpu_has_lapic(vcpu) && kvm_apic_hw_enabled(vcpu->arch.apic); +}
>> +EXPORT_SYMBOL_GPL(kvm_apic_present); +
> Why is this change? Drop it.
I cannot remember why. But it seems this change is needless now.
>> #define LVT_MASK \
>> (APIC_LVT_MASKED | APIC_SEND_PENDING | APIC_VECTOR_MASK)
>> @@ -150,23 +156,6 @@ static inline int kvm_apic_id(struct kvm_lapic *apic)
>> return (kvm_apic_get_reg(apic, APIC_ID) >> 24) & 0xff;
>> }
>> -static inline u16 apic_cluster_id(struct kvm_apic_map *map, u32 ldr)
>> -{
>> - u16 cid;
>> - ldr >>= 32 - map->ldr_bits;
>> - cid = (ldr >> map->cid_shift) & map->cid_mask;
>> -
>> - BUG_ON(cid >= ARRAY_SIZE(map->logical_map));
>> -
>> - return cid;
>> -}
>> -
>> -static inline u16 apic_logical_id(struct kvm_apic_map *map, u32 ldr)
>> -{
>> - ldr >>= (32 - map->ldr_bits);
>> - return ldr & map->lid_mask;
>> -}
>> -
>> static void recalculate_apic_map(struct kvm *kvm)
>> {
>> struct kvm_apic_map *new, *old = NULL;
>> @@ -236,12 +225,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)
>> @@ -345,6 +336,9 @@ static inline int apic_find_highest_irr(struct kvm_lapic
> *apic)
>> {
>> int result;
>> + /* Note that irr_pending is just a hint. In the platform
>> + * that has virtual interrupt delivery supporting, virr
>> + * is cleared by hardware without updating irr_pending. */
> Just say that with vid enables irr_pending will be always true.
Sure.
>> if (!apic->irr_pending)
>> return -1;
>> @@ -458,9 +452,13 @@ static void pv_eoi_clr_pending(struct kvm_vcpu
> *vcpu)
>> __clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention);
>> }
>> -static inline int apic_find_highest_isr(struct kvm_lapic *apic)
>> +int kvm_apic_find_highest_isr(struct kvm_lapic *apic)
>> {
>> int result;
>> +
>> + /* Note that isr_count is just a hint. In the platform
>> + * that has virtual interrupt delivery supporting, visr
>> + * will be set by hardware without updating irr_pending. */
> isr_count is not a hint! Without vid it has to be precise. Again just
> say that with vid enabled isr_count is always 1.
Ok.
>> @@ -1318,6 +1338,7 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64
> value)
>> else static_key_slow_inc(&apic_hw_disabled.key);
>> recalculate_apic_map(vcpu->kvm);
>> + ioapic_update_eoi_exitmap(apic->vcpu->kvm); }
>>
>> if (!kvm_vcpu_is_bsp(apic->vcpu)) @@ -1375,7 +1396,7 @@ void
>> kvm_lapic_reset(struct kvm_vcpu *vcpu) apic_set_reg(apic, APIC_TMR +
>> 0x10 * i, 0); } apic->irr_pending = false;
> irr_pending should be set to true if vid is enabled.
Yes, it must be true when vid is enabled.
>> @@ -433,6 +433,9 @@ struct vcpu_vmx {
>>
>> bool rdtscp_enabled;
>> + unsigned long eoi_exit_bitmap[4];
>> + spinlock_t eoi_bitmap_lock;
> Why not mutex?
Sure. mutex is a better choice.
>> @@ -6099,6 +6138,142 @@ static void update_cr8_intercept(struct kvm_vcpu
> *vcpu, int tpr, int irr)
>> vmcs_write32(TPR_THRESHOLD, irr);
>> }
>> +static int vmx_has_virtual_interrupt_delivery(struct kvm_vcpu *vcpu)
>> +{
>> + return enable_apicv_reg_vid;
>> +}
>> +
>> +static void vmx_restore_rvi(struct kvm_vcpu *vcpu)
> This has nothing to do with rvi. Call it set_svi() pass it highest isr
> as a parameter. Drop apic_find_highest_isr() renaming.
Ok.
>> +{
>> + int isr;
>> + u16 status;
>> + u8 old;
>> +
>> + if (!enable_apicv_reg_vid)
>> + return;
>> +
>> + isr = kvm_apic_find_highest_isr(vcpu->arch.apic);
>> + if (isr == -1)
>> + return;
> If there is not isr set svi should be set to zero.
Right.
>> +
>> + status = vmcs_read16(GUEST_INTR_STATUS);
>> + old = status >> 8;
>> + if (isr != old) {
>> + status &= 0xff;
>> + status |= isr << 8;
>> + vmcs_write16(GUEST_INTR_STATUS, status);
>> + }
>> +}
>> +
>> +static void vmx_update_rvi(int vector)
> set_rvi() for consistency.
Ok.
>> +{
>> + u16 status;
>> + u8 old;
>> +
>> + 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 (max_irr == -1)
>> + return;
>> +
>> + vmx_update_rvi(max_irr);
>> +}
>> +
>> +static void set_eoi_exitmap_one(struct kvm_vcpu *vcpu,
>> + u32 vector)
>> +{
>> + struct vcpu_vmx *vmx = to_vmx(vcpu);
>> +
>> + if (!enable_apicv_reg_vid)
>> + return;
>> +
>> + if (WARN_ONCE((vector > 255),
>> + "KVM VMX: vector (%d) out of range\n", vector))
>> + return;
>> +
>> + set_bit(vector, vmx->eoi_exit_bitmap);
> Since eoi_exit_bitmap is accessed under lock __set_bit() can be used
> here.
Right.
>> +
>> + kvm_make_request(KVM_REQ_EOIBITMAP, vcpu);
>> +}
>> +
>> +void vmx_update_eoi_exitmap(struct kvm *kvm, struct kvm_lapic_irq *irq)
>> +{
>> + struct kvm_vcpu *vcpu;
>> + struct kvm_lapic **dst;
>> + struct kvm_apic_map *map;
>> + unsigned long bitmap = 1;
>> + int i;
>> +
>> + rcu_read_lock();
>> + map = rcu_dereference(kvm->arch.apic_map);
>> +
>> + if (unlikely(!map)) {
>> + kvm_for_each_vcpu(i, vcpu, kvm)
>> + set_eoi_exitmap_one(vcpu, irq->vector);
>> + goto out;
>> + }
>> +
>> + if (irq->dest_mode == 0) { /* physical mode */
>> + if (irq->delivery_mode == APIC_DM_LOWEST ||
>> + irq->dest_id == 0xff) {
>> + kvm_for_each_vcpu(i, vcpu, kvm)
>> + set_eoi_exitmap_one(vcpu, irq->vector);
>> + goto out;
>> + }
>> + dst = &map->phys_map[irq->dest_id & 0xff];
>> + } else {
>> + u32 mda = irq->dest_id << (32 - map->ldr_bits);
>> +
>> + dst = map->logical_map[apic_cluster_id(map, mda)];
>> +
>> + bitmap = apic_logical_id(map, mda);
>> + }
>> +
>> + for_each_set_bit(i, &bitmap, 16) {
>> + if (!dst[i])
>> + continue;
>> + set_eoi_exitmap_one(dst[i]->vcpu, irq->vector);
>> + }
>> +
>> +out:
>> + rcu_read_unlock();
>> +}
>> +
>> +static void vmx_update_exitmap_start(struct kvm_vcpu *vcpu)
>> +{
>> + struct vcpu_vmx *vmx = to_vmx(vcpu);
>> +
>> + memset(vmx->eoi_exit_bitmap, 0, 32);
>> + spin_lock(&vmx->eoi_bitmap_lock);
> So you introduced the lock and access the thing that the lock should protect
> outside of it?
It's really a low-level mistake. I just copy it from other function and don't know why it will paste behind memset.
>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
>> index f3abbef..4341ad3 100644
>> --- a/virt/kvm/ioapic.c
>> +++ b/virt/kvm/ioapic.c
>> @@ -115,6 +115,46 @@ 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_x86_ops->update_eoi_exitmap(ioapic->kvm, &irqe);
>> + }
>> +}
>> +
>> +void ioapic_update_eoi_exitmap(struct kvm *kvm)
>> +{
>> + struct kvm_ioapic *ioapic = kvm->arch.vioapic;
>> + union kvm_ioapic_redirect_entry *e;
>> + int index;
>> + struct kvm_vcpu *vcpu;
>> +
> The function should return right here if vid is not enabled.
Right.
>> + kvm_for_each_vcpu(index, vcpu, kvm)
>> + kvm_x86_ops->update_exitmap_start(vcpu);
>> +
>> + for (index = 0; index < IOAPIC_NUM_PINS; index++) {
>> + e = &ioapic->redirtbl[index];
>> + if (!e->fields.mask)
>> + ioapic_update_eoi_exitmap_one(ioapic, index);
>> + }
>> +
>> + kvm_for_each_vcpu(index, vcpu, kvm) {
>> + kvm_x86_ops->update_exitmap_end(vcpu);
>> + kvm_vcpu_kick(vcpu);
>> + }
> Since you take all of the locks at the start of the bitmap update and release
> them at the end why not have global lock?
Right. Global lock is more reasonable.
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
