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

[Anthony Liguori]

Alexander Graf wrote:

On 30.10.2008, at 18:56, Anthony Liguori wrote:



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

Nice catch. The spec also says SVME is checked before CPL.
What error code exactly would we need here?

I don't know, I would have to look at the spec.  Joerg may know.

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

I figured warnings are nicer than having a blown-up patch. These 
functions are basically used within all patches from that one on, so 
it felt logical to split them up this way. How would you have split 
them up?

I would have folded them into the first user but this is just an issue 
of personal preference I think.

Regards,

Anthony Liguori


Alex


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html