On Fri, Dec 21, 2012 at 09:51:40AM +0200, Gleb Natapov wrote:
> On Thu, Dec 20, 2012 at 08:59:11PM -0200, Marcelo Tosatti wrote:
> > On Mon, Dec 17, 2012 at 01:30:49PM +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 | 6 ++
> > > arch/x86/include/asm/vmx.h | 11 +++
> > > arch/x86/kvm/irq.c | 56 +++++++++++++-
> > > arch/x86/kvm/lapic.c | 65 ++++++++++-------
> > > arch/x86/kvm/lapic.h | 28 ++++++-
> > > arch/x86/kvm/svm.c | 24 ++++++
> > > arch/x86/kvm/vmx.c | 154 ++++++++++++++++++++++++++++++++++++++-
> > > arch/x86/kvm/x86.c | 11 ++-
> > > include/linux/kvm_host.h | 2 +
> > > virt/kvm/ioapic.c | 36 +++++++++
> > > virt/kvm/ioapic.h | 1 +
> > > virt/kvm/irq_comm.c | 20 +++++
> > > 13 files changed, 379 insertions(+), 41 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..b63a144 100644
> > > --- a/arch/x86/include/asm/kvm_host.h
> > > +++ b/arch/x86/include/asm/kvm_host.h
> > > @@ -697,6 +697,11 @@ 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 (*reset_eoi_exitmap)(struct kvm_vcpu *vcpu);
> > > + void (*load_eoi_exitmap)(struct kvm_vcpu *vcpu);
> > > int (*set_tss_addr)(struct kvm *kvm, unsigned int addr);
> > > int (*get_tdp_level)(void);
> > > u64 (*get_mt_mask)(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio);
> >
> > EOI exit bitmap is problematic (its racy). Please do this:
> >
> > 1. Make a synchronous (1) KVM_REQ_EOIBITMAP request on IOAPIC register
> > modifications which require EOI exit bitmap updates.
> > 2. On VM-entry, during KVM_REQ_EOIBITMAP processing, each checks IOAPIC map
> > and adjusts its own EOI exit bitmap VMCS registers.
> >
> > 1) that waits until remote executing VCPUs have acknowledge the request,
> > using make_all_cpus_request (see virt/kvm/kvm_main.c), similarly to
> > remote TLB flushes.
> >
> > What is the problem now: there is no control over _when_ a VCPU updates
> > its EOI exit bitmap VMCS register from the (remotely updated) master EOI
> > exit bitmap. The VCPU can be processing a KVM_REQ_EOIBITMAP relative to
> > a precedence IOAPIC register write while the current IOAPIC register
> > write is updating the EOI exit bitmap. There is no way to fix that
> > without locking (which can be avoided if the IOAPIC->EOI exit bitmap
> > synchronization is vcpu local).
> >
> The race is benign. We have similar one for interrupt injection and the same race
> exists on a real HW. The two cases that can happen due to the race are:
>
> 1. exitbitmap bit X is changed from 1 to 0
> No problem. It is harmless to do an exit, on the next entry exitbitmap
> will be fixed.
> 2. exitbitmap bit X is changed from 0 to 1
> If vcpu serves X at the time this happens it was delivered as edge, so
> no need to exit. The exitbitmap will be updated after the next vmexit
> which will happen due to KVM_REQ_EOIBITMAP processing.
1. Missed the case where bitmap is being reset (where EOI exit bitmaps
are zeroed). In this case vcpu enters guest with incorrect EOI exit
bitmap.
2. Missed the case where current code allows vcpu to enter guest
with EOI exit bitmap unsynchronized relative to IOAPIC registers
(see one KVM_REQ made at a time, no IPI sent). In that case interrupt
can be delivered.
Thus the suggestions to update bitmap locally, on entry. Do you
see any disadvantage?
Other than that, there is a window between IOAPIC map update and
EOI bitmap request, where an interrupt can be delivered without
EOI bitmap being updated (which i think local updates don't cover,
either).
> But software really should take care of not changing interrupt vector
> configuration while there is an interrupt in flight with the same vector.
None of these are guest faults. As soon as interrupts are allowed they
must be handled properly (including synchronized EOI bitmap etc).
>
> > Questions below.
> >
> > > @@ -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);
> > > }
> >
> > Why is it necessary to update EOI exit bitmap here?
> >
> All the places where apic_map, which is used to build exit
> bitmap, can change should call ioapic_update_eoi_exitmap().
>
> > > 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);
> > > }
> >
> > And here?
> >
> > > +/*
> > > + * this interface assumes a trap-like exit, which has already finished
> > > + * desired side effect including vISR and vPPR update.
> > > + */
> > > +void kvm_apic_set_eoi_accelerated(struct kvm_vcpu *vcpu, int vector)
> > > +{
> > > + struct kvm_lapic *apic = vcpu->arch.apic;
> > > +
> > > + trace_kvm_eoi(apic, vector);
> > > +
> > > + kvm_ioapic_send_eoi(apic, vector);
> > > + kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
> > > +}
> >
> > Why is it necessary to set KVM_REQ_EVENT here?
> >
> > > apic_set_reg(apic, APIC_DFR, val | 0x0FFFFFFF);
> > > recalculate_apic_map(apic->vcpu->kvm);
> > > + ioapic_update_eoi_exitmap(apic->vcpu->kvm);
> > > } else
> > > ret = 1;
> > > break;
> > > @@ -1318,6 +1330,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);
> > > }
> >
> > Again, why all this EOI exit bitmap updates?
> >
> > > +static void svm_update_eoi_exitmap(struct kvm *kvm, struct kvm_lapic_irq *irq)
> > > +{
> > > + return ;
> > > +}
> > > +
> > > +static void svm_reset_eoi_exitmap(struct kvm_vcpu *vcpu)
> > > +{
> > > + return ;
> > > +}
> > > +
> > > +static void svm_load_eoi_exitmap(struct kvm_vcpu *vcpu)
> > > +{
> > > + return ;
> > > +}
> >
> > Please mind coding style.
> >
> > > +static void vmx_update_apic_irq(struct kvm_vcpu *vcpu, int max_irr)
> > > +{
> > > + if (max_irr == -1)
> > > + return ;
> >
> > Coding style.
>
> --
> 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
--
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
