Re: [Xen-devel] [PATCH v3 05/39] ARM: timer: Handle level triggered IRQs correctly

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 27 Mar 2018, Andre Przywara wrote:
> Hi,
> 
> On 26/03/18 21:28, Stefano Stabellini wrote:
> > On Wed, 21 Mar 2018, Andre Przywara wrote:
> >> The ARM Generic Timer uses a level-sensitive interrupt semantic. We
> >> easily catch when the line goes high, as this triggers the hardware IRQ.
> >> However we also have to keep track of when the line lowers, as the
> >> emulation depends on it: Upon entering the guest, the new VGIC will
> >> *clear* the virtual interrupt line, so it needs to re-sample the actual
> >> state after returning from the guest.
> >> So we have to sync the state of the interrupt condition at certain
> >> points to catch when the line goes low and we can remove the vtimer vIRQ
> >> from the vGIC (and the LR).
> >> The VGIC in Xen so far only implemented edge triggered vIRQs, really, so
> >> we need to add new functionality to re-sample the interrupt state.
> >> Do this only when the new VGIC is in use.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> >> ---
> >> Changelog v2 ... v3:
> >> - move vtimer_sync() from time.c into vtimer.c
> >> - rename function to vtimer_update_irqs()
> >> - refactor functionality into new static function, to ...
> >> - handle physical timer as well
> >> - extending comments
> >>
> >> Changelog v1 ... v2:
> >> - restrict to new VGIC
> >> - add TODO: comment
> >>
> >>  xen/arch/arm/traps.c         | 11 ++++++++++
> >>  xen/arch/arm/vtimer.c        | 49 
> >> ++++++++++++++++++++++++++++++++++++++++++++
> >>  xen/include/asm-arm/vtimer.h |  1 +
> >>  3 files changed, 61 insertions(+)
> >>
> >> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> >> index 7411bff7a7..2638446693 100644
> >> --- a/xen/arch/arm/traps.c
> >> +++ b/xen/arch/arm/traps.c
> >> @@ -2024,6 +2024,17 @@ static void enter_hypervisor_head(struct 
> >> cpu_user_regs *regs)
> >>          if ( current->arch.hcr_el2 & HCR_VA )
> >>              current->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
> >>  
> >> +#ifdef CONFIG_NEW_VGIC
> >> +        /*
> >> +         * We need to update the state of our emulated devices using level
> >> +         * triggered interrupts before syncing back the VGIC state.
> >> +         *
> >> +         * TODO: Investigate whether this is necessary to do on every
> >> +         * trap and how it can be optimised.
> >> +         */
> >> +        vtimer_update_irqs(current);
> >> +#endif
> >> +
> >>          vgic_sync_from_lrs(current);
> >>      }
> >>  }
> >> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> >> index 8164f6c7f1..c99dd237d1 100644
> >> --- a/xen/arch/arm/vtimer.c
> >> +++ b/xen/arch/arm/vtimer.c
> >> @@ -334,6 +334,55 @@ bool vtimer_emulate(struct cpu_user_regs *regs, union 
> >> hsr hsr)
> >>      }
> >>  }
> >>  
> >> +static void vtimer_update_irq(struct vcpu *v, struct vtimer *vtimer,
> >> +                              uint32_t vtimer_ctl)
> >> +{
> >> +    bool level;
> >> +
> >> +    /* Filter for the three bits that determine the status of the timer */
> >> +    vtimer_ctl &= (CNTx_CTL_ENABLE | CNTx_CTL_PENDING | CNTx_CTL_MASK);
> >> +
> >> +    /* The level is high if the timer is pending and enabled, but not 
> >> masked. */
> >> +    level = (vtimer_ctl == (CNTx_CTL_ENABLE | CNTx_CTL_PENDING));
> >> +
> >> +    /*
> >> +     * This is mostly here to *lower* the virtual interrupt line if the 
> >> timer
> >> +     * is no longer pending.
> >> +     * We would have injected an IRQ already via SOFTIRQ when the timer 
> >> expired.
> >> +     * Doing it here again is basically a NOP if the line was already 
> >> high.
> >> +     */
> >> +    vgic_inject_irq(v->domain, v, vtimer->irq, level);
> >> +}
> >> +
> >> +/**
> >> + * vtimer_update_irqs() - update the virtual timers' IRQ lines after a 
> >> guest run
> >> + * @vcpu: The VCPU to sync the timer state
> >> + *
> >> + * After returning from a guest, update the state of the timers' virtual
> >> + * interrupt lines, to model the level triggered interrupts correctly.
> >> + * If the guest has handled a timer interrupt, the virtual interrupt line
> >> + * needs to be lowered explicitly. vgic_inject_irq() takes care of that.
> >> + */
> >> +void vtimer_update_irqs(struct vcpu *v)
> >> +{
> >> +    /*
> >> +     * For the virtual timer we read the current state from the hardware.
> >> +     * Technically we should keep the CNTx_CTL_MASK bit here, to catch if
> >> +     * the timer interrupt is masked. However Xen *always* masks the timer
> >> +     * upon entering the hypervisor, leaving it up to the guest to 
> >> un-mask it.
> >> +     * So we would always read a "low" level, despite the condition being
> >> +     * actually "high".  Ignoring the mask bit solves this (for now).
> >> +     *
> >> +     * TODO: The proper fix for this is to make vtimer vIRQ hardware 
> >> mapped,
> >> +     * but this requires reworking the arch timer to implement this.
> >> +     */
> >> +    vtimer_update_irq(v, &v->arch.virt_timer,
> >> +                      READ_SYSREG32(CNTV_CTL_EL0) & ~CNTx_CTL_MASK);
> > 
> > Yes, but won't this have the opposite effect? Meaning that it will
> > always read as "high" for the virtual timer (because we remove the MASK
> > and that is the only thing that can cause a "low" read in
> > vtimer_update_irq if it's enabled and pending)?
> 
> What we want to know here is the status of the interrupt line of the
> virtual timer. We don't know if it's still pending or not. So we are
> very much interested in the pending bit of CNTV_CTL_EL0.
> We could read the distributor's ISPENDR register as well, but this is
> more costly.
> 
> > It seems to me that it would be better to remove the update of the
> > virtual timer -- this seems to have the potential of causing problems.
> 
> Removing this makes Dom0 hang very early. The reason is that in that
> case we never clear the line_level in the vtimer's struct vgic_irq:
> When the h/w IRQ fires, we set line_level by injecting the corresponding
> virtual IRQ. But if the emulated line_level is still high,
> vgic_inject_irq() will bail out early, as making an IRQ pending when it
> is already pending is a NOP, so vgic_validate_injection() denies that case.
> 
> Properly emulating the actual state of a virtual level triggered
> interrupt line is something we were totally ignoring so far, because we
> only dealt with edge interrupts. In case of the timer and also the event
> channel this is wrong, as both devices are actually using level
> triggered interrupts semantics.

OK, thanks for the explanation

Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> >> +    /* For the physical timer we rely on our emulated state. */
> >> +    vtimer_update_irq(v, &v->arch.phys_timer, v->arch.phys_timer.ctl);
> >> +}
> >> +
> >>  /*
> >>   * Local variables:
> >>   * mode: C
> >> diff --git a/xen/include/asm-arm/vtimer.h b/xen/include/asm-arm/vtimer.h
> >> index 5aaddc6f63..91d88b377f 100644
> >> --- a/xen/include/asm-arm/vtimer.h
> >> +++ b/xen/include/asm-arm/vtimer.h
> >> @@ -27,6 +27,7 @@ extern bool vtimer_emulate(struct cpu_user_regs *regs, 
> >> union hsr hsr);
> >>  extern int virt_timer_save(struct vcpu *v);
> >>  extern int virt_timer_restore(struct vcpu *v);
> >>  extern void vcpu_timer_destroy(struct vcpu *v);
> >> +void vtimer_update_irqs(struct vcpu *v);
> >>  
> >>  #endif
> >>  
> >> -- 
> >> 2.14.1
> >>
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel