Handle monitor trap flag during instruction emulation
v2 cover Handle monitor trap flag during instruction emulation
v1: http://lore.kernel.org/r/[email protected]
v1 => v2:
- Don't split the #DB delivery by vendors. Unconditionally injecting #DB payloads into the 'pending debug exceptions' field will cause KVM to get stuck in a loop. Per the SDM, when hardware injects an event resulting from this field's value, it is checked against the exception interception bitmap.
- Address Sean's comments by injecting the VM-exit into L1 from vmx_check_nested_events().
- Added fix for nested INIT VM-exits + 'pending debug exceptions' field as it was noticed in implementing v2.
- Drop Peter + Jim's Reviewed-by tags, as the patch set has changed since v1.
KVM already provides guests the ability to use the 'monitor trap flag' VM-execution control. Support for this flag is provided by the fact that KVM unconditionally forwards MTF VM-exits to the guest (if requested), as KVM doesn't utilize MTF. While this provides support during hardware instruction execution, it is insufficient for instruction emulation.
Should L0 emulate an instruction on the behalf of L2, L0 should also synthesize an MTF VM-exit into L1, should control be set.
The first patch corrects a nuanced difference between the definition of a #DB exception payload field and DR6 register. Mask off bit 12 which is defined in the 'pending debug exceptions' field when applying to DR6, since the payload field is said to be compatible with the aforementioned VMCS field.
The second patch sets the 'pending debug exceptions' VMCS field when delivering an INIT signal VM-exit to L1, as described in the SDM. This patch also introduces helpers for setting the 'pending debug exceptions' VMCS field.
The third patch massages KVM's handling of exception payloads with regard to API compatibility. Rather than immediately injecting the payload w/o opt-in, instead defer the payload + immediately inject before completing a KVM_GET_VCPU_EVENTS. This maintains API compatibility whilst correcting #DB behavior with regard to higher priority VM-exit events.
Fourth patch introduces MTF implementation for emulated instructions. Identify if an MTF is due on an instruction boundary from kvm_vcpu_do_singlestep(), however only deliver this VM-exit from vmx_check_nested_events() to respect the relative prioritization to other VM-exits. Since this augments the nested state, introduce a new flag for (de)serialization.
Last patch adds tests to kvm-unit-tests to assert the correctness of MTF under several conditions (concurrent #DB trap, #DB fault, etc). These tests pass under virtualization with this series as well as on bare-metal.
Oliver Upton (4):
KVM: x86: Mask off reserved bit from #DB exception payload
KVM: nVMX: Handle pending #DB when injecting INIT VM-exit
KVM: x86: Deliver exception payload on KVM_GET_VCPU_EVENTS
KVM: nVMX: Emulate MTF when performing instruction emulation
arch/x86/include/asm/kvm_host.h | 1 + arch/x86/include/uapi/asm/kvm.h | 1 + arch/x86/kvm/svm.c | 1 + arch/x86/kvm/vmx/nested.c | 60 ++++++++++++++++++++++++++++++++- arch/x86/kvm/vmx/nested.h | 5 +++ arch/x86/kvm/vmx/vmx.c | 22 ++++++++++++ arch/x86/kvm/vmx/vmx.h | 3 ++ arch/x86/kvm/x86.c | 52 +++++++++++++++++----------- 8 files changed, 125 insertions(+), 20 deletions(-)
Signature · 1 line
2.25.0.341.g760bfbb309-goog
PATCH v2 1/5 KVM: x86: Mask off reserved bit from #DB exception payload
KVM defines the #DB payload as compatible with the 'pending debug exceptions' field under VMX, not DR6. Mask off bit 12 when applying the payload to DR6, as it is reserved on DR6 but not the 'pending debug exceptions' field.
--- arch/x86/kvm/x86.c | 8 ++++++++ 1 file changed, 8 insertions(+)
arch/x86/kvm/x86.c +8 -0
@@ -437,6 +437,14 @@ void kvm_deliver_exception_payload(struct kvm_vcpu *vcpu)* for #DB exceptions under VMX. */ vcpu->arch.dr6 ^= payload &DR6_RTM ; + +/* +* The #DB payload is defined as compatible with the 'pending +* debug exceptions' field under VMX, not DR6. While bit 12 is +* defined in the 'pending debug exceptions' field (enabled +* breakpoint), it is reserved and must be zero in DR6. +*/ + vcpu->arch.dr6 &= ~BIT (12 );break ;case PF_VECTOR : vcpu->arch.cr2 = payload;
Signature · 1 line
2.25.0.341.g760bfbb309-goog
PATCH v2 2/5 KVM: nVMX: Handle pending #DB when injecting INIT VM-exit
In reply to Oliver UptonSDM 27.3.4 states that the 'pending debug exceptions' VMCS field will be populated if a VM-exit caused by an INIT signal takes priority over a debug-trap. Emulate this behavior when synthesizing an INIT signal VM-exit into L1.
--- arch/x86/kvm/vmx/nested.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
arch/x86/kvm/vmx/nested.c +23 -0
@@ -3572,6 +3572,27 @@ static void nested_vmx_inject_exception_vmexit(struct kvm_vcpu *vcpu,nested_vmx_vmexit (vcpu,EXIT_REASON_EXCEPTION_NMI , intr_info, exit_qual); } +static inline bool nested_vmx_check_pending_dbg (struct kvm_vcpu *vcpu) +{ +return vcpu->arch.exception.nr ==DB_VECTOR && + vcpu->arch.exception.pending && + vcpu->arch.exception.has_payload; +} + +/* +* If a higher priority VM-exit is delivered before a debug-trap, hardware will +* set the 'pending debug exceptions' field appropriately for reinjection on the +* next VM-entry. +*/ +static void nested_vmx_set_pending_dbg (struct kvm_vcpu *vcpu) +{ +vmcs_writel (GUEST_PENDING_DBG_EXCEPTIONS , vcpu->arch.exception.payload); + vcpu->arch.exception.has_payload = false; + vcpu->arch.exception.payload =0 ; + vcpu->arch.exception.pending = false; + vcpu->arch.exception.injected = true; +} +static int vmx_check_nested_events (struct kvm_vcpu *vcpu,bool external_intr) {struct vcpu_vmx *vmx =to_vmx (vcpu);
@@ -3584,6 +3605,8 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr) test_bit(KVM_APIC_INIT, &apic->pending_events)) {if (block_nested_events)return -EBUSY ; +if (nested_vmx_check_pending_dbg (vcpu)) +nested_vmx_set_pending_dbg (vcpu);clear_bit (KVM_APIC_INIT , &apic->pending_events);nested_vmx_vmexit (vcpu,EXIT_REASON_INIT_SIGNAL ,0 ,0 );return 0 ;
Signature · 1 line
2.25.0.341.g760bfbb309-goog
Re: [PATCH v2 2/5] KVM: nVMX: Handle pending #DB when injecting INIT VM-exit
Quoted 16 lines · Oliver Upton
SDM 27.3.4 states that the 'pending debug exceptions' VMCS field will be populated if a VM-exit caused by an INIT signal takes priority over a debug-trap. Emulate this behavior when synthesizing an INIT signal VM-exit into L1.
Fixes 558b8d50dbff ("KVM: x86: Fix INIT signal handling in various CPU states")Signed-off-by Oliver Upton <[email protected]>--- arch/x86/kvm/vmx/nested.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)arch/x86/kvm/vmx/nested.c +0
-0
arch/x86/kvm/vmx/nested.c static void nested_vmx_inject_exception_vmexit(struct kvm_vcpu *vcpu, · line 3575
Quoted 4 lines
arch/x86/kvm/vmx/nested.c +1
-0@@ -3572,6 +3572,27 @@ static void nested_vmx_inject_exception_vmexit(struct kvm_vcpu *vcpu,nested_vmx_vmexit (vcpu,EXIT_REASON_EXCEPTION_NMI , intr_info, exit_qual); } +static inline bool nested_vmx_check_pending_dbg (struct kvm_vcpu *vcpu)
Really dislike the name, partially because the code checks @has_payload and partially because the part, nested_vmx_set_pending_dbg() "sets" completely different state than this checks.
Checking has_payload may also be wrong, e.g. wouldn't it make sense to update GUEST_PENDING_DBG_EXCEPTIONS, even if we crush it with '0'?
Quoted 8 lines
arch/x86/kvm/vmx/nested.c +8
-0+{ +return vcpu->arch.exception.nr ==DB_VECTOR && + vcpu->arch.exception.pending && + vcpu->arch.exception.has_payload; +} + +/* +* If a higher priority VM-exit is delivered before a debug-trap, hardware will
arch/x86/kvm/vmx/nested.c static void nested_vmx_inject_exception_vmexit(struct kvm_vcpu *vcpu, · line 3593
Quoted 10 lines
arch/x86/kvm/vmx/nested.c +10
-0+* set the 'pending debug exceptions' field appropriately for reinjection on the +* next VM-entry. +*/ +static void nested_vmx_set_pending_dbg (struct kvm_vcpu *vcpu) +{ +vmcs_writel (GUEST_PENDING_DBG_EXCEPTIONS , vcpu->arch.exception.payload); + vcpu->arch.exception.has_payload = false; + vcpu->arch.exception.payload =0 ; + vcpu->arch.exception.pending = false; + vcpu->arch.exception.injected = true;
This looks wrong. The #DB hasn't been injected, KVM is simply emulating the side effect of the VMCS field being updated. E.g. KVM will have different architecturally visible behavior depending on @has_payload.
It's not KVM's responsibility to update the pending exceptions, e.g. if L1 wants to emulate INIT for L2 then it needs to purge the VMCS fields that are affected by INIT.
arch/x86/kvm/vmx/nested.c static int vmx_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr) · line 3609
Quoted 11 lines
arch/x86/kvm/vmx/nested.c +4
-0+} +static int vmx_check_nested_events (struct kvm_vcpu *vcpu,bool external_intr) {struct vcpu_vmx *vmx =to_vmx (vcpu);@@ -3584,6 +3605,8 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr) test_bit(KVM_APIC_INIT, &apic->pending_events)) {if (block_nested_events)return -EBUSY ; +if (nested_vmx_check_pending_dbg (vcpu)) +nested_vmx_set_pending_dbg (vcpu);
I'd strongly prefer to open code the whole thing. Alternatively, if there are other events that will need similar logic, add a single helper to take care of everything.
E.g.
if (vcpu->arch.exception.pending && vcpu->arch.exception.nr == DB_VECTOR && vcpu->arch.exception.has_payload) vmcs_writel(GUEST_PENDING_DBG_EXCEPTIONS, vcpu->arch.exception.payload);
or
if (vcpu->arch.exception.pending &&
vcpu->arch.exception.nr == DB_VECTOR)
vmcs_writel(GUEST_PENDING_DBG_EXCEPTIONS,
vcpu->arch.exception.payload);or
nested_vmx_update_guest_pending_dbg();
Quoted 6 lines
arch/x86/kvm/vmx/nested.c +0
-0clear_bit (KVM_APIC_INIT , &apic->pending_events);nested_vmx_vmexit (vcpu,EXIT_REASON_INIT_SIGNAL ,0 ,0 );return 0 ;Signature · 1 line
2.25.0.341.g760bfbb309-goog
Re: [PATCH v2 2/5] KVM: nVMX: Handle pending #DB when injecting INIT VM-exit
Quoted 43 lines · Sean Christopherson
Quoted 16 lines · Oliver Upton
SDM 27.3.4 states that the 'pending debug exceptions' VMCS field will be populated if a VM-exit caused by an INIT signal takes priority over a debug-trap. Emulate this behavior when synthesizing an INIT signal VM-exit into L1.
Fixes 558b8d50dbff ("KVM: x86: Fix INIT signal handling in various CPU states")Signed-off-by Oliver Upton <[email protected]>--- arch/x86/kvm/vmx/nested.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)arch/x86/kvm/vmx/nested.c +0
-0arch/x86/kvm/vmx/nested.c static void nested_vmx_inject_exception_vmexit(struct kvm_vcpu *vcpu, · line 3575
Quoted 4 lines
arch/x86/kvm/vmx/nested.c +1
-0@@ -3572,6 +3572,27 @@ static void nested_vmx_inject_exception_vmexit(struct kvm_vcpu *vcpu,nested_vmx_vmexit (vcpu,EXIT_REASON_EXCEPTION_NMI , intr_info, exit_qual); } +static inline bool nested_vmx_check_pending_dbg (struct kvm_vcpu *vcpu)Really dislike the name, partially because the code checks @has_payload and partially because the part,
nested_vmx_set_pending_dbg()"sets" completely different state than this checks.Checking
has_payloadmay also be wrong, e.g. wouldn't it make sense to updateGUEST_PENDING_DBG_EXCEPTIONS, even if we crush it with '0'?Quoted 14 lines
arch/x86/kvm/vmx/nested.c +14
-0+{ +return vcpu->arch.exception.nr ==DB_VECTOR && + vcpu->arch.exception.pending && + vcpu->arch.exception.has_payload; +} + +/* +* If a higher priority VM-exit is delivered before a debug-trap, hardware will +* set the 'pending debug exceptions' field appropriately for reinjection on the +* next VM-entry. +*/ +static void nested_vmx_set_pending_dbg (struct kvm_vcpu *vcpu) +{ +vmcs_writel (GUEST_PENDING_DBG_EXCEPTIONS , vcpu->arch.exception.payload);
Quoted 8 lines
arch/x86/kvm/vmx/nested.c static void nested_vmx_inject_exception_vmexit(struct kvm_vcpu *vcpu, · line 3593
Quoted 4 lines
arch/x86/kvm/vmx/nested.c +4
-0+ vcpu->arch.exception.has_payload = false; + vcpu->arch.exception.payload =0 ; + vcpu->arch.exception.pending = false; + vcpu->arch.exception.injected = true;This looks wrong. The #DB hasn't been injected, KVM is simply emulating the side effect of the VMCS field being updated. E.g. KVM will have different architecturally visible behavior depending on @has_payload.
My head is spinning trying to work through the #DB/MTF interactions. I think this ends up being a moot point because prepare_vmcs12() will purge the pending exceptions. If it is a moot point, then I'd prefer to not do the explicit arch.exception updates so as to keep this similar to other exceptions.
PATCH v2 3/5 KVM: x86: Deliver exception payload on KVM_GET_VCPU_EVENTS
In reply to Oliver UptonKVM doesn't utilize exception payloads by default, as this behavior diverges from the expectations of the userspace API. However, this constraint only applies if KVM is servicing a KVM_GET_VCPU_EVENTS ioctl before delivering the exception.
Use exception payloads unconditionally if the vcpu is in guest mode. Deliver the exception payload before completing a KVM_GET_VCPU_EVENTS to ensure API compatibility.
--- arch/x86/kvm/x86.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-)
arch/x86/kvm/x86.c +16 -13
@@ -497,19 +497,7 @@ static void kvm_multiple_exception(struct kvm_vcpu *vcpu, vcpu->arch.exception.error_code = error_code; vcpu->arch.exception.has_payload = has_payload; vcpu->arch.exception.payload = payload; -/* -* In guest mode, payload delivery should be deferred, -* so that the L1 hypervisor can intercept #PF before -* CR2 is modified (or intercept #DB before DR6 is -* modified under nVMX). However, for ABI -* compatibility with KVM_GET_VCPU_EVENTS and -* KVM_SET_VCPU_EVENTS, we can't delay payload -* delivery unless userspace has enabled this -* functionality via the per-VM capability, -* KVM_CAP_EXCEPTION_PAYLOAD. -*/ -if (!vcpu->kvm->arch.exception_payload_enabled || - !is_guest_mode (vcpu)) +if (!is_guest_mode (vcpu))kvm_deliver_exception_payload (vcpu);return ; }
@@ -3790,6 +3778,21 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu, {process_nmi (vcpu); +/* +* In guest mode, payload delivery should be deferred, +* so that the L1 hypervisor can intercept #PF before +* CR2 is modified (or intercept #DB before DR6 is +* modified under nVMX). However, for ABI +* compatibility with KVM_GET_VCPU_EVENTS and +* KVM_SET_VCPU_EVENTS, we can't delay payload +* delivery unless userspace has enabled this +* functionality via the per-VM capability, +* KVM_CAP_EXCEPTION_PAYLOAD. +*/ +if (!vcpu->kvm->arch.exception_payload_enabled && + vcpu->arch.exception.pending && vcpu->arch.exception.has_payload) +kvm_deliver_exception_payload (vcpu); +/* * The API doesn't provide the instruction length for software * exceptions, so don't report them. As long as the guest RIP
Signature · 1 line
2.25.0.341.g760bfbb309-goog
Re: [PATCH v2 3/5] KVM: x86: Deliver exception payload on KVM_GET_VCPU_EVENTS
Quoted 6 lines · Oliver Upton
KVM doesn't utilize exception payloads by default, as this behavior diverges from the expectations of the userspace API. However, this constraint only applies if KVM is servicing a
KVM_GET_VCPU_EVENTSioctl before delivering the exception.Use exception payloads unconditionally if the vcpu is in guest mode.
This sentence is super confusing. It doesn't align with the code, which is clearly handling "not is in guest mode". And KVM already uses payloads unconditionally, it's only the deferring behavior that is changing.
Quoted 38 lines
Deliver the exception payload before completing a
KVM_GET_VCPU_EVENTSto ensure API compatibility.Signed-off-by Oliver Upton <[email protected]>--- arch/x86/kvm/x86.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-)arch/x86/kvm/x86.c +2
-13@@ -497,19 +497,7 @@ static void kvm_multiple_exception(struct kvm_vcpu *vcpu, vcpu->arch.exception.error_code = error_code; vcpu->arch.exception.has_payload = has_payload; vcpu->arch.exception.payload = payload; -/* -* In guest mode, payload delivery should be deferred, -* so that the L1 hypervisor can intercept #PF before -* CR2 is modified (or intercept #DB before DR6 is -* modified under nVMX). However, for ABI -* compatibility with KVM_GET_VCPU_EVENTS and -* KVM_SET_VCPU_EVENTS, we can't delay payload -* delivery unless userspace has enabled this -* functionality via the per-VM capability, -* KVM_CAP_EXCEPTION_PAYLOAD. -*/ -if (!vcpu->kvm->arch.exception_payload_enabled || - !is_guest_mode (vcpu)) +if (!is_guest_mode (vcpu))kvm_deliver_exception_payload (vcpu);return ; }@@ -3790,6 +3778,21 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu, {process_nmi (vcpu); +/*
arch/x86/kvm/x86.c static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu, · line 3791
Quoted 10 lines
arch/x86/kvm/x86.c +10
-0+* In guest mode, payload delivery should be deferred, +* so that the L1 hypervisor can intercept #PF before +* CR2 is modified (or intercept #DB before DR6 is +* modified under nVMX). However, for ABI +* compatibility with KVM_GET_VCPU_EVENTS and +* KVM_SET_VCPU_EVENTS, we can't delay payload +* delivery unless userspace has enabled this +* functionality via the per-VM capability, +* KVM_CAP_EXCEPTION_PAYLOAD. +*/
This comment needs to be rewritten. It makes sense in the context of kvm_multiple_exception(), here it's just confusing.
arch/x86/kvm/x86.c static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu, · line 3794
Quoted 3 lines
arch/x86/kvm/x86.c +3
-0+if (!vcpu->kvm->arch.exception_payload_enabled && + vcpu->arch.exception.pending && vcpu->arch.exception.has_payload) +kvm_deliver_exception_payload (vcpu);
Crushing CR2/DR6 just because userspace is retrieving info can't possibly be correct. If it somehow is correct then this needs big fat comment.
What is the ABI compatibility issue? E.g. can KVM simply hide the payload info on KVM_GET_VCPU_EVENT and then drop it on KVM_SET_VCPU_EVENTS?
Quoted 7 lines
arch/x86/kvm/x86.c +1
-0+/* * The API doesn't provide the instruction length for software * exceptions, so don't report them. As long as the guest RIP Signature · 1 line
2.25.0.341.g760bfbb309-goog
PATCH v2 4/5 KVM: nVMX: Emulate MTF when performing instruction emulation
In reply to Oliver UptonSince commit 5f3d45e7f282 ("kvm/x86: add support for MONITOR_TRAP_FLAG"), KVM has allowed an L1 guest to use the monitor trap flag processor-based execution control for its L2 guest. KVM simply forwards any MTF VM-exits to the L1 guest, which works for normal instruction execution.
However, when KVM needs to emulate an instruction on the behalf of an L2 guest, the monitor trap flag is not emulated. Add the necessary logic to kvm_skip_emulated_instruction() to synthesize an MTF VM-exit to L1 upon instruction emulation for L2.
--- arch/x86/include/asm/kvm_host.h | 1 + arch/x86/include/uapi/asm/kvm.h | 1 + arch/x86/kvm/svm.c | 1 + arch/x86/kvm/vmx/nested.c | 37 ++++++++++++++++++++++++++++++++- arch/x86/kvm/vmx/nested.h | 5 +++++ arch/x86/kvm/vmx/vmx.c | 22 ++++++++++++++++++++ arch/x86/kvm/vmx/vmx.h | 3 +++ arch/x86/kvm/x86.c | 15 +++++++------ 8 files changed, 78 insertions(+), 7 deletions(-)
arch/x86/include/asm/kvm_host.h +1 -0
@@ -1103,6 +1103,7 @@ struct kvm_x86_ops {int (*handle_exit)(struct kvm_vcpu *vcpu,enum exit_fastpath_completion exit_fastpath);int (*skip_emulated_instruction)(struct kvm_vcpu *vcpu); +void (*do_singlestep)(struct kvm_vcpu *vcpu);void (*set_interrupt_shadow)(struct kvm_vcpu *vcpu,int mask);u32 (*get_interrupt_shadow)(struct kvm_vcpu *vcpu); void (*patch_hypercall)(struct kvm_vcpu *vcpu,
arch/x86/include/uapi/asm/kvm.h +1 -0
@@ -390,6 +390,7 @@ struct kvm_sync_regs {#define KVM_STATE_NESTED_GUEST_MODE 0x00000001#define KVM_STATE_NESTED_RUN_PENDING 0x00000002#define KVM_STATE_NESTED_EVMCS 0x00000004 +#define KVM_STATE_NESTED_MTF_PENDING 0x00000008#define KVM_STATE_NESTED_SMM_GUEST_MODE 0x00000001#define KVM_STATE_NESTED_SMM_VMXON 0x00000002
arch/x86/kvm/svm.c +1 -0
@@ -7316,6 +7316,7 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = { .run = svm_vcpu_run, .handle_exit = handle_exit, .skip_emulated_instruction = skip_emulated_instruction, + .do_singlestep = NULL, .set_interrupt_shadow = svm_set_interrupt_shadow, .get_interrupt_shadow = svm_get_interrupt_shadow, .patch_hypercall = svm_patch_hypercall,
arch/x86/kvm/vmx/nested.c +36 -1
@@ -3599,8 +3599,15 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr)unsigned long exit_qual;bool block_nested_events = vmx->nested.nested_run_pending ||kvm_event_needs_reinjection (vcpu); +bool mtf_pending = vmx->nested.mtf_pending;struct kvm_lapic *apic = vcpu->arch.apic; +/* +* Clear the MTF state. If a higher priority VM-exit is delivered first, +* this state is discarded. +*/ + vmx->nested.mtf_pending = false; +if (lapic_in_kernel (vcpu) &&test_bit (KVM_APIC_INIT , &apic->pending_events)) {if (block_nested_events)
@@ -3612,8 +3619,30 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr)return 0 ; } +/* +* Process non-debug exceptions first before MTF. +*/ if (vcpu->arch.exception.pending && -nested_vmx_check_exception (vcpu, &exit_qual)) { + !nested_vmx_check_pending_dbg (vcpu) && +nested_vmx_check_exception (vcpu, &exit_qual)) { +if (block_nested_events) +return -EBUSY ; +nested_vmx_inject_exception_vmexit (vcpu, exit_qual); +return 0 ; + } + +if (mtf_pending) { +if (block_nested_events) +return -EBUSY ; +if (nested_vmx_check_pending_dbg (vcpu)) +nested_vmx_set_pending_dbg (vcpu); +nested_vmx_vmexit (vcpu,EXIT_REASON_MONITOR_TRAP_FLAG ,0 ,0 ); +return 0 ; + } + +if (vcpu->arch.exception.pending && +nested_vmx_check_pending_dbg (vcpu) && +nested_vmx_check_exception (vcpu, &exit_qual)) {if (block_nested_events)return -EBUSY ;nested_vmx_inject_exception_vmexit (vcpu, exit_qual);
@@ -5705,6 +5734,9 @@ static int vmx_get_nested_state(struct kvm_vcpu *vcpu,if (vmx->nested.nested_run_pending) kvm_state.flags |=KVM_STATE_NESTED_RUN_PENDING ; + +if (vmx->nested.mtf_pending) + kvm_state.flags |=KVM_STATE_NESTED_MTF_PENDING ; } }
@@ -5885,6 +5917,9 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, vmx->nested.nested_run_pending = !!(kvm_state->flags &KVM_STATE_NESTED_RUN_PENDING ); + vmx->nested.mtf_pending = + !!(kvm_state->flags &KVM_STATE_NESTED_MTF_PENDING ); + ret = -EINVAL ;if (nested_cpu_has_shadow_vmcs (vmcs12) && vmcs12->vmcs_link_pointer !=-1ull ) {
arch/x86/kvm/vmx/nested.h +5 -0
@@ -175,6 +175,11 @@ static inline bool nested_cpu_has_virtual_nmis(struct vmcs12 *vmcs12)return vmcs12->pin_based_vm_exec_control &PIN_BASED_VIRTUAL_NMIS ; } +static inline int nested_cpu_has_mtf (struct vmcs12 *vmcs12) +{ +return nested_cpu_has (vmcs12,CPU_BASED_MONITOR_TRAP_FLAG ); +} +static inline int nested_cpu_has_ept (struct vmcs12 *vmcs12) {return nested_cpu_has2 (vmcs12,SECONDARY_EXEC_ENABLE_EPT );
arch/x86/kvm/vmx/vmx.c +22 -0
@@ -1601,6 +1601,27 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)return 1 ; } +static void vmx_do_singlestep (struct kvm_vcpu *vcpu) +{ +struct vcpu_vmx *vmx; + +if (!(is_guest_mode (vcpu) && +nested_cpu_has_mtf (get_vmcs12 (vcpu)))) +return ; + + vmx =to_vmx (vcpu); + +/* +* Per the SDM, MTF takes priority over debug-trap exception besides +* T-bit traps. As instruction emulation is completed (i.e. at the end +* of an instruction boundary), any #DB exception pending delivery must +* be a debug-trap. Record the pending MTF state to be delivered in +* vmx_check_nested_events(). +*/ + vmx->nested.mtf_pending = (!vcpu->arch.exception.pending || + vcpu->arch.exception.nr ==DB_VECTOR ); +} +static void vmx_clear_hlt (struct kvm_vcpu *vcpu) {/*
@@ -7797,6 +7818,7 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = { .run = vmx_vcpu_run, .handle_exit = vmx_handle_exit, .skip_emulated_instruction = skip_emulated_instruction, + .do_singlestep = vmx_do_singlestep, .set_interrupt_shadow = vmx_set_interrupt_shadow, .get_interrupt_shadow = vmx_get_interrupt_shadow, .patch_hypercall = vmx_patch_hypercall,
arch/x86/kvm/vmx/vmx.h +3 -0
@@ -150,6 +150,9 @@ struct nested_vmx {/* L2 must run next, and mustn't decide to exit to L1. */ bool nested_run_pending; +/* Pending MTF VM-exit into L1. */ +bool mtf_pending; +struct loaded_vmcs vmcs02;/*
arch/x86/kvm/x86.c +9 -6
@@ -6626,10 +6626,15 @@ static int kvm_vcpu_check_hw_bp(unsigned long addr, u32 type, u32 dr7,return dr6; } -static int kvm_vcpu_do_singlestep (struct kvm_vcpu *vcpu) +static int kvm_vcpu_do_singlestep (struct kvm_vcpu *vcpu,bool tf) {struct kvm_run *kvm_run = vcpu->run; +if (kvm_x86_ops->do_singlestep) + kvm_x86_ops->do_singlestep (vcpu); +if (!tf) +return 1 ; +if (vcpu->guest_debug &KVM_GUESTDBG_SINGLESTEP ) { kvm_run->debug.arch.dr6 =DR6_BS |DR6_FIXED_1 |DR6_RTM ; kvm_run->debug.arch.pc = vcpu->arch.singlestep_rip;
@@ -6658,9 +6663,7 @@ int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu)* processor will not generate this exception after the instruction * that sets the TF flag". */ -if (unlikely (rflags &X86_EFLAGS_TF )) - r =kvm_vcpu_do_singlestep (vcpu); -return r; +return kvm_vcpu_do_singlestep (vcpu, rflags &X86_EFLAGS_TF ); }EXPORT_SYMBOL_GPL (kvm_skip_emulated_instruction);
@@ -6876,8 +6879,8 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,if (!ctxt->have_exception ||exception_type (ctxt->exception.vector) ==EXCPT_TRAP ) {kvm_rip_write (vcpu, ctxt->eip); -if (r && ctxt->tf) - r =kvm_vcpu_do_singlestep (vcpu); +if (r) + r =kvm_vcpu_do_singlestep (vcpu, ctxt->tf);__kvm_set_rflags (vcpu, ctxt->eflags); }
Signature · 1 line
2.25.0.341.g760bfbb309-goog
Re: [PATCH v2 4/5] KVM: nVMX: Emulate MTF when performing instruction emulation
Quoted 81 lines · Oliver Upton
Since commit
5f3d45e7f282("kvm/x86: add support forMONITOR_TRAP_FLAG"), KVM has allowed an L1 guest to use the monitor trap flag processor-based execution control for its L2 guest. KVM simply forwards any MTF VM-exits to the L1 guest, which works for normal instruction execution.However, when KVM needs to emulate an instruction on the behalf of an L2 guest, the monitor trap flag is not emulated. Add the necessary logic to
kvm_skip_emulated_instruction()to synthesize an MTF VM-exit to L1 upon instruction emulation for L2.Fixes 5f3d45e7f282 ("kvm/x86: add support for MONITOR_TRAP_FLAG")Signed-off-by Oliver Upton <[email protected]>--- arch/x86/include/asm/kvm_host.h | 1 + arch/x86/include/uapi/asm/kvm.h | 1 + arch/x86/kvm/svm.c | 1 + arch/x86/kvm/vmx/nested.c | 37 ++++++++++++++++++++++++++++++++- arch/x86/kvm/vmx/nested.h | 5 +++++ arch/x86/kvm/vmx/vmx.c | 22 ++++++++++++++++++++ arch/x86/kvm/vmx/vmx.h | 3 +++ arch/x86/kvm/x86.c | 15 +++++++------ 8 files changed, 78 insertions(+), 7 deletions(-)arch/x86/include/asm/kvm_host.h +1
-0@@ -1103,6 +1103,7 @@ struct kvm_x86_ops {int (*handle_exit)(struct kvm_vcpu *vcpu,enum exit_fastpath_completion exit_fastpath);int (*skip_emulated_instruction)(struct kvm_vcpu *vcpu); +void (*do_singlestep)(struct kvm_vcpu *vcpu);void (*set_interrupt_shadow)(struct kvm_vcpu *vcpu,int mask);u32 (*get_interrupt_shadow)(struct kvm_vcpu *vcpu); void (*patch_hypercall)(struct kvm_vcpu *vcpu,arch/x86/include/uapi/asm/kvm.h +1
-0@@ -390,6 +390,7 @@ struct kvm_sync_regs {#define KVM_STATE_NESTED_GUEST_MODE 0x00000001#define KVM_STATE_NESTED_RUN_PENDING 0x00000002#define KVM_STATE_NESTED_EVMCS 0x00000004 +#define KVM_STATE_NESTED_MTF_PENDING 0x00000008#define KVM_STATE_NESTED_SMM_GUEST_MODE 0x00000001#define KVM_STATE_NESTED_SMM_VMXON 0x00000002arch/x86/kvm/svm.c +1
-0@@ -7316,6 +7316,7 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = { .run = svm_vcpu_run, .handle_exit = handle_exit, .skip_emulated_instruction = skip_emulated_instruction, + .do_singlestep = NULL, .set_interrupt_shadow = svm_set_interrupt_shadow, .get_interrupt_shadow = svm_get_interrupt_shadow, .patch_hypercall = svm_patch_hypercall,arch/x86/kvm/vmx/nested.c +7
-0@@ -3599,8 +3599,15 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr)unsigned long exit_qual;bool block_nested_events = vmx->nested.nested_run_pending ||kvm_event_needs_reinjection (vcpu); +bool mtf_pending = vmx->nested.mtf_pending;struct kvm_lapic *apic = vcpu->arch.apic; +/* +* Clear the MTF state. If a higher priority VM-exit is delivered first, +* this state is discarded. +*/ + vmx->nested.mtf_pending = false; +if (lapic_in_kernel (vcpu) &&test_bit (KVM_APIC_INIT , &apic->pending_events)) {if (block_nested_events)
arch/x86/kvm/vmx/nested.c static int vmx_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr) · line 3626
Quoted 9 lines
arch/x86/kvm/vmx/nested.c +4
-1@@ -3612,8 +3619,30 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr)return 0 ; } +/* +* Process non-debug exceptions first before MTF. +*/ if (vcpu->arch.exception.pending && -nested_vmx_check_exception (vcpu, &exit_qual)) { + !nested_vmx_check_pending_dbg(vcpu) &&
Oof. So @has_payload is set %true only by single-step #DB and #PF, and by #DBs injected from userspace. Now I understand where the "pending_dbg() comes from.
The part that gets really confusing is that there's "pending" from KVM's perspective, which can be any kind of #DB, e.g. DR7.GD and icebrk, and pending from an architectural perspective, which is single-step #DB and data #DBs, the latter of which isn't manually emulated by KVM (I think?).
Not sure if there's a better name than check_pending_dbg(), but either way I think a function comment is in order.
Quoted 63 lines
arch/x86/kvm/vmx/nested.c +25
-0+ nested_vmx_check_exception(vcpu, &exit_qual)) { +if (block_nested_events) +return -EBUSY ; +nested_vmx_inject_exception_vmexit (vcpu, exit_qual); +return 0 ; + } + +if (mtf_pending) { +if (block_nested_events) +return -EBUSY ; +if (nested_vmx_check_pending_dbg (vcpu)) +nested_vmx_set_pending_dbg (vcpu); +nested_vmx_vmexit (vcpu,EXIT_REASON_MONITOR_TRAP_FLAG ,0 ,0 ); +return 0 ; + } + +if (vcpu->arch.exception.pending && +nested_vmx_check_pending_dbg (vcpu) && +nested_vmx_check_exception (vcpu, &exit_qual)) {if (block_nested_events)return -EBUSY ;nested_vmx_inject_exception_vmexit (vcpu, exit_qual);@@ -5705,6 +5734,9 @@ static int vmx_get_nested_state(struct kvm_vcpu *vcpu,if (vmx->nested.nested_run_pending) kvm_state.flags |=KVM_STATE_NESTED_RUN_PENDING ; + +if (vmx->nested.mtf_pending) + kvm_state.flags |=KVM_STATE_NESTED_MTF_PENDING ; } }@@ -5885,6 +5917,9 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, vmx->nested.nested_run_pending = !!(kvm_state->flags &KVM_STATE_NESTED_RUN_PENDING ); + vmx->nested.mtf_pending = + !!(kvm_state->flags &KVM_STATE_NESTED_MTF_PENDING ); + ret = -EINVAL ;if (nested_cpu_has_shadow_vmcs (vmcs12) && vmcs12->vmcs_link_pointer !=-1ull ) {arch/x86/kvm/vmx/nested.h +5
-0@@ -175,6 +175,11 @@ static inline bool nested_cpu_has_virtual_nmis(struct vmcs12 *vmcs12)return vmcs12->pin_based_vm_exec_control &PIN_BASED_VIRTUAL_NMIS ; } +static inline int nested_cpu_has_mtf (struct vmcs12 *vmcs12) +{ +return nested_cpu_has (vmcs12,CPU_BASED_MONITOR_TRAP_FLAG ); +} +static inline int nested_cpu_has_ept (struct vmcs12 *vmcs12) {return nested_cpu_has2 (vmcs12,SECONDARY_EXEC_ENABLE_EPT );arch/x86/kvm/vmx/vmx.c +0
-0
arch/x86/kvm/vmx/vmx.c static int skip_emulated_instruction(struct kvm_vcpu *vcpu) · line 1609
Quoted 9 lines
arch/x86/kvm/vmx/vmx.c +6
-0@@ -1601,6 +1601,27 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)return 1 ; } +static void vmx_do_singlestep (struct kvm_vcpu *vcpu) +{ +struct vcpu_vmx *vmx; + +if (!(is_guest_mode (vcpu) && +nested_cpu_has_mtf (get_vmcs12 (vcpu))))
Haven't followed all the paths, but does nested.mtf_pending need to be cleared here in case L1 disabled MTF?1
Quoted 62 lines
arch/x86/kvm/vmx/vmx.c +16
-0+return ; + + vmx =to_vmx (vcpu); + +/* +* Per the SDM, MTF takes priority over debug-trap exception besides +* T-bit traps. As instruction emulation is completed (i.e. at the end +* of an instruction boundary), any #DB exception pending delivery must +* be a debug-trap. Record the pending MTF state to be delivered in +* vmx_check_nested_events(). +*/ + vmx->nested.mtf_pending = (!vcpu->arch.exception.pending || + vcpu->arch.exception.nr ==DB_VECTOR ); +} +static void vmx_clear_hlt (struct kvm_vcpu *vcpu) {/* @@ -7797,6 +7818,7 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = { .run = vmx_vcpu_run, .handle_exit = vmx_handle_exit, .skip_emulated_instruction = skip_emulated_instruction, + .do_singlestep = vmx_do_singlestep, .set_interrupt_shadow = vmx_set_interrupt_shadow, .get_interrupt_shadow = vmx_get_interrupt_shadow, .patch_hypercall = vmx_patch_hypercall,arch/x86/kvm/vmx/vmx.h +3
-0@@ -150,6 +150,9 @@ struct nested_vmx {/* L2 must run next, and mustn't decide to exit to L1. */ bool nested_run_pending; +/* Pending MTF VM-exit into L1. */ +bool mtf_pending; +struct loaded_vmcs vmcs02;/* arch/x86/kvm/x86.c +6
-1@@ -6626,10 +6626,15 @@ static int kvm_vcpu_check_hw_bp(unsigned long addr, u32 type, u32 dr7,return dr6; } -static int kvm_vcpu_do_singlestep (struct kvm_vcpu *vcpu) +static int kvm_vcpu_do_singlestep (struct kvm_vcpu *vcpu,bool tf) {struct kvm_run *kvm_run = vcpu->run; +if (kvm_x86_ops->do_singlestep) + kvm_x86_ops->do_singlestep (vcpu); +if (!tf) +return 1 ; +if (vcpu->guest_debug &KVM_GUESTDBG_SINGLESTEP ) { kvm_run->debug.arch.dr6 =DR6_BS |DR6_FIXED_1 |DR6_RTM ; kvm_run->debug.arch.pc = vcpu->arch.singlestep_rip;
arch/x86/kvm/x86.c int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu) · line 6666
Quoted 7 lines
arch/x86/kvm/x86.c +1
-3@@ -6658,9 +6663,7 @@ int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu)* processor will not generate this exception after the instruction * that sets the TF flag". */ -if (unlikely (rflags &X86_EFLAGS_TF )) - r =kvm_vcpu_do_singlestep (vcpu); -return r; +return kvm_vcpu_do_singlestep (vcpu, rflags &X86_EFLAGS_TF );
The extra retpoline, i.e. ->do_singlestep(), can be avoided by handling the kvm_skip_emulated_instruction() purely in VMX:
arch/x86/kvm/vmx/vmx.c +15 -1
@@ -1601,6 +1601,20 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)return 1 ; } +static int vmx_skip_emulated_instruction (struct kvm_vcpu *vcpu) +{ +if (is_guest_mode (vcpu)) { +if (nested_cpu_has_mtf (get_vmcs12 (vcpu) && + (!vcpu->arch.exception.pending || + vcpu->arch.exception.nr ==DB_VECTOR ))) +to_vmx (vcpu)->nested.mtf_pending = true; +else +to_vmx (vcpu)->nested.mtf_pending = false; + } + +return skip_emulated_instruction (vcpu); +} +static void vmx_clear_hlt (struct kvm_vcpu *vcpu) {/*
@@ -7796,7 +7810,7 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = { .run = vmx_vcpu_run, .handle_exit = vmx_handle_exit, - .skip_emulated_instruction = skip_emulated_instruction, + .skip_emulated_instruction = vmx_skip_emulated_instruction, .set_interrupt_shadow = vmx_set_interrupt_shadow, .get_interrupt_shadow = vmx_get_interrupt_shadow, .patch_hypercall = vmx_patch_hypercall,
arch/x86/kvm/x86.c int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, · line 6883
Quoted 11 lines
arch/x86/kvm/x86.c +2
-2}EXPORT_SYMBOL_GPL (kvm_skip_emulated_instruction);@@ -6876,8 +6879,8 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,if (!ctxt->have_exception ||exception_type (ctxt->exception.vector) ==EXCPT_TRAP ) {kvm_rip_write (vcpu, ctxt->eip); -if (r && ctxt->tf) - r =kvm_vcpu_do_singlestep (vcpu); +if (r) + r =kvm_vcpu_do_singlestep (vcpu, ctxt->tf);
Hrm. I like the current code of calling do_singlestep() iff EFLAGS.TF=1. The non-emulator case can be handled purely in VMX, per above. Maybe do something similar for the emulator with a generic "emulated instruction" hook? Not sure what to call it...
kvm_x86_ops->update_emulated_instruction(vcpu);
Quoted 6 lines
arch/x86/kvm/x86.c +0
-0__kvm_set_rflags (vcpu, ctxt->eflags); }Signature · 1 line
2.25.0.341.g760bfbb309-goog
Re: [PATCH v2 4/5] KVM: nVMX: Emulate MTF when performing instruction emulation
Quoted 96 lines · Sean Christopherson
Quoted 81 lines · Oliver Upton
Since commit
5f3d45e7f282("kvm/x86: add support forMONITOR_TRAP_FLAG"), KVM has allowed an L1 guest to use the monitor trap flag processor-based execution control for its L2 guest. KVM simply forwards any MTF VM-exits to the L1 guest, which works for normal instruction execution.However, when KVM needs to emulate an instruction on the behalf of an L2 guest, the monitor trap flag is not emulated. Add the necessary logic to
kvm_skip_emulated_instruction()to synthesize an MTF VM-exit to L1 upon instruction emulation for L2.Fixes 5f3d45e7f282 ("kvm/x86: add support for MONITOR_TRAP_FLAG")Signed-off-by Oliver Upton <[email protected]>--- arch/x86/include/asm/kvm_host.h | 1 + arch/x86/include/uapi/asm/kvm.h | 1 + arch/x86/kvm/svm.c | 1 + arch/x86/kvm/vmx/nested.c | 37 ++++++++++++++++++++++++++++++++- arch/x86/kvm/vmx/nested.h | 5 +++++ arch/x86/kvm/vmx/vmx.c | 22 ++++++++++++++++++++ arch/x86/kvm/vmx/vmx.h | 3 +++ arch/x86/kvm/x86.c | 15 +++++++------ 8 files changed, 78 insertions(+), 7 deletions(-)arch/x86/include/asm/kvm_host.h +1
-0@@ -1103,6 +1103,7 @@ struct kvm_x86_ops {int (*handle_exit)(struct kvm_vcpu *vcpu,enum exit_fastpath_completion exit_fastpath);int (*skip_emulated_instruction)(struct kvm_vcpu *vcpu); +void (*do_singlestep)(struct kvm_vcpu *vcpu);void (*set_interrupt_shadow)(struct kvm_vcpu *vcpu,int mask);u32 (*get_interrupt_shadow)(struct kvm_vcpu *vcpu); void (*patch_hypercall)(struct kvm_vcpu *vcpu,arch/x86/include/uapi/asm/kvm.h +1
-0@@ -390,6 +390,7 @@ struct kvm_sync_regs {#define KVM_STATE_NESTED_GUEST_MODE 0x00000001#define KVM_STATE_NESTED_RUN_PENDING 0x00000002#define KVM_STATE_NESTED_EVMCS 0x00000004 +#define KVM_STATE_NESTED_MTF_PENDING 0x00000008#define KVM_STATE_NESTED_SMM_GUEST_MODE 0x00000001#define KVM_STATE_NESTED_SMM_VMXON 0x00000002arch/x86/kvm/svm.c +1
-0@@ -7316,6 +7316,7 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = { .run = svm_vcpu_run, .handle_exit = handle_exit, .skip_emulated_instruction = skip_emulated_instruction, + .do_singlestep = NULL, .set_interrupt_shadow = svm_set_interrupt_shadow, .get_interrupt_shadow = svm_get_interrupt_shadow, .patch_hypercall = svm_patch_hypercall,arch/x86/kvm/vmx/nested.c +7
-0@@ -3599,8 +3599,15 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr)unsigned long exit_qual;bool block_nested_events = vmx->nested.nested_run_pending ||kvm_event_needs_reinjection (vcpu); +bool mtf_pending = vmx->nested.mtf_pending;struct kvm_lapic *apic = vcpu->arch.apic; +/* +* Clear the MTF state. If a higher priority VM-exit is delivered first, +* this state is discarded. +*/ + vmx->nested.mtf_pending = false; +if (lapic_in_kernel (vcpu) &&test_bit (KVM_APIC_INIT , &apic->pending_events)) {if (block_nested_events)arch/x86/kvm/vmx/nested.c static int vmx_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr) · line 3626
Quoted 9 lines
arch/x86/kvm/vmx/nested.c +4
-1@@ -3612,8 +3619,30 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr)return 0 ; } +/* +* Process non-debug exceptions first before MTF. +*/ if (vcpu->arch.exception.pending && -nested_vmx_check_exception (vcpu, &exit_qual)) { + !nested_vmx_check_pending_dbg(vcpu) &&Oof. So @has_payload is set %true only by single-step #DB and #PF, and by #DBs injected from userspace. Now I understand where the "
pending_dbg()comes from.
Quoted 8 lines
The part that gets really confusing is that there's "pending" from KVM's perspective, which can be any kind of #DB, e.g. DR7.GD and icebrk, and pending from an architectural perspective, which is single-step #DB and data #DBs, the latter of which isn't manually emulated by KVM (I think?).
Not sure if there's a better name than
check_pending_dbg(), but either way I think a function comment is in order.
Completely agree. To somewhat disambiguate the overload of "pending", I'll add a descriptive comment.
Quoted 66 lines
Quoted 66 lines
arch/x86/kvm/vmx/nested.c +25
-0+ nested_vmx_check_exception(vcpu, &exit_qual)) { +if (block_nested_events) +return -EBUSY ; +nested_vmx_inject_exception_vmexit (vcpu, exit_qual); +return 0 ; + } + +if (mtf_pending) { +if (block_nested_events) +return -EBUSY ; +if (nested_vmx_check_pending_dbg (vcpu)) +nested_vmx_set_pending_dbg (vcpu); +nested_vmx_vmexit (vcpu,EXIT_REASON_MONITOR_TRAP_FLAG ,0 ,0 ); +return 0 ; + } + +if (vcpu->arch.exception.pending && +nested_vmx_check_pending_dbg (vcpu) && +nested_vmx_check_exception (vcpu, &exit_qual)) {if (block_nested_events)return -EBUSY ;nested_vmx_inject_exception_vmexit (vcpu, exit_qual);@@ -5705,6 +5734,9 @@ static int vmx_get_nested_state(struct kvm_vcpu *vcpu,if (vmx->nested.nested_run_pending) kvm_state.flags |=KVM_STATE_NESTED_RUN_PENDING ; + +if (vmx->nested.mtf_pending) + kvm_state.flags |=KVM_STATE_NESTED_MTF_PENDING ; } }@@ -5885,6 +5917,9 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, vmx->nested.nested_run_pending = !!(kvm_state->flags &KVM_STATE_NESTED_RUN_PENDING ); + vmx->nested.mtf_pending = + !!(kvm_state->flags &KVM_STATE_NESTED_MTF_PENDING ); + ret = -EINVAL ;if (nested_cpu_has_shadow_vmcs (vmcs12) && vmcs12->vmcs_link_pointer !=-1ull ) {arch/x86/kvm/vmx/nested.h +5
-0@@ -175,6 +175,11 @@ static inline bool nested_cpu_has_virtual_nmis(struct vmcs12 *vmcs12)return vmcs12->pin_based_vm_exec_control &PIN_BASED_VIRTUAL_NMIS ; } +static inline int nested_cpu_has_mtf (struct vmcs12 *vmcs12) +{ +return nested_cpu_has (vmcs12,CPU_BASED_MONITOR_TRAP_FLAG ); +} +static inline int nested_cpu_has_ept (struct vmcs12 *vmcs12) {return nested_cpu_has2 (vmcs12,SECONDARY_EXEC_ENABLE_EPT );arch/x86/kvm/vmx/vmx.c +0
-0@@ -1601,6 +1601,27 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)return 1 ; }
Quoted 9 lines
arch/x86/kvm/vmx/vmx.c static int skip_emulated_instruction(struct kvm_vcpu *vcpu) · line 1609
Quoted 6 lines
arch/x86/kvm/vmx/vmx.c +6
-0+static void vmx_do_singlestep (struct kvm_vcpu *vcpu) +{ +struct vcpu_vmx *vmx; + +if (!(is_guest_mode (vcpu) && +nested_cpu_has_mtf (get_vmcs12 (vcpu))))Haven't followed all the paths, but does nested.mtf_pending need to be cleared here in case L1 disabled MTF?1
Hmm. I'm unsure of a path that would allow the mtf_pending flag to be raised but not lowered in vmx_check_nested_events(). Regardless, I can lower it here should L1 change its MTF control.
Quoted 108 lines
Quoted 62 lines
arch/x86/kvm/vmx/vmx.c +16
-0+return ; + + vmx =to_vmx (vcpu); + +/* +* Per the SDM, MTF takes priority over debug-trap exception besides +* T-bit traps. As instruction emulation is completed (i.e. at the end +* of an instruction boundary), any #DB exception pending delivery must +* be a debug-trap. Record the pending MTF state to be delivered in +* vmx_check_nested_events(). +*/ + vmx->nested.mtf_pending = (!vcpu->arch.exception.pending || + vcpu->arch.exception.nr ==DB_VECTOR ); +} +static void vmx_clear_hlt (struct kvm_vcpu *vcpu) {/* @@ -7797,6 +7818,7 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = { .run = vmx_vcpu_run, .handle_exit = vmx_handle_exit, .skip_emulated_instruction = skip_emulated_instruction, + .do_singlestep = vmx_do_singlestep, .set_interrupt_shadow = vmx_set_interrupt_shadow, .get_interrupt_shadow = vmx_get_interrupt_shadow, .patch_hypercall = vmx_patch_hypercall,arch/x86/kvm/vmx/vmx.h +3
-0@@ -150,6 +150,9 @@ struct nested_vmx {/* L2 must run next, and mustn't decide to exit to L1. */ bool nested_run_pending; +/* Pending MTF VM-exit into L1. */ +bool mtf_pending; +struct loaded_vmcs vmcs02;/* arch/x86/kvm/x86.c +6
-1@@ -6626,10 +6626,15 @@ static int kvm_vcpu_check_hw_bp(unsigned long addr, u32 type, u32 dr7,return dr6; } -static int kvm_vcpu_do_singlestep (struct kvm_vcpu *vcpu) +static int kvm_vcpu_do_singlestep (struct kvm_vcpu *vcpu,bool tf) {struct kvm_run *kvm_run = vcpu->run; +if (kvm_x86_ops->do_singlestep) + kvm_x86_ops->do_singlestep (vcpu); +if (!tf) +return 1 ; +if (vcpu->guest_debug &KVM_GUESTDBG_SINGLESTEP ) { kvm_run->debug.arch.dr6 =DR6_BS |DR6_FIXED_1 |DR6_RTM ; kvm_run->debug.arch.pc = vcpu->arch.singlestep_rip;arch/x86/kvm/x86.c int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu) · line 6666
Quoted 7 lines
arch/x86/kvm/x86.c +1
-3@@ -6658,9 +6663,7 @@ int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu)* processor will not generate this exception after the instruction * that sets the TF flag". */ -if (unlikely (rflags &X86_EFLAGS_TF )) - r =kvm_vcpu_do_singlestep (vcpu); -return r; +return kvm_vcpu_do_singlestep (vcpu, rflags &X86_EFLAGS_TF );The extra retpoline, i.e. ->do_singlestep(), can be avoided by handling the
kvm_skip_emulated_instruction()purely in VMX:arch/x86/kvm/vmx/vmx.c +15
-1@@ -1601,6 +1601,20 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)return 1 ; } +static int vmx_skip_emulated_instruction (struct kvm_vcpu *vcpu) +{ +if (is_guest_mode (vcpu)) { +if (nested_cpu_has_mtf (get_vmcs12 (vcpu) && + (!vcpu->arch.exception.pending || + vcpu->arch.exception.nr ==DB_VECTOR ))) +to_vmx (vcpu)->nested.mtf_pending = true; +else +to_vmx (vcpu)->nested.mtf_pending = false; + } + +return skip_emulated_instruction (vcpu); +} +static void vmx_clear_hlt (struct kvm_vcpu *vcpu) {/* @@ -7796,7 +7810,7 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = { .run = vmx_vcpu_run, .handle_exit = vmx_handle_exit, - .skip_emulated_instruction = skip_emulated_instruction, + .skip_emulated_instruction = vmx_skip_emulated_instruction, .set_interrupt_shadow = vmx_set_interrupt_shadow, .get_interrupt_shadow = vmx_get_interrupt_shadow, .patch_hypercall = vmx_patch_hypercall,
Quoted 1 line
arch/x86/kvm/x86.c int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu) · line 6667
Quoted 1 line
arch/x86/kvm/x86.c +0
-0}
Awesome, thank you for the suggestion. I'll work this in :)
Quoted 16 lines
arch/x86/kvm/x86.c int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, · line 6883
Quoted 10 lines
arch/x86/kvm/x86.c +2
-2EXPORT_SYMBOL_GPL (kvm_skip_emulated_instruction);@@ -6876,8 +6879,8 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,if (!ctxt->have_exception ||exception_type (ctxt->exception.vector) ==EXCPT_TRAP ) {kvm_rip_write (vcpu, ctxt->eip); -if (r && ctxt->tf) - r =kvm_vcpu_do_singlestep (vcpu); +if (r) + r =kvm_vcpu_do_singlestep (vcpu, ctxt->tf);Hrm. I like the current code of calling
do_singlestep()iff EFLAGS.TF=1. The non-emulator case can be handled purely in VMX, per above. Maybe do something similar for the emulator with a generic "emulated instruction" hook? Not sure what to call it...
Quoted 1 line
kvm_x86_ops->update_emulated_instruction(vcpu);
Yeah, agreed. I'll add in a hook for the emulator to do this rather than reworking do_singlestep().
Quoted 5 lines
Quoted 5 lines
arch/x86/kvm/x86.c +0
-0__kvm_set_rflags (vcpu, ctxt->eflags); }Signature · 1 line
2.25.0.341.g760bfbb309-goog
Thanks Sean!
Re: [PATCH v2 4/5] KVM: nVMX: Emulate MTF when performing instruction emulation
In reply to Oliver UptonQuoted 42 lines · Oliver Upton
Since commit
5f3d45e7f282("kvm/x86: add support forMONITOR_TRAP_FLAG"), KVM has allowed an L1 guest to use the monitor trap flag processor-based execution control for its L2 guest. KVM simply forwards any MTF VM-exits to the L1 guest, which works for normal instruction execution.However, when KVM needs to emulate an instruction on the behalf of an L2 guest, the monitor trap flag is not emulated. Add the necessary logic to
kvm_skip_emulated_instruction()to synthesize an MTF VM-exit to L1 upon instruction emulation for L2.Fixes 5f3d45e7f282 ("kvm/x86: add support for MONITOR_TRAP_FLAG")Signed-off-by Oliver Upton <[email protected]>--- arch/x86/include/asm/kvm_host.h | 1 + arch/x86/include/uapi/asm/kvm.h | 1 + arch/x86/kvm/svm.c | 1 + arch/x86/kvm/vmx/nested.c | 37 ++++++++++++++++++++++++++++++++- arch/x86/kvm/vmx/nested.h | 5 +++++ arch/x86/kvm/vmx/vmx.c | 22 ++++++++++++++++++++ arch/x86/kvm/vmx/vmx.h | 3 +++ arch/x86/kvm/x86.c | 15 +++++++------ 8 files changed, 78 insertions(+), 7 deletions(-)arch/x86/include/asm/kvm_host.h +1
-0@@ -1103,6 +1103,7 @@ struct kvm_x86_ops {int (*handle_exit)(struct kvm_vcpu *vcpu,enum exit_fastpath_completion exit_fastpath);int (*skip_emulated_instruction)(struct kvm_vcpu *vcpu); +void (*do_singlestep)(struct kvm_vcpu *vcpu);void (*set_interrupt_shadow)(struct kvm_vcpu *vcpu,int mask);u32 (*get_interrupt_shadow)(struct kvm_vcpu *vcpu); void (*patch_hypercall)(struct kvm_vcpu *vcpu,arch/x86/include/uapi/asm/kvm.h +0
-0
arch/x86/include/uapi/asm/kvm.h struct kvm_sync_regs { · line 393
Quoted 4 lines
arch/x86/include/uapi/asm/kvm.h +1
-0@@ -390,6 +390,7 @@ struct kvm_sync_regs {#define KVM_STATE_NESTED_GUEST_MODE 0x00000001#define KVM_STATE_NESTED_RUN_PENDING 0x00000002#define KVM_STATE_NESTED_EVMCS 0x00000004 +#define KVM_STATE_NESTED_MTF_PENDING 0x00000008
Maybe I don't understand the distinction, but shouldn't this new flag have a KVM_STATE_NESTED_VMX prefix and live with KVM_STATE_VMX_PREEMPTION_TIMER_DEADLINE, below?
Quoted 3 lines
arch/x86/include/uapi/asm/kvm.h +0
-0#define KVM_STATE_NESTED_SMM_GUEST_MODE 0x00000001#define KVM_STATE_NESTED_SMM_VMXON 0x00000002
Sean Christopherson
maintainerRe: [PATCH v2 4/5] KVM: nVMX: Emulate MTF when performing instruction emulation
Quoted 16 lines · Jim Mattson
Quoted 15 lines · Oliver Upton
Since commit
5f3d45e7f282("kvm/x86: add support forMONITOR_TRAP_FLAG"), KVM has allowed an L1 guest to use the monitor trap flag processor-based execution control for its L2 guest. KVM simply forwards any MTF VM-exits to the L1 guest, which works for normal instruction execution.However, when KVM needs to emulate an instruction on the behalf of an L2 guest, the monitor trap flag is not emulated. Add the necessary logic to
kvm_skip_emulated_instruction()to synthesize an MTF VM-exit to L1 upon instruction emulation for L2.Fixes 5f3d45e7f282 ("kvm/x86: add support for MONITOR_TRAP_FLAG")Signed-off-by Oliver Upton <[email protected]>---
...
Quoted 5 lines
Quoted 5 lines
arch/x86/include/uapi/asm/kvm.h +0
-0@@ -390,6 +390,7 @@ struct kvm_sync_regs {
Quoted 8 lines
arch/x86/include/uapi/asm/kvm.h struct kvm_sync_regs { · line 393
Quoted 4 lines
arch/x86/include/uapi/asm/kvm.h +1
-0#define KVM_STATE_NESTED_GUEST_MODE 0x00000001#define KVM_STATE_NESTED_RUN_PENDING 0x00000002#define KVM_STATE_NESTED_EVMCS 0x00000004 +#define KVM_STATE_NESTED_MTF_PENDING 0x00000008Maybe I don't understand the distinction, but shouldn't this new flag have a
KVM_STATE_NESTED_VMXprefix and live withKVM_STATE_VMX_PREEMPTION_TIMER_DEADLINE, below?
That does seem to be the case, seems highly unlikely SVM will add MTF. And SVM's KVM_STATE_NESTED_GIF_SET should have been SVM specific, but kvm_svm_nested_state_hdr doesn't even reserve a flags field :-/
Quoted 2 lines
Quoted 2 lines
arch/x86/include/uapi/asm/kvm.h +0
-0#define KVM_STATE_NESTED_SMM_GUEST_MODE 0x00000001#define KVM_STATE_NESTED_SMM_VMXON 0x00000002
Re: [PATCH v2 4/5] KVM: nVMX: Emulate MTF when performing instruction emulation
Quoted 35 lines · Sean Christopherson
Quoted 16 lines · Jim Mattson
Quoted 15 lines · Oliver Upton
Since commit
5f3d45e7f282("kvm/x86: add support forMONITOR_TRAP_FLAG"), KVM has allowed an L1 guest to use the monitor trap flag processor-based execution control for its L2 guest. KVM simply forwards any MTF VM-exits to the L1 guest, which works for normal instruction execution.However, when KVM needs to emulate an instruction on the behalf of an L2 guest, the monitor trap flag is not emulated. Add the necessary logic to
kvm_skip_emulated_instruction()to synthesize an MTF VM-exit to L1 upon instruction emulation for L2.Fixes 5f3d45e7f282 ("kvm/x86: add support for MONITOR_TRAP_FLAG")Signed-off-by Oliver Upton <[email protected]>---
...
Quoted 5 lines
Quoted 5 lines
arch/x86/include/uapi/asm/kvm.h +0
-0@@ -390,6 +390,7 @@ struct kvm_sync_regs {Quoted 8 lines
arch/x86/include/uapi/asm/kvm.h struct kvm_sync_regs { · line 393
Quoted 4 lines
arch/x86/include/uapi/asm/kvm.h +1
-0#define KVM_STATE_NESTED_GUEST_MODE 0x00000001#define KVM_STATE_NESTED_RUN_PENDING 0x00000002#define KVM_STATE_NESTED_EVMCS 0x00000004 +#define KVM_STATE_NESTED_MTF_PENDING 0x00000008Maybe I don't understand the distinction, but shouldn't this new flag have a
KVM_STATE_NESTED_VMXprefix and live withKVM_STATE_VMX_PREEMPTION_TIMER_DEADLINE, below?
Quoted 3 lines
That does seem to be the case, seems highly unlikely SVM will add MTF. And SVM's
KVM_STATE_NESTED_GIF_SETshould have been SVM specific, butkvm_svm_nested_state_hdrdoesn't even reserve a flags field :-/
APIs are hard.
PATCH v2 5/5 x86: VMX: Add tests for monitor trap flag
In reply to Oliver UptonTest to verify that MTF VM-exits into host are synthesized when the 'monitor trap flag' processor-based VM-execution control is set under various conditions.
Expect an MTF VM-exit if instruction execution produces no events other than MTF. Should instruction execution produce a concurrent debug-trap and MTF event, expect an MTF VM-exit with the 'pending debug exceptions' VMCS field set. Expect an MTF VM-exit to follow event delivery should instruction execution generate a higher-priority event, such as a general-protection fault. Lastly, expect an MTF VM-exit to follow delivery of a debug-trap software exception (INT1/INT3/INTO/INT n).
--- x86/vmx.h | 1 + x86/vmx_tests.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 158 insertions(+)
x86/vmx.h +1 -0
@@ -399,6 +399,7 @@ enum Ctrl0 {CPU_NMI_WINDOW =1ul <<22 ,CPU_IO =1ul <<24 ,CPU_IO_BITMAP =1ul <<25 , +CPU_MTF =1ul <<27 ,CPU_MSR_BITMAP =1ul <<28 ,CPU_MONITOR =1ul <<29 , CPU_PAUSE = 1ul << 30,
x86/vmx_tests.c +157 -0
168 lines not on this page · raw message
Signature · 1 line
2.25.0.341.g760bfbb309-goog
Quoted 49 lines · Oliver Upton
v1: http://lore.kernel.org/r/[email protected]
v1 => v2:
- Don't split the #DB delivery by vendors. Unconditionally injecting #DB payloads into the 'pending debug exceptions' field will cause KVM to get stuck in a loop. Per the SDM, when hardware injects an event resulting from this field's value, it is checked against the exception interception bitmap.
- Address Sean's comments by injecting the VM-exit into L1 fromvmx_check_nested_events().
- Added fix for nested INIT VM-exits + 'pending debug exceptions' field as it was noticed in implementing v2.
- Drop Peter + Jim's Reviewed-by tags, as the patch set has changed since v1.KVM already provides guests the ability to use the 'monitor trap flag' VM-execution control. Support for this flag is provided by the fact that KVM unconditionally forwards MTF VM-exits to the guest (if requested), as KVM doesn't utilize MTF. While this provides support during hardware instruction execution, it is insufficient for instruction emulation.
Should L0 emulate an instruction on the behalf of L2, L0 should also synthesize an MTF VM-exit into L1, should control be set.
The first patch corrects a nuanced difference between the definition of a #DB exception payload field and DR6 register. Mask off bit 12 which is defined in the 'pending debug exceptions' field when applying to DR6, since the payload field is said to be compatible with the aforementioned VMCS field.
The second patch sets the 'pending debug exceptions' VMCS field when delivering an INIT signal VM-exit to L1, as described in the SDM. This patch also introduces helpers for setting the 'pending debug exceptions' VMCS field.
The third patch massages KVM's handling of exception payloads with regard to API compatibility. Rather than immediately injecting the payload w/o opt-in, instead defer the payload + immediately inject before completing a
KVM_GET_VCPU_EVENTS. This maintains API compatibility whilst correcting #DB behavior with regard to higher priority VM-exit events.Fourth patch introduces MTF implementation for emulated instructions. Identify if an MTF is due on an instruction boundary from
kvm_vcpu_do_singlestep(), however only deliver this VM-exit fromvmx_check_nested_events()to respect the relative prioritization to other VM-exits. Since this augments the nested state, introduce a new flag for (de)serialization.
Quoted 4 lines
Last patch adds tests to kvm-unit-tests to assert the correctness of MTF under several conditions (concurrent #DB trap, #DB fault, etc). These tests pass under virtualization with this series as well as on bare-metal.
Based on tip of kvm/next as of Jan 28:
f1b0bc14f2db ("KVM: x86: Use a typedef for fastop functions")
Quoted 19 lines
Oliver Upton (4):
KVM: x86: Mask off reserved bit from #DB exception payload
KVM: nVMX: Handle pending #DB when injecting INIT VM-exit
KVM: x86: Deliver exception payload onKVM_GET_VCPU_EVENTS
KVM: nVMX: Emulate MTF when performing instruction emulationarch/x86/include/asm/kvm_host.h | 1 + arch/x86/include/uapi/asm/kvm.h | 1 + arch/x86/kvm/svm.c | 1 + arch/x86/kvm/vmx/nested.c | 60 ++++++++++++++++++++++++++++++++- arch/x86/kvm/vmx/nested.h | 5 +++ arch/x86/kvm/vmx/vmx.c | 22 ++++++++++++ arch/x86/kvm/vmx/vmx.h | 3 ++ arch/x86/kvm/x86.c | 52 +++++++++++++++++----------- 8 files changed, 125 insertions(+), 20 deletions(-)Signature · 1 line
2.25.0.341.g760bfbb309-goog