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

[Don Slutz <dslutz@xxxxxxxxxxx>]

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:
> > > v3:
> > >    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.
Yes.

>   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
name.

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
                                         63
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


> Tim.

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