From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 24/28] nVMX: Handling of CR0 and CR4 modifying instructions Date: Thu, 09 Dec 2010 15:19:17 +0200 Message-ID: <4D00D755.5070908@redhat.com> References: <1291827596-nyh@il.ibm.com> <201012081712.oB8HCFak008814@rice.haifa.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, gleb@redhat.com To: "Nadav Har'El" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:8591 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755146Ab0LINTW (ORCPT ); Thu, 9 Dec 2010 08:19:22 -0500 In-Reply-To: <201012081712.oB8HCFak008814@rice.haifa.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: On 12/08/2010 07:12 PM, Nadav Har'El wrote: > When L2 tries to modify CR0 or CR4 (with mov or clts), and modifies a bit > which L1 asked to shadow (via CR[04]_GUEST_HOST_MASK), we already do the right > thing: we let L1 handle the trap (see nested_vmx_exit_handled_cr() in a > previous patch). > When L2 modifies bits that L1 doesn't care about, we let it think (via > CR[04]_READ_SHADOW) that it did these modifications, while only changing > (in GUEST_CR[04]) the bits that L0 doesn't shadow. > > This is needed for corect handling of CR0.TS for lazy FPU loading: L0 may > want to leave TS on, while pretending to allow the guest to change it. > > Signed-off-by: Nadav Har'El > --- > arch/x86/kvm/vmx.c | 54 ++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 51 insertions(+), 3 deletions(-) > > --- .before/arch/x86/kvm/vmx.c 2010-12-08 18:56:51.000000000 +0200 > +++ .after/arch/x86/kvm/vmx.c 2010-12-08 18:56:51.000000000 +0200 > @@ -3877,6 +3877,54 @@ static void complete_insn_gp(struct kvm_ > skip_emulated_instruction(vcpu); > } > > +/* called to set cr0 as approriate for a mov-to-cr0 exit. */ > +static int handle_set_cr0(struct kvm_vcpu *vcpu, unsigned long val) > +{ > + 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. > + */ > + vmcs_writel(GUEST_CR0, > + (val& vcpu->arch.cr0_guest_owned_bits) | > + (vmcs_readl(GUEST_CR0)& ~vcpu->arch.cr0_guest_owned_bits)); > + vmcs_writel(CR0_READ_SHADOW, val); > + vcpu->arch.cr0 = val; > + return 0; > + } else > + return kvm_set_cr0(vcpu, val); > +} Easier way: update val to reflect the change, and call kvm_set_cr0(val). This allows any side effects by kvm_set_cr4() to take place (for example the guest may allow the nested guest to change cr0.pg, and we need to kvm_set_cr0() to make note of that). > + > +static int handle_set_cr4(struct kvm_vcpu *vcpu, unsigned long val) > +{ > + if (is_guest_mode(vcpu)) { > + vmcs_writel(GUEST_CR4, > + (val& vcpu->arch.cr4_guest_owned_bits) | > + (vmcs_readl(GUEST_CR4)& ~vcpu->arch.cr4_guest_owned_bits)); > + vmcs_writel(CR4_READ_SHADOW, val); > + vcpu->arch.cr4 = val; > + return 0; > + } else > + return kvm_set_cr4(vcpu, val); > +} Ditto. > + > + > +/* called to set cr0 as approriate for clts instruction exit. */ > +static void handle_clts(struct kvm_vcpu *vcpu) > +{ > + if (is_guest_mode(vcpu)) { > + /* As in handle_set_cr0(), we can't call vmx_set_cr0 here */ > + vmcs_writel(GUEST_CR0, vmcs_readl(GUEST_CR0)& ~X86_CR0_TS); > + vmcs_writel(CR0_READ_SHADOW, > + vmcs_readl(CR0_READ_SHADOW)& ~X86_CR0_TS); > + vcpu->arch.cr0&= ~X86_CR0_TS; > + } else > + vmx_set_cr0(vcpu, kvm_read_cr0_bits(vcpu, ~X86_CR0_TS)); > +} Here, too. -- error compiling committee.c: too many arguments to function