Re: [PATCH 5/9] [VMX] Do not re-execute INTn instruction.

[Gleb Natapov <gleb@redhat.com>]

On Wed, May 06, 2009 at 02:57:10PM +0800, Sheng Yang wrote:
> On Tuesday 05 May 2009 16:14:31 Gleb Natapov wrote:
> > Re-inject event instead. This is what Intel suggest. Also use correct
> > instruction length when re-injecting soft fault/interrupt.
> 
> Thanks for fixing this!
> 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > ---
> >  arch/x86/include/asm/kvm_host.h |    5 ++++-
> >  arch/x86/kvm/svm.c              |    6 +++---
> >  arch/x86/kvm/vmx.c              |   29 ++++++++++++++++++++++-------
> >  arch/x86/kvm/x86.c              |   13 ++++++++-----
> >  arch/x86/kvm/x86.h              |    9 ++++++++-
> >  5 files changed, 45 insertions(+), 17 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h
> > b/arch/x86/include/asm/kvm_host.h index cc892f5..fea0429 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -319,6 +319,8 @@ struct kvm_vcpu_arch {
> >  	struct kvm_pio_request pio;
> >  	void *pio_data;
> >
> > +	u8 event_exit_inst_len;
> > +
> 
> Can we simply read the field when we need, instead of a new field?
> 
Usually relying on vm exit information to be valid before vm entry
is wrong because migration can happen in a meantime. In this particular
case it is not so obvious since we don't want to migrate pending soft
interrupt, but re-execute instruction instead (we could migrate it
theoretically and may be we should, but when migrating from AMD to
Intel we don't have this info anyway). Another case where instruction
length as read from vmx may be outdated at interrupt injection time is
if exception happened during interrupt delivery and exception should be
re-injected first.

> >  	struct kvm_queued_exception {
> >  		bool pending;
> >  		bool has_error_code;
> > @@ -328,6 +330,7 @@ struct kvm_vcpu_arch {
> >
> >  	struct kvm_queued_interrupt {
> >  		bool pending;
> > +		bool soft;
> >  		u8 nr;
> >  	} interrupt;
> >
> > @@ -510,7 +513,7 @@ struct kvm_x86_ops {
> >  	void (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
> >  	void (*patch_hypercall)(struct kvm_vcpu *vcpu,
> >  				unsigned char *hypercall_addr);
> > -	void (*set_irq)(struct kvm_vcpu *vcpu, int vec);
> > +	void (*set_irq)(struct kvm_vcpu *vcpu, int vec, bool soft);
> 
> Seems all current interrupt info are go with struct kvm_vcpu now, how about 
> only take vcpu as parameter? Or using another struct kvm_queued_interrupt 
> pointer in addition to vcpu?
> 
Don't have any particular feelings one way or the other. Lets make it
kvm_vcpu only.

> >  	void (*set_nmi)(struct kvm_vcpu *vcpu);
> >  	void (*queue_exception)(struct kvm_vcpu *vcpu, unsigned nr,
> >  				bool has_error_code, u32 error_code);
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index 14cdfce..d5173a2 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -2284,7 +2284,7 @@ static void svm_queue_irq(struct kvm_vcpu *vcpu,
> > unsigned nr) SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_INTR;
> >  }
> >
> > -static void svm_set_irq(struct kvm_vcpu *vcpu, int irq)
> > +static void svm_set_irq(struct kvm_vcpu *vcpu, int irq, bool soft)
> >  {
> >  	struct vcpu_svm *svm = to_svm(vcpu);
> >
> > @@ -2392,7 +2392,7 @@ static void svm_complete_interrupts(struct vcpu_svm
> > *svm) case SVM_EXITINTINFO_TYPE_EXEPT:
> >  		/* In case of software exception do not reinject an exception
> >  		   vector, but re-execute and instruction instead */
> > -		if (vector == BP_VECTOR || vector == OF_VECTOR)
> > +		if (kvm_exception_is_soft(vector))
> >  			break;
> >  		if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
> >  			u32 err = svm->vmcb->control.exit_int_info_err;
> > @@ -2402,7 +2402,7 @@ static void svm_complete_interrupts(struct vcpu_svm
> > *svm) kvm_queue_exception(&svm->vcpu, vector);
> >  		break;
> >  	case SVM_EXITINTINFO_TYPE_INTR:
> > -		kvm_queue_interrupt(&svm->vcpu, vector);
> > +		kvm_queue_interrupt(&svm->vcpu, vector, false);
> >  		break;
> >  	default:
> >  		break;
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index a9b30e6..092a3ee 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -779,8 +779,9 @@ static void vmx_queue_exception(struct kvm_vcpu *vcpu,
> > unsigned nr, return;
> >  	}
> >
> > -	if (nr == BP_VECTOR || nr == OF_VECTOR) {
> > -		vmcs_write32(VM_ENTRY_INSTRUCTION_LEN, 1);
> > +	if (kvm_exception_is_soft(nr)) {
> 
> I noticed that DB_VECTOR was added as a soft one, but don't see the related 
> chapter in the spec?
> 
I was confused by AMD spec that defines int1 instruction (ICEBP), but now
I see that this instruction is not intercepted as #DB exception. Will fix.

--
			Gleb.