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 <[email protected]> > > 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
