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

2017-08-04 Thread Anthony PERARD
On Thu, Aug 03, 2017 at 09:29:02AM -0600, Jan Beulich wrote:
> >>> Anthony PERARD  07/18/17 7:10 PM >>>
> >@@ -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?

Yeah, I guess it is. I'll move this if block into the next one, which is
if (delta && X).

Thanks,

-- 
Anthony PERARD

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


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

2017-08-03 Thread Jan Beulich
>>> Anthony PERARD  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@lists.xen.org
https://lists.xen.org/xen-devel


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

2017-07-18 Thread Anthony PERARD
The description in the Intel SDM of how the divide configuration
register is used: "The APIC timer frequency will be the processor's bus
clock or core crystal clock frequency divided by the value specified in
the divide configuration register."

Observation of baremetal shown that when the TDCR is change, the TMCCT
does not change or make a big jump in value, but the rate at which it
count down change.

The patch update the emulation to APIC timer to so that a change to the
divide configuration would be reflected in the value of the counter and
when the next interrupt is triggered.

Signed-off-by: Anthony PERARD 
---
 xen/arch/x86/hvm/vlapic.c | 29 +++--
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 71a6010ee4..62d171061f 100644
--- 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.
  * 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)
 {
 uint64_t period;
 uint64_t delta = 0;
@@ -686,7 +687,7 @@ static void vlapic_update_timer(struct vlapic *vlapic, 
uint32_t lvtt);
 is_oneshot = (lvtt & APIC_TIMER_MODE_MASK) == APIC_TIMER_MODE_ONESHOT;
 
 period = (uint64_t)vlapic_get_reg(vlapic, APIC_TMICT)
-* APIC_BUS_CYCLE_NS * vlapic->hw.timer_divisor;
+* APIC_BUS_CYCLE_NS * old_divisor;
 
 /* Calculate the next time the timer should trigger an interrupt. */
 if ( period && vlapic->timer_last_update )
@@ -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;
+}
+
 if ( delta && (is_oneshot || is_periodic) )
 {
 TRACE_2_LONG_3D(TRC_HVM_EMUL_LAPIC_START_TIMER, TRC_PAR_LONG(delta),
@@ -813,7 +821,7 @@ static void vlapic_reg_write(struct vcpu *v,
 }
 vlapic->pt.irq = val & APIC_VECTOR_MASK;
 
-vlapic_update_timer(vlapic, val);
+vlapic_update_timer(vlapic, val, vlapic->hw.timer_divisor);
 
 /* fallthrough */
 case APIC_LVTTHMR:  /* LVT Thermal Monitor */
@@ -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;
 vlapic_set_tdcr(vlapic, val & 0xb);
+
+vlapic_update_timer(vlapic, vlapic_get_reg(vlapic, APIC_LVTT),
+current_divisor);
 HVM_DBG_LOG(DBG_LEVEL_VLAPIC_TIMER, "timer divisor is %#x",
 vlapic->hw.timer_divisor);
 break;
 }
+}
 }
 
 static int vlapic_write(struct vcpu *v, unsigned long address,
-- 
Anthony PERARD


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