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

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

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.