For an approach like that used in "x86: detect PIT aliasing on ports
other than 0x4[0-3]" [1] to work, channel 2 may not (appear to) continue
counting when "gate" is low. Record the time when "gate" goes low, and
adjust pit_get_{count,out}() accordingly. Additionally for most of the
modes a rising edge of "gate" doesn't mean just "resume counting", but
"initiate counting", i.e. specifically the reloading of the counter with
its init value.

No special handling for state save/load: See the comment near the end of
pit_load().

[1] https://lists.xen.org/archives/html/xen-devel/2023-05/msg00898.html

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: "gate" can only ever be low for chan2 (with "x86/vPIT: check/bound
     values loaded from state save record" [2] in place), so in
     principle we could get away without a new pair of arrays, but just
     two individual fields. At the expense of more special casing in
     code.

TBD: Should we deal with other aspects of "gate low" in pit_get_out()
     here as well, right away? I was hoping to get away without ...
     (Note how the two functions also disagree in their placement of the
     "default" labels, even if that's largely benign when taking into
     account that modes 6 and 7 are transformed to 2 and 3 respectively
     by pit_load(). A difference would occur only before the guest first
     sets the mode, as pit_reset() sets it to 7.)

Other observations:
- Loading of new counts occurs too early in some of the modes (2/3: at
  end of current sequence or when gate goes high; 1/5: only when gate
  goes high).
- BCD counting doesn't appear to be properly supported either (at least
  that's mentioned in the public header).

[2] https://lists.xen.org/archives/html/xen-devel/2023-05/msg00887.html
---
v2: In pit_load_count() also set count_stop_time from count_load_time
    (in case the counter is stopped). Correct spelling in comments.
    Correct calculations in pit_get_{count,out}().

--- a/xen/arch/x86/emul-i8254.c
+++ b/xen/arch/x86/emul-i8254.c
@@ -65,7 +65,10 @@ static int pit_get_count(PITState *pit,
 
     ASSERT(spin_is_locked(&pit->lock));
 
-    d = muldiv64(get_guest_time(v) - pit->count_load_time[channel],
+    d = pit->hw.channels[channel].gate || (c->mode & 3) == 1
+        ? get_guest_time(v)
+        : pit->count_stop_time[channel];
+    d = muldiv64(d - pit->count_load_time[channel] - pit->stopped_time[channel],
                  PIT_FREQ, SYSTEM_TIME_HZ);
 
     switch ( c->mode )
@@ -110,6 +113,10 @@ static void pit_load_count(PITState *pit
         pit->count_load_time[channel] = 0;
     else
         pit->count_load_time[channel] = get_guest_time(v);
+
+    pit->count_stop_time[channel] = pit->count_load_time[channel];
+    pit->stopped_time[channel] = 0;
+
     s->count = val;
     period = DIV_ROUND(val * SYSTEM_TIME_HZ, PIT_FREQ);
 
@@ -148,7 +155,10 @@ static int pit_get_out(PITState *pit, in
 
     ASSERT(spin_is_locked(&pit->lock));
 
-    d = muldiv64(get_guest_time(v) - pit->count_load_time[channel], 
+    d = pit->hw.channels[channel].gate || (s->mode & 3) == 1
+        ? get_guest_time(v)
+        : pit->count_stop_time[channel];
+    d = muldiv64(d - pit->count_load_time[channel] - pit->stopped_time[channel],
                  PIT_FREQ, SYSTEM_TIME_HZ);
 
     switch ( s->mode )
@@ -182,22 +192,39 @@ static void pit_set_gate(PITState *pit,
 
     ASSERT(spin_is_locked(&pit->lock));
 
-    switch ( s->mode )
-    {
-    default:
-    case 0:
-    case 4:
-        /* XXX: just disable/enable counting */
-        break;
-    case 1:
-    case 5:
-    case 2:
-    case 3:
-        /* Restart counting on rising edge. */
-        if ( s->gate < val )
-            pit->count_load_time[channel] = get_guest_time(v);
-        break;
-    }
+    if ( s->gate > val )
+        switch ( s->mode )
+        {
+        case 0:
+        case 2:
+        case 3:
+        case 4:
+            /* Disable counting. */
+            if ( !channel )
+                destroy_periodic_time(&pit->pt0);
+            pit->count_stop_time[channel] = get_guest_time(v);
+            break;
+        }
+
+    if ( s->gate < val )
+        switch ( s->mode )
+        {
+        default:
+        case 0:
+        case 4:
+            /* Enable counting. */
+            pit->stopped_time[channel] += get_guest_time(v) -
+                                          pit->count_stop_time[channel];
+            break;
+
+        case 1:
+        case 5:
+        case 2:
+        case 3:
+            /* Initiate counting on rising edge. */
+            pit_load_count(pit, channel, pit->hw.channels[channel].count);
+            break;
+        }
 
     s->gate = val;
 }
--- a/xen/arch/x86/include/asm/hvm/vpt.h
+++ b/xen/arch/x86/include/asm/hvm/vpt.h
@@ -48,8 +48,14 @@ struct periodic_time {
 typedef struct PITState {
     /* Hardware state */
     struct hvm_hw_pit hw;
-    /* Last time the counters read zero, for calcuating counter reads */
+
+    /* Last time the counters read zero, for calculating counter reads */
     int64_t count_load_time[3];
+    /* Last time the counters were stopped, for calculating counter reads */
+    int64_t count_stop_time[3];
+    /* Accumulate "stopped" time, since the last counter write/reload. */
+    uint64_t stopped_time[3];
+
     /* Channel 0 IRQ handling. */
     struct periodic_time pt0;
     spinlock_t lock;

On Thu, Jun 15, 2023 at 04:56:22PM +0200, Jan Beulich wrote:
> For an approach like that used in "x86: detect PIT aliasing on ports
> other than 0x4[0-3]" [1] to work, channel 2 may not (appear to) continue
> counting when "gate" is low. Record the time when "gate" goes low, and
> adjust pit_get_{count,out}() accordingly. Additionally for most of the
> modes a rising edge of "gate" doesn't mean just "resume counting", but
> "initiate counting", i.e. specifically the reloading of the counter with
> its init value.
> 
> No special handling for state save/load: See the comment near the end of
> pit_load().
> 
> [1] https://lists.xen.org/archives/html/xen-devel/2023-05/msg00898.html
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Albeit I have one request below I would like you to considerate.

> ---
> TBD: "gate" can only ever be low for chan2 (with "x86/vPIT: check/bound
>      values loaded from state save record" [2] in place), so in
>      principle we could get away without a new pair of arrays, but just
>      two individual fields. At the expense of more special casing in
>      code.

One bit I'm missing is how is the gate for timers 0 and 1 is accessed.
Is such line simply not accessible?

My i8254 spec doesn't mention this, and the (kind of random) copy of
the ICH7 Spec I'm looking at also doesn't mention enable bits for
timers 0 and 1 being available.  I assume those are always enabled?

> 
> TBD: Should we deal with other aspects of "gate low" in pit_get_out()
>      here as well, right away? I was hoping to get away without ...
>      (Note how the two functions also disagree in their placement of the
>      "default" labels, even if that's largely benign when taking into
>      account that modes 6 and 7 are transformed to 2 and 3 respectively
>      by pit_load(). A difference would occur only before the guest first
>      sets the mode, as pit_reset() sets it to 7.)

I wouldn't, but as mentioned before I would also avoid touching the
PIT much unless it's fixing an issue that's reproducible.  I think the
chances of us messing up while modifying the code is high due to the
lack of testing.

> 
> Other observations:
> - Loading of new counts occurs too early in some of the modes (2/3: at
>   end of current sequence or when gate goes high; 1/5: only when gate
>   goes high).
> - BCD counting doesn't appear to be properly supported either (at least
>   that's mentioned in the public header).
> 
> [2] https://lists.xen.org/archives/html/xen-devel/2023-05/msg00887.html
> ---
> v2: In pit_load_count() also set count_stop_time from count_load_time
>     (in case the counter is stopped). Correct spelling in comments.
>     Correct calculations in pit_get_{count,out}().
> 
> --- a/xen/arch/x86/emul-i8254.c
> +++ b/xen/arch/x86/emul-i8254.c
> @@ -65,7 +65,10 @@ static int pit_get_count(PITState *pit,
>  
>      ASSERT(spin_is_locked(&pit->lock));
>  
> -    d = muldiv64(get_guest_time(v) - pit->count_load_time[channel],
> +    d = pit->hw.channels[channel].gate || (c->mode & 3) == 1
> +        ? get_guest_time(v)
> +        : pit->count_stop_time[channel];
> +    d = muldiv64(d - pit->count_load_time[channel] - pit->stopped_time[channel],
>                   PIT_FREQ, SYSTEM_TIME_HZ);
>  
>      switch ( c->mode )
> @@ -110,6 +113,10 @@ static void pit_load_count(PITState *pit
>          pit->count_load_time[channel] = 0;
>      else
>          pit->count_load_time[channel] = get_guest_time(v);
> +
> +    pit->count_stop_time[channel] = pit->count_load_time[channel];
> +    pit->stopped_time[channel] = 0;
> +
>      s->count = val;
>      period = DIV_ROUND(val * SYSTEM_TIME_HZ, PIT_FREQ);
>  
> @@ -148,7 +155,10 @@ static int pit_get_out(PITState *pit, in
>  
>      ASSERT(spin_is_locked(&pit->lock));
>  
> -    d = muldiv64(get_guest_time(v) - pit->count_load_time[channel], 
> +    d = pit->hw.channels[channel].gate || (s->mode & 3) == 1
> +        ? get_guest_time(v)
> +        : pit->count_stop_time[channel];
> +    d = muldiv64(d - pit->count_load_time[channel] - pit->stopped_time[channel],
>                   PIT_FREQ, SYSTEM_TIME_HZ);

The above logic is repeated here and in pit_get_count(), maybe could
be abstracted into a common helper to keep both in sync? (get_counter())

Thanks, Roger.

On 16.06.2023 16:18, Roger Pau Monné wrote:
> On Thu, Jun 15, 2023 at 04:56:22PM +0200, Jan Beulich wrote:
>> For an approach like that used in "x86: detect PIT aliasing on ports
>> other than 0x4[0-3]" [1] to work, channel 2 may not (appear to) continue
>> counting when "gate" is low. Record the time when "gate" goes low, and
>> adjust pit_get_{count,out}() accordingly. Additionally for most of the
>> modes a rising edge of "gate" doesn't mean just "resume counting", but
>> "initiate counting", i.e. specifically the reloading of the counter with
>> its init value.
>>
>> No special handling for state save/load: See the comment near the end of
>> pit_load().
>>
>> [1] https://lists.xen.org/archives/html/xen-devel/2023-05/msg00898.html
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> Albeit I have one request below I would like you to considerate.
> 
>> ---
>> TBD: "gate" can only ever be low for chan2 (with "x86/vPIT: check/bound
>>      values loaded from state save record" [2] in place), so in
>>      principle we could get away without a new pair of arrays, but just
>>      two individual fields. At the expense of more special casing in
>>      code.
> 
> One bit I'm missing is how is the gate for timers 0 and 1 is accessed.
> Is such line simply not accessible?

On old 8086 systems I believe there were actual signals, ...

> My i8254 spec doesn't mention this, and the (kind of random) copy of
> the ICH7 Spec I'm looking at also doesn't mention enable bits for
> timers 0 and 1 being available.  I assume those are always enabled?

... but from all I could collect the gates are always enabled in even
just half-way modern systems.

>> --- a/xen/arch/x86/emul-i8254.c
>> +++ b/xen/arch/x86/emul-i8254.c
>> @@ -65,7 +65,10 @@ static int pit_get_count(PITState *pit,
>>  
>>      ASSERT(spin_is_locked(&pit->lock));
>>  
>> -    d = muldiv64(get_guest_time(v) - pit->count_load_time[channel],
>> +    d = pit->hw.channels[channel].gate || (c->mode & 3) == 1
>> +        ? get_guest_time(v)
>> +        : pit->count_stop_time[channel];
>> +    d = muldiv64(d - pit->count_load_time[channel] - pit->stopped_time[channel],
>>                   PIT_FREQ, SYSTEM_TIME_HZ);
>>  
>>      switch ( c->mode )
>>[...]
>> @@ -148,7 +155,10 @@ static int pit_get_out(PITState *pit, in
>>  
>>      ASSERT(spin_is_locked(&pit->lock));
>>  
>> -    d = muldiv64(get_guest_time(v) - pit->count_load_time[channel], 
>> +    d = pit->hw.channels[channel].gate || (s->mode & 3) == 1
>> +        ? get_guest_time(v)
>> +        : pit->count_stop_time[channel];
>> +    d = muldiv64(d - pit->count_load_time[channel] - pit->stopped_time[channel],
>>                   PIT_FREQ, SYSTEM_TIME_HZ);
> 
> The above logic is repeated here and in pit_get_count(), maybe could
> be abstracted into a common helper to keep both in sync? (get_counter())

I was pondering whether to do so, but decided not to because the earlier
code could already have benefited the same, just to a lesser extent. But
since you ask for it - sure, can do. Unless told otherwise I'll assume
your ack hold with that transformation. (While it'll end up inconsistent
with other code, I'm pretty determined to make the necessary "channel"
parameter of the helper "unsigned int". That's an aspect I was trying to
"escape" by not introducing a helper ...)

Jan