Re: [Xen-devel] [PATCH v3 1/2] xen/arm: remove physical timer offset

[Julien Grall <julien@xen.org>]

Hi Jeff,

On 11/12/2019 21:13, Jeff Kubascik wrote:
> The physical timer traps apply an offset so that time starts at 0 for
> the guest. However, this offset is not currently applied to the physical
> counter. Per the ARMv8 Reference Manual (ARM DDI 0487E.a), section
> D11.2.4 Timers, the "Offset" between the counter and timer should be
> zero for a physical timer. This removes the offset to make the timer and
> counter consistent.
> 
> This also cleans up the physical timer implementation to better match
> the virtual timer - both cval's now hold the hardware value.
> 
> Signed-off-by: Jeff Kubascik <jeff.kubascik@dornerworks.com>
> ---
>   xen/arch/arm/vtimer.c        | 34 ++++++++++++++++++----------------
>   xen/include/asm-arm/domain.h |  3 ---
>   2 files changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index e6aebdac9e..21b98ec20a 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -62,7 +62,6 @@ static void virt_timer_expired(void *data)
>   
>   int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
>   {
> -    d->arch.phys_timer_base.offset = NOW();
>       d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
>       d->time_offset_seconds = ticks_to_ns(d->arch.virt_timer_base.offset - boot_count);
>       do_div(d->time_offset_seconds, 1000000000);
> @@ -108,7 +107,6 @@ int vcpu_vtimer_init(struct vcpu *v)
>   
>       init_timer(&t->timer, phys_timer_expired, t, v->processor);
>       t->ctl = 0;
> -    t->cval = NOW();
>       t->irq = d0
>           ? timer_get_irq(TIMER_PHYS_NONSECURE_PPI)
>           : GUEST_TIMER_PHYS_NS_PPI;
> @@ -167,6 +165,7 @@ void virt_timer_restore(struct vcpu *v)
>   static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t *r, bool read)
>   {
>       struct vcpu *v = current;
> +    s_time_t expires;
>   
>       if ( !ACCESS_ALLOWED(regs, EL0PTEN) )
>           return false;
> @@ -184,8 +183,9 @@ static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t *r, bool read)
>   
>           if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
>           {
> -            set_timer(&v->arch.phys_timer.timer,
> -                      v->arch.phys_timer.cval + v->domain->arch.phys_timer_base.offset);
> +            expires = v->arch.phys_timer.cval > boot_count
> +                      ? ticks_to_ns(v->arch.phys_timer.cval - boot_count) : 0;
> +            set_timer(&v->arch.phys_timer.timer, expires);
>           }
>           else
>               stop_timer(&v->arch.phys_timer.timer);
> @@ -197,26 +197,27 @@ static bool vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r,
>                                bool read)
>   {
>       struct vcpu *v = current;
> -    s_time_t now;
> +    uint64_t cntpct;
> +    s_time_t expires;
>   
>       if ( !ACCESS_ALLOWED(regs, EL0PTEN) )
>           return false;
>   
> -    now = NOW() - v->domain->arch.phys_timer_base.offset;
> +    cntpct = get_cycles();
>   
>       if ( read )
>       {
> -        *r = (uint32_t)(ns_to_ticks(v->arch.phys_timer.cval - now) & 0xffffffffull);
> +        *r = (uint32_t)((v->arch.phys_timer.cval - cntpct) & 0xffffffffull);
>       }
>       else
>       {
> -        v->arch.phys_timer.cval = now + ticks_to_ns(*r);
> +        v->arch.phys_timer.cval = cntpct + *r;
>           if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
>           {
>               v->arch.phys_timer.ctl &= ~CNTx_CTL_PENDING;
> -            set_timer(&v->arch.phys_timer.timer,
> -                      v->arch.phys_timer.cval +
> -                      v->domain->arch.phys_timer_base.offset);
> +            expires = v->arch.phys_timer.cval > boot_count
> +                      ? ticks_to_ns(v->arch.phys_timer.cval - boot_count) : 0;

You probably want a comment to explain why you set to 0 here.

> +            set_timer(&v->arch.phys_timer.timer, expires);
>           }
>       }
>       return true;
> @@ -226,23 +227,24 @@ static bool vtimer_cntp_cval(struct cpu_user_regs *regs, uint64_t *r,
>                                bool read)
>   {
>       struct vcpu *v = current;
> +    s_time_t expires;
>   
>       if ( !ACCESS_ALLOWED(regs, EL0PTEN) )
>           return false;
>   
>       if ( read )
>       {
> -        *r = ns_to_ticks(v->arch.phys_timer.cval);
> +        *r = v->arch.phys_timer.cval;
>       }
>       else
>       {
> -        v->arch.phys_timer.cval = ticks_to_ns(*r);
> +        v->arch.phys_timer.cval = *r;
>           if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
>           {
>               v->arch.phys_timer.ctl &= ~CNTx_CTL_PENDING;
> -            set_timer(&v->arch.phys_timer.timer,
> -                      v->arch.phys_timer.cval +
> -                      v->domain->arch.phys_timer_base.offset);
> +            expires = v->arch.phys_timer.cval > boot_count
> +                      ? ticks_to_ns(v->arch.phys_timer.cval - boot_count) : 0;

Same here. But I am wondering whether we could factor this code in a 
function. This would avoid code duplication and make the code simpler.

This can be done as a follow-up as we may want to backport the fix.

> +            set_timer(&v->arch.phys_timer.timer, expires);
>           }
>       }
>       return true;
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index f3f3fb7d7f..adc7fe7210 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -65,9 +65,6 @@ struct arch_domain
>           RELMEM_done,
>       } relmem;
>   
> -    struct {
> -        uint64_t offset;
> -    } phys_timer_base;
>       struct {
>           uint64_t offset;
>       } virt_timer_base;
> 

Cheers,

-- 
Julien Grall

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