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

[Gleb Natapov]

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 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.