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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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)?

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.


> +    /* 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