Re: [PATCH 2/5] kvm/svm: enhance MOV CR intercept handler

[Andre Przywara <andre.przywara@amd.com>]

Marcelo Tosatti wrote:
> On Fri, Dec 10, 2010 at 02:51:25PM +0100, Andre Przywara wrote:
>> Newer SVM implementations provide the GPR number in the VMCB, so
>> that the emulation path is no longer necesarry to handle CR
>> register access intercepts. Implement the handling in svm.c and
>> use it when the info is provided.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
>> ---
>>  arch/x86/include/asm/svm.h |    2 +
>>  arch/x86/kvm/svm.c         |   91 ++++++++++++++++++++++++++++++++++++++-----
>>  2 files changed, 82 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
>> index 11dbca7..589fc25 100644
>> --- a/arch/x86/include/asm/svm.h
>> +++ b/arch/x86/include/asm/svm.h
>> @@ -256,6 +256,8 @@ struct __attribute__ ((__packed__)) vmcb {
>>  #define SVM_EXITINFOSHIFT_TS_REASON_JMP 38
>>  #define SVM_EXITINFOSHIFT_TS_HAS_ERROR_CODE 44
>>  
>> +#define SVM_EXITINFO_REG_MASK 0x0F
>> +
>>  #define	SVM_EXIT_READ_CR0 	0x000
>>  #define	SVM_EXIT_READ_CR3 	0x003
>>  #define	SVM_EXIT_READ_CR4 	0x004
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 298ff79..ee5f100 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -2594,12 +2594,81 @@ static int emulate_on_interception(struct vcpu_svm *svm)
>>  	return emulate_instruction(&svm->vcpu, 0, 0, 0) == EMULATE_DONE;
>>  }
>>  
>> +static int cr_interception(struct vcpu_svm *svm)
>> +{
>> +	int reg, cr;
>> +	unsigned long val;
>> +	int err;
>> +
>> +	if (!static_cpu_has(X86_FEATURE_DECODEASSISTS))
>> +		return emulate_on_interception(svm);
>> +
>> +	/* bit 63 is the valid bit, as not all instructions (like lmsw)
>> +	   provide the information */
>> +	if (unlikely((svm->vmcb->control.exit_info_1 & (1ULL << 63)) == 0))
>> +		return emulate_on_interception(svm);
>> +
>> +	reg = svm->vmcb->control.exit_info_1 & SVM_EXITINFO_REG_MASK;
>> +	cr = svm->vmcb->control.exit_code - SVM_EXIT_READ_CR0;
>> +
>> +	err = 0;
>> +	if (cr >= 16) { /* mov to cr */
>> +		cr -= 16;
>> +		val = kvm_register_read(&svm->vcpu, reg);
>> +		switch (cr) {
>> +		case 0:
>> +			err = kvm_set_cr0(&svm->vcpu, val);
>> +			break;
>> +		case 3:
>> +			err = kvm_set_cr3(&svm->vcpu, val);
>> +			break;
>> +		case 4:
>> +			err = kvm_set_cr4(&svm->vcpu, val);
>> +			break;
>> +		case 8:
>> +			err = kvm_set_cr8(&svm->vcpu, val);
>> +			break;
>> +		default:
>> +			WARN(1, "unhandled write to CR%d", cr);
>> +			return EMULATE_FAIL;
>> +		}
> 
> Wrong return value?
Thats right, thanks for catching this. I copied this from somewhere else ;-(

> Is WARN() really wanted?
Well, this is a rather pathological code path. Illegal CR accesses 
generate #UD before interception, and additionally we don't set the 
intercept bits for these, so this should really never occur. But just 
for the sake of completeness I can inject an #UD here and return 1.
So I leave the WARN in to inform the user that something stupid is going 
on, but it could also be BUG().

Regards,
Andre.

-- 
Andre Przywara
AMD-OSRC (Dresden)
Tel: x29712