| Message ID | 123a9ae8-caab-01ae-5bea-8c590bd8f9d9@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: further improve timer freq calibration accuracy | expand |
On Mon, Feb 14, 2022 at 10:25:57AM +0100, Jan Beulich wrote: > Making adjustments to arbitrarily chosen values shouldn't require > auditing the code for possible derived numbers - such a change should > be doable in a single place, having an effect on all code depending on > that choice. > > For one make the TDCR write actually use APIC_DIVISOR. With the > necessary mask constant introduced, also use that in vLAPIC code. While > introducing the constant, drop APIC_TDR_DIV_TMBASE: The bit has been > undefined in halfway recent SDM and PM versions. > > And then introduce a constant tying together the scale used when > converting nanoseconds to bus clocks. > > No functional change intended. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > --- > I thought we have a generic "glue" macro, but I couldn't find one. Hence > I'm (ab)using _AC(). I would be fine if you want to introduce something right in this commit to cover those needs, using _AC is not overly nice (or clear) IMO. Thanks, Roger.
On 11.03.2022 15:24, Roger Pau Monné wrote: > On Mon, Feb 14, 2022 at 10:25:57AM +0100, Jan Beulich wrote: >> Making adjustments to arbitrarily chosen values shouldn't require >> auditing the code for possible derived numbers - such a change should >> be doable in a single place, having an effect on all code depending on >> that choice. >> >> For one make the TDCR write actually use APIC_DIVISOR. With the >> necessary mask constant introduced, also use that in vLAPIC code. While >> introducing the constant, drop APIC_TDR_DIV_TMBASE: The bit has been >> undefined in halfway recent SDM and PM versions. >> >> And then introduce a constant tying together the scale used when >> converting nanoseconds to bus clocks. >> >> No functional change intended. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. >> --- >> I thought we have a generic "glue" macro, but I couldn't find one. Hence >> I'm (ab)using _AC(). > > I would be fine if you want to introduce something right in this > commit to cover those needs, using _AC is not overly nice (or > clear) IMO. Hmm, I was rather hoping that you (or someone else) would point me at what I thought I'm overlooking. If anything I'd likely clone Linux'es __PASTE() (avoiding the leading underscores), but their placement in linux/compiler_types.h seems pretty arbitrary and hence not a good guideline for placement in our tree. To be honest the only thing that would seem halfway consistent to me would be a separate header, yet that seems somewhat overkill ... Or wait - maybe xen/lib.h could be viewed as kind of suitable. Of course there's then the immediate question of whether to make _AC() use the new macro instead of open-coding it. Jan
On Mon, Mar 14, 2022 at 09:19:01AM +0100, Jan Beulich wrote: > On 11.03.2022 15:24, Roger Pau Monné wrote: > > On Mon, Feb 14, 2022 at 10:25:57AM +0100, Jan Beulich wrote: > >> Making adjustments to arbitrarily chosen values shouldn't require > >> auditing the code for possible derived numbers - such a change should > >> be doable in a single place, having an effect on all code depending on > >> that choice. > >> > >> For one make the TDCR write actually use APIC_DIVISOR. With the > >> necessary mask constant introduced, also use that in vLAPIC code. While > >> introducing the constant, drop APIC_TDR_DIV_TMBASE: The bit has been > >> undefined in halfway recent SDM and PM versions. > >> > >> And then introduce a constant tying together the scale used when > >> converting nanoseconds to bus clocks. > >> > >> No functional change intended. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks. > > >> --- > >> I thought we have a generic "glue" macro, but I couldn't find one. Hence > >> I'm (ab)using _AC(). > > > > I would be fine if you want to introduce something right in this > > commit to cover those needs, using _AC is not overly nice (or > > clear) IMO. > > Hmm, I was rather hoping that you (or someone else) would point me > at what I thought I'm overlooking. If anything I'd likely clone > Linux'es __PASTE() (avoiding the leading underscores), but their > placement in linux/compiler_types.h seems pretty arbitrary and > hence not a good guideline for placement in our tree. To be honest > the only thing that would seem halfway consistent to me would be a > separate header, yet that seems somewhat overkill ... Or wait - > maybe xen/lib.h could be viewed as kind of suitable. Of course > there's then the immediate question of whether to make _AC() use > the new macro instead of open-coding it. I think if possible _AC should be switched to use the new macro, yes. Thanks, Roger.
--- a/xen/arch/x86/apic.c +++ b/xen/arch/x86/apic.c @@ -1078,8 +1078,8 @@ static void __setup_APIC_LVTT(unsigned i lvtt_value = /*APIC_TIMER_MODE_PERIODIC |*/ LOCAL_TIMER_VECTOR; apic_write(APIC_LVTT, lvtt_value); - tmp_value = apic_read(APIC_TDCR); - apic_write(APIC_TDCR, tmp_value | APIC_TDR_DIV_1); + tmp_value = apic_read(APIC_TDCR) & ~APIC_TDR_DIV_MASK; + apic_write(APIC_TDCR, tmp_value | _AC(APIC_TDR_DIV_, APIC_DIVISOR)); apic_write(APIC_TMICT, clocks / APIC_DIVISOR); } @@ -1196,6 +1196,8 @@ static void __init check_deadline_errata * APIC irq that way. */ +#define BUS_SCALE_SHIFT 18 + static void __init calibrate_APIC_clock(void) { unsigned long bus_freq; /* KAF: pointer-size avoids compile warns. */ @@ -1249,8 +1251,8 @@ static void __init calibrate_APIC_clock( /* set up multipliers for accurate timer code */ bus_cycle = 1000000000000UL / 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; + bus_scale = (1000 << BUS_SCALE_SHIFT) / bus_cycle; + bus_scale += ((1000 << BUS_SCALE_SHIFT) % bus_cycle) * 2 > bus_cycle; apic_printk(APIC_VERBOSE, "..... bus_scale = %#x\n", bus_scale); /* reset APIC to zero timeout value */ @@ -1337,7 +1339,8 @@ int reprogram_timer(s_time_t timeout) } if ( timeout && ((expire = timeout - NOW()) > 0) ) - apic_tmict = min_t(u64, (bus_scale * expire) >> 18, UINT_MAX); + apic_tmict = min_t(uint64_t, (bus_scale * expire) >> BUS_SCALE_SHIFT, + UINT32_MAX); apic_write(APIC_TMICT, (unsigned long)apic_tmict); --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -580,7 +580,7 @@ static uint32_t vlapic_get_tmcct(const s static void vlapic_set_tdcr(struct vlapic *vlapic, unsigned int val) { /* Only bits 0, 1 and 3 are settable; others are MBZ. */ - val &= 0xb; + val &= APIC_TDR_DIV_MASK; vlapic_set_reg(vlapic, APIC_TDCR, val); /* Update the demangled hw.timer_divisor. */ @@ -887,7 +887,7 @@ void vlapic_reg_write(struct vcpu *v, un { uint32_t current_divisor = vlapic->hw.timer_divisor; - vlapic_set_tdcr(vlapic, val & 0xb); + vlapic_set_tdcr(vlapic, val); vlapic_update_timer(vlapic, vlapic_get_reg(vlapic, APIC_LVTT), false, current_divisor); @@ -1019,7 +1019,7 @@ int guest_wrmsr_x2apic(struct vcpu *v, u break; case APIC_TDCR: - if ( msr_content & ~APIC_TDR_DIV_1 ) + if ( msr_content & ~APIC_TDR_DIV_MASK ) return X86EMUL_EXCEPTION; break; --- a/xen/arch/x86/include/asm/apicdef.h +++ b/xen/arch/x86/include/asm/apicdef.h @@ -106,7 +106,7 @@ #define APIC_TMICT 0x380 #define APIC_TMCCT 0x390 #define APIC_TDCR 0x3E0 -#define APIC_TDR_DIV_TMBASE (1<<2) +#define APIC_TDR_DIV_MASK 0xB #define APIC_TDR_DIV_1 0xB #define APIC_TDR_DIV_2 0x0 #define APIC_TDR_DIV_4 0x1
Making adjustments to arbitrarily chosen values shouldn't require auditing the code for possible derived numbers - such a change should be doable in a single place, having an effect on all code depending on that choice. For one make the TDCR write actually use APIC_DIVISOR. With the necessary mask constant introduced, also use that in vLAPIC code. While introducing the constant, drop APIC_TDR_DIV_TMBASE: The bit has been undefined in halfway recent SDM and PM versions. And then introduce a constant tying together the scale used when converting nanoseconds to bus clocks. No functional change intended. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- I thought we have a generic "glue" macro, but I couldn't find one. Hence I'm (ab)using _AC(). --- v2: New.