[PATCH] KVM: x86: Add instruction length to VCPU event state

[PATCH] KVM: x86: Add instruction length to VCPU event state

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2430 bytes --]

From: Jan Kiszka <jan.kiszka@siemens.com>

VMX requires a properly set instruction length VM entry field when
trying to inject soft exception and interrupts. We have to preserve this
state across VM save/restore to avoid breaking the re-injection of such
events on Intel. So add it to the new VCPU event state.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Existing qemu[-kvm]-0.12 that is already prepared for 2.6.33 will need
an update now. Whenever we actually ran into the case that
event_exit_inst_len was evaluated by VMX, we were playing roulette with
a high probability to crash the guest. This will not changes for already
released 0.12.x versions.

 Documentation/kvm/api.txt  |    2 ++
 arch/x86/include/asm/kvm.h |    3 ++-
 arch/x86/kvm/x86.c         |    4 ++++
 3 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/Documentation/kvm/api.txt b/Documentation/kvm/api.txt
index c6416a3..aa11d70 100644
--- a/Documentation/kvm/api.txt
+++ b/Documentation/kvm/api.txt
@@ -686,6 +686,8 @@ struct kvm_vcpu_events {
 	} nmi;
 	__u32 sipi_vector;
 	__u32 flags;
+	__u32 instruction_length; /* used by VMX */
+	__u32 reserved[9];
 };
 
 4.30 KVM_SET_VCPU_EVENTS
diff --git a/arch/x86/include/asm/kvm.h b/arch/x86/include/asm/kvm.h
index f46b79f..570b6cc 100644
--- a/arch/x86/include/asm/kvm.h
+++ b/arch/x86/include/asm/kvm.h
@@ -281,7 +281,8 @@ struct kvm_vcpu_events {
 	} nmi;
 	__u32 sipi_vector;
 	__u32 flags;
-	__u32 reserved[10];
+	__u32 instruction_length; /* used by VMX */
+	__u32 reserved[9];
 };
 
 #endif /* _ASM_X86_KVM_H */
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 86b739f..0cc6cfb 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2134,6 +2134,8 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
 	events->nmi.pending = vcpu->arch.nmi_pending;
 	events->nmi.masked = kvm_x86_ops->get_nmi_mask(vcpu);
 
+	events->instruction_length = vcpu->arch.event_exit_inst_len;
+
 	events->sipi_vector = vcpu->arch.sipi_vector;
 
 	events->flags = (KVM_VCPUEVENT_VALID_NMI_PENDING
@@ -2170,6 +2172,8 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 	if (events->flags & KVM_VCPUEVENT_VALID_SIPI_VECTOR)
 		vcpu->arch.sipi_vector = events->sipi_vector;
 
+	vcpu->arch.event_exit_inst_len = events->instruction_length;
+
 	vcpu_put(vcpu);
 
 	return 0;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] KVM: x86: Add instruction length to VCPU event state

[Avi Kivity]

On 02/13/2010 11:51 AM, Jan Kiszka wrote:
> From: Jan Kiszka<jan.kiszka@siemens.com>
>
> VMX requires a properly set instruction length VM entry field when
> trying to inject soft exception and interrupts. We have to preserve this
> state across VM save/restore to avoid breaking the re-injection of such
> events on Intel. So add it to the new VCPU event state.
>
>    

Can't we fake it?  set instruction length to 1 and rewind rip by 1.

The only case where I think this can fail is if we have a fault during 
the soft exception injection.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] KVM: x86: Add instruction length to VCPU event state

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 838 bytes --]

Avi Kivity wrote:
> On 02/13/2010 11:51 AM, Jan Kiszka wrote:
>> From: Jan Kiszka<jan.kiszka@siemens.com>
>>
>> VMX requires a properly set instruction length VM entry field when
>> trying to inject soft exception and interrupts. We have to preserve this
>> state across VM save/restore to avoid breaking the re-injection of such
>> events on Intel. So add it to the new VCPU event state.
>>
>>    
> 
> Can't we fake it?  set instruction length to 1 and rewind rip by 1.
> 
> The only case where I think this can fail is if we have a fault during
> the soft exception injection.

