Re: [PATCH v2 5/8] KVM: nVMX: Fix guest CR3 read-back on VM-exit

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2013-08-07 15:54, Paolo Bonzini wrote:
> On 08/07/2013 03:38 PM, Gleb Natapov wrote:
>> On Wed, Aug 07, 2013 at 03:32:37PM +0200, Paolo Bonzini wrote:
>>>>>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>>>>>> index 44494ed..60a3644 100644
>>>>>>>> --- a/arch/x86/kvm/vmx.c
>>>>>>>> +++ b/arch/x86/kvm/vmx.c
>>>>>>>> @@ -3375,8 +3375,10 @@ static void vmx_set_cr3(struct kvm_vcpu
>>>>>>>> *vcpu, unsigned long cr3)
>>>>>>>>       if (enable_ept) {
>>>>>>>>           eptp = construct_eptp(cr3);
>>>>>>>>           vmcs_write64(EPT_POINTER, eptp);
>>>>>>>> -        guest_cr3 = is_paging(vcpu) ? kvm_read_cr3(vcpu) :
>>>>>>>> -            vcpu->kvm->arch.ept_identity_map_addr;
>>>>>>>> +        if (is_paging(vcpu) || is_guest_mode(vcpu))
>>>>>>>> +            guest_cr3 = kvm_read_cr3(vcpu) :
>>>>>>>> +        else
>>>>>>>> +            guest_cr3 = vcpu->kvm->arch.ept_identity_map_addr;
>>>>>>>>           ept_load_pdptrs(vcpu);
>>>>>>>>       }
>>>>>>>>
>>>>>>> That what I am thinking, will think about it some more tomorrow.
>>>>>>
>>>>>> OK, and I'll feed it into a local test.
>>>>>>
>>>>> Thought about is some more. So without nested unrestricted guest (nUG)
>>>>> is_paging() will always be true (since without nUG guest entry is not
>>>>> possible otherwise) and guest's cr3 will be used, but with nUG
>>>>> identity
>>>>> map is not used (that is why L2 still works even though wrong identity
>>>>> map pointer is assigned to cr3), so the code here just corrupts nested
>>>>> guest's cr3 for no reason and that is why you had to use
>>>>> kvm_read_cr3()
>>>>> in prepare_vmcs12() to get correct cr3 value. The patch above
>>>>> should be
>>>>> used instead of original one IMO. How is testing going?
>>>>
>>>> Yes, testing worked fine. I've queued above patch and will send it out
>>>> within the next round.
>>>
>>> Just reply here with the commit message you desire and
>>> Signed-off-by, so I can queue it for people who wish to play with
>>> nEPT.
>>
>> I would love to have a comment there too :)
> 
> Ok, then it can wait since it is only needed with nested unrestricted
> guest.

Yes, it's related to that feature.

> On the other hand, it should come before patch 4 on the next
> submission.

I'll reorder the whole series, moving the feature enabling to the end.
The ordering still reflects more the history than the dependencies.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux