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

[Anthony Liguori <anthony-rdkfGonbjUSkNkDKm+mE6A@public.gmane.org>]

I don't really like the idea of having other modules register
hypercalls.  The hypercall address space is fixed and should be
maintained by KVM.

I think things like virtio are rare and important enough, that it
warrants having kvm.ko know enough about them to provide a higher level
registration mechanism.  The virtio network driver should register the
fact that it's a virtio device, not which hypercalls it cares about.

I'm not sure you gain anything by doing the virtio infrastructure in a
module either.  If anything, a config option in kvm.ko would suffice.

Regards,

Anthony Liguori


On Fri, 2007-08-24 at 16:59 -0700, 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.
> 
> Signed-off-by: Dor Laor <dor.laor-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/kvm/kvm.h        |    9 ++
>  drivers/kvm/kvm_main.c   |  223
> ++++++++++++++++++++++++++++++++++++++--------
>  include/linux/kvm_para.h |    4 +-
>  3 files changed, 197 insertions(+), 39 deletions(-)
>  mode change 100644 => 100755 drivers/kvm/kvm_main.c
> 
> diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
> index a42a6f3..37240cf 100644
> --- a/drivers/kvm/kvm.h
> +++ b/drivers/kvm/kvm.h
> @@ -472,6 +472,12 @@ struct kvm_arch_ops {
>  				unsigned char *hypercall_addr);
>  };
>  
> +struct kvm_hypercall {
> +	unsigned long (*hypercall)(struct kvm_vcpu*, unsigned long
> args[]);
> +	struct module *module;
> +	int idx;
> +};
> +
>  extern struct kvm_arch_ops *kvm_arch_ops;
>  
>  /* The guest did something we don't support. */
> @@ -588,6 +594,9 @@ int kvm_mmu_load(struct kvm_vcpu *vcpu);
>  void kvm_mmu_unload(struct kvm_vcpu *vcpu);
>  
>  int kvm_hypercall(struct kvm_vcpu *vcpu, struct kvm_run *run);
> +int kvm_register_hypercall(struct module *module, struct kvm_hypercall
> *hypercall);
> +int kvm_unregister_hypercall(struct kvm_hypercall *hypercall);
> +
>  
>  static inline int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t gva,
>  				     u32 error_code)
> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
> old mode 100644
> new mode 100755
> index abd7498..051b47a
> --- a/drivers/kvm/kvm_main.c
> +++ b/drivers/kvm/kvm_main.c
> @@ -20,6 +20,7 @@
>  #include "segment_descriptor.h"
>  
>  #include <linux/kvm.h>
> +#include <linux/kvm_para.h>
>  #include <linux/module.h>
>  #include <linux/errno.h>
>  #include <linux/percpu.h>
> @@ -84,6 +85,23 @@ static struct kvm_stats_debugfs_item {
>  
>  static struct dentry *debugfs_dir;
>  
> +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,
> +};
> +
> +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);
> +		if (ret < 0) {
> +			printk(KERN_DEBUG "%s: kvm_register_hypercall
> error, "
> +			       "hypercall: %s\n",
> +			       __FUNCTION__, "test");
> +			goto out_test;
> +		}
> +	}
> +
> +out_test:
> +	return ret;
> +}
> +
>  /*
>   * Free any memory in @free but not in @dont.
>   */
> @@ -371,6 +407,20 @@ static void kvm_free_vcpus(struct kvm *kvm)
>  
>  }
>  
> +static int kvm_dev_release(struct inode *inode, struct file *filp)
> +{
> +	int ret = 0;
> +
> +	atomic_dec(&dev_kvm_open_count);
> +	ret = kvm_unregister_hypercall(&__hypercall_test);
> +	if (ret < 0) {
> +		printk(KERN_DEBUG "%s:kvm_unregister_hypercall error "
> +		       "hypercall: %s\n", __FUNCTION__,
> "hypercall_test");
> +	}
> +
> +	return ret;
> +}
> +
>  static void kvm_destroy_vm(struct kvm *kvm)
>  {
>  	spin_lock(&kvm_lock);
> @@ -1267,50 +1317,145 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_emulate_halt);
>  
> -int kvm_hypercall(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +int kvm_register_hypercall(struct module* module, struct kvm_hypercall
> *hypercall)
>  {
> -	unsigned long nr, a0, a1, a2, a3, a4, a5, ret;
> +	int r = 0;
>  
> -	kvm_arch_ops->cache_regs(vcpu);
> -	ret = -KVM_EINVAL;
> -#ifdef CONFIG_X86_64
> -	if (is_long_mode(vcpu)) {
> -		nr = vcpu->regs[VCPU_REGS_RAX];
> -		a0 = vcpu->regs[VCPU_REGS_RDI];
> -		a1 = vcpu->regs[VCPU_REGS_RSI];
> -		a2 = vcpu->regs[VCPU_REGS_RDX];
> -		a3 = vcpu->regs[VCPU_REGS_RCX];
> -		a4 = vcpu->regs[VCPU_REGS_R8];
> -		a5 = vcpu->regs[VCPU_REGS_R9];
> -	} else
> -#endif
> -	{
> +	if (hypercall->idx >= KVM_KERNEL_NR_HYPERCALLS ||
> +	    hypercall->idx < 0) {
> +		printk(KERN_DEBUG "%s:hypercall registration idx(%d)\n",
> +			 __FUNCTION__, hypercall->idx);
> +		return -EINVAL;
> +	}
> +
> +	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;
> +	hypercalls[hypercall->idx].module = module;
> +
> +out:
> +	spin_unlock(&kvm_lock);
> +
> +	return r;
> +}
> +EXPORT_SYMBOL_GPL(kvm_register_hypercall);
> +
> +int kvm_unregister_hypercall(struct kvm_hypercall *hypercall)
> +{
> +	if (hypercall->idx >= KVM_KERNEL_NR_HYPERCALLS ||
> +	    hypercall->idx < 0) {
> +		printk(KERN_DEBUG "%s:hypercall unregistration
> idx(%d)\n",
> +			 __FUNCTION__, hypercall->idx);
> +		return -EINVAL;
> +	}
> +
> +	spin_lock(&kvm_lock);
> +	if (!hypercalls[hypercall->idx].hypercall) {
> +		printk(KERN_DEBUG "%s:hypercall idx(%d) was not
> registered\n",
> +			__FUNCTION__, hypercall->idx);
> +		spin_unlock(&kvm_lock);
> +		return -EEXIST;
> +	}
> +
> +	hypercalls[hypercall->idx].hypercall = 0;
> +	module_put(hypercalls[hypercall->idx].module);
> +	spin_unlock(&kvm_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(kvm_unregister_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)
> + {
> +	unsigned long nr, ret;
> +	unsigned long args[6];
> + 
> + 	kvm_arch_ops->cache_regs(vcpu);
> + 	ret = -KVM_EINVAL;
> + #ifdef CONFIG_X86_64
> + 	if (is_long_mode(vcpu)) {
> + 		nr = vcpu->regs[VCPU_REGS_RAX];
> +		args[0] = vcpu->regs[VCPU_REGS_RDI];
> +		args[1] = vcpu->regs[VCPU_REGS_RSI];
> +		args[2] = vcpu->regs[VCPU_REGS_RDX];
> +		args[3] = vcpu->regs[VCPU_REGS_RCX];
> +		args[4] = vcpu->regs[VCPU_REGS_R8];
> +		args[5] = vcpu->regs[VCPU_REGS_R9];
> + 	} else
> + #endif
> + 	{
>  		nr = vcpu->regs[VCPU_REGS_RBX] & -1u;
> -		a0 = vcpu->regs[VCPU_REGS_RAX] & -1u;
> -		a1 = vcpu->regs[VCPU_REGS_RCX] & -1u;
> -		a2 = vcpu->regs[VCPU_REGS_RDX] & -1u;
> -		a3 = vcpu->regs[VCPU_REGS_RSI] & -1u;
> -		a4 = vcpu->regs[VCPU_REGS_RDI] & -1u;
> -		a5 = vcpu->regs[VCPU_REGS_RBP] & -1u;
> -	}
> -	switch (nr) {
> -	default:
> -		run->hypercall.nr = nr;
> -		run->hypercall.args[0] = a0;
> -		run->hypercall.args[1] = a1;
> -		run->hypercall.args[2] = a2;
> -		run->hypercall.args[3] = a3;
> -		run->hypercall.args[4] = a4;
> -		run->hypercall.args[5] = a5;
> -		run->hypercall.ret = ret;
> -		run->hypercall.longmode = is_long_mode(vcpu);
> -		kvm_arch_ops->decache_regs(vcpu);
> +		args[0] = vcpu->regs[VCPU_REGS_RAX] & -1u;
> +		args[1] = vcpu->regs[VCPU_REGS_RCX] & -1u;
> +		args[2] = vcpu->regs[VCPU_REGS_RDX] & -1u;
> +		args[3] = vcpu->regs[VCPU_REGS_RSI] & -1u;
> +		args[4] = vcpu->regs[VCPU_REGS_RDI] & -1u;
> +		args[5] = vcpu->regs[VCPU_REGS_RBP] & -1u;
> +	}
> +
> +	if (nr >= KVM_KERNEL_NR_HYPERCALLS || nr < 0) {
> + 		run->hypercall.nr = nr;
> +		memcpy(run->hypercall.args, args, sizeof(args));
> +		run->hypercall.ret = 0;
> + 		run->hypercall.longmode = is_long_mode(vcpu);
> + 		kvm_arch_ops->decache_regs(vcpu);
>  		run->exit_reason = KVM_EXIT_HYPERCALL;
>  
> -		return 0;
> + 		return 0;
> + 	}
> +
> +	/* The hypercall might block or do intensive work */
> +	vcpu_put(vcpu);
> +
> +	/*
> +	 * Increase the hypercall's module ref count assures
> +	 * that no one will remove the module while a hypercall
> +	 * is executing.
> +	 * Theoretically we need to lock to get coherent
> module-hypercall
> +	 * view but practically there's almost no users this mechanism
> now.
> +	 */
> +	if (try_module_get(hypercalls[nr].module) < 0) {
> +		printk(KERN_DEBUG "%s: module reference count++
> failed\n",
> +			__FUNCTION__);
> +		ret = -EINVAL;
> +		goto out;
>  	}
> -	vcpu->regs[VCPU_REGS_RAX] = ret;
> -	kvm_arch_ops->decache_regs(vcpu);
> +
> +	if (!hypercalls[nr].hypercall) {
> +		printk(KERN_ERR "%s: hypercall nr(%lx) was not yet
> registered\n",
> +		       __FUNCTION__, nr);
> +		ret = -EINVAL;
> +		goto out_put;
> +	}
> +
> +	ret = hypercalls[nr].hypercall(vcpu, args);
> +
> +out_put:
> +	module_put(hypercalls[nr].module);
> +out:
> +	vcpu_load(vcpu);
> +
> + 	vcpu->regs[VCPU_REGS_RAX] = ret;
> + 	kvm_arch_ops->decache_regs(vcpu);
> +
>  	return 1;
>  }
>  EXPORT_SYMBOL_GPL(kvm_hypercall);
> @@ -2847,6 +2992,8 @@ out:
>  }
>  
>  static struct file_operations kvm_chardev_ops = {
> +	.open           = kvm_dev_open,
> +	.release        = kvm_dev_release,
>  	.unlocked_ioctl = kvm_dev_ioctl,
>  	.compat_ioctl   = kvm_dev_ioctl,
>  };
> diff --git a/include/linux/kvm_para.h b/include/linux/kvm_para.h
> index 754e29d..0cf516a 100644
> --- a/include/linux/kvm_para.h
> +++ b/include/linux/kvm_para.h
> @@ -71,6 +71,8 @@ struct kvm_vcpu_para_state {
>   * No registers are clobbered by the hypercall, except that the
>   * return value is in RAX.
>   */
> -#define __NR_hypercalls			0
> +
> +#define KVM_KERNEL_NR_HYPERCALLS		1
> +#define __NR_hypercall_test			0
>  
>  #endif
> 
> 
> -----
> In simplicity there is elegance.
> Dor Laor ;)
> 
> 
> -------------------------------------------------------------------------
> 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/
> _______________________________________________
> kvm-devel mailing list
> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/kvm-devel


-------------------------------------------------------------------------
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/