Re: [Xen-devel] [PATCH v2 3/3] x86/vlapic: Apply change to TDCR right away to the timer

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Anthony PERARD <anthony.perard@xxxxxxxxxx> 07/18/17 7:10 PM >>>
>--- a/xen/arch/x86/hvm/vlapic.c
>+++ b/xen/arch/x86/hvm/vlapic.c
>@@ -671,12 +671,13 @@ static void vlapic_tdt_pt_cb(struct vcpu *v, void *data)
 >
>/*
>* This function is used when a register related to the APIC timer is updated.
>- * It expect the new value for the register TMICT to be set *before*
>- * been called.
>+ * It expect the new value for the register TMICT and TDCR to be set *before*
>+ * been called, and the previous value of TDCR to be passed as parametter.

argument

>* It expect the new value of LVTT to be set *after* been called, with this new
>* values passed as parameter (only APIC_TIMER_MODE_MASK bits matter).
  >*/
>-static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt);
>+static void vlapic_update_timer(struct vlapic *vlapic, uint32_t lvtt,
>+                                uint32_t old_divisor)

To match up with "lvtt", perhaps "old_tdcr" then? Or wait, having seen
the call site, I think the parameter name is fine, but the comment shouldn't
say TDCR (as its really the derived divisor value which you care about).

>@@ -701,6 +702,13 @@ static void vlapic_update_timer(struct vlapic *vlapic, 
>uint32_t lvtt);
>delta = period - time_passed;
>}
 >
>+    if ( vlapic->hw.timer_divisor != old_divisor )
>+    {
>+        period = (uint64_t)vlapic_get_reg(vlapic, APIC_TMICT)
>+            * APIC_BUS_CYCLE_NS * vlapic->hw.timer_divisor;
>+        delta = delta * vlapic->hw.timer_divisor / old_divisor;
>+    }

Isn't this calculation pointless when delta is zero?

>@@ -843,15 +851,24 @@ static void vlapic_reg_write(struct vcpu *v,
>vlapic->timer_last_update = hvm_get_guest_time(current);
>vlapic_set_reg(vlapic, APIC_TMICT, val);
 >
>-        vlapic_update_timer(vlapic, vlapic_get_reg(vlapic, APIC_LVTT));
>+        vlapic_update_timer(vlapic, vlapic_get_reg(vlapic, APIC_LVTT),
>+                            vlapic->hw.timer_divisor);
>break;
 >
>case APIC_TDCR:
>+    {
>+        uint32_t current_divisor;
>+
>+        current_divisor = vlapic->hw.timer_divisor;

Please make this the initializer of the variable.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel