| Message ID | 355c5379-ea9e-582c-0131-816204eb3ace@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/vPIT: account for "counter stopped" time | expand |
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.
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
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.
--- 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; 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 ) + switch ( s->mode ) + { + default: + case 0: + case 4: + /* Enable counting. */ + pit->stopped_time[channel] += get_guest_time(v) - + pit->count_stop_time[channel]; + break; + + case 1: + case 5: + case 2: + case 3: + /* Initiate counting on rising edge. */ + pit_load_count(pit, channel, pit->hw.channels[channel].count); + break; + } s->gate = val; } --- a/xen/arch/x86/include/asm/hvm/vpt.h +++ b/xen/arch/x86/include/asm/hvm/vpt.h @@ -48,8 +48,14 @@ struct periodic_time { typedef struct PITState { /* Hardware state */ struct hvm_hw_pit hw; + /* Last time the counters read zero, for calcuating counter reads */ int64_t count_load_time[3]; + /* Last time the counters were stopped, for calcuating counter reads */ + int64_t count_stop_time[3]; + /* Accumulate "stopped" time, since the last counter write/reload. */ + uint64_t stopped_time[3]; + /* Channel 0 IRQ handling. */ struct periodic_time pt0; spinlock_t lock;
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. 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.) 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