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.

--- 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

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