I don't think so. If e.g. privileged soft exception delivery failed, we
happen to return to user space at this point and we then start a
migration, we have to deal with arbitrary lengths on re-injection on the
migration target.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] KVM: x86: Add instruction length to VCPU event state

[Gleb Natapov]

On Sat, Feb 13, 2010 at 10:51:40AM +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> VMX requires a properly set instruction length VM entry field when
> trying to inject soft exception and interrupts. We have to preserve this
> state across VM save/restore to avoid breaking the re-injection of such
> events on Intel. So add it to the new VCPU event state.
> 
We shouldn't re-inject soft exceptions/interrupts after migration, but
re-execute instruction instead. Instruction length field doesn't exist
on SVM and migration shouldn't expose implementation details.

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Existing qemu[-kvm]-0.12 that is already prepared for 2.6.33 will need
> an update now. Whenever we actually ran into the case that
> event_exit_inst_len was evaluated by VMX, we were playing roulette with
> a high probability to crash the guest. This will not changes for already
> released 0.12.x versions.
> 
>  Documentation/kvm/api.txt  |    2 ++
>  arch/x86/include/asm/kvm.h |    3 ++-
>  arch/x86/kvm/x86.c         |    4 ++++
>  3 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/kvm/api.txt b/Documentation/kvm/api.txt
> index c6416a3..aa11d70 100644
> --- a/Documentation/kvm/api.txt
> +++ b/Documentation/kvm/api.txt
> @@ -686,6 +686,8 @@ struct kvm_vcpu_events {
>  	} nmi;
>  	__u32 sipi_vector;
>  	__u32 flags;
> +	__u32 instruction_length; /* used by VMX */
> +	__u32 reserved[9];
>  };
>  
>  4.30 KVM_SET_VCPU_EVENTS
> diff --git a/arch/x86/include/asm/kvm.h b/arch/x86/include/asm/kvm.h
> index f46b79f..570b6cc 100644
> --- a/arch/x86/include/asm/kvm.h
> +++ b/arch/x86/include/asm/kvm.h
> @@ -281,7 +281,8 @@ struct kvm_vcpu_events {
>  	} nmi;
>  	__u32 sipi_vector;
>  	__u32 flags;
> -	__u32 reserved[10];
> +	__u32 instruction_length; /* used by VMX */
> +	__u32 reserved[9];
>  };
>  
>  #endif /* _ASM_X86_KVM_H */
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 86b739f..0cc6cfb 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2134,6 +2134,8 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
>  	events->nmi.pending = vcpu->arch.nmi_pending;
>  	events->nmi.masked = kvm_x86_ops->get_nmi_mask(vcpu);
>  
> +	events->instruction_length = vcpu->arch.event_exit_inst_len;
> +
>  	events->sipi_vector = vcpu->arch.sipi_vector;
>  
>  	events->flags = (KVM_VCPUEVENT_VALID_NMI_PENDING
> @@ -2170,6 +2172,8 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
>  	if (events->flags & KVM_VCPUEVENT_VALID_SIPI_VECTOR)
>  		vcpu->arch.sipi_vector = events->sipi_vector;
>  
> +	vcpu->arch.event_exit_inst_len = events->instruction_length;
> +
>  	vcpu_put(vcpu);
>  
>  	return 0;
> 



--
			Gleb.

Re: [PATCH] KVM: x86: Add instruction length to VCPU event state

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3929 bytes --]

Gleb Natapov wrote:
> On Sat, Feb 13, 2010 at 10:51:40AM +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> VMX requires a properly set instruction length VM entry field when
>> trying to inject soft exception and interrupts. We have to preserve this
>> state across VM save/restore to avoid breaking the re-injection of such
>> events on Intel. So add it to the new VCPU event state.
>>
> We shouldn't re-inject soft exceptions/interrupts after migration, but
> re-execute instruction instead. Instruction length field doesn't exist
> on SVM and migration shouldn't expose implementation details.
> 

Hmm, then I guess this totally untested patch should fly:

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index f9a2f66..f87c3a5 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -315,8 +315,6 @@ struct kvm_vcpu_arch {
 	struct kvm_pio_request pio;
 	void *pio_data;
 
-	u8 event_exit_inst_len;
-
 	struct kvm_queued_exception {
 		bool pending;
 		bool has_error_code;
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index f82b072..a7111da 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -889,8 +889,7 @@ static void vmx_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
 		vmx->rmode.irq.vector = nr;
 		vmx->rmode.irq.rip = kvm_rip_read(vcpu);
 		if (kvm_exception_is_soft(nr))
-			vmx->rmode.irq.rip +=
-				vmx->vcpu.arch.event_exit_inst_len;
+			vmx->rmode.irq.rip++;
 		intr_info |= INTR_TYPE_SOFT_INTR;
 		vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, intr_info);
 		vmcs_write32(VM_ENTRY_INSTRUCTION_LEN, 1);
@@ -899,8 +898,7 @@ static void vmx_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
 	}
 
 	if (kvm_exception_is_soft(nr)) {
-		vmcs_write32(VM_ENTRY_INSTRUCTION_LEN,
-			     vmx->vcpu.arch.event_exit_inst_len);
+		vmcs_write32(VM_ENTRY_INSTRUCTION_LEN, 1);
 		intr_info |= INTR_TYPE_SOFT_EXCEPTION;
 	} else
 		intr_info |= INTR_TYPE_HARD_EXCEPTION;
@@ -2639,8 +2637,7 @@ static void vmx_inject_irq(struct kvm_vcpu *vcpu)
 		vmx->rmode.irq.vector = irq;
 		vmx->rmode.irq.rip = kvm_rip_read(vcpu);
 		if (vcpu->arch.interrupt.soft)
-			vmx->rmode.irq.rip +=
-				vmx->vcpu.arch.event_exit_inst_len;
+			vmx->rmode.irq.rip += 2;
 		vmcs_write32(VM_ENTRY_INTR_INFO_FIELD,
 			     irq | INTR_TYPE_SOFT_INTR | INTR_INFO_VALID_MASK);
 		vmcs_write32(VM_ENTRY_INSTRUCTION_LEN, 1);
@@ -2650,8 +2647,7 @@ static void vmx_inject_irq(struct kvm_vcpu *vcpu)
 	intr = irq | INTR_INFO_VALID_MASK;
 	if (vcpu->arch.interrupt.soft) {
 		intr |= INTR_TYPE_SOFT_INTR;
-		vmcs_write32(VM_ENTRY_INSTRUCTION_LEN,
-			     vmx->vcpu.arch.event_exit_inst_len);
+		vmcs_write32(VM_ENTRY_INSTRUCTION_LEN, 2);
 	} else
 		intr |= INTR_TYPE_EXT_INTR;
 	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, intr);
@@ -3688,9 +3684,6 @@ static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 				GUEST_INTR_STATE_NMI);
 		break;
 	case INTR_TYPE_SOFT_EXCEPTION:
-		vmx->vcpu.arch.event_exit_inst_len =
-			vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
-		/* fall through */
 	case INTR_TYPE_HARD_EXCEPTION:
 		if (idt_vectoring_info & VECTORING_INFO_DELIVER_CODE_MASK) {
 			u32 err = vmcs_read32(IDT_VECTORING_ERROR_CODE);
@@ -3699,9 +3692,6 @@ static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 			kvm_queue_exception(&vmx->vcpu, vector);
 		break;
 	case INTR_TYPE_SOFT_INTR:
-		vmx->vcpu.arch.event_exit_inst_len =
-			vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
-		/* fall through */
 	case INTR_TYPE_EXT_INTR:
 		kvm_queue_interrupt(&vmx->vcpu, vector,
 			type == INTR_TYPE_SOFT_INTR);


As we actually do not support privileged soft exceptions, we are left
with int3 and into as single-byte instructions triggering soft
exceptions and int N as a two-byte instruction a the reason for soft
interrupts. Am I missing something?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] KVM: x86: Add instruction length to VCPU event state

[Gleb Natapov]

On Sat, Feb 13, 2010 at 06:49:44PM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Sat, Feb 13, 2010 at 10:51:40AM +0100, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> VMX requires a properly set instruction length VM entry field when
> >> trying to inject soft exception and interrupts. We have to preserve this
> >> state across VM save/restore to avoid breaking the re-injection of such
> >> events on Intel. So add it to the new VCPU event state.
> >>
> > We shouldn't re-inject soft exceptions/interrupts after migration, but
> > re-execute instruction instead. Instruction length field doesn't exist
> > on SVM and migration shouldn't expose implementation details.
> > 
> 
> Hmm, then I guess this totally untested patch should fly:
> 
I don't understand what problem are you trying to solve by your patch.
During normal operation event_exit_inst_len will be set to correct
value. After migration rip will point to int instruction an no even will
be pending at all. Here is the patch:

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a1b71da..519f867 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2121,14 +2121,18 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
 {
 	vcpu_load(vcpu);
 
-	events->exception.injected = vcpu->arch.exception.pending;
-	events->exception.nr = vcpu->arch.exception.nr;
-	events->exception.has_error_code = vcpu->arch.exception.has_error_code;
-	events->exception.error_code = vcpu->arch.exception.error_code;
-
-	events->interrupt.injected = vcpu->arch.interrupt.pending;
-	events->interrupt.nr = vcpu->arch.interrupt.nr;
-	events->interrupt.soft = vcpu->arch.interrupt.soft;
+	if (!kvm_exception_is_soft(nr)) {
+		events->exception.injected = vcpu->arch.exception.pending;
+		events->exception.nr = vcpu->arch.exception.nr;
+		events->exception.has_error_code = vcpu->arch.exception.has_error_code;
+		events->exception.error_code = vcpu->arch.exception.error_code;
+	}
+
+	if (!events->interrupt.soft) {
+		events->interrupt.injected = vcpu->arch.interrupt.pending;
+		events->interrupt.nr = vcpu->arch.interrupt.nr;
+		events->interrupt.soft = vcpu->arch.interrupt.soft;
+	}
 
 	events->nmi.injected = vcpu->arch.nmi_injected;
 	events->nmi.pending = vcpu->arch.nmi_pending;
--
			Gleb.

Re: [PATCH] KVM: x86: Add instruction length to VCPU event state

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1359 bytes --]

Gleb Natapov wrote:
> On Sat, Feb 13, 2010 at 06:49:44PM +0100, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Sat, Feb 13, 2010 at 10:51:40AM +0100, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> VMX requires a properly set instruction length VM entry field when
>>>> trying to inject soft exception and interrupts. We have to preserve this
>>>> state across VM save/restore to avoid breaking the re-injection of such
>>>> events on Intel. So add it to the new VCPU event state.
>>>>
>>> We shouldn't re-inject soft exceptions/interrupts after migration, but
>>> re-execute instruction instead. Instruction length field doesn't exist
>>> on SVM and migration shouldn't expose implementation details.
>>>
>> Hmm, then I guess this totally untested patch should fly:
>>
> I don't understand what problem are you trying to solve by your patch.
> During normal operation event_exit_inst_len will be set to correct
> value. After migration rip will point to int instruction an no even will
> be pending at all. Here is the patch:

The patch will cause an endless loop if BP interception is enabled.

What is the purpose of keeping event_exit_inst_len around? Either we
need it also across user space exists, then we have to save/restore or
reconstruct it, or we don't need it, then simply drop it.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] KVM: x86: Add instruction length to VCPU event state

[Gleb Natapov]

On Sat, Feb 13, 2010 at 07:41:35PM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Sat, Feb 13, 2010 at 06:49:44PM +0100, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Sat, Feb 13, 2010 at 10:51:40AM +0100, Jan Kiszka wrote:
> >>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>
> >>>> VMX requires a properly set instruction length VM entry field when
> >>>> trying to inject soft exception and interrupts. We have to preserve this
> >>>> state across VM save/restore to avoid breaking the re-injection of such
> >>>> events on Intel. So add it to the new VCPU event state.
> >>>>
> >>> We shouldn't re-inject soft exceptions/interrupts after migration, but
> >>> re-execute instruction instead. Instruction length field doesn't exist
> >>> on SVM and migration shouldn't expose implementation details.
> >>>
> >> Hmm, then I guess this totally untested patch should fly:
> >>
> > I don't understand what problem are you trying to solve by your patch.
> > During normal operation event_exit_inst_len will be set to correct
> > value. After migration rip will point to int instruction an no even will
> > be pending at all. Here is the patch:
> 
> The patch will cause an endless loop if BP interception is enabled.
> 
How? This code path is not executed normally.

> What is the purpose of keeping event_exit_inst_len around? Either we
> need it also across user space exists, then we have to save/restore or
> reconstruct it, or we don't need it, then simply drop it.
> 
Why we need to save/restore is if we need it across user space exits?
We need to save/restore it only if we nedd it across migration.

When exception happens during soft interrupt/exception delivery soft i/e
should be retried somehow. There are two ways to do that. First one is just
reenter guest with the same rip. Instruction will be reexecuted and
event redelivered. Another is to reinject event via event reinjection
mechanism and for that we need to tell CPU how to calculate rip of a next
instruction and this is done by providing event_exit_inst_len. The
problem is that SVM supports only the first way. Intel advised us to use
reinjection mechanism, so that what we use on VMX, but since migration
can happen from Intel to AMD and vice versa we chose to reexecute
instruction after migration on those rare occasions that migration
happens exactly after intercepted soft i/e.

--
			Gleb.

Re: [PATCH] KVM: x86: Add instruction length to VCPU event state

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2783 bytes --]

Gleb Natapov wrote:
> On Sat, Feb 13, 2010 at 07:41:35PM +0100, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Sat, Feb 13, 2010 at 06:49:44PM +0100, Jan Kiszka wrote:
>>>> Gleb Natapov wrote:
>>>>> On Sat, Feb 13, 2010 at 10:51:40AM +0100, Jan Kiszka wrote:
>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>
>>>>>> VMX requires a properly set instruction length VM entry field when
>>>>>> trying to inject soft exception and interrupts. We have to preserve this
>>>>>> state across VM save/restore to avoid breaking the re-injection of such
>>>>>> events on Intel. So add it to the new VCPU event state.
>>>>>>
>>>>> We shouldn't re-inject soft exceptions/interrupts after migration, but
>>>>> re-execute instruction instead. Instruction length field doesn't exist
>>>>> on SVM and migration shouldn't expose implementation details.
>>>>>
>>>> Hmm, then I guess this totally untested patch should fly:
>>>>
>>> I don't understand what problem are you trying to solve by your patch.
>>> During normal operation event_exit_inst_len will be set to correct
>>> value. After migration rip will point to int instruction an no even will
>>> be pending at all. Here is the patch:
>> The patch will cause an endless loop if BP interception is enabled.
>>
> How? This code path is not executed normally.

Oh, I read it the other way around, but it is supposed to mask soft
exceptions/irqs (clearing *.injected is missing then).

> 
>> What is the purpose of keeping event_exit_inst_len around? Either we
>> need it also across user space exists, then we have to save/restore or
>> reconstruct it, or we don't need it, then simply drop it.
>>
> Why we need to save/restore is if we need it across user space exits?
> We need to save/restore it only if we nedd it across migration.
> 
> When exception happens during soft interrupt/exception delivery soft i/e
> should be retried somehow. There are two ways to do that. First one is just
> reenter guest with the same rip. Instruction will be reexecuted and
> event redelivered. Another is to reinject event via event reinjection
> mechanism and for that we need to tell CPU how to calculate rip of a next
> instruction and this is done by providing event_exit_inst_len. The

