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

["Roger Pau Monné" <roger.pau@citrix.com>]

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.