[PATCH v4] xen/arm: avoid overflow when setting vtimer in context switch

Jiamei Xie posted 1 patch 1 year, 8 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20220706082558.1116811-1-jiamei.xie@arm.com
xen/arch/arm/include/asm/domain.h | 1 +
xen/arch/arm/vtimer.c             | 9 ++++++---
2 files changed, 7 insertions(+), 3 deletions(-)
[PATCH v4] xen/arm: avoid overflow when setting vtimer in context switch
Posted by Jiamei Xie 1 year, 8 months ago
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.

Signed-off-by: Jiamei Xie <jiamei.xie@arm.com>
---
was "xen/arm: avoid vtimer flip-flop transition in context switch".
v4 changes:
- re-write commit message
---
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
Re: [PATCH v4] xen/arm: avoid overflow when setting vtimer in context switch
Posted by Julien Grall 1 year, 8 months ago
Hi Jiamei,

On 06/07/2022 09:25, Jiamei Xie wrote:
> 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.
> 
> Signed-off-by: Jiamei Xie <jiamei.xie@arm.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

The commit message is my own, I would like to Bertrand or Stefano to 
confirm they are happy with it :).

Cheers,

-- 
Julien Grall
Re: [PATCH v4] xen/arm: avoid overflow when setting vtimer in context switch
Posted by Bertrand Marquis 1 year, 8 months ago
Hi Julien,

> On 7 Jul 2022, at 16:33, Julien Grall <julien@xen.org> wrote:
> 
> Hi Jiamei,
> 
> On 06/07/2022 09:25, Jiamei Xie wrote:
>> 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.
>> Signed-off-by: Jiamei Xie <jiamei.xie@arm.com>
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>
> 
> The commit message is my own, I would like to Bertrand or Stefano to confirm they are happy with it :).

I am ok with it so:
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall
Re: [PATCH v4] xen/arm: avoid overflow when setting vtimer in context switch
Posted by Julien Grall 1 year, 8 months ago
Hi Bertrand,

On 07/07/2022 16:35, Bertrand Marquis wrote:
> Hi Julien,
> 
>> On 7 Jul 2022, at 16:33, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Jiamei,
>>
>> On 06/07/2022 09:25, Jiamei Xie wrote:
>>> 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.
>>> Signed-off-by: Jiamei Xie <jiamei.xie@arm.com>
>>
>> Reviewed-by: Julien Grall <jgrall@amazon.com>
>>
>> The commit message is my own, I would like to Bertrand or Stefano to confirm they are happy with it :).
> 
> I am ok with it so:
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Thanks. I have committed the patch.

Cheers,

-- 
Julien Grall