On Mon, Apr 16, 2012 at 02:24:46PM +0300, Gleb Natapov wrote:
> On Mon, Apr 16, 2012 at 02:09:20PM +0300, Michael S. Tsirkin wrote:
> > 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?
> >
> We do not have shortage of memory.
> Better make all MSRs works the same
> way.
I agree it's nice to have EOI and ASYNC_PF look similar
but wasting memory is also bad. I'll ponder this some more.
> BTW have you added new MSR to msrs_to_save array? I forgot to
> checked.
I didn't yet. Trying to understand how will that affect
cross-version migration - any input?
> > > > +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.
> >
> How much time those extra things are taking compared to vmexit you
> already serving? And there is a good chance you will do them during
> vmentry anyway while trying to inject (or just check for) new interrupt.
No need to do them twice :)
> > 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?
> I think that simple things are better then complex things if the end result is
> the same :) Try it and see how much simpler it is.
It doesn't seem to be simpler at all. The common functionality is
about 4 lines.
> Have you measured
> that what you are trying to optimize actually worth optimizing? That you
> can measure the optimization at all?
The claim is not that it's measureable. The claim is that
it does not scale to keep adding things to do on each entry.
> >
> > > 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.
> >
> It does it only if vapic is in use (and it is usually not).
When it's not we don't need to update ppr and so
no need to update isr on this exit.
> But the if()
> is already there so we do not need to worry that one additional if() on
> the exit path will slow KVM to the crawl.
The number of things we need to do on each entry keeps going up, if we
just keep adding stuff it won't end well.
> --
> 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
