KVM: x86: Add instruction length to VCPU event state

From: Jan Kiszka<[email protected]>

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.

Quoted 8 lines · Jan Kiszka

From: Jan Kiszka<[email protected]>

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.

From: Jan Kiszka <[email protected]>

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 <[email protected]>

---

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(-)

Documentation/kvm/api.txt +2 -0

index c6416a3..aa11d70 100644
@@ -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

arch/x86/include/asm/kvm.h +2 -1

index f46b79f..570b6cc 100644
@@ -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 */

arch/x86/kvm/x86.c +2 -0

index 86b739f..0cc6cfb 100644
@@ -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

Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

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.

Quoted 8 lines · Gleb Natapov

Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

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.

Quoted 4 lines

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:

Quoted 13 lines · Jan Kiszka

Quoted 12 lines · Gleb Natapov

Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

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.

Quoted 2 lines

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:

Quoted 14 lines · Gleb Natapov

Quoted 13 lines · Jan Kiszka

Quoted 12 lines · Gleb Natapov

Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 17 lines · Jan Kiszka

Quoted 16 lines · Gleb Natapov

Quoted 15 lines · Jan Kiszka

Quoted 12 lines · Gleb Natapov

Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

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:

Quoted 6 lines

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 18 lines · Gleb Natapov

Quoted 17 lines · Jan Kiszka

Quoted 16 lines · Gleb Natapov

Quoted 15 lines · Jan Kiszka

Quoted 12 lines · Gleb Natapov

Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

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:

Quoted 7 lines

Quoted 6 lines

Quoted 4 lines

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).

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 5 lines

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

Quoted 42 lines · Jan Kiszka

Quoted 18 lines · Gleb Natapov

Quoted 17 lines · Jan Kiszka

Quoted 16 lines · Gleb Natapov

Quoted 15 lines · Jan Kiszka

Quoted 12 lines · Gleb Natapov

Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

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:

Quoted 7 lines

Quoted 6 lines

Quoted 4 lines

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).

Quoted 13 lines

Quoted 4 lines

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

Quoted 4 lines

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?

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.

Quoted 3 lines · Jan Kiszka

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).

Quoted 3 lines · Jan Kiszka

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).