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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 25/11/2019 16:14, 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 Arch Reference Manual, the offset between the
Which bit of the Arm Arm do you refer to here? In general, I would 
recommend to give the exact section and version of the manual you use to 
avoid any misunderstanding.
> physical timer and counter should be 0. This removes the offset to make
> the timer and counter consistent.
>
> Xen time is at offset boot_count from the physical counter, so we need
> to take this into account when reading/writing to CNTP_CVAL.
>
> Signed-off-by: Jeff Kubascik <jeff.kubascik@xxxxxxxxxxxxxxx>
> ---
>   xen/arch/arm/vtimer.c        | 18 ++++++------------
>   xen/include/asm-arm/domain.h |  3 ---
>   2 files changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index e6aebdac9e..4790b5ce58 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);
> @@ -184,8 +183,7 @@ 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);
> +            set_timer(&v->arch.phys_timer.timer, v->arch.phys_timer.cval);
>           }
>           else
>               stop_timer(&v->arch.phys_timer.timer);
> @@ -202,7 +200,7 @@ static bool vtimer_cntp_tval(struct cpu_user_regs *regs, 
> uint32_t *r,
>       if ( !ACCESS_ALLOWED(regs, EL0PTEN) )
>           return false;
>   
> -    now = NOW() - v->domain->arch.phys_timer_base.offset;
> +    now = NOW();
>   
>       if ( read )
>       {
> @@ -214,9 +212,7 @@ static bool vtimer_cntp_tval(struct cpu_user_regs *regs, 
> uint32_t *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);
> +            set_timer(&v->arch.phys_timer.timer, v->arch.phys_timer.cval);
>           }
>       }
>       return true;
> @@ -232,17 +228,15 @@ static bool vtimer_cntp_cval(struct cpu_user_regs *regs, 
> uint64_t *r,
>   
>       if ( read )
>       {
> -        *r = ns_to_ticks(v->arch.phys_timer.cval);
> +        *r = ns_to_ticks(v->arch.phys_timer.cval) + boot_count;
>       }
>       else
>       {
> -        v->arch.phys_timer.cval = ticks_to_ns(*r);
> +        v->arch.phys_timer.cval = ticks_to_ns(*r - boot_count);
I know that this is already like that in the code. But it feels weird 
(to not say wrong) that cval will have a different meaning between the 
virtual timer and physical timer.
Indeed, in the former case it is an exact copy of the hardware value 
whilst in the latter it is the hardware value - NOW().
While you are reworking a big chunk of the physical timer emulation, 
could you looking at removing this discrepancy?
>           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);
> +            set_timer(&v->arch.phys_timer.timer, v->arch.phys_timer.cval);
>           }
>       }
>       return true;
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 86ebdd2bcf..16a7150a95 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel