Re: [Xen-devel] [RFC PATCH 17/49] ARM: timer: Handle level triggered IRQs correctly

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 09/02/18 14:39, 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.
You might want to make a summary of the discussion we had with Marc Z. 
today here. This would help the other to understand why sample the 
interrupt state is necessary :).
Also do we need to do that for the emulated physical timer?

> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> ---
>   xen/arch/arm/time.c     | 34 ++++++++++++++++++++++++++++++++++
>   xen/arch/arm/traps.c    |  1 +
>   xen/include/xen/timer.h |  2 ++
>   3 files changed, 37 insertions(+)
>
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index c11fcfeadd..98ebb4305d 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -263,6 +263,40 @@ 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);
>   }
>   
> +/**
One * is enough.

> + * 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;
> +    bool level;
> +
> +    vtimer->ctl = READ_SYSREG32(CNTV_CTL_EL0);
> +    vtimer->cval = READ_SYSREG64(CNTV_CVAL_EL0);
Why do you need to save cval?

> +
> +    /*
> +     * Technically we should mask with 0x7 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". Igoring the mask bit solves this (for now).
s/Igoring/Ignoring/

> +     * Another possible check would be to compare the value of CNTVCT_EL0
> +     * against vtimer->cval and derive the interrupt state from that.
> +     *
> +     * TODO: The proper fix for this is to make vtimer vIRQ hardware mapped,
> +     * but this requires reworking the arch timer to implement this.
That something we should look at it once the vGIC is done :).

> +     */
> +    level = (vtimer->ctl & 0x5) == (CNTx_CTL_ENABLE | CNTx_CTL_PENDING);
Can you please use the proper define rather than plain value?

> +
> +    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 1cba7e584d..2d770a14a5 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2024,6 +2024,7 @@ 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);
>   
You need to sample the virtual timer before clearing the LRs, right? If 
so, you likely want to add a comment here to avoid reshuffling the code.
> +        vtimer_sync(current);
I am a bit worry about re-sampling the virtual interrupt state at every 
traps. It might be worth thinking to do the re-sample when syncing the 
LRs (as you do for HW level interrupt in patch #25). Probably once we 
get the new vGIC merged.
>           gic_clear_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