Hi Julien,
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: 2022年6月17日 16:32
> To: Wei Chen <wei.c...@arm.com>; xen-devel@lists.xenproject.org
> Cc: nd <n...@arm.com>; Stefano Stabellini <sstabell...@kernel.org>; Bertrand
> Marquis <bertrand.marq...@arm.com>; Volodymyr Babchuk
> <volodymyr_babc...@epam.com>
> Subject: Re: [PATCH] xen/arm: avoid vtimer flip-flop transition in context
> switch
>
>
>
> On 15/06/2022 11:36, Wei Chen wrote:
> > Hi Julien,
>
> Hi Wei,
>
> >> -----Original Message-----
> >> From: Julien Grall <jul...@xen.org>
> >> Sent: 2022年6月15日 17:47
> >> To: Wei Chen <wei.c...@arm.com>; xen-devel@lists.xenproject.org
> >> Cc: nd <n...@arm.com>; Stefano Stabellini <sstabell...@kernel.org>;
> Bertrand
> >> Marquis <bertrand.marq...@arm.com>; Volodymyr Babchuk
> >> <volodymyr_babc...@epam.com>
> >> Subject: Re: [PATCH] xen/arm: avoid vtimer flip-flop transition in
> context
> >> switch
> >>>
> >>> 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.
> >>
> >
> > Yes, GuestOS can issue any valid 64-bit value for their usage.
> >
> >> So I think the equation below should be reworked to...
> >>
> >>>
> >>> Signed-off-by: Wei Chen <wei.c...@arm.com>
> >>> ---
> >>> 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.
> >>
> >
> > If you think there is still some values to continue this patch, I will
> > address this comment : )
>
> I think there are. This can be easily triggered by a vCPU setting a
> large cval.
>
Ok, thanks! We will address it and refine the subject and descriptions.
> Cheers,
>
> --
> Julien Grall
