[Xen-devel] [PATCH v2 03/10] hvm/hpet: Only set comparator or period not both.

[Don Slutz <dslutz@xxxxxxxxxxx>]

The current code sets both.  If setting the comparator also set
comparator64 (the hidden version).

Based on:

software-developers-hpet-spec-1-0a.pdf

A write call should only change comparator or period, not both.

Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>
---
 xen/arch/x86/hvm/hpet.c | 58 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 16c0f07..0795f29 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -399,30 +399,48 @@ static int hpet_write(
         tn = HPET_TN(CMP, addr);
         if ( timer_is_32bit(h, tn) )
             new_val = (uint32_t)new_val;
-        h->hpet.timers[tn].cmp = new_val;
-        if ( h->hpet.timers[tn].config & HPET_TN_SETVAL )
-            /*
-             * When SETVAL is one, software is able to "directly set a periodic
-             * timer's accumulator."  That is, set the comparator without
-             * adjusting the period.  Much the same as just setting the
-             * comparator on an enabled one-shot timer.
-             *
-             * This configuration bit clears when the comparator is written.
-             */
-            h->hpet.timers[tn].config &= ~HPET_TN_SETVAL;
-        else if ( timer_is_periodic(h, tn) )
+        if ( timer_is_periodic(h, tn) )
+        {
+            if ( h->hpet.timers[tn].config & HPET_TN_SETVAL )
+            {
+                /*
+                 * When SETVAL is one, software is able to "directly
+                 * set a periodic timer's accumulator."  That is,
+                 * set the comparator without adjusting the period.
+                 * Much the same as just setting the comparator on
+                 * an enabled one-shot timer.
+                 *
+                 * This configuration bit clears when the comparator
+                 * is written.
+                 */
+                h->hpet.timers[tn].config &= ~HPET_TN_SETVAL;
+                h->hpet.timers[tn].cmp = new_val;
+                h->hpet.comparator64[tn] = new_val;
+            }
+            else
+            {
+                /*
+                 * Clamp period to reasonable min/max values:
+                 *  - minimum is 100us, same as timers controlled by vpt.c
+                 *  - maximum is to prevent overflow in time_after()
+                 *    calculations
+                 */
+                if ( hpet_tick_to_ns(h, new_val) < MICROSECS(100) )
+                    new_val = (MICROSECS(100) << 10) / h->hpet_to_ns_scale;
+                new_val &= (timer_is_32bit(h, tn) ? ~0u : ~0ull) >> 1;
+                h->hpet.period[tn] = new_val;
+            }
+        }
+        else
         {
             /*
-             * Clamp period to reasonable min/max values:
-             *  - minimum is 100us, same as timers controlled by vpt.c
-             *  - maximum is to prevent overflow in time_after() calculations
+             * The spec says "do not use", but clear if specified.
              */
-            if ( hpet_tick_to_ns(h, new_val) < MICROSECS(100) )
-                new_val = (MICROSECS(100) << 10) / h->hpet_to_ns_scale;
-            new_val &= (timer_is_32bit(h, tn) ? ~0u : ~0ull) >> 1;
-            h->hpet.period[tn] = new_val;
+            if ( h->hpet.timers[tn].config & HPET_TN_SETVAL )
+                h->hpet.timers[tn].config &= ~HPET_TN_SETVAL;
+            h->hpet.timers[tn].cmp = new_val;
+            h->hpet.comparator64[tn] = new_val;
         }
-        h->hpet.comparator64[tn] = new_val;
         if ( hpet_enabled(h) && timer_enabled(h, tn) )
             set_restart_timer(tn);
         break;
-- 
1.8.4


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