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 <jbeul...@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.coop...@citrix.com>
