| Message ID | 9460a5f8-5b6e-bba9-79fc-dae54cc6b348@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: reduce errors in frequency calculations / unrelated cleanup | expand |
On 13/03/2020 09:25, Jan Beulich wrote: > Plain (unsigned) integer division simply truncates the results. The > overall errors are smaller though if we use proper rounding. (Extend > this to the purely cosmetic aspect of time.c's freq_string(), which > before this change I've frequently observed to report e.g. NN.999MHz > HPET clock speeds.) > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > We could switch at least the first div/rem pair in calibrate_APIC_clock() > to use do_div(), but this would imply switching bus_freq (and then also > result) to unsigned int (as the divisor has to be 32-bit). While I think > there's pretty little risk for bus frequencies to go beyond 4GHz in the > next so many years, I still wasn't certain enough this would be a welcome > change. Honestly, do_div() is very easy to get wrong (and in security relevant ways in Linux). I'd advocate for phasing its use out, irrespective of this frequency concern. As for 4GHz, I think its unlikely, but better to be safe in the code. > > --- a/xen/arch/x86/apic.c > +++ b/xen/arch/x86/apic.c > @@ -1261,7 +1261,9 @@ static int __init calibrate_APIC_clock(v > /* set up multipliers for accurate timer code */ > bus_freq = result*HZ; > bus_cycle = (u32) (1000000000000LL/bus_freq); /* in pico seconds */ > + bus_cycle += (1000000000000UL % bus_freq) * 2 > bus_freq; These two differ in signedness of the numerator. GCC seems to cope with combining the two into a single div instruction, but I think we should be consistent with the constant nevertheless. Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
On 13.03.2020 16:14, Andrew Cooper wrote: > On 13/03/2020 09:25, Jan Beulich wrote: >> Plain (unsigned) integer division simply truncates the results. The >> overall errors are smaller though if we use proper rounding. (Extend >> this to the purely cosmetic aspect of time.c's freq_string(), which >> before this change I've frequently observed to report e.g. NN.999MHz >> HPET clock speeds.) >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> We could switch at least the first div/rem pair in calibrate_APIC_clock() >> to use do_div(), but this would imply switching bus_freq (and then also >> result) to unsigned int (as the divisor has to be 32-bit). While I think >> there's pretty little risk for bus frequencies to go beyond 4GHz in the >> next so many years, I still wasn't certain enough this would be a welcome >> change. > > Honestly, do_div() is very easy to get wrong (and in security relevant > ways in Linux). I'd advocate for phasing its use out, irrespective of > this frequency concern. > > As for 4GHz, I think its unlikely, but better to be safe in the code. > >> >> --- a/xen/arch/x86/apic.c >> +++ b/xen/arch/x86/apic.c >> @@ -1261,7 +1261,9 @@ static int __init calibrate_APIC_clock(v >> /* set up multipliers for accurate timer code */ >> bus_freq = result*HZ; >> bus_cycle = (u32) (1000000000000LL/bus_freq); /* in pico seconds */ >> + bus_cycle += (1000000000000UL % bus_freq) * 2 > bus_freq; > > These two differ in signedness of the numerator. GCC seems to cope with > combining the two into a single div instruction, but I think we should > be consistent with the constant nevertheless. IOW you'd like me to change the other line too, to have a UL suffix? If so, at that point I'd drop the stray cast, too. > Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks, but please let me know if the above is a correct understanding of mine. Jan
On 16/03/2020 08:58, Jan Beulich wrote: > On 13.03.2020 16:14, Andrew Cooper wrote: >> On 13/03/2020 09:25, Jan Beulich wrote: >>> Plain (unsigned) integer division simply truncates the results. The >>> overall errors are smaller though if we use proper rounding. (Extend >>> this to the purely cosmetic aspect of time.c's freq_string(), which >>> before this change I've frequently observed to report e.g. NN.999MHz >>> HPET clock speeds.) >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> We could switch at least the first div/rem pair in calibrate_APIC_clock() >>> to use do_div(), but this would imply switching bus_freq (and then also >>> result) to unsigned int (as the divisor has to be 32-bit). While I think >>> there's pretty little risk for bus frequencies to go beyond 4GHz in the >>> next so many years, I still wasn't certain enough this would be a welcome >>> change. >> >> Honestly, do_div() is very easy to get wrong (and in security relevant >> ways in Linux). I'd advocate for phasing its use out, irrespective of >> this frequency concern. >> >> As for 4GHz, I think its unlikely, but better to be safe in the code. >> >>> >>> --- a/xen/arch/x86/apic.c >>> +++ b/xen/arch/x86/apic.c >>> @@ -1261,7 +1261,9 @@ static int __init calibrate_APIC_clock(v >>> /* set up multipliers for accurate timer code */ >>> bus_freq = result*HZ; >>> bus_cycle = (u32) (1000000000000LL/bus_freq); /* in pico seconds */ >>> + bus_cycle += (1000000000000UL % bus_freq) * 2 > bus_freq; >> >> These two differ in signedness of the numerator. GCC seems to cope with >> combining the two into a single div instruction, but I think we should >> be consistent with the constant nevertheless. > > IOW you'd like me to change the other line too, to have a UL > suffix? If so, at that point I'd drop the stray cast, too. That would be fine yes. ~Andrew > >> Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Thanks, but please let me know if the above is a correct > understanding of mine. > > Jan >
--- a/xen/arch/x86/apic.c +++ b/xen/arch/x86/apic.c @@ -1261,7 +1261,9 @@ static int __init calibrate_APIC_clock(v /* set up multipliers for accurate timer code */ bus_freq = result*HZ; bus_cycle = (u32) (1000000000000LL/bus_freq); /* in pico seconds */ + bus_cycle += (1000000000000UL % bus_freq) * 2 > bus_freq; bus_scale = (1000*262144)/bus_cycle; + bus_scale += ((1000 * 262144) % bus_cycle) * 2 > bus_cycle; apic_printk(APIC_VERBOSE, "..... bus_scale = %#x\n", bus_scale); /* reset APIC to zero timeout value */ --- a/xen/arch/x86/hpet.c +++ b/xen/arch/x86/hpet.c @@ -799,9 +799,9 @@ u64 __init hpet_setup(void) hpet_resume(hpet_boot_cfg); hpet_rate = 1000000000000000ULL; /* 10^15 */ - (void)do_div(hpet_rate, hpet_period); + last = do_div(hpet_rate, hpet_period); - return hpet_rate; + return hpet_rate + (last * 2 > hpet_period); } void hpet_resume(u32 *boot_cfg) --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -275,7 +275,10 @@ static char *freq_string(u64 freq) { static char s[20]; unsigned int x, y; - y = (unsigned int)do_div(freq, 1000000) / 1000; + + if ( do_div(freq, 1000) > 500 ) + ++freq; + y = (unsigned int)do_div(freq, 1000); x = (unsigned int)freq; snprintf(s, sizeof(s), "%u.%03uMHz", x, y); return s;
Plain (unsigned) integer division simply truncates the results. The overall errors are smaller though if we use proper rounding. (Extend this to the purely cosmetic aspect of time.c's freq_string(), which before this change I've frequently observed to report e.g. NN.999MHz HPET clock speeds.) Signed-off-by: Jan Beulich <jbeulich@suse.com> --- We could switch at least the first div/rem pair in calibrate_APIC_clock() to use do_div(), but this would imply switching bus_freq (and then also result) to unsigned int (as the divisor has to be 32-bit). While I think there's pretty little risk for bus frequencies to go beyond 4GHz in the next so many years, I still wasn't certain enough this would be a welcome change.