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

[Anthony Liguori <anthony@codemonkey.ws>]

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
>