Re: [PATCH] xen/arm: avoid vtimer flip-flop transition in context switch

[Julien Grall <julien@xxxxxxx>]

Hi Wei,

Title: I don't quite understand what you mean by "flip-flop transition".

On 15/06/2022 02:39, Wei Chen wrote:
> virt_vtimer_save is calculating the new time for the vtimer and
> has a potential risk of timer flip in:
> "v->arch.virt_timer.cval + v->domain->arch.virt_timer_base.offset
> - boot_count".
> In this formula, "cval + offset" could make uint64_t overflow.
> Generally speaking, this is difficult to trigger. 
> But unfortunately
> the problem was encountered with a platform where the timer started
> with a very huge initial value, like 0xF333899122223333. On this
> platform cval + offset is overflowing after running for a while.
I am not sure how this is a problem. Yes, uint64_t will overflow with 
"cval + offset", but AFAIK the overall result will still be correct and 
not undefined.
> So in this patch, we adjust the formula to use "offset - boot_count"
> first, and then use the result to plus cval. This will avoid the
> uint64_t overflow.
Technically, the overflow is still present because the (offset - 
boot_count) is a non-zero value *and* cval is a 64-bit value.
So I think the equation below should be reworked to...

> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> ---
>   xen/arch/arm/vtimer.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index 5bb5970f58..86e63303c8 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -144,8 +144,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,
> +                  ticks_to_ns(v->domain->arch.virt_timer_base.offset -
> +                              boot_count + v->arch.virt_timer.cval));
... something like:

ticks_to_ns(offset - boot_count) + ticks_to_ns(cval);

The first part of the equation should always be the same. So it could be 
stored in struct domain.
Cheers,

--
Julien Grall