KVM: x86: Minor emulator cleanup
v2 cover KVM: x86: Minor emulator cleanup
Three small patches to move emulator specific variables from 'struct kvm_vcpu_arch' to 'struct x86_emulate_ctxt'.
v2:
- Rebase to kvm/queue, 2c2787938512 ("KVM: selftests: Stop ...")
Sean Christopherson (3):
KVM: x86: Add EMULTYPE_PF when emulation is triggered by a page fault
KVM: x86: Move gpa_val and gpa_available into the emulator context
KVM: x86: Move #PF retry tracking variables into emulation context
arch/x86/include/asm/kvm_emulate.h | 8 ++++++ arch/x86/include/asm/kvm_host.h | 19 ++++++------- arch/x86/kvm/mmu/mmu.c | 10 ++----- arch/x86/kvm/x86.c | 45 +++++++++++++++++++----------- 4 files changed, 48 insertions(+), 34 deletions(-)
Signature · 1 line
2.24.1
PATCH v2 1/3 KVM: x86: Add EMULTYPE_PF when emulation is triggered by a page fault
Add a new emulation type flag to explicitly mark emulation related to a page fault. Move the propation of the GPA into the emulator from the page fault handler into x86_emulate_instruction, using EMULTYPE_PF as an indicator that cr2 is valid. Similarly, don't propagate cr2 into the exception.address when it's *not* valid.
Signed-off-by Sean Christopherson <[email protected]>
--- arch/x86/include/asm/kvm_host.h | 12 +++++++++--- arch/x86/kvm/mmu/mmu.c | 10 ++-------- arch/x86/kvm/x86.c | 25 +++++++++++++++++++------ 3 files changed, 30 insertions(+), 17 deletions(-)
arch/x86/include/asm/kvm_host.h +9 -3
@@ -1370,8 +1370,9 @@ extern u64 kvm_mce_cap_supported;* decode the instruction length. For use *only* by * kvm_x86_ops->skip_emulated_instruction() implementations. * -* EMULTYPE_ALLOW_RETRY - Set when the emulator should resume the guest to -* retry native execution under certain conditions. +* EMULTYPE_ALLOW_RETRY_PF - Set when the emulator should resume the guest to +* retry native execution under certain conditions, +* Can only be set in conjunction with EMULTYPE_PF. * * EMULTYPE_TRAP_UD_FORCED - Set when emulating an intercepted #UD that was * triggered by KVM's magic "force emulation" prefix,
@@ -1384,13 +1385,18 @@ extern u64 kvm_mce_cap_supported;* backdoor emulation, which is opt in via module param. * VMware backoor emulation handles select instructions * and reinjects the #GP for all other cases. +* +* EMULTYPE_PF - Set when emulating MMIO by way of an intercepted #PF, in which +* case the CR2/GPA value pass on the stack is valid. */ #define EMULTYPE_NO_DECODE (1 << 0)#define EMULTYPE_TRAP_UD (1 << 1)#define EMULTYPE_SKIP (1 << 2) -#define EMULTYPE_ALLOW_RETRY (1 << 3) +#define EMULTYPE_ALLOW_RETRY_PF (1 << 3)#define EMULTYPE_TRAP_UD_FORCED (1 << 4)#define EMULTYPE_VMWARE_GP (1 << 5) +#define EMULTYPE_PF (1 << 6) +int kvm_emulate_instruction (struct kvm_vcpu *vcpu,int emulation_type);int kvm_emulate_instruction_from_buffer (struct kvm_vcpu *vcpu,void *insn,int insn_len);
arch/x86/kvm/mmu/mmu.c +2 -8
@@ -5416,18 +5416,12 @@ EXPORT_SYMBOL_GPL(kvm_mmu_unprotect_page_virt);int kvm_mmu_page_fault (struct kvm_vcpu *vcpu,gpa_t cr2_or_gpa,u64 error_code,void *insn,int insn_len) { -int r, emulation_type =0 ; +int r, emulation_type =EMULTYPE_PF ;bool direct = vcpu->arch.mmu->direct_map;if (WARN_ON (!VALID_PAGE (vcpu->arch.mmu->root_hpa)))return RET_PF_RETRY ; -/* With shadow page tables, fault_address contains a GVA or nGPA. */ -if (vcpu->arch.mmu->direct_map) { - vcpu->arch.gpa_available = true; - vcpu->arch.gpa_val = cr2_or_gpa; - } - r =RET_PF_INVALID ;if (unlikely (error_code &PFERR_RSVD_MASK )) { r =handle_mmio_page_fault (vcpu, cr2_or_gpa, direct);
@@ -5472,7 +5466,7 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 error_code,* for L1 isn't going to magically fix whatever issue cause L2 to fail. */ if (!mmio_info_in_cache (vcpu, cr2_or_gpa, direct) && !is_guest_mode (vcpu)) - emulation_type =EMULTYPE_ALLOW_RETRY ; + emulation_type |=EMULTYPE_ALLOW_RETRY_PF ; emulate:/* * On AMD platforms, under certain conditions insn_len may be zero on #NPF.
arch/x86/kvm/x86.c +19 -6
@@ -6483,10 +6483,11 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,gpa_t gpa = cr2_or_gpa;kvm_pfn_t pfn; -if (!(emulation_type &EMULTYPE_ALLOW_RETRY )) +if (!(emulation_type &EMULTYPE_ALLOW_RETRY_PF ))return false; -if (WARN_ON_ONCE (is_guest_mode (vcpu))) +if (WARN_ON_ONCE (is_guest_mode (vcpu)) || +WARN_ON_ONCE (!(emulation_type &EMULTYPE_PF )))return false;if (!vcpu->arch.mmu->direct_map) {
@@ -6574,10 +6575,11 @@ static bool retry_instruction(struct x86_emulate_ctxt *ctxt,*/ vcpu->arch.last_retry_eip = vcpu->arch.last_retry_addr =0 ; -if (!(emulation_type &EMULTYPE_ALLOW_RETRY )) +if (!(emulation_type &EMULTYPE_ALLOW_RETRY_PF ))return false; -if (WARN_ON_ONCE (is_guest_mode (vcpu))) +if (WARN_ON_ONCE (is_guest_mode (vcpu)) || +WARN_ON_ONCE (!(emulation_type &EMULTYPE_PF )))return false;if (x86_page_table_writing_insn (ctxt))
@@ -6830,8 +6832,19 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, } restart: -/* Save the faulting GPA (cr2) in the address field */ - ctxt->exception.address = cr2_or_gpa; +if (emulation_type &EMULTYPE_PF ) { +/* Save the faulting GPA (cr2) in the address field */ + ctxt->exception.address = cr2_or_gpa; + +/* With shadow page tables, cr2 contains a GVA or nGPA. */ +if (vcpu->arch.mmu->direct_map) { + vcpu->arch.gpa_available = true; + vcpu->arch.gpa_val = cr2_or_gpa; + } + }else { +/* Sanitize the address out of an abundance of paranoia. */ + ctxt->exception.address =0 ; + } r =x86_emulate_insn (ctxt);
Signature · 1 line
2.24.1
Re: [PATCH v2 1/3] KVM: x86: Add EMULTYPE_PF when emulation is triggered by a page fault
Quoted 32 lines · Sean Christopherson
Add a new emulation type flag to explicitly mark emulation related to a page fault. Move the propation of the GPA into the emulator from the page fault handler into
x86_emulate_instruction, usingEMULTYPE_PFas an indicator that cr2 is valid. Similarly, don't propagate cr2 into the exception.address when it's *not* valid.Signed-off-by Sean Christopherson <[email protected]>--- arch/x86/include/asm/kvm_host.h | 12 +++++++++--- arch/x86/kvm/mmu/mmu.c | 10 ++-------- arch/x86/kvm/x86.c | 25 +++++++++++++++++++------ 3 files changed, 30 insertions(+), 17 deletions(-)arch/x86/include/asm/kvm_host.h +3
-2@@ -1370,8 +1370,9 @@ extern u64 kvm_mce_cap_supported;* decode the instruction length. For use *only* by * kvm_x86_ops->skip_emulated_instruction() implementations. * -* EMULTYPE_ALLOW_RETRY - Set when the emulator should resume the guest to -* retry native execution under certain conditions. +* EMULTYPE_ALLOW_RETRY_PF - Set when the emulator should resume the guest to +* retry native execution under certain conditions, +* Can only be set in conjunction with EMULTYPE_PF. * * EMULTYPE_TRAP_UD_FORCED - Set when emulating an intercepted #UD that was * triggered by KVM's magic "force emulation" prefix, @@ -1384,13 +1385,18 @@ extern u64 kvm_mce_cap_supported;* backdoor emulation, which is opt in via module param. * VMware backoor emulation handles select instructions
arch/x86/include/asm/kvm_host.h extern u64 kvm_mce_cap_supported; · line 1395
Quoted 10 lines
arch/x86/include/asm/kvm_host.h +4
-1* and reinjects the #GP for all other cases. +* +* EMULTYPE_PF - Set when emulating MMIO by way of an intercepted #PF, in which +* case the CR2/GPA value pass on the stack is valid. */ #define EMULTYPE_NO_DECODE (1 << 0)#define EMULTYPE_TRAP_UD (1 << 1)#define EMULTYPE_SKIP (1 << 2) -#define EMULTYPE_ALLOW_RETRY (1 << 3) +#define EMULTYPE_ALLOW_RETRY_PF (1 << 3)
How about naming it as EMULTYPE_PF_ALLOW_RETRY and exchanging the bit position with EMULTYPE_PF ?
Quoted 95 lines
arch/x86/include/asm/kvm_host.h +2
-0#define EMULTYPE_TRAP_UD_FORCED (1 << 4)#define EMULTYPE_VMWARE_GP (1 << 5) +#define EMULTYPE_PF (1 << 6) +int kvm_emulate_instruction (struct kvm_vcpu *vcpu,int emulation_type);int kvm_emulate_instruction_from_buffer (struct kvm_vcpu *vcpu,void *insn,int insn_len);arch/x86/kvm/mmu/mmu.c +2
-8@@ -5416,18 +5416,12 @@ EXPORT_SYMBOL_GPL(kvm_mmu_unprotect_page_virt);int kvm_mmu_page_fault (struct kvm_vcpu *vcpu,gpa_t cr2_or_gpa,u64 error_code,void *insn,int insn_len) { -int r, emulation_type =0 ; +int r, emulation_type =EMULTYPE_PF ;bool direct = vcpu->arch.mmu->direct_map;if (WARN_ON (!VALID_PAGE (vcpu->arch.mmu->root_hpa)))return RET_PF_RETRY ; -/* With shadow page tables, fault_address contains a GVA or nGPA. */ -if (vcpu->arch.mmu->direct_map) { - vcpu->arch.gpa_available = true; - vcpu->arch.gpa_val = cr2_or_gpa; - } - r =RET_PF_INVALID ;if (unlikely (error_code &PFERR_RSVD_MASK )) { r =handle_mmio_page_fault (vcpu, cr2_or_gpa, direct);@@ -5472,7 +5466,7 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 error_code,* for L1 isn't going to magically fix whatever issue cause L2 to fail. */ if (!mmio_info_in_cache (vcpu, cr2_or_gpa, direct) && !is_guest_mode (vcpu)) - emulation_type =EMULTYPE_ALLOW_RETRY ; + emulation_type |=EMULTYPE_ALLOW_RETRY_PF ; emulate:/* * On AMD platforms, under certain conditions insn_len may be zero on #NPF. arch/x86/kvm/x86.c +19
-6@@ -6483,10 +6483,11 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,gpa_t gpa = cr2_or_gpa;kvm_pfn_t pfn; -if (!(emulation_type &EMULTYPE_ALLOW_RETRY )) +if (!(emulation_type &EMULTYPE_ALLOW_RETRY_PF ))return false; -if (WARN_ON_ONCE (is_guest_mode (vcpu))) +if (WARN_ON_ONCE (is_guest_mode (vcpu)) || +WARN_ON_ONCE (!(emulation_type &EMULTYPE_PF )))return false;if (!vcpu->arch.mmu->direct_map) {@@ -6574,10 +6575,11 @@ static bool retry_instruction(struct x86_emulate_ctxt *ctxt,*/ vcpu->arch.last_retry_eip = vcpu->arch.last_retry_addr =0 ; -if (!(emulation_type &EMULTYPE_ALLOW_RETRY )) +if (!(emulation_type &EMULTYPE_ALLOW_RETRY_PF ))return false; -if (WARN_ON_ONCE (is_guest_mode (vcpu))) +if (WARN_ON_ONCE (is_guest_mode (vcpu)) || +WARN_ON_ONCE (!(emulation_type &EMULTYPE_PF )))return false;if (x86_page_table_writing_insn (ctxt))@@ -6830,8 +6832,19 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, } restart: -/* Save the faulting GPA (cr2) in the address field */ - ctxt->exception.address = cr2_or_gpa; +if (emulation_type &EMULTYPE_PF ) { +/* Save the faulting GPA (cr2) in the address field */ + ctxt->exception.address = cr2_or_gpa; + +/* With shadow page tables, cr2 contains a GVA or nGPA. */ +if (vcpu->arch.mmu->direct_map) { + vcpu->arch.gpa_available = true; + vcpu->arch.gpa_val = cr2_or_gpa; + } + }else { +/* Sanitize the address out of an abundance of paranoia. */ + ctxt->exception.address =0 ; + } r =x86_emulate_insn (ctxt);
Re: [PATCH v2 1/3] KVM: x86: Add EMULTYPE_PF when emulation is triggered by a page fault
Quoted 38 lines · Xiaoyao Li
Quoted 37 lines · Sean Christopherson
Add a new emulation type flag to explicitly mark emulation related to a page fault. Move the propation of the GPA into the emulator from the page fault handler into
x86_emulate_instruction, usingEMULTYPE_PFas an indicator that cr2 is valid. Similarly, don't propagate cr2 into the exception.address when it's *not* valid.Signed-off-by Sean Christopherson <[email protected]>--- arch/x86/include/asm/kvm_host.h | 12 +++++++++--- arch/x86/kvm/mmu/mmu.c | 10 ++-------- arch/x86/kvm/x86.c | 25 +++++++++++++++++++------ 3 files changed, 30 insertions(+), 17 deletions(-)arch/x86/include/asm/kvm_host.h +6
-2@@ -1370,8 +1370,9 @@ extern u64 kvm_mce_cap_supported;* decode the instruction length. For use *only* by * kvm_x86_ops->skip_emulated_instruction() implementations. * -* EMULTYPE_ALLOW_RETRY - Set when the emulator should resume the guest to -* retry native execution under certain conditions. +* EMULTYPE_ALLOW_RETRY_PF - Set when the emulator should resume the guest to +* retry native execution under certain conditions, +* Can only be set in conjunction with EMULTYPE_PF. * * EMULTYPE_TRAP_UD_FORCED - Set when emulating an intercepted #UD that was * triggered by KVM's magic "force emulation" prefix, @@ -1384,13 +1385,18 @@ extern u64 kvm_mce_cap_supported;* backdoor emulation, which is opt in via module param. * VMware backoor emulation handles select instructions * and reinjects the #GP for all other cases. +* +* EMULTYPE_PF - Set when emulating MMIO by way of an intercepted #PF, in which +* case the CR2/GPA value pass on the stack is valid. */
Quoted 8 lines
arch/x86/include/asm/kvm_host.h extern u64 kvm_mce_cap_supported; · line 1395
Quoted 5 lines
arch/x86/include/asm/kvm_host.h +1
-1#define EMULTYPE_NO_DECODE (1 << 0)#define EMULTYPE_TRAP_UD (1 << 1)#define EMULTYPE_SKIP (1 << 2) -#define EMULTYPE_ALLOW_RETRY (1 << 3) +#define EMULTYPE_ALLOW_RETRY_PF (1 << 3)How about naming it as
EMULTYPE_PF_ALLOW_RETRYand exchanging the bit position withEMULTYPE_PF?
Hmm, EMULTYPE_PF_ALLOW_RETRY does sound better. I'm on the fence regarding shuffling the bits. If I were to shuffle the bits, I'd do a more thorough reorder so that the #UD and #PF types are consecutive, e.g.
#define EMULTYPE_NO_DECODE (1 << 0) #define EMULTYPE_TRAP_UD (1 << 1) #define EMULTYPE_TRAP_UD_FORCED (1 << 2) #define EMULTYPE_SKIP (1 << 3) #define EMULTYPE_VMWARE_GP (1 << 4) #define EMULTYPE_PF (1 << 5) #define EMULTYPE_PF_ALLOW_RETRY (1 << 6)
Part of me really wants to do that, the other part of me thinks it's unnecessary thrash.
Quoted 85 lines
Quoted 84 lines
arch/x86/include/asm/kvm_host.h +2
-0#define EMULTYPE_TRAP_UD_FORCED (1 << 4)#define EMULTYPE_VMWARE_GP (1 << 5) +#define EMULTYPE_PF (1 << 6) +int kvm_emulate_instruction (struct kvm_vcpu *vcpu,int emulation_type);int kvm_emulate_instruction_from_buffer (struct kvm_vcpu *vcpu,void *insn,int insn_len);arch/x86/kvm/mmu/mmu.c +2
-8@@ -5416,18 +5416,12 @@ EXPORT_SYMBOL_GPL(kvm_mmu_unprotect_page_virt);int kvm_mmu_page_fault (struct kvm_vcpu *vcpu,gpa_t cr2_or_gpa,u64 error_code,void *insn,int insn_len) { -int r, emulation_type =0 ; +int r, emulation_type =EMULTYPE_PF ;bool direct = vcpu->arch.mmu->direct_map;if (WARN_ON (!VALID_PAGE (vcpu->arch.mmu->root_hpa)))return RET_PF_RETRY ; -/* With shadow page tables, fault_address contains a GVA or nGPA. */ -if (vcpu->arch.mmu->direct_map) { - vcpu->arch.gpa_available = true; - vcpu->arch.gpa_val = cr2_or_gpa; - } - r =RET_PF_INVALID ;if (unlikely (error_code &PFERR_RSVD_MASK )) { r =handle_mmio_page_fault (vcpu, cr2_or_gpa, direct);@@ -5472,7 +5466,7 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 error_code,* for L1 isn't going to magically fix whatever issue cause L2 to fail. */ if (!mmio_info_in_cache (vcpu, cr2_or_gpa, direct) && !is_guest_mode (vcpu)) - emulation_type =EMULTYPE_ALLOW_RETRY ; + emulation_type |=EMULTYPE_ALLOW_RETRY_PF ; emulate:/* * On AMD platforms, under certain conditions insn_len may be zero on #NPF. arch/x86/kvm/x86.c +19
-6@@ -6483,10 +6483,11 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,gpa_t gpa = cr2_or_gpa;kvm_pfn_t pfn; -if (!(emulation_type &EMULTYPE_ALLOW_RETRY )) +if (!(emulation_type &EMULTYPE_ALLOW_RETRY_PF ))return false; -if (WARN_ON_ONCE (is_guest_mode (vcpu))) +if (WARN_ON_ONCE (is_guest_mode (vcpu)) || +WARN_ON_ONCE (!(emulation_type &EMULTYPE_PF )))return false;if (!vcpu->arch.mmu->direct_map) {@@ -6574,10 +6575,11 @@ static bool retry_instruction(struct x86_emulate_ctxt *ctxt,*/ vcpu->arch.last_retry_eip = vcpu->arch.last_retry_addr =0 ; -if (!(emulation_type &EMULTYPE_ALLOW_RETRY )) +if (!(emulation_type &EMULTYPE_ALLOW_RETRY_PF ))return false; -if (WARN_ON_ONCE (is_guest_mode (vcpu))) +if (WARN_ON_ONCE (is_guest_mode (vcpu)) || +WARN_ON_ONCE (!(emulation_type &EMULTYPE_PF )))return false;if (x86_page_table_writing_insn (ctxt))@@ -6830,8 +6832,19 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, } restart: -/* Save the faulting GPA (cr2) in the address field */ - ctxt->exception.address = cr2_or_gpa; +if (emulation_type &EMULTYPE_PF ) { +/* Save the faulting GPA (cr2) in the address field */ + ctxt->exception.address = cr2_or_gpa; + +/* With shadow page tables, cr2 contains a GVA or nGPA. */ +if (vcpu->arch.mmu->direct_map) { + vcpu->arch.gpa_available = true; + vcpu->arch.gpa_val = cr2_or_gpa; + } + }else { +/* Sanitize the address out of an abundance of paranoia. */ + ctxt->exception.address =0 ; + } r =x86_emulate_insn (ctxt);
Re: [PATCH v2 1/3] KVM: x86: Add EMULTYPE_PF when emulation is triggered by a page fault
Quoted 60 lines · Sean Christopherson
Quoted 38 lines · Xiaoyao Li
Quoted 37 lines · Sean Christopherson
Add a new emulation type flag to explicitly mark emulation related to a page fault. Move the propation of the GPA into the emulator from the page fault handler into
x86_emulate_instruction, usingEMULTYPE_PFas an indicator that cr2 is valid. Similarly, don't propagate cr2 into the exception.address when it's *not* valid.Signed-off-by Sean Christopherson <[email protected]>--- arch/x86/include/asm/kvm_host.h | 12 +++++++++--- arch/x86/kvm/mmu/mmu.c | 10 ++-------- arch/x86/kvm/x86.c | 25 +++++++++++++++++++------ 3 files changed, 30 insertions(+), 17 deletions(-)arch/x86/include/asm/kvm_host.h +6
-2@@ -1370,8 +1370,9 @@ extern u64 kvm_mce_cap_supported;* decode the instruction length. For use *only* by * kvm_x86_ops->skip_emulated_instruction() implementations. * -* EMULTYPE_ALLOW_RETRY - Set when the emulator should resume the guest to -* retry native execution under certain conditions. +* EMULTYPE_ALLOW_RETRY_PF - Set when the emulator should resume the guest to +* retry native execution under certain conditions, +* Can only be set in conjunction with EMULTYPE_PF. * * EMULTYPE_TRAP_UD_FORCED - Set when emulating an intercepted #UD that was * triggered by KVM's magic "force emulation" prefix, @@ -1384,13 +1385,18 @@ extern u64 kvm_mce_cap_supported;* backdoor emulation, which is opt in via module param. * VMware backoor emulation handles select instructions * and reinjects the #GP for all other cases. +* +* EMULTYPE_PF - Set when emulating MMIO by way of an intercepted #PF, in which +* case the CR2/GPA value pass on the stack is valid. */ Quoted 8 lines
arch/x86/include/asm/kvm_host.h extern u64 kvm_mce_cap_supported; · line 1395
Quoted 5 lines
arch/x86/include/asm/kvm_host.h +1
-1#define EMULTYPE_NO_DECODE (1 << 0)#define EMULTYPE_TRAP_UD (1 << 1)#define EMULTYPE_SKIP (1 << 2) -#define EMULTYPE_ALLOW_RETRY (1 << 3) +#define EMULTYPE_ALLOW_RETRY_PF (1 << 3)How about naming it as
EMULTYPE_PF_ALLOW_RETRYand exchanging the bit position withEMULTYPE_PF?Hmm,
EMULTYPE_PF_ALLOW_RETRYdoes sound better. I'm on the fence regarding shuffling the bits. If I were to shuffle the bits, I'd do a more thorough reorder so that the #UD and #PF types are consecutive, e.g.#define EMULTYPE_NO_DECODE (1 << 0) #define EMULTYPE_TRAP_UD (1 << 1) #define EMULTYPE_TRAP_UD_FORCED (1 << 2) #define EMULTYPE_SKIP (1 << 3) #define EMULTYPE_VMWARE_GP (1 << 4) #define EMULTYPE_PF (1 << 5) #define EMULTYPE_PF_ALLOW_RETRY (1 << 6)
Quoted 3 lines
Part of me really wants to do that, the other part of me thinks it's unnecessary thrash.
I'm fine with thorough reorder, it helps read the codes. It's up to Paolo, anyway.
Quoted 85 lines
Quoted 85 lines
Quoted 84 lines
arch/x86/include/asm/kvm_host.h +2
-0#define EMULTYPE_TRAP_UD_FORCED (1 << 4)#define EMULTYPE_VMWARE_GP (1 << 5) +#define EMULTYPE_PF (1 << 6) +int kvm_emulate_instruction (struct kvm_vcpu *vcpu,int emulation_type);int kvm_emulate_instruction_from_buffer (struct kvm_vcpu *vcpu,void *insn,int insn_len);arch/x86/kvm/mmu/mmu.c +2
-8@@ -5416,18 +5416,12 @@ EXPORT_SYMBOL_GPL(kvm_mmu_unprotect_page_virt);int kvm_mmu_page_fault (struct kvm_vcpu *vcpu,gpa_t cr2_or_gpa,u64 error_code,void *insn,int insn_len) { -int r, emulation_type =0 ; +int r, emulation_type =EMULTYPE_PF ;bool direct = vcpu->arch.mmu->direct_map;if (WARN_ON (!VALID_PAGE (vcpu->arch.mmu->root_hpa)))return RET_PF_RETRY ; -/* With shadow page tables, fault_address contains a GVA or nGPA. */ -if (vcpu->arch.mmu->direct_map) { - vcpu->arch.gpa_available = true; - vcpu->arch.gpa_val = cr2_or_gpa; - } - r =RET_PF_INVALID ;if (unlikely (error_code &PFERR_RSVD_MASK )) { r =handle_mmio_page_fault (vcpu, cr2_or_gpa, direct);@@ -5472,7 +5466,7 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 error_code,* for L1 isn't going to magically fix whatever issue cause L2 to fail. */ if (!mmio_info_in_cache (vcpu, cr2_or_gpa, direct) && !is_guest_mode (vcpu)) - emulation_type =EMULTYPE_ALLOW_RETRY ; + emulation_type |=EMULTYPE_ALLOW_RETRY_PF ; emulate:/* * On AMD platforms, under certain conditions insn_len may be zero on #NPF. arch/x86/kvm/x86.c +19
-6@@ -6483,10 +6483,11 @@ static bool reexecute_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,gpa_t gpa = cr2_or_gpa;kvm_pfn_t pfn; -if (!(emulation_type &EMULTYPE_ALLOW_RETRY )) +if (!(emulation_type &EMULTYPE_ALLOW_RETRY_PF ))return false; -if (WARN_ON_ONCE (is_guest_mode (vcpu))) +if (WARN_ON_ONCE (is_guest_mode (vcpu)) || +WARN_ON_ONCE (!(emulation_type &EMULTYPE_PF )))return false;if (!vcpu->arch.mmu->direct_map) {@@ -6574,10 +6575,11 @@ static bool retry_instruction(struct x86_emulate_ctxt *ctxt,*/ vcpu->arch.last_retry_eip = vcpu->arch.last_retry_addr =0 ; -if (!(emulation_type &EMULTYPE_ALLOW_RETRY )) +if (!(emulation_type &EMULTYPE_ALLOW_RETRY_PF ))return false; -if (WARN_ON_ONCE (is_guest_mode (vcpu))) +if (WARN_ON_ONCE (is_guest_mode (vcpu)) || +WARN_ON_ONCE (!(emulation_type &EMULTYPE_PF )))return false;if (x86_page_table_writing_insn (ctxt))@@ -6830,8 +6832,19 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, } restart: -/* Save the faulting GPA (cr2) in the address field */ - ctxt->exception.address = cr2_or_gpa; +if (emulation_type &EMULTYPE_PF ) { +/* Save the faulting GPA (cr2) in the address field */ + ctxt->exception.address = cr2_or_gpa; + +/* With shadow page tables, cr2 contains a GVA or nGPA. */ +if (vcpu->arch.mmu->direct_map) { + vcpu->arch.gpa_available = true; + vcpu->arch.gpa_val = cr2_or_gpa; + } + }else { +/* Sanitize the address out of an abundance of paranoia. */ + ctxt->exception.address =0 ; + } r =x86_emulate_insn (ctxt);
Paolo Bonzini
maintainerRe: [PATCH v2 1/3] KVM: x86: Add EMULTYPE_PF when emulation is triggered by a page fault
In reply to Sean ChristophersonQuoted 5 lines · Sean Christopherson
Quoted 2 lines
How about naming it as
EMULTYPE_PF_ALLOW_RETRYand exchanging the bit position withEMULTYPE_PF?Hmm,
EMULTYPE_PF_ALLOW_RETRYdoes sound better. I'm on the fence regarding shuffling the bits. If I were to shuffle the bits, I'd do a more thorough reorder so that the #UD and #PF types are consecutive, e.g.
Let's just change the name, I can do it.
Paolo
PATCH v2 2/3 KVM: x86: Move gpa_val and gpa_available into the emulator context
In reply to Sean ChristophersonMove the GPA tracking into the emulator context now that the context is guaranteed to be initialized via __init_emulate_ctxt() prior to dereferencing gpa_{available,val}, i.e. now that seeing a stale gpa_available will also trigger a WARN due to an invalid context.
Signed-off-by Sean Christopherson <[email protected]>
--- arch/x86/include/asm/kvm_emulate.h | 4 ++++ arch/x86/include/asm/kvm_host.h | 4 ---- arch/x86/kvm/x86.c | 13 ++++++------- 3 files changed, 10 insertions(+), 11 deletions(-)
arch/x86/include/asm/kvm_emulate.h +4 -0
@@ -311,6 +311,10 @@ struct x86_emulate_ctxt {bool have_exception;struct x86_exception exception; +/* GPA available */ +bool gpa_available; +gpa_t gpa_val; +/* * decode cache */
arch/x86/include/asm/kvm_host.h +0 -4
@@ -798,10 +798,6 @@ struct kvm_vcpu_arch {int pending_ioapic_eoi;int pending_external_vector; -/* GPA available */ -bool gpa_available; -gpa_t gpa_val; -/* be preempted when it's in kernel-mode(cpl=0) */ bool preempted_in_kernel;
arch/x86/kvm/x86.c +6 -7
@@ -5736,10 +5736,9 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,* operation using rep will only have the initial GPA from the NPF * occurred. */ -if (vcpu->arch.gpa_available && -emulator_can_use_gpa (ctxt) && - (addr & ~PAGE_MASK ) == (vcpu->arch.gpa_val & ~PAGE_MASK )) { - gpa = vcpu->arch.gpa_val; +if (ctxt->gpa_available &&emulator_can_use_gpa (ctxt) && + (addr & ~PAGE_MASK ) == (ctxt->gpa_val & ~PAGE_MASK )) { + gpa = ctxt->gpa_val; ret =vcpu_is_mmio_gpa (vcpu, addr, gpa, write); }else { ret =vcpu_mmio_gva_to_gpa (vcpu, addr, &gpa, exception, write);
@@ -6408,6 +6407,7 @@ static void init_emulate_ctxt(struct kvm_vcpu *vcpu) kvm_x86_ops->get_cs_db_l_bits (vcpu, &cs_db, &cs_l); + ctxt->gpa_available = false; ctxt->eflags =kvm_get_rflags (vcpu); ctxt->tf = (ctxt->eflags &X86_EFLAGS_TF ) !=0 ;
@@ -6838,8 +6838,8 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,/* With shadow page tables, cr2 contains a GVA or nGPA. */ if (vcpu->arch.mmu->direct_map) { - vcpu->arch.gpa_available = true; - vcpu->arch.gpa_val = cr2_or_gpa; + ctxt->gpa_available = true; + ctxt->gpa_val = cr2_or_gpa; } }else {/* Sanitize the address out of an abundance of paranoia. */
@@ -8443,7 +8443,6 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)if (vcpu->arch.apic_attention)kvm_lapic_sync_from_vapic (vcpu); - vcpu->arch.gpa_available = false; r = kvm_x86_ops->handle_exit (vcpu, exit_fastpath);return r;
Signature · 1 line
2.24.1
PATCH v2 3/3 KVM: x86: Move #PF retry tracking variables into emulation context
In reply to Sean ChristophersonMove last_retry_eip and last_retry_addr into the emulation context as they are specific to retrying an instruction after emulation failure.
Signed-off-by Sean Christopherson <[email protected]>
--- arch/x86/include/asm/kvm_emulate.h | 4 ++++ arch/x86/include/asm/kvm_host.h | 3 --- arch/x86/kvm/x86.c | 11 ++++++----- 3 files changed, 10 insertions(+), 8 deletions(-)
arch/x86/include/asm/kvm_emulate.h +4 -0
@@ -315,6 +315,10 @@ struct x86_emulate_ctxt {bool gpa_available;gpa_t gpa_val; +/* Track EIP and CR2/GPA when retrying faulting instruction on #PF. */ +unsigned long last_retry_eip; +unsigned long last_retry_addr; +/* * decode cache */
arch/x86/include/asm/kvm_host.h +0 -3
@@ -752,9 +752,6 @@ struct kvm_vcpu_arch {cpumask_var_t wbinvd_dirty_mask; -unsigned long last_retry_eip; -unsigned long last_retry_addr; -struct {bool halted;gfn_t gfns[roundup_pow_of_two (ASYNC_PF_PER_VCPU )];
arch/x86/kvm/x86.c +6 -5
@@ -6407,6 +6407,7 @@ static void init_emulate_ctxt(struct kvm_vcpu *vcpu) kvm_x86_ops->get_cs_db_l_bits (vcpu, &cs_db, &cs_l); +/* last_retry_{eip,addr} are persistent and must not be init'd here. */ ctxt->gpa_available = false; ctxt->eflags =kvm_get_rflags (vcpu); ctxt->tf = (ctxt->eflags &X86_EFLAGS_TF ) !=0 ;
@@ -6557,8 +6558,8 @@ static bool retry_instruction(struct x86_emulate_ctxt *ctxt,struct kvm_vcpu *vcpu =emul_to_vcpu (ctxt);unsigned long last_retry_eip, last_retry_addr, gpa = cr2_or_gpa; - last_retry_eip = vcpu->arch.last_retry_eip; - last_retry_addr = vcpu->arch.last_retry_addr; + last_retry_eip = ctxt->last_retry_eip; + last_retry_addr = ctxt->last_retry_addr;/* * If the emulation is caused by #PF and it is non-page_table
@@ -6573,7 +6574,7 @@ static bool retry_instruction(struct x86_emulate_ctxt *ctxt,* and the address again, we can break out of the potential infinite * loop. */ - vcpu->arch.last_retry_eip = vcpu->arch.last_retry_addr =0 ; + ctxt->last_retry_eip = ctxt->last_retry_addr =0 ;if (!(emulation_type &EMULTYPE_ALLOW_RETRY_PF ))return false;
@@ -6588,8 +6589,8 @@ static bool retry_instruction(struct x86_emulate_ctxt *ctxt,if (ctxt->eip == last_retry_eip && last_retry_addr == cr2_or_gpa)return false; - vcpu->arch.last_retry_eip = ctxt->eip; - vcpu->arch.last_retry_addr = cr2_or_gpa; + ctxt->last_retry_eip = ctxt->eip; + ctxt->last_retry_addr = cr2_or_gpa;if (!vcpu->arch.mmu->direct_map) gpa =kvm_mmu_gva_to_gpa_write (vcpu, cr2_or_gpa,NULL );
Signature · 1 line
2.24.1
Paolo Bonzini
maintainerRe: [PATCH v2 3/3] KVM: x86: Move #PF retry tracking variables into emulation context
Quoted 4 lines · Sean Christopherson
Move
last_retry_eipandlast_retry_addrinto the emulation context as they are specific to retrying an instruction after emulation failure.Signed-off-by Sean Christopherson <[email protected]>
I'm not sure about this, since it's not used by emulate.c. The other two patches are good.
Paolo
Quoted 79 lines
--- arch/x86/include/asm/kvm_emulate.h | 4 ++++ arch/x86/include/asm/kvm_host.h | 3 --- arch/x86/kvm/x86.c | 11 ++++++----- 3 files changed, 10 insertions(+), 8 deletions(-)arch/x86/include/asm/kvm_emulate.h +4
-0@@ -315,6 +315,10 @@ struct x86_emulate_ctxt {bool gpa_available;gpa_t gpa_val; +/* Track EIP and CR2/GPA when retrying faulting instruction on #PF. */ +unsigned long last_retry_eip; +unsigned long last_retry_addr; +/* * decode cache */ arch/x86/include/asm/kvm_host.h +0
-3@@ -752,9 +752,6 @@ struct kvm_vcpu_arch {cpumask_var_t wbinvd_dirty_mask; -unsigned long last_retry_eip; -unsigned long last_retry_addr; -struct {bool halted;gfn_t gfns[roundup_pow_of_two (ASYNC_PF_PER_VCPU )];arch/x86/kvm/x86.c +6
-5@@ -6407,6 +6407,7 @@ static void init_emulate_ctxt(struct kvm_vcpu *vcpu) kvm_x86_ops->get_cs_db_l_bits (vcpu, &cs_db, &cs_l); +/* last_retry_{eip,addr} are persistent and must not be init'd here. */ ctxt->gpa_available = false; ctxt->eflags =kvm_get_rflags (vcpu); ctxt->tf = (ctxt->eflags &X86_EFLAGS_TF ) !=0 ;@@ -6557,8 +6558,8 @@ static bool retry_instruction(struct x86_emulate_ctxt *ctxt,struct kvm_vcpu *vcpu =emul_to_vcpu (ctxt);unsigned long last_retry_eip, last_retry_addr, gpa = cr2_or_gpa; - last_retry_eip = vcpu->arch.last_retry_eip; - last_retry_addr = vcpu->arch.last_retry_addr; + last_retry_eip = ctxt->last_retry_eip; + last_retry_addr = ctxt->last_retry_addr;/* * If the emulation is caused by #PF and it is non-page_table @@ -6573,7 +6574,7 @@ static bool retry_instruction(struct x86_emulate_ctxt *ctxt,* and the address again, we can break out of the potential infinite * loop. */ - vcpu->arch.last_retry_eip = vcpu->arch.last_retry_addr =0 ; + ctxt->last_retry_eip = ctxt->last_retry_addr =0 ;if (!(emulation_type &EMULTYPE_ALLOW_RETRY_PF ))return false;@@ -6588,8 +6589,8 @@ static bool retry_instruction(struct x86_emulate_ctxt *ctxt,if (ctxt->eip == last_retry_eip && last_retry_addr == cr2_or_gpa)return false; - vcpu->arch.last_retry_eip = ctxt->eip; - vcpu->arch.last_retry_addr = cr2_or_gpa; + ctxt->last_retry_eip = ctxt->eip; + ctxt->last_retry_addr = cr2_or_gpa;if (!vcpu->arch.mmu->direct_map) gpa =kvm_mmu_gva_to_gpa_write (vcpu, cr2_or_gpa,NULL );
Re: [PATCH v2 3/3] KVM: x86: Move #PF retry tracking variables into emulation context
Quoted 8 lines · Paolo Bonzini
Quoted 4 lines · Sean Christopherson
Move
last_retry_eipandlast_retry_addrinto the emulation context as they are specific to retrying an instruction after emulation failure.Signed-off-by Sean Christopherson <[email protected]>I'm not sure about this, since it's not used by
emulate.c. The other two patches are good.
The easy solution to that is to move retry_instruction() into emulate.c. That would also allow making x86_page_table_writing_insn() static. All other functions invoked from retry_instruction() are exposed via kvm_host.h.
Moving last_retry_* into the emulator context hopefully makes it more clear that this code is the only user of the variables, e.g. last_retry_eip can't be set by some other non-emulator flow.
if (ctxt->eip == last_retry_eip && last_retry_addr == cr2) return false; vcpu->arch.last_retry_eip = ctxt->eip; vcpu->arch.last_retry_addr = cr2;
Quoted 80 lines
Quoted 79 lines
--- arch/x86/include/asm/kvm_emulate.h | 4 ++++ arch/x86/include/asm/kvm_host.h | 3 --- arch/x86/kvm/x86.c | 11 ++++++----- 3 files changed, 10 insertions(+), 8 deletions(-)arch/x86/include/asm/kvm_emulate.h +4
-0@@ -315,6 +315,10 @@ struct x86_emulate_ctxt {bool gpa_available;gpa_t gpa_val; +/* Track EIP and CR2/GPA when retrying faulting instruction on #PF. */ +unsigned long last_retry_eip; +unsigned long last_retry_addr; +/* * decode cache */ arch/x86/include/asm/kvm_host.h +0
-3@@ -752,9 +752,6 @@ struct kvm_vcpu_arch {cpumask_var_t wbinvd_dirty_mask; -unsigned long last_retry_eip; -unsigned long last_retry_addr; -struct {bool halted;gfn_t gfns[roundup_pow_of_two (ASYNC_PF_PER_VCPU )];arch/x86/kvm/x86.c +6
-5@@ -6407,6 +6407,7 @@ static void init_emulate_ctxt(struct kvm_vcpu *vcpu) kvm_x86_ops->get_cs_db_l_bits (vcpu, &cs_db, &cs_l); +/* last_retry_{eip,addr} are persistent and must not be init'd here. */ ctxt->gpa_available = false; ctxt->eflags =kvm_get_rflags (vcpu); ctxt->tf = (ctxt->eflags &X86_EFLAGS_TF ) !=0 ;@@ -6557,8 +6558,8 @@ static bool retry_instruction(struct x86_emulate_ctxt *ctxt,struct kvm_vcpu *vcpu =emul_to_vcpu (ctxt);unsigned long last_retry_eip, last_retry_addr, gpa = cr2_or_gpa; - last_retry_eip = vcpu->arch.last_retry_eip; - last_retry_addr = vcpu->arch.last_retry_addr; + last_retry_eip = ctxt->last_retry_eip; + last_retry_addr = ctxt->last_retry_addr;/* * If the emulation is caused by #PF and it is non-page_table @@ -6573,7 +6574,7 @@ static bool retry_instruction(struct x86_emulate_ctxt *ctxt,* and the address again, we can break out of the potential infinite * loop. */ - vcpu->arch.last_retry_eip = vcpu->arch.last_retry_addr =0 ; + ctxt->last_retry_eip = ctxt->last_retry_addr =0 ;if (!(emulation_type &EMULTYPE_ALLOW_RETRY_PF ))return false;@@ -6588,8 +6589,8 @@ static bool retry_instruction(struct x86_emulate_ctxt *ctxt,if (ctxt->eip == last_retry_eip && last_retry_addr == cr2_or_gpa)return false; - vcpu->arch.last_retry_eip = ctxt->eip; - vcpu->arch.last_retry_addr = cr2_or_gpa; + ctxt->last_retry_eip = ctxt->eip; + ctxt->last_retry_addr = cr2_or_gpa;if (!vcpu->arch.mmu->direct_map) gpa =kvm_mmu_gva_to_gpa_write (vcpu, cr2_or_gpa,NULL );
Paolo Bonzini
maintainerRe: [PATCH v2 3/3] KVM: x86: Move #PF retry tracking variables into emulation context
Quoted 3 lines · Sean Christopherson
The easy solution to that is to move
retry_instruction()intoemulate.c. That would also allow makingx86_page_table_writing_insn()static. All other functions invoked fromretry_instruction()are exposed viakvm_host.h.
emulate.c is supposed to invoke no (or almost no) function outside the ctxt->ops struct. In particular, retry_instruction() invokes kvm_mmu_gva_to_gpa_write and kvm_mmu_unprotect_page.
Paolo
Re: [PATCH v2 3/3] KVM: x86: Move #PF retry tracking variables into emulation context
Quoted 8 lines · Paolo Bonzini
Quoted 3 lines · Sean Christopherson
The easy solution to that is to move
retry_instruction()intoemulate.c. That would also allow makingx86_page_table_writing_insn()static. All other functions invoked fromretry_instruction()are exposed viakvm_host.h.
emulate.cis supposed to invoke no (or almost no) function outside thectxt->opsstruct. In particular,retry_instruction()invokeskvm_mmu_gva_to_gpa_writeandkvm_mmu_unprotect_page.
Ah, right. We could split the logic, e.g.
if (x86_retry_pf_instruction(ctxt, cr2_or_gpa, emulation_type)) {
gpa_t = gpa = cr2_or_gpa;
if (!vcpu->arch.mmu->direct_map)
gpa = kvm_mmu_gva_to_gpa_write(vcpu, cr2_or_gpa, NULL);
kvm_mmu_unprotect_page(vcpu->kvm, gpa_to_gfn(gpa));
return 1;
}but that's probably a net negative in terms of clarity. And there's also vcpu->arch.write_fault_to_shadow_pgtable, which is consumed only by reexecute_instruction(), and I 100% agree that that variable should stay in vcpu->arch. Moving one flag used to retry #PF instructions and not the other would be weird.
That was a long winded way of saying I agree we should drop this patch :-)