Re: [Qemu-devel] [PATCH] KVM: emulate lapic tsc deadline timer for hvm

[Avi Kivity <avi@redhat.com>]

On 09/06/2011 02:18 PM, Liu, Jinsong wrote:
> >>    			struct x86_instruction_info *info,
> >>    			enum x86_intercept_stage stage);
> >>  +	u64 (*guest_to_host_tsc)(u64 guest_tsc);
> >>    };
> >
> >  Please put this near the other tsc functions.  Add a comment
> >  explaining what value is returned under nested virtualization.
>
> OK
>
> >
> >  Please add the svm callback implementation.
> >
>
> It's un-necessary to add svm callback.
> AMD lapic timer has no tsc deadline mode, svm callback would be an unused dummy function.

It's a generic function, at the moment it's only used by tsc deadline 
timer but it could be used tomorrow for something else.

> >>  +		u64 tsc_now, tsc_target, tsc_delta, nsec;
> >>  +
> >>  +		if (!apic->lapic_timer.tscdeadline)
> >>  +			return;
> >>  +
> >>  +		tsc_target = kvm_x86_ops->
> >>  +			guest_to_host_tsc(apic->lapic_timer.tscdeadline);
> >>  +		rdtscll(tsc_now); +		tsc_delta = tsc_target - tsc_now;
> >
> >  This only works if we have a constant tsc, that's not true for large
>
> Agree with Kevin, variable tsc exposes tricky issues to time virtualization in general.
> At some very old processors it maybe variable tsc, but all recent processors work on constant tsc regardless of Px and Cx.
> For lapic tsc deadline timer capacibility cpu, I think it works on constant tsc otherwise os has no way to expect next absolute timepoint.

Well, we need something better here.  It's not just old cpus, it's also 
large multi-board machines.

> >>  +u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu) +{
> >>  +	struct kvm_lapic *apic = vcpu->arch.apic;
> >>  +
> >>  +	if (apic_lvtt_oneshot(apic) || apic_lvtt_period(apic))
> >>  +		return 0;
> >
> >  Why?
> >
>
> Intel SDM define such hardware behavior (10.5.4.1):
> 'In other timer modes (LVT bit 18 = 0), the IA32_TSC_DEADLINE MSR reads zero and writes are ignored.'

Ok.

>
> >
> >>    /*
> >>     * Empty call-back. Needs to be implemented when VMX enables the
> >>  SET_TSC_KHZ
> >>     * ioctl. In this case the call-back should update internal vmx
> >>  state to make @@ -6270,6 +6278,16 @@ static void
> >>    		vmx_cpuid_update(struct kvm_vcpu *vcpu)   			} }
> >>    	}
> >>  +
> >>  +	/*
> >>  +	 * Emulate Intel lapic tsc deadline timer even if host not support
> >>  it. +	 * Open CPUID.1.ECX[24] and use bit17/18 as timer mode mask.
> >>  +	 */ +	best = kvm_find_cpuid_entry(vcpu, 1, 0);
> >>  +	if (best) {
> >>  +		best->ecx |= bit(X86_FEATURE_TSC_DEADLINE_TIMER);
> >>  +		vcpu->arch.apic->lapic_timer.timer_mode_mask = (3<<   17); +	}
> >>    }
> >
> >  Should be in common code; there is nothing vmx specific about it
> >  (although it is Intel specific at present).
>
> AMD lapic timer has no tsc deadline mode and so cpuid.1.ecx[24], and its timer_mode_mask should be 1<<  17 (AMD APM 16.4.1.)
> It would be more safe to handle cpuid and lapic timer_mode_mask in arch code.
>

Again, vmx.c is NOT about Intel specific code.  It is about vmx-specific 
code - programming vmx via VMREAD and VMWRITE.

(not to mention that we don't really need host cpu support for this - we 
can emulate tsc deadline mode without host cpu support at all, as this 
patchset shows).

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