Re: [PATCH] xen/arm: avoid extra caclulations when setting vtimer in context switch

[Julien Grall <julien@xxxxxxx>]

Hi Jiami

Title: s/caclulations/calculations/

However, I think the title should mention the overflow rather than the 
extra calculations. The former is more important the latter.
On 27/06/2022 03:58, Jiamei Xie wrote:
> virt_vtimer_save is calculating the new time for the vtimer in:
> "v->arch.virt_timer.cval + v->domain->arch.virt_timer_base.offset
> - boot_count".
> In this formula, "cval + offset" might cause uint64_t overflow.
> Changing it to "v->domain->arch.virt_timer_base.offset - boot_count +
> v->arch.virt_timer.cval" can reduce the possibility of overflow
This read strange to me. We want to remove the overflow completely not 
reducing it. The overflow is completely removed by converting the 
"offset - bount_count" to ns upfront.
AFAICT, the commit message doesn't explain that.

> , and
> "arch.virt_timer_base.offset - boot_count" will be always the same,
> which has been caculated in domain_vtimer_init. Introduce a new field
> vtimer_offset.nanoseconds to store this value for arm in struct
> arch_domain, so we can use it directly and extra caclulations can be
> avoided.
>
> This patch is enlightened from [1].
>
> Signed-off-by: Jiamei Xie <jiamei.xie@xxxxxxx>
>
> [1] https://www.mail-archive.com/xen-devel@xxxxxxxxxxxxxxxxxxxx/msg123139.htm
This link doesn't work. But I would personally remove it from the commit 
message (or add ---) because it doesn't bring value (this patch looks 
like a v2 to me).
> ---
> xen/arch/arm/include/asm/domain.h | 4 ++++
>   xen/arch/arm/vtimer.c             | 6 ++++--
>   2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/domain.h 
> b/xen/arch/arm/include/asm/domain.h
> index ed63c2b6f9..94fe5b6444 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -73,6 +73,10 @@ struct arch_domain
>           uint64_t offset;
>       } virt_timer_base;
>   
> +    struct {
> +        int64_t nanoseconds;
This should be s_time_t to match the argument of set_timer() and return 
of ticks_to_ns().
> +    } vtimer_offset;
Why are you adding a new structure rather than re-using virt_timer_base?

> +
>       struct vgic_dist vgic;
>   
>       struct vuart {
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index 6b78fea77d..54161e5fea 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -64,6 +64,7 @@ int domain_vtimer_init(struct domain *d, struct 
> xen_arch_domainconfig *config)
>   {
>       d->arch.virt_timer_base.offset = get_cycles();
>       d->time_offset.seconds = ticks_to_ns(d->arch.virt_timer_base.offset - 
> boot_count);
> +    d->arch.vtimer_offset.nanoseconds = d->time_offset.seconds;
Hmmm... I find odd to assign a field "nanoseconds" to "seconds". I would 
suggest to re-order so you first set nanoseconds and then set seconds.
This will make more obvious that this is not a mistake and "seconds" 
will be closer to the do_div() below.
>       do_div(d->time_offset.seconds, 1000000000);
>   
>       config->clock_frequency = timer_dt_clock_frequency;
> @@ -144,8 +145,9 @@ void virt_timer_save(struct vcpu *v)
>       if ( (v->arch.virt_timer.ctl & CNTx_CTL_ENABLE) &&
>            !(v->arch.virt_timer.ctl & CNTx_CTL_MASK))
>       {
> -        set_timer(&v->arch.virt_timer.timer, 
> ticks_to_ns(v->arch.virt_timer.cval +
> -                  v->domain->arch.virt_timer_base.offset - boot_count));
> +        set_timer(&v->arch.virt_timer.timer,
> +                  v->domain->arch.vtimer_offset.nanoseconds +
> +                  ticks_to_ns(v->arch.virt_timer.cval));
>       }
>   }
>   
Cheers,

--
Julien Grall