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. Cheers Bertrand
