Re: [PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 2294 bytes --]

On 2013-02-23 23:21, Jan Kiszka wrote:
> On 2013-02-23 22:57, Jan Kiszka wrote:
>> On 2013-02-23 22:45, Nadav Har'El wrote:
>>> On Sat, Feb 23, 2013, Jan Kiszka wrote about "[PATCH] KVM: nVMX: Replace kvm_set_cr0 with vmx_set_cr0 in load_vmcs12_host_state":
>>>> -	kvm_set_cr0(vcpu, vmcs12->host_cr0);
>>>> +	vmx_set_cr0(vcpu, vmcs12->host_cr0);
>>>
>>> I don't remember now why I did this (and I'm not looking at the code),
>>> but this you'll need to really test carefully, including
>>> shadow-on-shadow mode (ept=0 in L0), to verify you're not missing any
>>> important side-effect of kvm_set_cr0.
>>>
>>> Also, if I remember correctly, during nVMX's review, Avi Kivity asked
>>> in several places that when I called vmx_set_cr0, I should instead call
>>> kvm_set_cr0(), because it does some extra stuff and does some extra
>>> checks. Hmm, see, see this:
>>> 	http://markmail.org/message/hhidqyhbo2mrgxxc
>>>
>>> where Avi asked for the reverse patch you're attempting now.
>>
>> At least, kvm_set_cr0 can't be used as it assumes an otherwise
>> consistent guest state and an explicitly initiated transition - which is
>> naturally not the case while emulating a vmexit.
>>
>> However, there are some side effects that need a closer look, likely
>> also in other places where vmx_set_cr0 is used directly. E.g.
>> load_pdptrs. The lack of calling them on host->guest switch may explain
>> the PAE issues you once saw with your nEPT implementation.
>>
>> OK, more work...
> 
> Checked again, and the patch should be fine: load_pdptrs is executed by
> kvm_set_cr3 later in this functions (same for prepare_vmcs02, so no
> problem for PAE here), kvm_mmu_reset_context is even run explicitly and
> clearing the async-pf queue makes no sense when leaving the guest (the
> guest can't use it unless L1 is broken and passes this through).
> However, we should clear the queue when leaving the host - separate patch.

Thought about the async-pf thing again: It rather looks to me like the
host does not have to worry about it at all. It's L1's job to either
disable async-pf before entering L2 or trap PFs and handle paravirtual
reasons before they hit L2. I don't see that KVM already does this, but
I don't think that matters at this point.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]