[PATCH v4 3/5] ARM: KVM: arch_timers: Add guest timer core support

Will Deacon will.deacon at arm.com
Fri Nov 23 12:00:36 EST 2012


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



More information about the linux-arm-kernel mailing list