But I still fail to see the case where event_exit_inst_len is set to
anything but 1 or 2 and where it is related to anything else than exits
at INT3, INT X, or INTO.

> problem is that SVM supports only the first way. Intel advised us to use
> reinjection mechanism, so that what we use on VMX, but since migration
> can happen from Intel to AMD and vice versa we chose to reexecute
> instruction after migration on those rare occasions that migration
> happens exactly after intercepted soft i/e.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] KVM: x86: Add instruction length to VCPU event state

[Gleb Natapov]

On Sat, Feb 13, 2010 at 08:20:41PM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Sat, Feb 13, 2010 at 07:41:35PM +0100, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Sat, Feb 13, 2010 at 06:49:44PM +0100, Jan Kiszka wrote:
> >>>> Gleb Natapov wrote:
> >>>>> On Sat, Feb 13, 2010 at 10:51:40AM +0100, Jan Kiszka wrote:
> >>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>>
> >>>>>> VMX requires a properly set instruction length VM entry field when
> >>>>>> trying to inject soft exception and interrupts. We have to preserve this
> >>>>>> state across VM save/restore to avoid breaking the re-injection of such
> >>>>>> events on Intel. So add it to the new VCPU event state.
> >>>>>>
> >>>>> We shouldn't re-inject soft exceptions/interrupts after migration, but
> >>>>> re-execute instruction instead. Instruction length field doesn't exist
> >>>>> on SVM and migration shouldn't expose implementation details.
> >>>>>
> >>>> Hmm, then I guess this totally untested patch should fly:
> >>>>
> >>> I don't understand what problem are you trying to solve by your patch.
> >>> During normal operation event_exit_inst_len will be set to correct
> >>> value. After migration rip will point to int instruction an no even will
> >>> be pending at all. Here is the patch:
> >> The patch will cause an endless loop if BP interception is enabled.
> >>
> > How? This code path is not executed normally.
> 
> Oh, I read it the other way around, but it is supposed to mask soft
> exceptions/irqs (clearing *.injected is missing then).
> 
> > 
> >> What is the purpose of keeping event_exit_inst_len around? Either we
> >> need it also across user space exists, then we have to save/restore or
> >> reconstruct it, or we don't need it, then simply drop it.
> >>
> > Why we need to save/restore is if we need it across user space exits?
> > We need to save/restore it only if we nedd it across migration.
> > 
> > When exception happens during soft interrupt/exception delivery soft i/e
> > should be retried somehow. There are two ways to do that. First one is just
> > reenter guest with the same rip. Instruction will be reexecuted and
> > event redelivered. Another is to reinject event via event reinjection
> > mechanism and for that we need to tell CPU how to calculate rip of a next
> > instruction and this is done by providing event_exit_inst_len. The
> 
> But I still fail to see the case where event_exit_inst_len is set to
> anything but 1 or 2 and where it is related to anything else than exits
> at INT3, INT X, or INTO.
> 
You can't know real instruction length without decoding it or relying on
VMX exit info. What if prefix were used for INT X?

> > problem is that SVM supports only the first way. Intel advised us to use
> > reinjection mechanism, so that what we use on VMX, but since migration
> > can happen from Intel to AMD and vice versa we chose to reexecute
> > instruction after migration on those rare occasions that migration
> > happens exactly after intercepted soft i/e.
> 
> Jan
> 



--
			Gleb.

Re: [PATCH] KVM: x86: Add instruction length to VCPU event state

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2864 bytes --]

