* [PATCH] x86: replace a few do_div() uses
@ 2022-01-12 9:00 Jan Beulich
2022-01-12 9:22 ` Andrew Cooper
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2022-01-12 9:00 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
When the macro's "return value" is not used, the macro use can be
replaced by a simply division, avoiding some obfuscation.
According to my observations, no change to generated code.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Arguably the ULL suffix (in write_watchdog_counter()) or the cast to
uint64_t (in div_sc()) aren't really needed in code which gets built for
64-bit only.
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -392,9 +392,8 @@ static void intel_log_freq(const struct
unsigned long long val = ecx;
val *= ebx;
- do_div(val, eax);
printk("CPU%u: TSC: %u Hz * %u / %u = %Lu Hz\n",
- smp_processor_id(), ecx, ebx, eax, val);
+ smp_processor_id(), ecx, ebx, eax, val / eax);
}
else if ( ecx | eax | ebx )
{
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -105,10 +105,7 @@ custom_param("hpet", parse_hpet_param);
static inline unsigned long div_sc(unsigned long ticks, unsigned long nsec,
int shift)
{
- uint64_t tmp = ((uint64_t)ticks) << shift;
-
- do_div(tmp, nsec);
- return (unsigned long) tmp;
+ return ((uint64_t)ticks << shift) / nsec;
}
/*
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -292,10 +292,9 @@ static void clear_msr_range(unsigned int
static inline void write_watchdog_counter(const char *descr)
{
- u64 count = (u64)cpu_khz * 1000;
+ uint64_t count = cpu_khz * 1000ULL / nmi_hz;
- do_div(count, nmi_hz);
- if(descr)
+ if ( descr )
Dprintk("setting %s to -%#"PRIx64"\n", descr, count);
wrmsrl(nmi_perfctr_msr, 0 - count);
}
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -610,8 +610,7 @@ static uint64_t xen_timer_cpu_frequency(
struct vcpu_time_info *info = &this_cpu(vcpu_info)->time;
uint64_t freq;
- freq = 1000000000ULL << 32;
- do_div(freq, info->tsc_to_system_mul);
+ freq = (1000000000ULL << 32) / info->tsc_to_system_mul;
if ( info->tsc_shift < 0 )
freq <<= -info->tsc_shift;
else
@@ -2173,8 +2172,7 @@ void __init early_time_init(void)
set_time_scale(&t->tsc_scale, tmp);
t->stamp.local_tsc = boot_tsc_stamp;
- do_div(tmp, 1000);
- cpu_khz = (unsigned long)tmp;
+ cpu_khz = tmp / 1000;
printk("Detected %lu.%03lu MHz processor.\n",
cpu_khz / 1000, cpu_khz % 1000);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86: replace a few do_div() uses
2022-01-12 9:00 [PATCH] x86: replace a few do_div() uses Jan Beulich
@ 2022-01-12 9:22 ` Andrew Cooper
2022-01-12 9:28 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Cooper @ 2022-01-12 9:22 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
On 12/01/2022 09:00, Jan Beulich wrote:
> When the macro's "return value" is not used, the macro use can be
> replaced by a simply division, avoiding some obfuscation.
>
> According to my observations, no change to generated code.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
I like this change in principle, but see below.
do_div() needs to be deleted, because it's far too easy screw up. At a
bare minimum, it should be replaced with a static inline that takes it's
first parameter by pointer, because then at least every callsite reads
correctly in terms of the C language.
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -610,8 +610,7 @@ static uint64_t xen_timer_cpu_frequency(
> struct vcpu_time_info *info = &this_cpu(vcpu_info)->time;
> uint64_t freq;
>
> - freq = 1000000000ULL << 32;
> - do_div(freq, info->tsc_to_system_mul);
> + freq = (1000000000ULL << 32) / info->tsc_to_system_mul;
> if ( info->tsc_shift < 0 )
> freq <<= -info->tsc_shift;
do_div()'s output is consumed here. I don't think this hunk is safe to
convert.
~Andrew
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86: replace a few do_div() uses
2022-01-12 9:22 ` Andrew Cooper
@ 2022-01-12 9:28 ` Jan Beulich
2022-02-18 8:39 ` Ping: " Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2022-01-12 9:28 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, xen-devel
On 12.01.2022 10:22, Andrew Cooper wrote:
> On 12/01/2022 09:00, Jan Beulich wrote:
>> When the macro's "return value" is not used, the macro use can be
>> replaced by a simply division, avoiding some obfuscation.
>>
>> According to my observations, no change to generated code.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> I like this change in principle, but see below.
>
> do_div() needs to be deleted, because it's far too easy screw up. At a
> bare minimum, it should be replaced with a static inline that takes it's
> first parameter by pointer, because then at least every callsite reads
> correctly in terms of the C language.
That ought to be a 2nd step, requiring agreement with Arm folks (and
adjustments to their code).
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -610,8 +610,7 @@ static uint64_t xen_timer_cpu_frequency(
>> struct vcpu_time_info *info = &this_cpu(vcpu_info)->time;
>> uint64_t freq;
>>
>> - freq = 1000000000ULL << 32;
>> - do_div(freq, info->tsc_to_system_mul);
>> + freq = (1000000000ULL << 32) / info->tsc_to_system_mul;
>> if ( info->tsc_shift < 0 )
>> freq <<= -info->tsc_shift;
>
> do_div()'s output is consumed here. I don't think this hunk is safe to
> convert.
If by "output" you mean its "return value", then it clearly isn't
consumed. And I continue to think that I did express correctly the
effect do_div() did have on "freq".
Jan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Ping: [PATCH] x86: replace a few do_div() uses
2022-01-12 9:28 ` Jan Beulich
@ 2022-02-18 8:39 ` Jan Beulich
2022-02-18 13:22 ` Andrew Cooper
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2022-02-18 8:39 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, xen-devel
On 12.01.2022 10:28, Jan Beulich wrote:
> On 12.01.2022 10:22, Andrew Cooper wrote:
>> On 12/01/2022 09:00, Jan Beulich wrote:
>>> When the macro's "return value" is not used, the macro use can be
>>> replaced by a simply division, avoiding some obfuscation.
>>>
>>> According to my observations, no change to generated code.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> I like this change in principle, but see below.
>>
>> do_div() needs to be deleted, because it's far too easy screw up. At a
>> bare minimum, it should be replaced with a static inline that takes it's
>> first parameter by pointer, because then at least every callsite reads
>> correctly in terms of the C language.
>
> That ought to be a 2nd step, requiring agreement with Arm folks (and
> adjustments to their code).
>
>>> --- a/xen/arch/x86/time.c
>>> +++ b/xen/arch/x86/time.c
>>> @@ -610,8 +610,7 @@ static uint64_t xen_timer_cpu_frequency(
>>> struct vcpu_time_info *info = &this_cpu(vcpu_info)->time;
>>> uint64_t freq;
>>>
>>> - freq = 1000000000ULL << 32;
>>> - do_div(freq, info->tsc_to_system_mul);
>>> + freq = (1000000000ULL << 32) / info->tsc_to_system_mul;
>>> if ( info->tsc_shift < 0 )
>>> freq <<= -info->tsc_shift;
>>
>> do_div()'s output is consumed here. I don't think this hunk is safe to
>> convert.
>
> If by "output" you mean its "return value", then it clearly isn't
> consumed. And I continue to think that I did express correctly the
> effect do_div() did have on "freq".
I think I did address both points (the earlier one was actually more a
remark imo anyway, not a request to change anything right in this patch),
so may I please ask for an ack (or a response clarifying what I'm not
understanding in what you have said)?
Thanks, Jan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Ping: [PATCH] x86: replace a few do_div() uses
2022-02-18 8:39 ` Ping: " Jan Beulich
@ 2022-02-18 13:22 ` Andrew Cooper
0 siblings, 0 replies; 5+ messages in thread
From: Andrew Cooper @ 2022-02-18 13:22 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper; +Cc: Wei Liu, Roger Pau Monne, xen-devel
On 18/02/2022 08:39, Jan Beulich wrote:
> On 12.01.2022 10:28, Jan Beulich wrote:
>> On 12.01.2022 10:22, Andrew Cooper wrote:
>>> On 12/01/2022 09:00, Jan Beulich wrote:
>>>> When the macro's "return value" is not used, the macro use can be
>>>> replaced by a simply division, avoiding some obfuscation.
>>>>
>>>> According to my observations, no change to generated code.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> I like this change in principle, but see below.
>>>
>>> do_div() needs to be deleted, because it's far too easy screw up. At a
>>> bare minimum, it should be replaced with a static inline that takes it's
>>> first parameter by pointer, because then at least every callsite reads
>>> correctly in terms of the C language.
>> That ought to be a 2nd step, requiring agreement with Arm folks (and
>> adjustments to their code).
>>
>>>> --- a/xen/arch/x86/time.c
>>>> +++ b/xen/arch/x86/time.c
>>>> @@ -610,8 +610,7 @@ static uint64_t xen_timer_cpu_frequency(
>>>> struct vcpu_time_info *info = &this_cpu(vcpu_info)->time;
>>>> uint64_t freq;
>>>>
>>>> - freq = 1000000000ULL << 32;
>>>> - do_div(freq, info->tsc_to_system_mul);
>>>> + freq = (1000000000ULL << 32) / info->tsc_to_system_mul;
>>>> if ( info->tsc_shift < 0 )
>>>> freq <<= -info->tsc_shift;
>>> do_div()'s output is consumed here. I don't think this hunk is safe to
>>> convert.
>> If by "output" you mean its "return value", then it clearly isn't
>> consumed. And I continue to think that I did express correctly the
>> effect do_div() did have on "freq".
> I think I did address both points (the earlier one was actually more a
> remark imo anyway, not a request to change anything right in this patch),
> so may I please ask for an ack (or a response clarifying what I'm not
> understanding in what you have said)?
No - you're right. My mistake.
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-02-18 13:23 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-12 9:00 [PATCH] x86: replace a few do_div() uses Jan Beulich
2022-01-12 9:22 ` Andrew Cooper
2022-01-12 9:28 ` Jan Beulich
2022-02-18 8:39 ` Ping: " Jan Beulich
2022-02-18 13:22 ` Andrew Cooper
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.