Re: [RFC][PATCH 1/2] KVM: In-kernel PIT model

Yang, Sheng
2008-01-23T06:00:35Z

On Tuesday 22 January 2008 21:54:10 Avi Kivity wrote:
> Yang, Sheng wrote:
> > +
> > +/* Compute with 96 bit intermediate result: (a*b)/c */
> > +static u64 muldiv64(u64 a, u32 b, u32 c)
>
> Why do we need such high accuracy for the pit?

The direct reason is we using TSC as the reference of pit count_load_time. I
think the only alternative is using host ACPI PM timer. But it will wrapped
soon...

>
> > +
> > +static int pit_get_out(struct kvm *kvm, int channel)
> > +{
> > + struct PITChannelState *c =
> > + &kvm->arch.vpit->pit_state.channels[channel];
> > + struct kvm_vcpu *vcpu = kvm->vcpus[0];
> > + u64 d, t;
> > + int out;
> > +
> > + ASSERT(mutex_is_locked(&kvm->arch.vpit->pit_state.lock));
> > +
> > + kvm_get_msr(vcpu, MSR_IA32_TIME_STAMP_COUNTER, &t);
> > + d = muldiv64(t - c->count_load_time, PIT_FREQ, cpu_khz * 1000);
>
> I assume this is to correlate the tsc and the pit counters. Doesn't
> this break with cpu frequency scaling or with vcpu migrations?

I think the cpu frequency scaling will affect TSC, though cpu_khz would be
changed along with it.

>
> What if this is called within the context of vcpu 1? Not sure
> kvm_get_msr() will work correctly at all.

I don't think so. kvm_get_msr() with TSC only read guest tsc, which is
indentity for the domain, not specific for vcpu. And I indicated vcpu0 as
vcpu(though won't be refered). But I may do it more clear in the future.

>
> > +struct PITTimer {
> >
> > +
> > +struct PITChannelState {
> > +
> > +struct PITState {
>
> Please use the traditional naming convention (pit_timer,
> pit_channel_state, pit_state)

Sure.

--
Thanks
Yang, Sheng