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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 21/01/2020 15:07, 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@xxxxxxxxxxxxxxx>
> ---
>   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 240a850b6e..0c78a65863 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);
While the factoring was optional, my request of a comment wasn't (even 
if it requires to duplicate 3 times).
If you suggest a comment and an update of the commit message, I can 
merge it in this patch on commit.
Cheers,

--
Julien Grall

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