[PATCH 0/2] x86/vPIT: account for "counter stopped" time

[PATCH 0/2] x86/vPIT: account for "counter stopped" time

[Jan Beulich]

This addresses an observation made while putting together "x86: detect
PIT aliasing on ports other than 0x4[0-3]".

1: re-order functions
2: account for "counter stopped" time

Jan

[PATCH 1/2] x86/vPIT: re-order functions

[Jan Beulich]

To avoid the need for a forward declaration of pit_load_count() in a
subsequent change, move it earlier in the file (along with its helper
callback).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/emul-i8254.c
+++ b/xen/arch/x86/emul-i8254.c
@@ -87,6 +87,57 @@ static int pit_get_count(PITState *pit,
     return counter;
 }
 
+static void cf_check pit_time_fired(struct vcpu *v, void *priv)
+{
+    uint64_t *count_load_time = priv;
+    TRACE_0D(TRC_HVM_EMUL_PIT_TIMER_CB);
+    *count_load_time = get_guest_time(v);
+}
+
+static void pit_load_count(PITState *pit, int channel, int val)
+{
+    u32 period;
+    struct hvm_hw_pit_channel *s = &pit->hw.channels[channel];
+    struct vcpu *v = vpit_vcpu(pit);
+
+    ASSERT(spin_is_locked(&pit->lock));
+
+    if ( val == 0 )
+        val = 0x10000;
+
+    if ( v == NULL )
+        pit->count_load_time[channel] = 0;
+    else
+        pit->count_load_time[channel] = get_guest_time(v);
+    s->count = val;
+    period = DIV_ROUND(val * SYSTEM_TIME_HZ, PIT_FREQ);
+
+    if ( (v == NULL) || !is_hvm_vcpu(v) || (channel != 0) )
+        return;
+
+    switch ( s->mode )
+    {
+    case 2:
+    case 3:
+        /* Periodic timer. */
+        TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, period);
+        create_periodic_time(v, &pit->pt0, period, period, 0, pit_time_fired,
+                             &pit->count_load_time[channel], false);
+        break;
+    case 1:
+    case 4:
+        /* One-shot timer. */
+        TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, 0);
+        create_periodic_time(v, &pit->pt0, period, 0, 0, pit_time_fired,
+                             &pit->count_load_time[channel], false);
+        break;
+    default:
+        TRACE_0D(TRC_HVM_EMUL_PIT_STOP_TIMER);
+        destroy_periodic_time(&pit->pt0);
+        break;
+    }
+}
+
 static int pit_get_out(PITState *pit, int channel)
 {
     struct hvm_hw_pit_channel *s = &pit->hw.channels[channel];
@@ -156,57 +207,6 @@ static int pit_get_gate(PITState *pit, i
     return pit->hw.channels[channel].gate;
 }
 
-static void cf_check pit_time_fired(struct vcpu *v, void *priv)
-{
-    uint64_t *count_load_time = priv;
-    TRACE_0D(TRC_HVM_EMUL_PIT_TIMER_CB);
-    *count_load_time = get_guest_time(v);
-}
-
-static void pit_load_count(PITState *pit, int channel, int val)
-{
-    u32 period;
-    struct hvm_hw_pit_channel *s = &pit->hw.channels[channel];
-    struct vcpu *v = vpit_vcpu(pit);
-
-    ASSERT(spin_is_locked(&pit->lock));
-
-    if ( val == 0 )
-        val = 0x10000;
-
-    if ( v == NULL )
-        pit->count_load_time[channel] = 0;
-    else
-        pit->count_load_time[channel] = get_guest_time(v);
-    s->count = val;
-    period = DIV_ROUND(val * SYSTEM_TIME_HZ, PIT_FREQ);
-
-    if ( (v == NULL) || !is_hvm_vcpu(v) || (channel != 0) )
-        return;
-
-    switch ( s->mode )
-    {
-    case 2:
-    case 3:
-        /* Periodic timer. */
-        TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, period);
-        create_periodic_time(v, &pit->pt0, period, period, 0, pit_time_fired, 
-                             &pit->count_load_time[channel], false);
-        break;
-    case 1:
-    case 4:
-        /* One-shot timer. */
-        TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, 0);
-        create_periodic_time(v, &pit->pt0, period, 0, 0, pit_time_fired,
-                             &pit->count_load_time[channel], false);
-        break;
-    default:
-        TRACE_0D(TRC_HVM_EMUL_PIT_STOP_TIMER);
-        destroy_periodic_time(&pit->pt0);
-        break;
-    }
-}
-
 static void pit_latch_count(PITState *pit, int channel)
 {
     struct hvm_hw_pit_channel *c = &pit->hw.channels[channel];

[PATCH 2/2] x86/vPIT: account for "counter stopped" time

[Jan Beulich]

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

--- 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_load_time[channel]
+        : pit->count_stop_time[channel];
+    d = muldiv64(d - pit->stopped_time[channel],
                  PIT_FREQ, SYSTEM_TIME_HZ);
 
     switch ( c->mode )
@@ -109,6 +112,7 @@ static void pit_load_count(PITState *pit
         pit->count_load_time[channel] = 0;
     else
         pit->count_load_time[channel] = get_guest_time(v);
+    pit->stopped_time[channel] = 0;
     s->count = val;
     period = DIV_ROUND(val * SYSTEM_TIME_HZ, PIT_FREQ);
 
@@ -147,7 +151,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_load_time[channel]
+        : pit->count_stop_time[channel];
+    d = muldiv64(d - pit->stopped_time[channel],
                  PIT_FREQ, SYSTEM_TIME_HZ);
 
     switch ( s->mode )
@@ -181,22 +188,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 */
     int64_t count_load_time[3];
+    /* Last time the counters were stopped, for calcuating 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;

Re: [PATCH 1/2] x86/vPIT: re-order functions

[Roger Pau Monné]

On Tue, May 30, 2023 at 05:30:02PM +0200, Jan Beulich wrote:
> To avoid the need for a forward declaration of pit_load_count() in a
> subsequent change, move it earlier in the file (along with its helper
> callback).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Just a couple of nits, which you might also noticed but decided to not
fix given this is just code movement.

> 
> --- a/xen/arch/x86/emul-i8254.c
> +++ b/xen/arch/x86/emul-i8254.c
> @@ -87,6 +87,57 @@ static int pit_get_count(PITState *pit,
>      return counter;
>  }
>  
> +static void cf_check pit_time_fired(struct vcpu *v, void *priv)

Seems like v could be constified?

> +{
> +    uint64_t *count_load_time = priv;
> +    TRACE_0D(TRC_HVM_EMUL_PIT_TIMER_CB);
> +    *count_load_time = get_guest_time(v);
> +}
> +
> +static void pit_load_count(PITState *pit, int channel, int val)
> +{
> +    u32 period;

uint32_t

Thanks, Roger.

Re: [PATCH 1/2] x86/vPIT: re-order functions

[Jan Beulich]

On 01.06.2023 11:17, Roger Pau Monné wrote:
> On Tue, May 30, 2023 at 05:30:02PM +0200, Jan Beulich wrote:
>> To avoid the need for a forward declaration of pit_load_count() in a
>> subsequent change, move it earlier in the file (along with its helper
>> callback).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> Just a couple of nits, which you might also noticed but decided to not
> fix given this is just code movement.

Indeed, I meant this to be pure code movement. Nevertheless I'd be happy
to take care of style issues, if that's deemed okay in a "pure code
movement" patch. However, ...

>> --- a/xen/arch/x86/emul-i8254.c
>> +++ b/xen/arch/x86/emul-i8254.c
>> @@ -87,6 +87,57 @@ static int pit_get_count(PITState *pit,
>>      return counter;
>>  }
>>  
>> +static void cf_check pit_time_fired(struct vcpu *v, void *priv)
> 
> Seems like v could be constified?

... the function being used as a callback, I doubt adding const would
be possible. Otoh ...

>> +{
>> +    uint64_t *count_load_time = priv;

... there's a blank line missing here, if I was to go for style
adjustments.

Jan

>> +    TRACE_0D(TRC_HVM_EMUL_PIT_TIMER_CB);
>> +    *count_load_time = get_guest_time(v);
>> +}
>> +
>> +static void pit_load_count(PITState *pit, int channel, int val)
>> +{
>> +    u32 period;
> 
> uint32_t
> 
> Thanks, Roger.

Re: [PATCH 2/2] x86/vPIT: account for "counter stopped" time

[Roger Pau Monné]

On Tue, May 30, 2023 at 05:30:40PM +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>
> ---
> 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.

Hm, I guess we could rename to pit_set_gate_ch2 and remove the ch
parameter.  That would be OK for me.

> 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'm in general afraid of doing changes here (apart from bugfixes)
because we don't really have a good way to test them AFAIK, maybe you
do have some XTF or similar tests to exercise those paths?

> 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
> 
> --- 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_load_time[channel]
> +        : pit->count_stop_time[channel];
> +    d = muldiv64(d - pit->stopped_time[channel],
>                   PIT_FREQ, SYSTEM_TIME_HZ);
>  
>      switch ( c->mode )
> @@ -109,6 +112,7 @@ static void pit_load_count(PITState *pit
>          pit->count_load_time[channel] = 0;
>      else
>          pit->count_load_time[channel] = get_guest_time(v);
> +    pit->stopped_time[channel] = 0;

Don't you need to also set count_stop_time == count_load_time in case
the counter is disabled? (s->gate == 0).

>      s->count = val;
>      period = DIV_ROUND(val * SYSTEM_TIME_HZ, PIT_FREQ);
>  
> @@ -147,7 +151,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_load_time[channel]
> +        : pit->count_stop_time[channel];
> +    d = muldiv64(d - pit->stopped_time[channel],
>                   PIT_FREQ, SYSTEM_TIME_HZ);
>  
>      switch ( s->mode )
> @@ -181,22 +188,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 )

Shouldn't this be an else if?

Thanks, Roger.

Re: [PATCH 1/2] x86/vPIT: re-order functions

[Roger Pau Monné]

On Thu, Jun 01, 2023 at 11:56:12AM +0200, Jan Beulich wrote:
> On 01.06.2023 11:17, Roger Pau Monné wrote:
> > On Tue, May 30, 2023 at 05:30:02PM +0200, Jan Beulich wrote:
> >> To avoid the need for a forward declaration of pit_load_count() in a
> >> subsequent change, move it earlier in the file (along with its helper
> >> callback).
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> > Just a couple of nits, which you might also noticed but decided to not
> > fix given this is just code movement.
> 
> Indeed, I meant this to be pure code movement. Nevertheless I'd be happy
> to take care of style issues, if that's deemed okay in a "pure code
> movement" patch. However, ...

It's just small style issues, so it would be OK for me.

> >> --- a/xen/arch/x86/emul-i8254.c
> >> +++ b/xen/arch/x86/emul-i8254.c
> >> @@ -87,6 +87,57 @@ static int pit_get_count(PITState *pit,
> >>      return counter;
> >>  }
> >>  
> >> +static void cf_check pit_time_fired(struct vcpu *v, void *priv)
> > 
> > Seems like v could be constified?
> 
> ... the function being used as a callback, I doubt adding const would
> be possible. Otoh ...

Oh, I see.

> >> +{
> >> +    uint64_t *count_load_time = priv;
> 
> ... there's a blank line missing here, if I was to go for style
> adjustments.

Sure.

Thanks, Roger.

Re: [PATCH 1/2] x86/vPIT: re-order functions

[Jan Beulich]

On 01.06.2023 13:50, Roger Pau Monné wrote:
> On Thu, Jun 01, 2023 at 11:56:12AM +0200, Jan Beulich wrote:
>> On 01.06.2023 11:17, Roger Pau Monné wrote:
>>> On Tue, May 30, 2023 at 05:30:02PM +0200, Jan Beulich wrote:
>>>> To avoid the need for a forward declaration of pit_load_count() in a
>>>> subsequent change, move it earlier in the file (along with its helper
>>>> callback).
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Thanks.
>>
>>> Just a couple of nits, which you might also noticed but decided to not
>>> fix given this is just code movement.
>>
>> Indeed, I meant this to be pure code movement. Nevertheless I'd be happy
>> to take care of style issues, if that's deemed okay in a "pure code
>> movement" patch. However, ...
> 
> It's just small style issues, so it would be OK for me.

So I've done the obvious ones. There's a further signed/unsigned issue
which isn't quite as clear whether to take care of "on the fly": The
function's 2nd and 3rd parameters both ought to be unsigned, yet
throughout the full file the same issue exists many more times. So I
guess I'll leave those untouched for now.

Jan

Re: [PATCH 2/2] x86/vPIT: account for "counter stopped" time

[Jan Beulich]

On 01.06.2023 13:48, Roger Pau Monné wrote:
> On Tue, May 30, 2023 at 05:30:40PM +0200, Jan Beulich wrote:
>> 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.
> 
> Hm, I guess we could rename to pit_set_gate_ch2 and remove the ch
> parameter.  That would be OK for me.

Well, simplifying the function is the less ugly part, so I'd be okay
doing that. But doing _just_ that feels wrong: Why would we make the
function less general when we still maintain all the data also for
the other channels, just that we don't update it. My concern was
really towards the further special casing of channel 2 that would be
required if I didn't introduce two new arrays, but just two new
fields.

>> 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'm in general afraid of doing changes here (apart from bugfixes)
> because we don't really have a good way to test them AFAIK,

Right, hence why I'm asking.

> maybe you
> do have some XTF or similar tests to exercise those paths?

I did consider making something, but I can't go the route of "try it
directly and then compare with emulation results". Yet without that
I'm not sure such a test (and the time spent putting it together) are
worth it, the more that without being able to compare I might also
end up testing some wrong behavior, simply because of not properly
understanding the somewhat scarce documentation that's available.
(I already had to resort to 30 years old hardcopy documentation to
at least stand a chance of getting things right.)

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

Because of this ...

>> @@ -109,6 +112,7 @@ static void pit_load_count(PITState *pit
>>          pit->count_load_time[channel] = 0;
>>      else
>>          pit->count_load_time[channel] = get_guest_time(v);
>> +    pit->stopped_time[channel] = 0;
> 
> Don't you need to also set count_stop_time == count_load_time in case
> the counter is disabled? (s->gate == 0).

... I think you're right, and I should do so unconditionally. In
principle I think this would need to be mode dependent.

>> @@ -181,22 +188,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 )
> 
> Shouldn't this be an else if?

They could, but they don't need to be. I ended up thinking that with the
blank line between both if()s things read slightly better. If you're
pretty convinced that's unhelpful, I'd be willing to adjust.

Jan

Re: [PATCH 1/2] x86/vPIT: re-order functions

[Roger Pau Monné]

On Thu, Jun 01, 2023 at 04:06:53PM +0200, Jan Beulich wrote:
> On 01.06.2023 13:50, Roger Pau Monné wrote:
> > On Thu, Jun 01, 2023 at 11:56:12AM +0200, Jan Beulich wrote:
> >> On 01.06.2023 11:17, Roger Pau Monné wrote:
> >>> On Tue, May 30, 2023 at 05:30:02PM +0200, Jan Beulich wrote:
> >>>> To avoid the need for a forward declaration of pit_load_count() in a
> >>>> subsequent change, move it earlier in the file (along with its helper
> >>>> callback).
> >>>>
> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>
> >>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Thanks.
> >>
> >>> Just a couple of nits, which you might also noticed but decided to not
> >>> fix given this is just code movement.
> >>
> >> Indeed, I meant this to be pure code movement. Nevertheless I'd be happy
> >> to take care of style issues, if that's deemed okay in a "pure code
> >> movement" patch. However, ...
> > 
> > It's just small style issues, so it would be OK for me.
> 
> So I've done the obvious ones. There's a further signed/unsigned issue
> which isn't quite as clear whether to take care of "on the fly": The
> function's 2nd and 3rd parameters both ought to be unsigned, yet
> throughout the full file the same issue exists many more times. So I
> guess I'll leave those untouched for now.

No strong opinion regarding those, I'm fine if you want to adjust also
to unsigned.

Thanks, Roger.

Re: [PATCH 2/2] x86/vPIT: account for "counter stopped" time

[Roger Pau Monné]

On Thu, Jun 01, 2023 at 04:20:13PM +0200, Jan Beulich wrote:
> On 01.06.2023 13:48, Roger Pau Monné wrote:
> > On Tue, May 30, 2023 at 05:30:40PM +0200, Jan Beulich wrote:
> >> 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.
> > 
> > Hm, I guess we could rename to pit_set_gate_ch2 and remove the ch
> > parameter.  That would be OK for me.
> 
> Well, simplifying the function is the less ugly part, so I'd be okay
> doing that. But doing _just_ that feels wrong: Why would we make the
> function less general when we still maintain all the data also for
> the other channels, just that we don't update it. My concern was
> really towards the further special casing of channel 2 that would be
> required if I didn't introduce two new arrays, but just two new
> fields.
> 
> >> 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'm in general afraid of doing changes here (apart from bugfixes)
> > because we don't really have a good way to test them AFAIK,
> 
> Right, hence why I'm asking.

I would say leave alone, unless you have a reason to fix them.

> > maybe you
> > do have some XTF or similar tests to exercise those paths?
> 
> I did consider making something, but I can't go the route of "try it
> directly and then compare with emulation results". Yet without that
> I'm not sure such a test (and the time spent putting it together) are
> worth it, the more that without being able to compare I might also
> end up testing some wrong behavior, simply because of not properly
> understanding the somewhat scarce documentation that's available.

Well, I would argue that just testing the device works as (you)
expected would already be an improvement, we could at least rule out
errors due to the device misbehaving related to our expectations.

> (I already had to resort to 30 years old hardcopy documentation to
> at least stand a chance of getting things right.)

I'm not sure I would be able to realize this errors with the
documentation I currently have.

> 
> >> 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).
> 
> Because of this ...
> 
> >> @@ -109,6 +112,7 @@ static void pit_load_count(PITState *pit
> >>          pit->count_load_time[channel] = 0;
> >>      else
> >>          pit->count_load_time[channel] = get_guest_time(v);
> >> +    pit->stopped_time[channel] = 0;
> > 
> > Don't you need to also set count_stop_time == count_load_time in case
> > the counter is disabled? (s->gate == 0).
> 
> ... I think you're right, and I should do so unconditionally. In
> principle I think this would need to be mode dependent.
> 
> >> @@ -181,22 +188,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 )
> > 
> > Shouldn't this be an else if?
> 
> They could, but they don't need to be. I ended up thinking that with the
> blank line between both if()s things read slightly better. If you're
> pretty convinced that's unhelpful, I'd be willing to adjust.

Just wanted to make sure my understanding was correct.  IMO it's clearer
if an else is used, to denote instances using the first branch won't
change s->gate or val and thus the second (else) branch is not taken
(iow: both branches are exclusive, and the code in the first branch
cannot affect the condition of entering the second if).

But maybe that's just my way of thinking, so I'm not going to insist.

Thanks, Roger.