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

[Don Slutz <dslutz@verizon.com>]

On 04/14/14 10:58, Jan Beulich wrote:
>>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:

> The if() is clearly unnecessary here, and with it removed I don't see
> any difference left with the inner if() path above. Hence I guess
> they should be folded. Once that's done, new_val as calculated
> before the outer if() isn't needed in the inner "else" path, and
> hence its truncation could be moved inside the if(). Which in turn
> allows you to fix the comparator64 related bug here too: All other
> code stores the non-truncated value there, just the code here
> doesn't.
>
> Jan

If I understand this correctly, this leads to the following change
(Hopefully not line wrapped):


diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index c3f286f..913beb1 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -404,20 +404,8 @@ static int hpet_write(
      case HPET_Tn_CMP(1):
      case HPET_Tn_CMP(2):
          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) &&
+             !(h->hpet.timers[tn].config & HPET_TN_SETVAL) )
          {
              /*
               * Clamp period to reasonable min/max values:
@@ -429,7 +417,25 @@ static int hpet_write(
              new_val &= (timer_is_32bit(h, tn) ? ~0u : ~0ull) >> 1;
              h->hpet.period[tn] = new_val;
          }
-        h->hpet.comparator64[tn] = new_val;
+        else
+        {
+            /*
+             * 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.comparator64[tn] = new_val;
+            if ( timer_is_32bit(h, tn) )
+                h->hpet.timers[tn].cmp = (uint32_t)new_val;
+            else
+                h->hpet.timers[tn].cmp = new_val;
+        }
          if ( hpet_enabled(h) && timer_enabled(h, tn) )
              set_restart_timer(tn);
          break;


I am still testing that this re-write still does the "right
thing".  Which is why it is yet to be a v3.  This preview
is sent to check that I did correctly do the requested change.


    -Don Slutz