From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH] KVM: nVMX: Fix setting of CR0 and CR4 in guest mode Date: Mon, 04 Mar 2013 15:09:51 +0100 Message-ID: <5134AB2F.20807@siemens.com> References: <512F270F.1020808@siemens.com> <20130304132234.GP23616@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , kvm , "Nadav Har'El" , "Nakajima, Jun" To: Gleb Natapov Return-path: Received: from thoth.sbs.de ([192.35.17.2]:22193 "EHLO thoth.sbs.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757540Ab3CDOKB (ORCPT ); Mon, 4 Mar 2013 09:10:01 -0500 In-Reply-To: <20130304132234.GP23616@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 2013-03-04 14:22, Gleb Natapov wrote: > On Thu, Feb 28, 2013 at 10:44:47AM +0100, Jan Kiszka wrote: >> The logic for calculating the value with which we call kvm_set_cr0/4 was >> broken (will definitely be visible with nested unrestricted guest mode >> support). Also, we performed the check regarding CR0_ALWAYSON too early >> when in guest mode. >> >> What really needs to be done on both CR0 and CR4 is to mask out L1-owned >> bits and merge them in from GUEST_CR0/4. In contrast, arch.cr0/4 and >> arch.cr0/4_guest_owned_bits contain the mangled L0+L1 state and, thus, >> are not suited as input. >> >> For both CRs, we can then apply the check against VMXON_CRx_ALWAYSON and >> refuse the update if it fails. To be fully consistent, we implement this >> check now also for CR4. >> >> Finally, we have to set the shadow to the value L2 wanted to write >> originally. >> >> Signed-off-by: Jan Kiszka >> --- >> >> Found while making unrestricted guest mode working. Not sure what impact >> the bugs had on current feature level, if any. >> >> For interested folks, I've pushed my nEPT environment here: >> >> git://git.kiszka.org/linux-kvm.git nept-hacking >> >> arch/x86/kvm/vmx.c | 49 ++++++++++++++++++++++++++++++------------------- >> 1 files changed, 30 insertions(+), 19 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index 7cc566b..d1dac08 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -4605,37 +4605,48 @@ vmx_patch_hypercall(struct kvm_vcpu *vcpu, unsigned char *hypercall) >> /* called to set cr0 as appropriate for a mov-to-cr0 exit. */ >> static int handle_set_cr0(struct kvm_vcpu *vcpu, unsigned long val) >> { >> - if (to_vmx(vcpu)->nested.vmxon && >> - ((val & VMXON_CR0_ALWAYSON) != VMXON_CR0_ALWAYSON)) >> - return 1; >> - >> if (is_guest_mode(vcpu)) { >> - /* >> - * We get here when L2 changed cr0 in a way that did not change >> - * any of L1's shadowed bits (see nested_vmx_exit_handled_cr), >> - * but did change L0 shadowed bits. This can currently happen >> - * with the TS bit: L0 may want to leave TS on (for lazy fpu >> - * loading) while pretending to allow the guest to change it. >> - */ > Can't say I understand this patch yet, but it looks like the comment is > still valid. Why have you removed it? L0 allows L1 or L2 at most to own TS, the rest is host-owned. I think the comment was always misleading. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux