Re: [Qemu-devel] [PATCH KVM v2 2/4] KVM: additional i8254 output fixes

[Matthew Ogilvie]

On Tue, Jan 08, 2013 at 09:41:36AM +0200, Gleb Natapov wrote:
> On Mon, Jan 07, 2013 at 06:35:47PM -0600, address@hidden wrote:
> > On Mon, 7 Jan 2013 14:04:03 +0200, Gleb Natapov <address@hidden> wrote:
> > > On Wed, Dec 26, 2012 at 10:39:54PM -0700, Matthew Ogilvie wrote:
> > >> Make git_get_out() consistent with spec.  Currently pit_get_out()
> > >> doesn't affect IRQ0, but it can be read by the guest in other ways.
> > >> This makes it consistent with proposed changes in qemu's i8254 model
> > >> as well.
> > >> 
> > >> See http://bochs.sourceforge.net/techspec/intel-82c54-timer.pdf.gz
> > >> or search the net for 23124406.pdf.
> > >> 
> > >> Signed-off-by: Matthew Ogilvie <address@hidden>
> > >> ---
> > >>  arch/x86/kvm/i8254.c | 44 ++++++++++++++++++++++++++++++++++----------
> > >>  1 file changed, 34 insertions(+), 10 deletions(-)
> > >> 
> > >> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> > >> index cd4ec60..fd38938 100644
> > >> --- a/arch/x86/kvm/i8254.c
> > >> +++ b/arch/x86/kvm/i8254.c
> > >> @@ -144,6 +144,10 @@ static int pit_get_count(struct kvm *kvm, int
> > >> channel)
> > >>  
> > >>          WARN_ON(!mutex_is_locked(&kvm->arch.vpit->pit_state.lock));
> > >>  
> > >> +        /* FIXME: Add some way to represent a paused timer and return
> > >> +         *   the paused-at counter value, to better model gate pausing,
> > >> +         *   "wait until next CLK pulse to load counter" logic, etc.
> > >> +         */
> > >>          t = kpit_elapsed(kvm, c, channel);
> > >>          d = muldiv64(t, KVM_PIT_FREQ, NSEC_PER_SEC);
> > >>  
> > >> @@ -155,8 +159,7 @@ static int pit_get_count(struct kvm *kvm, int
> > >> channel)
> > >>                  counter = (c->count - d) & 0xffff;
> > >>                  break;
> > >>          case 3:
> > >> -                /* XXX: may be incorrect for odd counts */
> > >> -                counter = c->count - (mod_64((2 * d), c->count));
> > >> +                counter = (c->count - (mod_64((2 * d), c->count))) & 
> > >> 0xfffe;
> > >>                  break;
> > >>          default:
> > >>                  counter = c->count - mod_64(d, c->count);
> > >> @@ -180,20 +183,18 @@ static int pit_get_out(struct kvm *kvm, int
> > >> channel)
> > >>          switch (c->mode) {
> > >>          default:
> > >>          case 0:
> > >> -                out = (d >= c->count);
> > >> -                break;
> > >>          case 1:
> > >> -                out = (d < c->count);
> > >> +                out = (d >= c->count);
> > >>                  break;
> > >>          case 2:
> > >> -                out = ((mod_64(d, c->count) == 0) && (d != 0));
> > >> +                out = (mod_64(d, c->count) != (c->count - 1) || c->gate 
> > >> == 0);
> > >>                  break;
> > >>          case 3:
> > >> -                out = (mod_64(d, c->count) < ((c->count + 1) >> 1));
> > >> +                out = (mod_64(d, c->count) < ((c->count + 1) >> 1) || 
> > >> c->gate == 0);
> > >>                  break;
> > >>          case 4:
> > >>          case 5:
> > >> -                out = (d == c->count);
> > >> +                out = (d != c->count);
> > >>                  break;
> > >>          }
> > >>  
> > >> @@ -367,7 +368,7 @@ static void pit_load_count(struct kvm *kvm, int
> > >> channel, u32 val)
> > >>  
> > >>          /*
> > >>           * The largest possible initial count is 0; this is equivalent
> > >> -         * to 216 for binary counting and 104 for BCD counting.
> > >> +         * to pow(2,16) for binary counting and pow(10,4) for BCD 
> > >> counting.
> > >>           */
> > >>          if (val == 0)
> > >>                  val = 0x10000;
> > >> @@ -376,6 +377,26 @@ static void pit_load_count(struct kvm *kvm, int
> > >> channel, u32 val)
> > >>  
> > >>          if (channel != 0) {
> > >>                  ps->channels[channel].count_load_time = ktime_get();
> > >> +
> > >> +                /* In gate-triggered one-shot modes,
> > >> +                 * indirectly model some pit_get_out()
> > >> +                 * cases by setting the load time way
> > >> +                 * back until gate-triggered.
> > >> +                 * (Generally only affects reading status
> > >> +                 * from channel 2 speaker,
> > >> +                 * due to hard-wired gates on other
> > >> +                 * channels.)
> > >> +                 *
> > >> +                 * FIXME: This might be redesigned if a paused
> > >> +                 * timer state is added for pit_get_count().
> > >> +                 */
> > >> +                if (ps->channels[channel].mode == 1 ||
> > >> +                    ps->channels[channel].mode == 5) {
> > >> +                    u64 delta = muldiv64(val+2, NSEC_PER_SEC, 
> > >> KVM_PIT_FREQ);
> > >> +                    ps->channels[channel].count_load_time =
> > >> +                           
> > >> ktime_sub(ps->channels[channel].count_load_time,
> > >> +                                      ns_to_ktime(delta));
> > > I do not understand what are you trying to do here. You assume that
> > > trigger will happen 2 clocks after counter is loaded?
> > > 
> > 
> > Modes 1 and 5 are single-shot, and they do not start counting until GATE
> > is triggered, potentially well after count is loaded.  So this is
> > attempting to model the "start of countdown has not been triggered"
> > state as being mostly identical to the "already triggered and also
> > expired some number of clocks (2) ago" state.
> > 
> So this is still not accurate. This assumes that guest loads counter and
> then immediately triggers the gate. If between loading the counter and
> triggering the gate guest does something else for a long time the result
> will still not be accurate.
> 
> > It might be clearer to have a way to explicitly model a
> > paused countdown, but such a mechanism doesn't currently exist.
> If it worth doing it worth doing right. Should not be hard. Like setting
> channels[channel].count_load_time on trigger instead of during count
> loading.

Note the pic_set_gate() function already has logic to set
count_load_time when gate is triggered in modes 1 and 5
(as well as some others).  But until it is triggered, you want
to set the OUT line high (and not risk any change until
it is triggered) as indicated in the timing diagrams, and
setting the load time sufficiently far back should do
that.

On the other hand, setting the load time
back does not fix reading back the counter, but
that can't be fixed completely without some kind of paused
or "not triggered" state.  Especially because in some modes,
lowering gate simply pauses the counter wherever it is at
the time.  Even though a "not triggered" state might be flagged
with special values in existing a variables, a partially-counted
state would probably need an additional variable in the structure
to store the count as of when it was paused (or equivalent)...

Also, in some modes (like 2), a low GATE should immediatly force
OUT high (in addition to pausing the counter), but neither the
existing code nor any of my patches tries to model this aspect
of operation.

> 
> > 
> > Note that modeling modes 1 and 5 is fairly low priority,
> > because channel 0's GATE line is generally hard-wired such that
> > GATE edges/triggers are impossible.  But it may still be
> > somewhat relevant to the PC speaker channel, or if someone
> > might want to use this in a model of non-PC hardware.
> > 
> And that is why I thing it is not worth doing :) In kernel pit is not
> suitable for non-PC hardware modeling either.
> 
> > >> +                }
> > >>                  return;
> > >>          }
> > >>  
> > >> @@ -383,7 +404,6 @@ static void pit_load_count(struct kvm *kvm, int
> > >> channel, u32 val)
> > >>           * mode 1 is one shot, mode 2 is period, otherwise del timer */
> > >>          switch (ps->channels[0].mode) {
> > >>          case 0:
> > >> -        case 1:
> > >>          /* FIXME: enhance mode 4 precision */
> > >>          case 4:
> > >>                  create_pit_timer(kvm, val, 0);
> > >> @@ -393,6 +413,10 @@ static void pit_load_count(struct kvm *kvm, int
> > >> channel, u32 val)
> > >>                  create_pit_timer(kvm, val, 1);
> > >>                  break;
> > >>          default:
> > >> +                /* Modes 1 and 5 are triggered by gate leading edge,
> > >> +                 * but channel 0's gate is hard-wired high and has
> > >> +                 * no edges (on normal real hardware).
> > >> +                 */
> > >>                  destroy_pit_timer(kvm->arch.vpit);
> > >>          }
> > >>  }
> > >> -- 
> > >> 1.7.10.2.484.gcd07cc5
> > > 
> > > --
> > >                   Gleb.
> 
> --
>                       Gleb.