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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxx>

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

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.