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

[Julien Grall <julien@xxxxxxx>]

On 15/06/2022 11:36, Wei Chen wrote:
> Hi Julien,
Hi Wei,

> > -----Original Message-----
> > From: Julien Grall <julien@xxxxxxx>
> > Sent: 2022年6月15日 17:47
> > To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: nd <nd@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Bertrand
> > Marquis <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> > <Volodymyr_Babchuk@xxxxxxxx>
> > 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.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.
> 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.
Cheers,

--
Julien Grall