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

[Jan Beulich <jbeulich@xxxxxxxx>]

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