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

[Avi Kivity <avi@redhat.com>]

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?

KVM_CAP_TSC_CONTROL depends on both kvm and the underlying cpu.  I guess 
it's okay.

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).

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

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

> +
> +		r = 0;
> +		goto out;
> +	}
>

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