[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 04/14/14 10:58, Jan Beulich wrote:
On 08.04.14 at 16:24, <dslutz@xxxxxxxxxxx> 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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.