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

[Don Slutz <dslutz@verizon.com>]

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@verizon.com> 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@verizon.com>
Date:   Thu May 1 14:02:47 2014 -0400

     hvm/hpet: Detect comparator values in the past

     Signed-off-by: Don Slutz <dslutz@verizon.com>

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.