On Tue, Apr 10, 2012 at 10:30:04PM +0300, Michael S. Tsirkin wrote:
> On Tue, Apr 10, 2012 at 08:59:21PM +0300, Gleb Natapov wrote:
> > Heh, I was working on that too.
> >
> > On Tue, Apr 10, 2012 at 05:26:18PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, Apr 10, 2012 at 05:03:22PM +0300, Avi Kivity wrote:
> > > > On 04/10/2012 04:27 PM, Michael S. Tsirkin wrote:
> > > > > I took a stub at implementing PV EOI using shared memory.
> > > > > This should reduce the number of exits an interrupt
> > > > > causes as much as by half.
> > > > >
> > > > > A partially complete draft for both host and guest parts
> > > > > is below.
> > > > >
> > > > > The idea is simple: there's a bit, per APIC, in guest memory,
> > > > > that tells the guest that it does not need EOI.
> > > > > We set it before injecting an interrupt and clear
> > > > > before injecting a nested one. Guest tests it using
> > > > > a test and clear operation - this is necessary
> > > > > so that host can detect interrupt nesting -
> > > > > and if set, it can skip the EOI MSR.
> > > > >
> > > > > There's a new MSR to set the address of said register
> > > > > in guest memory. Otherwise not much changes:
> > > > > - Guest EOI is not required
> > > > > - ISR is automatically cleared before injection
> > > > >
> > > > > Some things are incomplete: add feature negotiation
> > > > > options, qemu support for said options.
> > > > > No testing was done beyond compiling the kernel.
> > > > >
> > > > > I would appreciate early feedback.
> > > > >
> > > > > Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > > > >
> > > > > --
> > > > >
> > > > > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> > > > > index d854101..8430f41 100644
> > > > > --- a/arch/x86/include/asm/apic.h
> > > > > +++ b/arch/x86/include/asm/apic.h
> > > > > @@ -457,8 +457,13 @@ static inline u32 safe_apic_wait_icr_idle(void) { return 0; }
> > > > >
> > > > > #endif /* CONFIG_X86_LOCAL_APIC */
> > > > >
> > > > > +DECLARE_EARLY_PER_CPU(unsigned long, apic_eoi);
> > > > > +
> > > > > static inline void ack_APIC_irq(void)
> > > > > {
> > > > > + if (__test_and_clear_bit(0, &__get_cpu_var(apic_eoi)))
> > > > > + return;
> > > > > +
> > > >
> > > > While __test_and_clear_bit() is implemented in a single instruction,
> > > > it's not required to be. Better have the instruction there explicitly.
> > > >
> > > > > /*
> > > > > * ack_APIC_irq() actually gets compiled as a single instruction
> > > > > * ... yummie.
> > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > > > index e216ba0..0ee1472 100644
> > > > > --- a/arch/x86/include/asm/kvm_host.h
> > > > > +++ b/arch/x86/include/asm/kvm_host.h
> > > > > @@ -481,6 +481,12 @@ struct kvm_vcpu_arch {
> > > > > u64 length;
> > > > > u64 status;
> > > > > } osvw;
> > > > > +
> > > > > + struct {
> > > > > + u64 msr_val;
> > > > > + struct gfn_to_hva_cache data;
> > > > > + int vector;
> > > > > + } eoi;
> > > > > };
> > > >
> > > > Needs to be cleared on INIT.
> > >
> > > You mean kvm_arch_vcpu_reset?
> > >
> > > > >
> > > > >
> > > > > @@ -307,6 +308,9 @@ void __cpuinit kvm_guest_cpu_init(void)
> > > > > smp_processor_id());
> > > > > }
> > > > >
> > > > > + wrmsrl(MSR_KVM_EOI_EN, __pa(this_cpu_ptr(apic_eoi)) |
> > > > > + MSR_KVM_EOI_ENABLED);
> > > > > +
> > > >
> > > > Clear on kexec.
> > >
> > > With register_reboot_notifier?
> > >
> > > > > if (has_steal_clock)
> > > > > kvm_register_steal_time();
> > > > > }
> > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > > > index 8584322..9e38e12 100644
> > > > > --- a/arch/x86/kvm/lapic.c
> > > > > +++ b/arch/x86/kvm/lapic.c
> > > > > @@ -265,7 +265,61 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq)
> > > > > irq->level, irq->trig_mode);
> > > > > }
> > > > >
> > > > > -static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> > > > > +static int eoi_put_user(struct kvm_vcpu *vcpu, u32 val)
> > > > > +{
> > > > > +
> > > > > + return kvm_write_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, &val,
> > > > > + sizeof(val));
> > > > > +}
> > > > > +
> > > > > +static int eoi_get_user(struct kvm_vcpu *vcpu, u32 *val)
> > > > > +{
> > > > > +
> > > > > + return kvm_read_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, val,
> > > > > + sizeof(*val));
> > > > > +}
> > > > > +
> > > > > +static inline bool eoi_enabled(struct kvm_vcpu *vcpu)
> > > > > +{
> > > > > + return (vcpu->arch.eoi.msr_val & MSR_KVM_EOI_ENABLED);
> > > > > +}
> > > > > +
> > > > > +static int eoi_get_pending_vector(struct kvm_vcpu *vcpu)
> > > > > +{
> > > > > + u32 val;
> > > > > + if (eoi_get_user(vcpu, &val) < 0)
> > > > > + apic_debug("Can't read EOI MSR value: 0x%llx\n",
> > > > > + (unsigned long long)vcpi->arch.eoi.msr_val);
> > > > > + if (!(val & 0x1))
> > > > > + vcpu->arch.eoi.vector = -1;
> > > > > + return vcpu->arch.eoi.vector;
> > > > > +}
> > > > > +
> > > > > +static void eoi_set_pending_vector(struct kvm_vcpu *vcpu, int vector)
> > > > > +{
> > > > > + BUG_ON(vcpu->arch.eoi.vector != -1);
> > > > > + if (eoi_put_user(vcpu, 0x1) < 0) {
> > > > > + apic_debug("Can't set EOI MSR value: 0x%llx\n",
> > > > > + (unsigned long long)vcpi->arch.eoi.msr_val);
> > > > > + return;
> > > > > + }
> > > > > + vcpu->arch.eoi.vector = vector;
> > > > > +}
> > > > > +
> > > > > +static int eoi_clr_pending_vector(struct kvm_vcpu *vcpu)
> > > > > +{
> > > > > + int vector;
> > > > > + vector = vcpu->arch.eoi.vector;
> > > > > + if (vector != -1 && eoi_put_user(vcpu, 0x0) < 0) {
> > > > > + apic_debug("Can't clear EOI MSR value: 0x%llx\n",
> > > > > + (unsigned long long)vcpi->arch.eoi.msr_val);
> > > > > + return -1;
> > > > > + }
> > > > > + vcpu->arch.eoi.vector = -1;
> > > > > + return vector;
> > > > > +}
> > > >
> > > >
> > > >
> > > > > +
> > > > > +static inline int __apic_find_highest_isr(struct kvm_lapic *apic)
> > > > > {
> > > > > int result;
> > > > >
> > > > > @@ -275,6 +329,17 @@ static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> > > > > return result;
> > > > > }
> > > > >
> > > > > +static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> > > > > +{
> > > > > + int vector;
> > > > > + if (eoi_enabled(apic->vcpu)) {
> > > > > + vector = eoi_get_pending_vector(apic->vcpu);
> > > > > + if (vector != -1)
> > > > > + return vector;
> > > > > + }
> > > > > + return __apic_find_highest_isr(apic);
> > > > > +}
> > > >
> > > > Why aren't you modifying the ISR unconfitionally?
> > >
> > > ISR is not set if there won't be an EOI
> > > since it's EOI that normally clears it.
> > >
> > > > > +
> > > > > static void apic_update_ppr(struct kvm_lapic *apic)
> > > > > {
> > > > > u32 tpr, isrv, ppr, old_ppr;
> > > > > @@ -488,6 +553,8 @@ static void apic_set_eoi(struct kvm_lapic *apic)
> > > > > if (vector == -1)
> > > > > return;
> > > > >
> > > > > + if (eoi_enabled(apic->vcpu))
> > > > > + eoi_clr_pending_vector(apic->vcpu);
> > > > > apic_clear_vector(vector, apic->regs + APIC_ISR);
> > > > > apic_update_ppr(apic);
> > > > >
> > > > > @@ -1236,11 +1303,25 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
> > > > > {
> > > > > int vector = kvm_apic_has_interrupt(vcpu);
> > > > > struct kvm_lapic *apic = vcpu->arch.apic;
> > > > > + bool set_isr = true;
> > > > >
> > > > > if (vector == -1)
> > > > > return -1;
> > > > >
> > > > > - apic_set_vector(vector, apic->regs + APIC_ISR);
> > > > > + if (eoi_enabled(vcpu)) {
> > > > > + /* Anything pending? If yes disable eoi optimization. */
> > > > > + if (unlikely(apic_find_highest_isr(apic) >= 0)) {
> > > > > + int v = eoi_clr_pending_vector(vcpu);
> > > >
> > > > ISR != pending, that's IRR. If ISR vector has lower priority than the
> > > > new vector, then we don't need to disable eoi avoidance.
> > >
> > > Yes. But we can and it's easier than figuring out priorities.
> > > I am guessing such collisions are rare, right?
> > > I'll add a trace to make sure.
> > >
> > > > > + if (v != -1)
> > > > > + apic_set_vector(v, apic->regs + APIC_ISR);
> > > > > + } else {
> > > > > + eoi_set_pending_vector(vcpu, vector);
> > > > > + set_isr = false;
> > > >
> > > > Weird. Just set it normally. Remember that reading the ISR needs to
> > > > return the correct value.
> > >
> > > Marcelo said linux does not normally read ISR - not true?
> > > Note this has no effect if the PV optimization is not enabled.
> > >
> > > > We need to process the avoided EOI before any APIC read/writes, to be
> > > > sure the guest sees the updated values. Same for IOAPIC, EOI affects
> > > > remote_irr. That may been we need to sample it after every exit, or
> > > > perhaps disable the feature for level-triggered interrupts.
> > >
> > > Disabling would be very sad. Can we sample on remote irr read?
> > >
> > Nothing sad about it, just correct. MS requires this to be disabled for
> > level triggered interrupts.
>
> We don't try to match what HV does 100% anyway.
>
We should. The same code will be used for HV.
> > We have to notify IOAPIC about EOI ASAP. It
> > may hold another interrupt for us that has to be delivered.
>
> You mean the ack notifiers? We can skip just for the vectors
> which have ack notifiers or only if there are no notifiers.
>
No. I mean:
if (!ent->fields.mask && (ioapic->irr & (1 << i)))
ioapic_service(ioapic, i);
> > I was going to avoid most of the trickery in apic code and just check if
> > avoided EOI should be processed on each exit. This adds one if on exit
> > path instead of couple on interrupt injection path though.
> >
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + if (set_isr)
> > > > > + apic_set_vector(vector, apic->regs + APIC_ISR);
> > > > > apic_update_ppr(apic);
> > > > > apic_clear_irr(vector, apic);
> > > > > return vector;
> > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > >
> > > >
> > > > --
> > > > error compiling committee.c: too many arguments to function
> > > --
> > > 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
> >
> > --
> > Gleb.
--
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
