Re: [PATCH] tsc: use kvmclock for calibration

[Avi Kivity <avi@redhat.com>]

On 08/09/2012 05:01 PM, Avi Kivity wrote:
> On 08/09/2012 04:57 PM, Gerd Hoffmann wrote:
>>   Hi,
>> 
>>>> +u64 kvm_tsc_khz(void)
>>>> +{
>>>> +    u32 eax, ebx, ecx, edx, msr;
>>>> +    struct pvclock_vcpu_time_info time;
>>>> +    u32 addr = (u32)(&time);
>>>> +    u64 khz;
>>>> +
>>>> +    /* check presence and figure msr number */
>>>> +    cpuid(KVM_CPUID_FEATURES, &eax, &ebx, &ecx, &edx);
>>>> +    if (eax & KVM_FEATURE_CLOCKSOURCE2) {
>>>> +        msr = MSR_KVM_SYSTEM_TIME_NEW;
>>>> +    } else if (eax & KVM_FEATURE_CLOCKSOURCE) {
>>>> +        msr = MSR_KVM_SYSTEM_TIME;
>>>> +    } else {
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    /* ask kvm hypervisor to fill struct */
>>>> +    memset(&time, 0, sizeof(time));
>>>> +    wrmsr(msr, addr | 1);
>>> 
>>> How can this work?
>> 
>> It did in my testing, although maybe by pure luck ...
>> 
>>> There is a 64-byte alignment requirement.
>> 
>> 64 bytes?  Sure?  The whole struct is only 32 bytes in size ...
> 
> er, the documentation says 4 bytes (so stack alignment works).  I
> distinctly remember having a large alignment requirement so we don't
> cross a page or slot boundary... something's wrong here.

	case MSR_KVM_SYSTEM_TIME: {
		kvmclock_reset(vcpu);

		vcpu->arch.time = data;
		kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);

		/* we verify if the enable bit is set... */
		if (!(data & 1))
			break;

		/* ...but clean it before doing the actual write */
		vcpu->arch.time_offset = data & ~(PAGE_MASK | 1);

		vcpu->arch.time_page =
				gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT);

		if (is_error_page(vcpu->arch.time_page))
			vcpu->arch.time_page = NULL;

		break;

So your tests worked by pure luck, but the bug is in kvm.  We need to
grab two pages here.


-- 
error compiling committee.c: too many arguments to function