Re: [PATCH 6/6] KVM: X86: Implement userspace interface to set virtual_tsc_khz

[Joerg Roedel <joro@8bytes.org>]

On Thu, Mar 24, 2011 at 12:14:36PM +0200, Avi Kivity wrote:
> On 03/24/2011 09:40 AM, Joerg Roedel wrote:
>> This patch implements two new vm-ioctls to get and set the
>> virtual_tsc_khz if the machine supports tsc-scaling. Setting
>> the tsc-frequency is only possible before userspace creates
>> any vcpu.
>>
>>
>> +4.54 KVM_SET_TSC_KHZ
>> +
>> +Capability: KVM_CAP_TSC_CONTROL
>> +Architectures: x86
>> +Type: vm ioctl
>> +Parameters: __u32 (in)
>> +Returns: 0 on success, -1 on error
>> +
>> +Specifies the tsc frequency for the virtual machine. This IOCTL must be
>> +used before any vcpu is created. The unit of the frequency is KHz.
>
> Should it not be a vcpu ioctl?

The idea was that a vm ioctl will make sure that all vcpus in one vm
have the same tsc frequency. With a vm ioctl we force this. So the tsc
fequency is basically a vm capability which is mirrored into each vcpu
data structure for performance reasons.

> Need to return an error if the frequency cannot be accommodated.  In  
> theory we need to provide the range of supported frequencies, but we can  
> live without it (and it will be very difficult to provide if we take  
> accuracy into account).

Yes, -EINVAL is reported if the frequency can not be accomodated. But
userspace can't find out the lower or upper bounds of the valid range.
>
>> +
>> +4.55 KVM_GET_TSC_KHZ
>> +
>> +Capability: KVM_CAP_GET_TSC_KHZ
>> +Architectures: x86
>> +Type: vm ioctl
>> +Parameters: __u32 (out)
>> +Returns: 0 on success, -1 on error
>> +
>> +Returns the tsc frequency of the guest. The unit of the return value is
>> +KHz. If the host has unstable tsc this ioctl return an error.
>
> Which error?
>
> (it's important since this is an expected error, unlike most others)
>
>> @@ -3580,6 +3591,52 @@ long kvm_arch_vm_ioctl(struct file *filp,
>>   		r = 0;
>>   		break;
>>   	}
>> +	case KVM_SET_TSC_KHZ: {
>> +		u32 user_tsc_khz;
>> +
>> +		if (!kvm_has_tsc_control)
>> +			break;
>> +
>> +		r = -EFAULT;
>> +		if (copy_from_user(&user_tsc_khz, argp, sizeof(__u32)))
>> +			goto out;
>
> Can just use the input value (arg) instead of copy_from_user().
>
>> +
>> +		r = -EINVAL;
>> +		if (user_tsc_khz<  kvm_min_guest_tsc_khz ||
>> +		    user_tsc_khz>  kvm_max_guest_tsc_khz)
>
> <= and >= are probably safer.

Right, I'll change it.

>
>> +			goto out;
>> +
>> +		mutex_lock(&kvm->lock);
>> +		/*
>> +		 * We force the tsc frequency to be set before any
>> +		 * vcpu is created
>> +		 */
>> +		if (atomic_read(&kvm->online_vcpus)>  0) {
>> +			mutex_unlock(&kvm->lock);
>> +			goto out;
>> +		}
>> +
>> +		kvm_arch_set_tsc_khz(kvm, user_tsc_khz);
>> +
>> +		mutex_unlock(&kvm->lock);
>
> Making these vcpu ioctls will remove the locking.
>
>> +
>> +		r = 0;
>> +		goto out;
>> +	}
>> +	case KVM_GET_TSC_KHZ: {
>> +		u32 vtsc_khz = kvm->arch.virtual_tsc_khz;
>> +
>> +		r = -EIO;
>> +		if (check_tsc_unstable())
>> +			goto out;
>> +
>> +		r = -EFAULT;
>> +		if (copy_to_user(argp,&vtsc_khz, sizeof(__u32)))
>> +			goto out;
>
> And an ordinary return here.

Okay, I'll change that. But I would prefer to keep this as a vm ioctl. A
vcpu ioctl might be more flexible but I doubt anybody has a use-case for
different tsc_khz values in one VM.

	Joerg