Re: [Xen-devel] [PATCH 24/57] ARM: timer: Handle level triggered IRQs correctly

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 05/03/18 16:03, 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 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.
As requested on the previous series, can you please explain in the 
commit message/code why sampling is necessary?
Also, do we need to do that for the emulated physical timer?

> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> ---
> Changelog RFC ... v1:
> - extend comments
> - don't read CNTV_CVAL_EL0
> - use symbolic names for constants
>
>   xen/arch/arm/time.c     | 36 ++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/traps.c    |  6 ++++++
>   xen/include/xen/timer.h |  2 ++
>   3 files changed, 44 insertions(+)
>
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index c11fcfeadd..c0ae781ecd 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -263,6 +263,42 @@ static void vtimer_interrupt(int irq, void *dev_id, struct 
> cpu_user_regs *regs)
>       vgic_inject_irq(current->domain, current, current->arch.virt_timer.irq, 
> true);
>   }
>   
> +/**
> + * vtimer_sync() - update the state of the virtual timer after a guest run
> + * @vcpu: The VCPU to sync the arch timer state
> + *
> + * After returning from a guest, update the state of the virtual interrupt
> + * line, to model the level triggered interrupt 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_sync(struct vcpu *vcpu)
> +{
> +    struct vtimer *vtimer = &vcpu->arch.virt_timer;
> +    uint32_t vtimer_ctl = READ_SYSREG32(CNTV_CTL_EL0);
> +    bool level;
> +
> +    /*
> +     * Technically the mask should include 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).
> +     * Another possible check would be to compare the value of CNTVCT_EL0
> +     * against vtimer->cval and derive the interrupt state from that.
> +     */
> +    vtimer_ctl &= (CNTx_CTL_ENABLE | CNTx_CTL_PENDING);
> +    level = (vtimer_ctl == (CNTx_CTL_ENABLE | CNTx_CTL_PENDING));
> +
> +     /*
> +      * TODO: The proper fix for this is to make vtimer vIRQ hardware mapped,
> +      * but this requires reworking the arch timer to implement this.
> +      */
The comment seems to be offset by one space.

> +
> +    vgic_inject_irq(vcpu->domain, vcpu, vtimer->irq, level);
> +}
> +
>   /*
>    * Arch timer interrupt really ought to be level triggered, since the
>    * design of the timer/comparator mechanism is based around that
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 7411bff7a7..0713723bb7 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2024,6 +2024,12 @@ 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);
>   
> +        /*
> +         * We need to update the state of our emulated devices using level
> +         * triggered interrupts before syncing back the VGIC state.
> +         */
> +        vtimer_sync(current);
Am I wondering if it would be worth to #ifdef the code so it is only 
used for the new vGIC? After all, this will be a nop for it, right?
> +
>           vgic_sync_from_lrs(current);
>       }
>   }
> diff --git a/xen/include/xen/timer.h b/xen/include/xen/timer.h
> index 4513260b0d..eddbbf3903 100644
> --- a/xen/include/xen/timer.h
> +++ b/xen/include/xen/timer.h
> @@ -94,6 +94,8 @@ DECLARE_PER_CPU(s_time_t, timer_deadline);
>   /* Arch-defined function to reprogram timer hardware for new deadline. */
>   int reprogram_timer(s_time_t timeout);
>   
> +void vtimer_sync(struct vcpu *vcpu);
> +
>   /* Calculate the aligned first tick time for a given periodic timer. */
>   s_time_t align_timer(s_time_t firsttick, uint64_t period);
>   
Cheers,

--
Julien Grall

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