From mboxrd@z Thu Jan  1 00:00:00 1970
From: Anthony Liguori <anthony@codemonkey.ws>
Subject: Re: [PATCH 2/9] Add helper functions for nested SVM v5
Date: Thu, 30 Oct 2008 14:10:29 -0500
Message-ID: <490A06A5.1020803@codemonkey.ws>
References: <1224522290-11740-1-git-send-email-agraf@suse.de> <1224522290-11740-2-git-send-email-agraf@suse.de> <1224522290-11740-3-git-send-email-agraf@suse.de> <4909F54B.50100@codemonkey.ws> <1EFD5A82-CD91-4EAB-84DF-3D7BCB72D886@suse.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: kvm@vger.kernel.org, joro@8bytes.org, avi@redhat.com
To: Alexander Graf <agraf@suse.de>
Return-path: <kvm-owner@vger.kernel.org>
Received: from rn-out-0910.google.com ([64.233.170.185]:26956 "EHLO
	rn-out-0910.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752836AbYJ3TKf (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 30 Oct 2008 15:10:35 -0400
Received: by rn-out-0910.google.com with SMTP id k40so626832rnd.17
        for <kvm@vger.kernel.org>; Thu, 30 Oct 2008 12:10:34 -0700 (PDT)
In-Reply-To: <1EFD5A82-CD91-4EAB-84DF-3D7BCB72D886@suse.de>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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
>