From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@redhat.com>
Subject: Re: [PATCH 05/13] KVM: SVM: Add intercept check for emulated cr accesses
Date: Sat, 26 Mar 2011 11:37:42 +0200
Message-ID: <4D8DB3E6.4060202@redhat.com>
References: <1301045356-25257-1-git-send-email-joerg.roedel@amd.com> <1301045356-25257-6-git-send-email-joerg.roedel@amd.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: Marcelo Tosatti <mtosatti@redhat.com>, kvm@vger.kernel.org
To: Joerg Roedel <joerg.roedel@amd.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:33828 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752342Ab1CZJhr (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sat, 26 Mar 2011 05:37:47 -0400
In-Reply-To: <1301045356-25257-6-git-send-email-joerg.roedel@amd.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.