From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH v2] KVM: nVMX: Fix setting of CR0 and CR4 in guest mode Date: Thu, 07 Mar 2013 09:12:19 +0100 Message-ID: <51384BE3.1040202@web.de> References: <5134F8AD.8030607@web.de> <20130307075117.GY11223@redhat.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="----enig2GDNCCASWLSUFFLHURJPJ" Cc: Marcelo Tosatti , kvm , Nadav Har'El To: Gleb Natapov Return-path: Received: from mout.web.de ([212.227.17.12]:56478 "EHLO mout.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753879Ab3CGIMZ (ORCPT ); Thu, 7 Mar 2013 03:12:25 -0500 In-Reply-To: <20130307075117.GY11223@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2GDNCCASWLSUFFLHURJPJ Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 2013-03-07 08:51, Gleb Natapov wrote: > On Mon, Mar 04, 2013 at 08:40:29PM +0100, Jan Kiszka wrote: >> The logic for calculating the value with which we call kvm_set_cr0/4 w= as >> broken (will definitely be visible with nested unrestricted guest mode= >> support). Also, we performed the check regarding CR0_ALWAYSON too earl= y >> when in guest mode. >> >> What really needs to be done on both CR0 and CR4 is to mask out L1-own= ed >> 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 a= nd >> refuse the update if it fails. To be fully consistent, we implement th= is >> 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 >> --- >> >> Changes in v2: >> - keep the non-misleading part of the comment in handle_set_cr0 >> >> arch/x86/kvm/vmx.c | 46 +++++++++++++++++++++++++++++++------------= --- >> 1 files changed, 31 insertions(+), 15 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index 7cc566b..832b7b4 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -4605,37 +4605,53 @@ vmx_patch_hypercall(struct kvm_vcpu *vcpu, uns= igned 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) !=3D VMXON_CR0_ALWAYSON)) >> - return 1; >> - >> if (is_guest_mode(vcpu)) { >> + struct vmcs12 *vmcs12 =3D get_vmcs12(vcpu); >> + unsigned long orig_val =3D val; >> + >> /* >> * 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. >> + * but did change L0 shadowed bits. >> */ >> - if (kvm_set_cr0(vcpu, (val & vcpu->arch.cr0_guest_owned_bits) | >> - (vcpu->arch.cr0 & ~vcpu->arch.cr0_guest_owned_bits))) >> + val =3D (val & ~vmcs12->cr0_guest_host_mask) | >> + (vmcs_read64(GUEST_CR0) & vmcs12->cr0_guest_host_mask); > I think using GUEST_CR0 here is incorrect. It contains combination of b= its > set by L2, L1 and L0 and here we need to get only L2/L1 mix which is in= > vcpu->arch.cr0 (almost, but good enough for this case). Why vcpu->arch.= cr0 > contains right L2/L1 mix? L0/L1. E.g., kvm_set_cr0 unconditionally injects X86_CR0_ET and masks out reserved bits. But you are right, GUEST_CR0 isn't much better. And maybe that mangling in kvm_set_cr0 is a corner case we can ignore. > Because it was set to vmcs12->guest_cr0 during > L2 #vmentry. While L2 is running three things may happen to CR0: >=20 > 1. L2 writes to a bit that is not shadowed neither by L1 nor by L0. It= > will go strait to GUEST_CR0. > 2. L2 writes to a bit shadowed by L1. L1 #vmexit will be emulated. On = the > next #vmetry vcpu->arch.cr0 will be set to whatever value L1 calcul= ated. > 3. L2 writes to a bit shadowed by L0, but not L1. This is the case we > are handling here. And if we will do it right vcpu->arch.cr0 will b= e > up-to-date at the end. >=20 > The only case when, while this code running, vcpu->arch.cr0 has not > up-to-date value is if 1 happened, but since L2 guest overwriting cr0 > here anyway it does not matter what it previously set in GUEST_CR0. The= > correct bits are in a new cr0 value provided by val and accessible by > (val & ~vmcs12->cr0_guest_host_mask). I need to think about it again. Maybe vmcs12->guest_cr0 is best, but that's a shot from the hips now. Jan ------enig2GDNCCASWLSUFFLHURJPJ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlE4S+UACgkQitSsb3rl5xRrhQCfam4wekdwXqwwsh3QIR5GfCX0 4OIAn0by7sNMKReGgHue7+NyYeDXf5Ux =GCKS -----END PGP SIGNATURE----- ------enig2GDNCCASWLSUFFLHURJPJ--