Re: [PATCH 2/6] KVM: SVM: Emulate nRIP feature when reinjecting INT3

[Jan Kiszka <jan.kiszka@siemens.com>]

Gleb Natapov wrote:
> On Mon, Feb 22, 2010 at 06:51:19PM +0100, Jan Kiszka wrote:
>> When in guest debugging mode, we have to reinject those #BP software
>> exceptions that are caused by guest-injected INT3. As older AMD
>> processors to not support the required nRIP VMCB field, try to emulate
>> it by moving RIP by one on injection. Fix it up again in case the
>> injection failed and we were able to catch this. This does not work for
>> unintercepted faults, but it is better than doing nothing.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  arch/x86/kvm/svm.c |   72 +++++++++++++++++++++++++++++++++++++--------------
>>  1 files changed, 52 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 1d76899..754e2f7 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -46,6 +46,7 @@ MODULE_LICENSE("GPL");
>>  #define SVM_FEATURE_NPT  (1 << 0)
>>  #define SVM_FEATURE_LBRV (1 << 1)
>>  #define SVM_FEATURE_SVML (1 << 2)
>> +#define SVM_FEATURE_NRIP (1 << 3)
>>  #define SVM_FEATURE_PAUSE_FILTER (1 << 10)
>>  
>>  #define NESTED_EXIT_HOST	0	/* Exit handled on host level */
>> @@ -109,6 +110,10 @@ struct vcpu_svm {
>>  	struct nested_state nested;
>>  
>>  	bool nmi_singlestep;
>> +
>> +	unsigned int3_injected;
>> +	u16 int3_cs;
>> +	u64 int3_rip;
>>  };
>>  
>>  /* enable NPT for AMD64 and X86 with PAE */
>> @@ -235,23 +240,6 @@ static void svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
>>  	vcpu->arch.efer = efer;
>>  }
>>  
>> -static void svm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
>> -				bool has_error_code, u32 error_code)
>> -{
>> -	struct vcpu_svm *svm = to_svm(vcpu);
>> -
>> -	/* If we are within a nested VM we'd better #VMEXIT and let the
>> -	   guest handle the exception */
>> -	if (nested_svm_check_exception(svm, nr, has_error_code, error_code))
>> -		return;
>> -
>> -	svm->vmcb->control.event_inj = nr
>> -		| SVM_EVTINJ_VALID
>> -		| (has_error_code ? SVM_EVTINJ_VALID_ERR : 0)
>> -		| SVM_EVTINJ_TYPE_EXEPT;
>> -	svm->vmcb->control.event_inj_err = error_code;
>> -}
>> -
> Why have you moved svm_queue_exception() function?
> 

To avoid adding a prototype.

>>  static int is_external_interrupt(u32 info)
>>  {
>>  	info &= SVM_EVTINJ_TYPE_MASK | SVM_EVTINJ_VALID;
>> @@ -297,6 +285,39 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
>>  	svm_set_interrupt_shadow(vcpu, 0);
>>  }
>>  
>> +static void svm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
>> +				bool has_error_code, u32 error_code)
>> +{
>> +	struct vcpu_svm *svm = to_svm(vcpu);
>> +
>> +	/* If we are within a nested VM we'd better #VMEXIT and let the
>> +	   guest handle the exception */
>> +	if (nested_svm_check_exception(svm, nr, has_error_code, error_code))
>> +		return;
>> +
>> +	if (nr == BP_VECTOR && !svm_has(SVM_FEATURE_NRIP)) {
>> +		u64 old_rip = kvm_rip_read(&svm->vcpu);
>> +
>> +		/*
>> +		 * For guest debugging where we have to reinject #BP if some
>> +		 * INT3 is guest-owned:
>> +		 * Emulate nRIP by moving RIP forward. Will fail if injection
>> +		 * raises a fault that is not intercepted. Still better than
>> +		 * failing in all cases.
>> +		 */
>> +		skip_emulated_instruction(&svm->vcpu);
>> +		svm->int3_cs = svm->vmcb->save.cs.selector;
>> +		svm->int3_rip = kvm_rip_read(&svm->vcpu);
>> +		svm->int3_injected = svm->int3_rip - old_rip;
>> +	}
>> +
>> +	svm->vmcb->control.event_inj = nr
>> +		| SVM_EVTINJ_VALID
>> +		| (has_error_code ? SVM_EVTINJ_VALID_ERR : 0)
>> +		| SVM_EVTINJ_TYPE_EXEPT;
>> +	svm->vmcb->control.event_inj_err = error_code;
>> +}
>> +
>>  static int has_svm(void)
>>  {
>>  	const char *msg;
>> @@ -2703,8 +2724,10 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
>>  	kvm_clear_exception_queue(&svm->vcpu);
>>  	kvm_clear_interrupt_queue(&svm->vcpu);
>>
> int int3_injected = svm->int3_injected;
> svm->int3_injected = 0;
> 
> Will save us from zeroing svm->int3_injected in two places.

OK.

> 
>> -	if (!(exitintinfo & SVM_EXITINTINFO_VALID))
>> +	if (!(exitintinfo & SVM_EXITINTINFO_VALID)) {
>> +		svm->int3_injected = 0;
>>  		return;
>> +	}
>>  
>>  	vector = exitintinfo & SVM_EXITINTINFO_VEC_MASK;
>>  	type = exitintinfo & SVM_EXITINTINFO_TYPE_MASK;
>> @@ -2714,12 +2737,20 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
>>  		svm->vcpu.arch.nmi_injected = true;
>>  		break;
>>  	case SVM_EXITINTINFO_TYPE_EXEPT:
>> -		/* In case of software exception do not reinject an exception
>> -		   vector, but re-execute and instruction instead */
>>  		if (is_nested(svm))
>>  			break;
>>  		if (kvm_exception_is_soft(vector))
>> +			if (vector == BP_VECTOR && svm->int3_injected &&
>> +			    svm->vmcb->save.cs.selector == svm->int3_cs &&
>> +			    kvm_rip_read(&svm->vcpu) == svm->int3_rip)
>> +				kvm_rip_write(&svm->vcpu,
>> +					      kvm_rip_read(&svm->vcpu) -
>> +					      svm->int3_injected);
>>  			break;
> Something is very wrong here. break does not belong to any of above ifs,
> so code below is unreachable.

Ouch, forgotten braces. Will fix.

> 
>> +		/*
>> +		 * In case of other software exceptions, do not reinject the
>> +		 * vector, but re-execute the instruction instead.
>> +		 */
>>  		if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
>>  			u32 err = svm->vmcb->control.exit_int_info_err;
>>  			kvm_queue_exception_e(&svm->vcpu, vector, err);
>> @@ -2733,6 +2764,7 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
>>  	default:
>>  		break;
>>  	}
>> +	svm->int3_injected = 0;
>>  }
>>  
>>  #ifdef CONFIG_X86_64
>> -- 
>> 1.6.0.2
> 
> --
> 			Gleb.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux