* [PATCH] xen/arm: avoid extra caclulations when setting vtimer in context switch
@ 2022-06-27 2:58 Jiamei Xie
2022-06-27 10:35 ` Julien Grall
0 siblings, 1 reply; 5+ messages in thread
From: Jiamei Xie @ 2022-06-27 2:58 UTC (permalink / raw)
To: xen-devel
Cc: Jiamei Xie, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Wei Chen
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, 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@arm.com>
[1] https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg123139.htm
---
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;
+ } vtimer_offset;
+
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;
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));
}
}
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] xen/arm: avoid extra caclulations when setting vtimer in context switch
2022-06-27 2:58 [PATCH] xen/arm: avoid extra caclulations when setting vtimer in context switch Jiamei Xie
@ 2022-06-27 10:35 ` Julien Grall
2022-06-28 6:35 ` Jiamei Xie
0 siblings, 1 reply; 5+ messages in thread
From: Julien Grall @ 2022-06-27 10:35 UTC (permalink / raw)
To: Jiamei Xie, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk, Wei Chen
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.
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.
> , 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@arm.com>
>
> [1] https://www.mail-archive.com/xen-devel@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).
> ---
> 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?
> +
> 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.
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] xen/arm: avoid extra caclulations when setting vtimer in context switch
2022-06-27 10:35 ` Julien Grall
@ 2022-06-28 6:35 ` Jiamei Xie
2022-06-28 7:29 ` Bertrand Marquis
0 siblings, 1 reply; 5+ messages in thread
From: Jiamei Xie @ 2022-06-28 6:35 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk, Wei Chen
Hi Julien,
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 2022年6月27日 18:36
> To: Jiamei Xie <Jiamei.Xie@arm.com>; xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Bertrand Marquis
> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; Wei Chen <Wei.Chen@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.xie@arm.com>
> >
> > [1] https://www.mail-archive.com/xen-
> devel@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xen/arm: avoid extra caclulations when setting vtimer in context switch
2022-06-28 6:35 ` Jiamei Xie
@ 2022-06-28 7:29 ` Bertrand Marquis
2022-06-28 8:17 ` Jiamei Xie
0 siblings, 1 reply; 5+ messages in thread
From: Bertrand Marquis @ 2022-06-28 7:29 UTC (permalink / raw)
To: Jiamei Xie
Cc: Julien Grall, xen-devel, Stefano Stabellini, Volodymyr Babchuk, Wei Chen
Hi Jiamei,
> On 28 Jun 2022, at 07:35, Jiamei Xie <Jiamei.Xie@arm.com> wrote:
>
> Hi Julien,
>
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: 2022年6月27日 18:36
>> To: Jiamei Xie <Jiamei.Xie@arm.com>; xen-devel@lists.xenproject.org
>> Cc: Stefano Stabellini <sstabellini@kernel.org>; Bertrand Marquis
>> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
>> <Volodymyr_Babchuk@epam.com>; Wei Chen <Wei.Chen@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.xie@arm.com>
>>>
>>> [1] https://www.mail-archive.com/xen-
>> devel@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] xen/arm: avoid extra caclulations when setting vtimer in context switch
2022-06-28 7:29 ` Bertrand Marquis
@ 2022-06-28 8:17 ` Jiamei Xie
0 siblings, 0 replies; 5+ messages in thread
From: Jiamei Xie @ 2022-06-28 8:17 UTC (permalink / raw)
To: Bertrand Marquis
Cc: Julien Grall, xen-devel, Stefano Stabellini, Volodymyr Babchuk, Wei Chen
Hi Bertrand,
> -----Original Message-----
> From: Bertrand Marquis <Bertrand.Marquis@arm.com>
> Sent: 2022年6月28日 15:29
> To: Jiamei Xie <Jiamei.Xie@arm.com>
> Cc: Julien Grall <julien@xen.org>; xen-devel@lists.xenproject.org; Stefano
> Stabellini <sstabellini@kernel.org>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; Wei Chen <Wei.Chen@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.Xie@arm.com> wrote:
> >
> > Hi Julien,
> >
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: 2022年6月27日 18:36
> >> To: Jiamei Xie <Jiamei.Xie@arm.com>; xen-devel@lists.xenproject.org
> >> Cc: Stefano Stabellini <sstabellini@kernel.org>; Bertrand Marquis
> >> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> >> <Volodymyr_Babchuk@epam.com>; Wei Chen <Wei.Chen@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.xie@arm.com>
> >>>
> >>> [1] https://www.mail-archive.com/xen-
> >> devel@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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-06-28 8:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-27 2:58 [PATCH] xen/arm: avoid extra caclulations when setting vtimer in context switch Jiamei Xie
2022-06-27 10:35 ` Julien Grall
2022-06-28 6:35 ` Jiamei Xie
2022-06-28 7:29 ` Bertrand Marquis
2022-06-28 8:17 ` Jiamei Xie
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.