Gleb Natapov wrote:
> On Sat, Feb 13, 2010 at 08:20:41PM +0100, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Sat, Feb 13, 2010 at 07:41:35PM +0100, Jan Kiszka wrote:
>>>> Gleb Natapov wrote:
>>>>> On Sat, Feb 13, 2010 at 06:49:44PM +0100, Jan Kiszka wrote:
>>>>>> Gleb Natapov wrote:
>>>>>>> On Sat, Feb 13, 2010 at 10:51:40AM +0100, Jan Kiszka wrote:
>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>
>>>>>>>> VMX requires a properly set instruction length VM entry field when
>>>>>>>> trying to inject soft exception and interrupts. We have to preserve this
>>>>>>>> state across VM save/restore to avoid breaking the re-injection of such
>>>>>>>> events on Intel. So add it to the new VCPU event state.
>>>>>>>>
>>>>>>> We shouldn't re-inject soft exceptions/interrupts after migration, but
>>>>>>> re-execute instruction instead. Instruction length field doesn't exist
>>>>>>> on SVM and migration shouldn't expose implementation details.
>>>>>>>
>>>>>> Hmm, then I guess this totally untested patch should fly:
>>>>>>
>>>>> I don't understand what problem are you trying to solve by your patch.
>>>>> During normal operation event_exit_inst_len will be set to correct
>>>>> value. After migration rip will point to int instruction an no even will
>>>>> be pending at all. Here is the patch:
>>>> The patch will cause an endless loop if BP interception is enabled.
>>>>
>>> How? This code path is not executed normally.
>> Oh, I read it the other way around, but it is supposed to mask soft
>> exceptions/irqs (clearing *.injected is missing then).
>>
>>>> What is the purpose of keeping event_exit_inst_len around? Either we
>>>> need it also across user space exists, then we have to save/restore or
>>>> reconstruct it, or we don't need it, then simply drop it.
>>>>
>>> Why we need to save/restore is if we need it across user space exits?
>>> We need to save/restore it only if we nedd it across migration.
>>>
>>> When exception happens during soft interrupt/exception delivery soft i/e
>>> should be retried somehow. There are two ways to do that. First one is just
>>> reenter guest with the same rip. Instruction will be reexecuted and
>>> event redelivered. Another is to reinject event via event reinjection
>>> mechanism and for that we need to tell CPU how to calculate rip of a next
>>> instruction and this is done by providing event_exit_inst_len. The
>> But I still fail to see the case where event_exit_inst_len is set to
>> anything but 1 or 2 and where it is related to anything else than exits
>> at INT3, INT X, or INTO.
>>
> You can't know real instruction length without decoding it or relying on
> VMX exit info. What if prefix were used for INT X?
> 

OK, makes sense now. Then let's go with your suggestion, will post it as
a patch.

Thanks for explaining,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] KVM: x86: Add instruction length to VCPU event state

[Paolo Bonzini]

On 02/13/2010 08:20 PM, Jan Kiszka wrote:
> But I still fail to see the case where event_exit_inst_len is set to
> anything but 1 or 2 and where it is related to anything else than exits
> at INT3, INT X, or INTO.

What about BOUND?  (I want to hide for suggesting anyone uses that 
instruction).

Paolo

Re: [PATCH] KVM: x86: Add instruction length to VCPU event state

[Gleb Natapov]

On Sun, Feb 14, 2010 at 02:44:39PM +0100, Paolo Bonzini wrote:
> On 02/13/2010 08:20 PM, Jan Kiszka wrote:
> >But I still fail to see the case where event_exit_inst_len is set to
> >anything but 1 or 2 and where it is related to anything else than exits
> >at INT3, INT X, or INTO.
> 
> What about BOUND?  (I want to hide for suggesting anyone uses that
> instruction).
> 
It is fault not a trap IIRC.

--
			Gleb.

Re: [PATCH] KVM: x86: Add instruction length to VCPU event state

[Avi Kivity]

On 02/14/2010 03:44 PM, Paolo Bonzini wrote:
> On 02/13/2010 08:20 PM, Jan Kiszka wrote:
>> But I still fail to see the case where event_exit_inst_len is set to
>> anything but 1 or 2 and where it is related to anything else than exits
>> at INT3, INT X, or INTO.
>
> What about BOUND?  (I want to hide for suggesting anyone uses that 
> instruction). 

If compilers used it, software would run slower, but buffer overflows 
would be much rarer.

-- 
error compiling committee.c: too many arguments to function