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

[mmogilvi]

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.

It might be clearer to have a way to explicitly model a
paused countdown, but such a mechanism doesn't currently exist.

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.

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