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: 2282 bytes --]

On 2013-04-30 14:42, Jan Kiszka wrote:
> On 2013-04-30 13:46, Gleb Natapov wrote:
>> On Sun, Apr 28, 2013 at 12:20:38PM +0200, Jan Kiszka wrote:
>>> On 2013-02-23 22:35, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> Likely a typo, but a fatal one as kvm_set_cr0 performs checks on the
>>>> state transition that may prevent loading L1's cr0.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>  arch/x86/kvm/vmx.c |    2 +-
>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>> index 26d47e9..94f3b66 100644
>>>> --- a/arch/x86/kvm/vmx.c
>>>> +++ b/arch/x86/kvm/vmx.c
>>>> @@ -7429,7 +7429,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
>>>>  	 * fpu_active (which may have changed).
>>>>  	 * Note that vmx_set_cr0 refers to efer set above.
>>>>  	 */
>>>> -	kvm_set_cr0(vcpu, vmcs12->host_cr0);
>>>> +	vmx_set_cr0(vcpu, vmcs12->host_cr0);
>>>>  	/*
>>>>  	 * If we did fpu_activate()/fpu_deactivate() during L2's run, we need
>>>>  	 * to apply the same changes to L1's vmcs. We just set cr0 correctly,
>>>>
>>>
>>> This one still applies, is necessary for nested unrestricted guest mode,
>>> and I'm still convinced it's an appropriate way to fix the bug. How to
>>> proceed?
>>>
>> What check that is done by kvm_set_cr0() fails?
> 
> Would have to reproduce the bug to confirm, but from the top of my head
> and from looking at the code again:
> 
> if (!is_paging(vcpu) && (cr0 & X86_CR0_PG)) {
> 	if ((vcpu->arch.efer & EFER_LME)) {
> 		int cs_db, cs_l;
> 
> 		if (!is_pae(vcpu))
> 			return 1;
> 		kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
> 		if (cs_l)
> 			return 1;
> 
> I think to remember this last check triggered. When we come from the
> guest with paging off, we may run through this check an incorrectly bail
> out here when the host state fulfills the conditions (PG, EFER_LME, and
> L bit set).

Just retried, and actually the first check (!is_pae) fails right now
(with nested unrestricted guest mode patched in). The second one
stumbles if I set CR4 before CR1 in load_vmcs12_host_state.

So, however you put it, calling kvm_set_cr0 remains wrong.

Jan


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