Re: [PATCHv1 dont apply] RFC: kvm eoi PV using shared memory

["Michael S. Tsirkin" <mst@redhat.com>]

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