Re: [PATCH 2/9] Add helper functions for nested SVM v5

[Anthony Liguori <anthony@codemonkey.ws>]

Alexander Graf wrote:
> These are helpers for the nested SVM implementation.
>
> - nsvm_printk implements a debug printk variant
> - nested_svm_do calls a handler that can accesses gpa-based memory
>
> v3 makes use of the new permission checker
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/x86/kvm/svm.c |   88 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 88 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 4ee5376..a00421b 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -48,6 +48,16 @@ MODULE_LICENSE("GPL");
>  
>  #define DEBUGCTL_RESERVED_BITS (~(0x3fULL))
>  
> +/* Turn on to get debugging output*/
> +/* #define NESTED_DEBUG */
> +
> +#ifdef NESTED_DEBUG
> +#define nsvm_printk(fmt, args...) printk(KERN_INFO fmt, ## args)
> +#else
> +static inline void nsvm_printk(char *fmt, ...) {
> +}
> +#endif
>
>   

I understand why you have this form, but stylistically the '{' should 
come on the next line.  Personally, I'd stick with the standard do {} 
while(0) just to avoid confusion although in the context of the kernel, 
I think pr_debug() would be better although a little more annoying to use.

>  /* enable NPT for AMD64 and X86 with PAE */
>  #if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
>  static bool npt_enabled = true;
> @@ -1145,6 +1155,84 @@ static int vmmcall_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
>  	return 1;
>  }
>  
> +static int nested_svm_check_permissions(struct vcpu_svm *svm)
> +{
> +	if (svm->vmcb->save.cpl) {
> +		printk(KERN_ERR "%s: invalid cpl 0x%x at ip 0x%lx\n",
> +		       __func__, svm->vmcb->save.cpl, kvm_rip_read(&svm->vcpu));
> +		kvm_queue_exception(&svm->vcpu, GP_VECTOR);
>   

GPFs need an error code.  Do you really think a GP should be delivered 
before checking SVME though?  I think you ought to switch the order of 
these checks.

> +		return 1;
> +	}
> +
> +       if (!(svm->vcpu.arch.shadow_efer & MSR_EFER_SVME_MASK)
> +           || !is_paging(&svm->vcpu)) {
> +               kvm_queue_exception(&svm->vcpu, UD_VECTOR);
> +               return 1;
> +       }
> +
> +       return 0;
> +}
> +
> +static struct page *nested_svm_get_page(struct vcpu_svm *svm, u64 gpa)
> +{
> +	struct page *page;
> +
> +	down_read(&current->mm->mmap_sem);
> +	page = gfn_to_page(svm->vcpu.kvm, gpa >> PAGE_SHIFT);
> +	up_read(&current->mm->mmap_sem);
> +
> +	if (is_error_page(page)) {
> +		printk(KERN_ERR "%s: could not find page at 0x%llx\n",
> +		       __func__, gpa);
> +		kvm_release_page_clean(page);
> +		kvm_queue_exception(&svm->vcpu, GP_VECTOR);
> +		return NULL;
> +	}
> +	return page;
> +}
> +
> +static int nested_svm_do(struct vcpu_svm *svm,
> +			 u64 arg1_gpa, u64 arg2_gpa, void *opaque,
> +			 int (*handler)(struct vcpu_svm *svm,
> +					void *arg1,
> +					void *arg2,
> +					void *opaque))
> +{
> +	struct page *arg1_page;
> +	struct page *arg2_page = NULL;
> +	void *arg1;
> +	void *arg2 = NULL;
> +	int retval;
> +
> +	arg1_page = nested_svm_get_page(svm, arg1_gpa);
> +	if(arg1_page == NULL)
> +		return 1;
> +
> +	if (arg2_gpa) {
> +		arg2_page = nested_svm_get_page(svm, arg2_gpa);
> +		if(arg2_page == NULL) {
> +			kvm_release_page_clean(arg1_page);
> +			return 1;
> +		}
> +	}
> +
> +	arg1 = kmap_atomic(arg1_page, KM_USER0);
> +	if (arg2_gpa)
> +		arg2 = kmap_atomic(arg2_page, KM_USER1);
> +
> +	retval = handler(svm, arg1, arg2, opaque);
> +
> +	kunmap_atomic(arg1, KM_USER0);
> +	if (arg2_gpa)
> +		kunmap_atomic(arg2, KM_USER1);
> +
> +	kvm_release_page_dirty(arg1_page);
> +	if (arg2_gpa)
> +		kvm_release_page_dirty(arg2_page);
> +
> +	return retval;
> +}
> +
>   

I appreciate small patches but introducing statics that aren't used is 
going to generate warnings when bisecting.  I think that suggests your 
splitting things at the wrong level.

Regards,

Anthony Liguori

>  static int invalid_op_interception(struct vcpu_svm *svm,
>  				   struct kvm_run *kvm_run)
>  {
>