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

Re: [Xen-devel] [PATCH v3 11/11] hvm/hpet: handle 1st period special

On 05/01/14 06:31, Tim Deegan wrote:
At 13:32 +0100 on 25 Apr (1398429157), Jan Beulich wrote:
On 17.04.14 at 19:43, <dslutz@xxxxxxxxxxx> wrote:
   Better commit message.
   More setting of first_mc64 & first_enabled when needed.
   Switch to bool_t.

  xen/arch/x86/hvm/hpet.c       | 83 ++++++++++++++++++++++++++++++++++++-------
  xen/include/asm-x86/hvm/vpt.h |  2 ++
  2 files changed, 73 insertions(+), 12 deletions(-)
I still can't help myself thinking that this must be achievable with less
special casing - I just can't imagine that hardware also has this huge
an amount of special case logic just for the first period.
+1.  I didn't follow the explanation on v1 very clearly, so perhaps
you can correct my understanding, but I _think_ the issue you're
describing is:

  when we read the comparator, we try to wind it forward from its
  current value towards the main counter value, by adding as many
  periods as will fit.


  If the guest sets the comparator >1 period away, we'll incorrectly
  warp the comparator to some foolish value as soon as we read it.

Yes. The "foolish value" falls within the bounds of a period.

So, you're adding mechanism to detect the first interrupt after a
comparator set (or when the main counter is being started, or on hpet
load -- both of those rely on 'first_enabled' being essentialy
harmless if set when it's _not_ the first interrupt, which suggests
that maybe that's not the best name for the flag).

It is not harmless if it is set when _not_ in the first interval (before
first interrupt).  Since it is used by hpet_save during migration,
the correct values need to be saved.   I do not care about the

You are missing the case when the guest sets the main counter.

Couldn't it be fixed more simply by detecting comparator values in the
past in hpet_get_comparator()?

This statement only works using 64-bit arithmetic for the main counter never
changing by more then 2     .   (Which is a boundary case that should not
happen in my life time.)

Without patch #11 (this patch) and without patch #8 (Use signed divide...)
and adding:

commit 8c450bed530b13c3f3d18b9dafb3d1b1d69ac1f2
Author: Don Slutz <dslutz@xxxxxxxxxxx>
Date:   Thu May 1 14:02:47 2014 -0400

    hvm/hpet: Detect comparator values in the past

    Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 4e4da20..d93dd69 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -98,10 +98,12 @@ static uint64_t hpet_get_comparator(HPETState *h, unsigned 
int tn,
         uint64_t period = h->hpet.period[tn];
         if (period)
-            elapsed = hpet_read_maincounter(h, guest_time) +
-                period - comparator;
-            comparator += (elapsed / period) * period;
-            h->hpet.comparator64[tn] = comparator;
+            elapsed = hpet_read_maincounter(h, guest_time) - comparator;
+            if ( (int64_t)elapsed >= 0 )
+            {
+                comparator += ((elapsed + period) / period) * period;
+                h->hpet.comparator64[tn] = comparator;
+            }

Which I think was what you mean.

It looks to be "ok" (ignoring the boundary case).  So should
I send it as v4?

   -Don Slutz


Xen-devel mailing list



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