Hi Julien,
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: 2022年6月27日 18:36
> To: Jiamei Xie <jiamei....@arm.com>; xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini <sstabell...@kernel.org>; Bertrand Marquis
> <bertrand.marq...@arm.com>; Volodymyr Babchuk
> <volodymyr_babc...@epam.com>; Wei Chen <wei.c...@arm.com>
> Subject: Re: [PATCH] xen/arm: avoid extra caclulations when setting vtimer
> in context switch
>
> Hi Jiami
>
> Title: s/caclulations/calculations/
>
> However, I think the title should mention the overflow rather than the
> extra calculations. The former is more important the latter.
>
I will change the title to " xen/arm: avoid overflow when setting vtimer in
context switch"
> On 27/06/2022 03:58, 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 "v->domain->arch.virt_timer_base.offset - boot_count +
> > v->arch.virt_timer.cval" can reduce the possibility of overflow
>
> This read strange to me. We want to remove the overflow completely not
> reducing it. The overflow is completely removed by converting the
> "offset - bount_count" to ns upfront.
>
> AFAICT, the commit message doesn't explain that.
Thanks for pointing out that. How about putting the commit message like the
below:
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.
>
> > , and
> > "arch.virt_timer_base.offset - boot_count" will be always the same,
> > which has been caculated in domain_vtimer_init. Introduce a new field
> > vtimer_offset.nanoseconds to store this value for arm in struct
> > arch_domain, so we can use it directly and extra caclulations can be
> > avoided.
> >
> > This patch is enlightened from [1].
> >
> > Signed-off-by: Jiamei Xie <jiamei....@arm.com>
> >
> > [1] https://www.mail-archive.com/xen-
> de...@lists.xenproject.org/msg123139.htm
>
> This link doesn't work. But I would personally remove it from the commit
> message (or add ---) because it doesn't bring value (this patch looks
> like a v2 to me).
Sorry, a 'l' is missing at the end of the link. The link is
https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg123139.html .
I will put it after --- in v3.
>
> > ---
> > xen/arch/arm/include/asm/domain.h | 4 ++++
> > xen/arch/arm/vtimer.c | 6 ++++--
> > 2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/include/asm/domain.h
> b/xen/arch/arm/include/asm/domain.h
> > index ed63c2b6f9..94fe5b6444 100644
> > --- a/xen/arch/arm/include/asm/domain.h
> > +++ b/xen/arch/arm/include/asm/domain.h
> > @@ -73,6 +73,10 @@ struct arch_domain
> > uint64_t offset;
> > } virt_timer_base;
> >
> > + struct {
> > + int64_t nanoseconds;
>
> This should be s_time_t to match the argument of set_timer() and return
> of ticks_to_ns().
>
> > + } vtimer_offset;
>
> Why are you adding a new structure rather than re-using virt_timer_base?
Sure, I'll add this field in virt_timer_base.
struct {
uint64_t offset;
s_time_t nanoseconds;
} virt_timer_base;
>
> > +
> > struct vgic_dist vgic;
> >
> > struct vuart {
> > diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> > index 6b78fea77d..54161e5fea 100644
> > --- a/xen/arch/arm/vtimer.c
> > +++ b/xen/arch/arm/vtimer.c
> > @@ -64,6 +64,7 @@ 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.vtimer_offset.nanoseconds = d->time_offset.seconds;
>
> Hmmm... I find odd to assign a field "nanoseconds" to "seconds". I would
> suggest to re-order so you first set nanoseconds and then set seconds.
>
> This will make more obvious that this is not a mistake and "seconds"
> will be closer to the do_div() below.
Is it ok to remove do_div and write like below?
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 /
1000000000;
Best wishes
Jiamei Xie
>
> > do_div(d->time_offset.seconds, 1000000000);
> >
> > config->clock_frequency = timer_dt_clock_frequency;
> > @@ -144,8 +145,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.vtimer_offset.nanoseconds +
> > + ticks_to_ns(v->arch.virt_timer.cval));
> > }
> > }
> >
>
> Cheers,
>
> --
> Julien Grall
