Re: [PATCH 05/13] KVM: SVM: Add intercept check for emulated cr accesses

From: Avi Kivity <avi@redhat.com>
To: Joerg Roedel <joerg.roedel@amd.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>, kvm@vger.kernel.org
Subject: Re: [PATCH 05/13] KVM: SVM: Add intercept check for emulated cr accesses
Date: Sat, 26 Mar 2011 11:37:42 +0200	[thread overview]
Message-ID: <4D8DB3E6.4060202@redhat.com> (raw)
In-Reply-To: <1301045356-25257-6-git-send-email-joerg.roedel@amd.com>

On 03/25/2011 11:29 AM, Joerg Roedel wrote:
> This patch adds all necessary intercept checks for
> instructions that access the crX registers.
>
>
> @@ -3871,11 +3871,89 @@ static void svm_fpu_deactivate(struct kvm_vcpu *vcpu)
>   	update_cr0_intercept(svm);
>   }
>
> +#define POST_EX(exit) { .exit_code = (exit), \
> +			.stage = x86_icpt_post_except, \
> +			.valid = true }
> +
> +static struct __x86_intercept {
> +	u32 exit_code;
> +	enum x86_intercept_stage stage;
> +	bool valid;

Isn't .valid always true, even in later patches?

> +} x86_intercept_map[] = {

const?

> +	[x86_intercept_cr_read]		= POST_EX(SVM_EXIT_READ_CR0),
> +	[x86_intercept_cr_write]	= POST_EX(SVM_EXIT_WRITE_CR0),
> +	[x86_intercept_clts]		= POST_EX(SVM_EXIT_WRITE_CR0),
> +	[x86_intercept_lmsw]		= POST_EX(SVM_EXIT_WRITE_CR0),
> +	[x86_intercept_smsw]		= POST_EX(SVM_EXIT_READ_CR0),
> +};
> +
> +#undef POST_EX
> +
>   static int svm_check_intercept(struct kvm_vcpu *vcpu,
>   			       struct x86_instruction_info *info,
>   			       enum x86_intercept_stage stage)
>   {
> -	return X86EMUL_CONTINUE;
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +	int vmexit, ret = X86EMUL_CONTINUE;
> +	struct __x86_intercept icpt_info;
> +	struct vmcb *vmcb = svm->vmcb;
> +	int reg;
> +
> +	if (info->intercept>= ARRAY_SIZE(x86_intercept_map))
> +		goto out;
> +
> +	icpt_info = x86_intercept_map[info->intercept];
> +
> +	if (!icpt_info.valid || stage != icpt_info.stage)
> +		goto out;
> +
> +	reg = (info->modrm>>  3)&  7;

Doesn't work for r8-r15 or cr8-cr15; better to have the emulator put it 
into a separate variable.

> +
> +	switch (icpt_info.exit_code) {
> +	case SVM_EXIT_READ_CR0:
> +		if (info->intercept == x86_intercept_cr_read)
> +			icpt_info.exit_code += reg;

break; ?

(would work without it too, but we'll just get endless fixes for it)

> +	case SVM_EXIT_WRITE_CR0: {
> +		unsigned long cr0, val;
> +		u64 intercept;
> +
> +		if (info->intercept == x86_intercept_cr_write)
> +			icpt_info.exit_code += reg;
> +
> +		if (icpt_info.exit_code != SVM_EXIT_WRITE_CR0)
> +			break;
> +
> +		intercept = svm->nested.intercept;
> +
> +		if (!(intercept&  (1ULL<<  INTERCEPT_SELECTIVE_CR0)))
> +			break;
> +
> +		cr0 = vcpu->arch.cr0&  ~SVM_CR0_SELECTIVE_MASK;
> +		val = info->src_val&  ~SVM_CR0_SELECTIVE_MASK;
> +
> +		if (info->intercept == x86_intercept_lmsw) {
> +			cr0&= 0xfUL;
> +			val&= 0xfUL;

Probably, val |= X86_CR0_PE, since lmsw can't change that bit.

> +		}
> +
> +		if (cr0 ^ val)
> +			icpt_info.exit_code = SVM_EXIT_CR0_SEL_WRITE;
> +
> +		break;
> +	}
> +	default:
> +		break;
> +	}
> +
> +	vmcb->control.next_rip  = info->next_rip;
> +	vmcb->control.exit_code = icpt_info.exit_code;
> +	vmexit = nested_svm_exit_handled(svm);
> +
> +	ret = (vmexit == NESTED_EXIT_DONE) ? X86EMUL_INTERCEPTED
> +					   : X86EMUL_CONTINUE;
> +
> +out:
> +	return ret;
>   }
>
>   static struct kvm_x86_ops svm_x86_ops = {

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.