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 "ticks_to_ns(v->domain->arch.virt_timer_base.offset -
boot_count) + ticks_to_ns(v->arch.virt_timer.cval)" can avoid overflow,
and "ticks_to_ns(arch.virt_timer_base.offset - boot_count)" will be
always the same, which has been caculated in domain_vtimer_init.
Introduce a new field virt_timer_base.nanoseconds to store this value
for arm in struct arch_domain, so we can use it directly.

Signed-off-by: Jiamei Xie <jiamei.xie@arm.com>
Change-Id: Ib80cee51eaf844661e6f92154a0339ad2a652f9b
---
was "xen/arm: avoid vtimer flip-flop transition in context switch".
v3 changes:
-re-write commit message
-store nanoseconds in virt_timer_base instead of adding a new structure
-assign to nanoseconds first, then seconds
---
 xen/arch/arm/include/asm/domain.h | 1 +
 xen/arch/arm/vtimer.c             | 9 ++++++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index ed63c2b6f9..cd9ce19b4b 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -71,6 +71,7 @@ struct arch_domain
 
     struct {
         uint64_t offset;
+        s_time_t nanoseconds;
     } virt_timer_base;
 
     struct vgic_dist vgic;
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 6b78fea77d..aeaea78e4c 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -63,7 +63,9 @@ static void virt_timer_expired(void *data)
 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.virt_timer_base.nanoseconds =
+        ticks_to_ns(d->arch.virt_timer_base.offset - boot_count);
+    d->time_offset.seconds = d->arch.virt_timer_base.nanoseconds;
     do_div(d->time_offset.seconds, 1000000000);
 
     config->clock_frequency = timer_dt_clock_frequency;
@@ -144,8 +146,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.virt_timer_base.nanoseconds +
+                  ticks_to_ns(v->arch.virt_timer.cval));
     }
 }
 
-- 
2.25.1

Hi Jiamei,

On 30/06/2022 02:53, 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 "ticks_to_ns(v->domain->arch.virt_timer_base.offset -
> boot_count) + ticks_to_ns(v->arch.virt_timer.cval)" can avoid overflow,

 From the commit message, I can't quite make the connection between 
"cval + offset"  will overflow and "ticks_to_ns(...) + ticks_to_ns(...)" 
will help.

So how about the following:

"
virt_vtimer_save() will calculate the next deadline when the vCPU is 
scheduled out. At the moment, Xen will use the following equation:

  virt_timer.cval + virt_time_base.offset - boot_count

The three values are 64-bit and one (cval) is controlled by domain. In 
theory, it would be possible that the domain has started a long time
after the system boot. So virt_time_base.offset - boot_count may be a 
large numbers.

This means a domain may inadvertently set a cval so the result would 
overflow. Consequently, the deadline would be set very far in the 
future. This could result to loss of timer interrupts or the vCPU 
getting block "forever".

One way to solve the problem, would be to separately
   1) compute when the domain was created in ns
   2) convert cval to ns
   3) Add 1 and 2 together

The first part of the equation never change (the value is set/known at 
domain creation). So take the opportunity to store it in domain structure.
"

The code itself looks good to me.

Cheers,

-- 
Julien Grall

Hi,

> -----Original Message-----
> From: Jiamei Xie <jiamei.xie@arm.com>
> Sent: 2022年6月30日 9:54
> To: xen-devel@lists.xenproject.org
> Cc: Jiamei Xie <Jiamei.Xie@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Bertrand Marquis
> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; Wei Chen <Wei.Chen@arm.com>
> Subject: [PATCH v3] xen/arm: avoid overflow when setting vtimer in context
> switch
> 
> 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 "ticks_to_ns(v->domain->arch.virt_timer_base.offset -
> boot_count) + ticks_to_ns(v->arch.virt_timer.cval)" can avoid overflow,
> and "ticks_to_ns(arch.virt_timer_base.offset - boot_count)" will be
> always the same, which has been caculated in domain_vtimer_init.
> Introduce a new field virt_timer_base.nanoseconds to store this value
> for arm in struct arch_domain, so we can use it directly.
> 
> Signed-off-by: Jiamei Xie <jiamei.xie@arm.com>
> Change-Id: Ib80cee51eaf844661e6f92154a0339ad2a652f9b

I am sorry I forget to remove the Change-Id.

> ---
> was "xen/arm: avoid vtimer flip-flop transition in context switch".
> v3 changes:
> -re-write commit message
> -store nanoseconds in virt_timer_base instead of adding a new structure
> -assign to nanoseconds first, then seconds
> ---
>  xen/arch/arm/include/asm/domain.h | 1 +
>  xen/arch/arm/vtimer.c             | 9 ++++++---
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/domain.h
> b/xen/arch/arm/include/asm/domain.h
> index ed63c2b6f9..cd9ce19b4b 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -71,6 +71,7 @@ struct arch_domain
> 
>      struct {
>          uint64_t offset;
> +        s_time_t nanoseconds;
>      } virt_timer_base;
> 
>      struct vgic_dist vgic;
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index 6b78fea77d..aeaea78e4c 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -63,7 +63,9 @@ static void virt_timer_expired(void *data)
>  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.virt_timer_base.nanoseconds =
> +        ticks_to_ns(d->arch.virt_timer_base.offset - boot_count);
> +    d->time_offset.seconds = d->arch.virt_timer_base.nanoseconds;
>      do_div(d->time_offset.seconds, 1000000000);
> 
>      config->clock_frequency = timer_dt_clock_frequency;
> @@ -144,8 +146,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.virt_timer_base.nanoseconds +
> +                  ticks_to_ns(v->arch.virt_timer.cval));
>      }
>  }
> 
> --
> 2.25.1

On 30/06/2022 06:20, Jiamei Xie wrote:
> Hi,

Hi Jiamei,

>> -----Original Message-----
>> From: Jiamei Xie <jiamei.xie@arm.com>
>> Sent: 2022年6月30日 9:54
>> To: xen-devel@lists.xenproject.org
>> Cc: Jiamei Xie <Jiamei.Xie@arm.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Bertrand Marquis
>> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
>> <Volodymyr_Babchuk@epam.com>; Wei Chen <Wei.Chen@arm.com>
>> Subject: [PATCH v3] xen/arm: avoid overflow when setting vtimer in context
>> switch
>>
>> 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 "ticks_to_ns(v->domain->arch.virt_timer_base.offset -
>> boot_count) + ticks_to_ns(v->arch.virt_timer.cval)" can avoid overflow,
>> and "ticks_to_ns(arch.virt_timer_base.offset - boot_count)" will be
>> always the same, which has been caculated in domain_vtimer_init.
>> Introduce a new field virt_timer_base.nanoseconds to store this value
>> for arm in struct arch_domain, so we can use it directly.
>>
>> Signed-off-by: Jiamei Xie <jiamei.xie@arm.com>
>> Change-Id: Ib80cee51eaf844661e6f92154a0339ad2a652f9b
> 
> I am sorry I forget to remove the Change-Id.

No worries. This can be dropped on commit (if no other changes are 
requested).

Cheers,

-- 
Julien Grall