Re: [PATCH] xen/arm: avoid extra caclulations when setting vtimer in context switch

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jiamei,

> On 28 Jun 2022, at 07:35, Jiamei Xie <Jiamei.Xie@xxxxxxx> wrote:
> 
> Hi Julien,
> 
>> -----Original Message-----
>> From: Julien Grall <julien@xxxxxxx>
>> Sent: 2022年6月27日 18:36
>> To: Jiamei Xie <Jiamei.Xie@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Bertrand Marquis
>> <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
>> <Volodymyr_Babchuk@xxxxxxxx>; Wei Chen <Wei.Chen@xxxxxxx>
>> 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.xie@xxxxxxx>
>>> 
>>> [1] https://www.mail-archive.com/xen-
>> devel@xxxxxxxxxxxxxxxxxxxx/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@xxxxxxxxxxxxxxxxxxxx/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