Thanks very much for the review. I'll address the comments.
Some questions on your comments below.
On Mon, Apr 16, 2012 at 01:08:07PM +0300, Gleb Natapov wrote:
> > @@ -37,6 +38,8 @@
> > #define MSR_KVM_SYSTEM_TIME_NEW 0x4b564d01
> > #define MSR_KVM_ASYNC_PF_EN 0x4b564d02
> > #define MSR_KVM_STEAL_TIME 0x4b564d03
> > +#define MSR_KVM_EOI_EN 0x4b564d04
> > +#define MSR_KVM_EOI_DISABLED 0x0L
> This is valid gpa. Follow others MSR example i.e align the address to,
> lets say dword, and use lsb as enable bit.
We only need a single byte, since this is per-CPU -
it's better to save the memory, so no alignment is required.
An explicit disable msr would also address this, right?
> > +static void apic_update_isr(struct kvm_lapic *apic)
> > +{
> > + int vector;
> > + if (!eoi_enabled(apic->vcpu) ||
> > + !apic->vcpu->arch.eoi.pending ||
> > + eoi_get_pending(apic->vcpu))
> > + return;
> > + apic->vcpu->arch.eoi.pending = false;
> > + vector = apic_find_highest_isr(apic);
> > + if (vector == -1)
> > + return;
> > + apic_clear_vector(vector, apic->regs + APIC_ISR);
> > +}
> > +
> We should just call apic_set_eoi() on exit if eoi.pending && !eoi_get_pending().
> This removes the need for the function and its calls.
It's a bit of a waste: that one does all kind extra things
which we know we don't need, some of the atomics. And it's datapath
so extra stuff is not free.
Probably a good idea to replace the call on MSR disable - I think
apic_update_ppr is a better thing to call there.
Is there anything else that I missed?
> We already have
> call to kvm_lapic_sync_from_vapic() on exit path which should be
> extended to do the above.
It already does this. It calls apic_set_tpr
which calls apic_update_ppr which calls
apic_update_isr.
--
MST
--
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
