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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

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