On Fri, Nov 23, 2012 at 04:52:12PM +0000, Marc Zyngier wrote:
> On 23/11/12 16:17, Will Deacon wrote:
> >> diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
> >> index b80256b..7463f5b 100644
> >> --- a/arch/arm/kvm/reset.c
> >> +++ b/arch/arm/kvm/reset.c
> >> @@ -37,6 +37,12 @@ static struct kvm_regs a15_regs_reset = {
> >> .usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT,
> >> };
> >>
> >> +#ifdef CONFIG_KVM_ARM_TIMER
> >> +static const struct kvm_irq_level a15_virt_timer_ppi = {
> >> + { .irq = 27 }, /* irq: A7/A15 specific */
> >
> > This should be parameterised by the vCPU type.
>
> This is already A15 specific, and assigned in an A15 specific code
> section below.
Right, but we can take the interrupt number from the device-tree, like we do
for the host anyway.
> >> +static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
> >> +{
> >> + struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id;
> >> +
> >> + /*
> >> + * We disable the timer in the world switch and let it be
> >> + * handled by kvm_timer_sync_from_cpu(). Getting a timer
> >> + * interrupt at this point is a sure sign of some major
> >> + * breakage.
> >> + */
> >> + pr_warn("Unexpected interrupt %d on vcpu %p\n", irq, vcpu);
> >> + return IRQ_HANDLED;
> >
> > IRQ_NONE?
>
> I don't think so. We're actually handling the interrupt (admittedly in a
> very basic way), and as it is a per-cpu interrupt, there will be noone
> else to take care of it.
For an SPI, returning IRQ_NONE would (eventually) silence a screaming
interrupt because the generic IRQ bits would disable it. I'm not sure if that
applies to PPIs or not but if it does, I'd say that's a good reason to use it.
>
> >> + BUG_ON(timer->armed);
> >> +
> >> + if (cval <= now) {
> >> + /*
> >> + * Timer has already expired while we were not
> >> + * looking. Inject the interrupt and carry on.
> >> + */
> >> + kvm_timer_inject_irq(vcpu);
> >> + return;
> >> + }
> >
> > Does this buy you much? You still have to cope with the timer expiring here
> > anyway.
>
> It definitely does from a latency point of view. Programming a timer
> that will expire right away, calling the interrupt handler, queuing the
> work queue, waiting for the workqueue to be scheduled and finally
> delivering the interrupt... If we can catch a few of these early (and we
> do), it is worth it.
Ok, interesting. I wasn't sure how often that happened in practice.
> >> +int kvm_timer_init(struct kvm *kvm)
> >> +{
> >> + if (timecounter && wqueue) {
> >> + kvm->arch.timer.cntvoff = kvm_phys_timer_read();
> >
> > Shouldn't this be initialised to 0 and then updated on world switch?
>
> No. You do not want your virtual offset to drift. Otherwise you'll
> observe something like time dilatation, and your clocks will drift.
> Plus, you really want all your vcpus to be synchronized. Allowing them
> to drift apart could be an interesting experience... ;-)
In which case, why do we initialise it to the physical timer in the first
place? Surely the value doesn't matter, as long as everybody agrees on what
it is?
Will
--
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
