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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 26/11/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.
>
> Furthermore, section D11.2.4 specifies that the values in the TimerValue
> view of the timers are signed in standard two's complement form. When
> writing to the TimerValue register, it should be signed extended as
> described by the equation
>
>    CompareValue = (Counter[63:0] + SignExtend(TimerValue))[63:0]
I am a bit confused, is it a new bug introduced by the change or 
previously existing? If the latter, then I think this should be modified 
in a separate patch.
> Signed-off-by: Jeff Kubascik <jeff.kubascik@xxxxxxxxxxxxxxx>
> ---
> Changes in v2:
> - Update commit message to specify reference manual version and section
> - Change physical timer cval to hold hardware value
I think this change should be explained in the commit message.

> - Make sure to sign extend TimerValue on writes. This was done by first
>    casting the r pointer to (int32_t *), dereferencing it, then casting
>    to uint64_t. Please let me know if there is a more correct way to do
>    this
> ---
>   xen/arch/arm/vtimer.c        | 21 +++++++++------------
>   xen/include/asm-arm/domain.h |  3 ---
>   2 files changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index e6aebdac9e..eb12a08acf 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);
I think you need to update the initialization of cval to avoid storing 
ns. But CTNP_CVAL_EL0 is reset to a unknown value at reboot, so we 
should not need to set a value at all as the guest would have to set it.
> @@ -185,7 +184,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);
> +                      ticks_to_ns(v->arch.phys_timer.cval - boot_count));
cval may be smaller than boot_count. In that case, we will set the timer 
to expire a very long time. This is not the expected behavior from the 
guest.
Instead, we should either use 0 to create the timer or call 
phys_timer_expired directly.
>           }
>           else
>               stop_timer(&v->arch.phys_timer.timer);
> @@ -197,26 +196,25 @@ 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;
>   
>       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 + (uint64_t)(*((int32_t *)r));
I would prefer (uint64_t)(int32_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);
> +                      ticks_to_ns(v->arch.phys_timer.cval - boot_count));
>           }
>       }
>       return true;
> @@ -232,17 +230,16 @@ 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 = 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);
> +                      ticks_to_ns(v->arch.phys_timer.cval - boot_count));
>           }
>       }
>       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