Re: [PATCH 24/28] nVMX: Handling of CR0 and CR4 modifying instructions

[Avi Kivity <avi@redhat.com>]

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<nyh@il.ibm.com>
> ---
>   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