Hi Bertrand, > -----Original Message----- > From: Bertrand Marquis <bertrand.marq...@arm.com> > Sent: 2022年6月28日 15:29 > To: Jiamei Xie <jiamei....@arm.com> > Cc: Julien Grall <jul...@xen.org>; xen-devel@lists.xenproject.org; Stefano > Stabellini <sstabell...@kernel.org>; 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 Jiamei, > > > On 28 Jun 2022, at 07:35, Jiamei Xie <jiamei....@arm.com> wrote: > > > > 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; > > The implementation must use do_div to properly handle the division from a > 64bit by a 32bit on arm32 otherwise the code will be a lot slower.
Thanks for your explanation for this. I will keep the do_div. Best wishes Jiamei Xie > > Cheers > Bertrand >
