Re: [PATCH 4/4] [Hypercall] Allow modules registeration for hypercalls.

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Dor Laor wrote:
> New kernel modules for KVM are upcoming soon, these module
> will need to use hypercalls. Before calling the hypercall function,
> the kvm_main core module has to make sure it won't get unloaded.
> So hypercall register/unregister are added.
>
> Except that the kernel hypercalls handlers are numberes
> 0-KVM_NR_HYPERCALLS.
> All the userspace handlers are above KVM_NR_HYPERCALLS.
>   

But, some hypercalls can be handled either in userspace or in the
kernel, depending on what modules are loaded.  How is that handled?

>  
> +static struct kvm_hypercall hypercalls[KVM_KERNEL_NR_HYPERCALLS];
> +
> +static int test_hypercall(struct kvm_vcpu *vcpu, unsigned long args[])
> +{
> +	printk(KERN_DEBUG "%s: hypercall invoked\n", __FUNCTION__);
> +
> +	return 0;
> +}
> +
> +static struct kvm_hypercall __hypercall_test = {
> +	(unsigned long (*)(struct kvm_vcpu*, unsigned long
> args[]))test_hypercall,
> +	THIS_MODULE,
> +	__NR_hypercall_test,
> +};
>   

I think we can live without the test hypercall in production code.

> +
> +static atomic_t dev_kvm_open_count = ATOMIC_INIT(0);
> +
>  #define MAX_IO_MSRS 256
>  
>  #define CR0_RESERVED_BITS
> \
> @@ -302,6 +320,24 @@ static struct kvm *kvm_create_vm(void)
>  	return kvm;
>  }
>  
> +static int kvm_dev_open(struct inode *inode, struct file *filp)
> +{
> +	int ret = 0;
> +
> +	if (atomic_inc_return(&dev_kvm_open_count) == 1) {
> +		ret = kvm_register_hypercall(THIS_MODULE,
> &__hypercall_test);
>   

Instead of this hack, hypercalls in kvm.ko can have the .module field
zeroed, so there's no need to play with our own module count.  The
hypercalls can be initialized in their array directly.

> +
> +	spin_lock(&kvm_lock);
> +
> +	if (hypercalls[hypercall->idx].hypercall) {
> +		printk(KERN_DEBUG "%s:hypercall idx(%d) already
> taken\n",
> +			__FUNCTION__, hypercall->idx);
> +		r = -EEXIST;
> +		goto out;
> +	}
> +
> +	if (try_module_get(module) < 0) {
> +		printk(KERN_DEBUG "%s: module reference count++
> failed\n",
> +			__FUNCTION__);
> +		r = -EINVAL;
> +		goto out;
> +	}
> +
> +	hypercalls[hypercall->idx].hypercall = hypercall->hypercall;
>   

"smp_wmb();" here.

> +	hypercalls[hypercall->idx].module = module;
> +
> +out:
> +	spin_unlock(&kvm_lock);
> +
> +	return r;
> +}
> +EXPORT_SYMBOL_GPL(kvm_register_hypercall);
> +
>   


> +
> +/*
> + * Generic hypercall dispatcher routine.
> + * Returns 0 for user space handling, 1 on success handling
> + */
> + int kvm_hypercall(struct kvm_vcpu *vcpu, struct kvm_run *run)
> + {
> +	if (try_module_get(hypercalls[nr].module) < 0) {
> +		printk(KERN_DEBUG "%s: module reference count++
> failed\n",
> +			__FUNCTION__);
> +		ret = -EINVAL;
>   

EOPNOTSUPP or something.  EINVAL is overused.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/