KVM: x86: Push potential exception error code on task switches
When a fault triggers a task switch, the error code, if it exists, has to be pushed on the new task's stack. Implement the missing bits.
--- arch/x86/include/asm/kvm_emulate.h | 3 ++- arch/x86/include/asm/kvm_host.h | 3 ++- arch/x86/include/asm/svm.h | 1 + arch/x86/kvm/emulate.c | 21 +++++++++++++++++---- arch/x86/kvm/svm.c | 11 ++++++++++- arch/x86/kvm/vmx.c | 12 +++++++++++- arch/x86/kvm/x86.c | 6 ++++-- 7 files changed, 47 insertions(+), 10 deletions(-)
A version for stable / .34 exists as well and will follow along with the back-ported TSS size check fix. A micro-test for qemu-kvm exists as well and will be posted. That test interestingly seems to exposes a regression on PIO string handling in master.
arch/x86/include/asm/kvm_emulate.h +2 -1
@@ -230,6 +230,7 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops);int emulator_task_switch (struct x86_emulate_ctxt *ctxt,struct x86_emulate_ops *ops, -u16 tss_selector,int reason); +u16 tss_selector,int reason, +bool has_error_code,u32 error_code);#endif /* _ASM_X86_KVM_X86_EMULATE_H */
arch/x86/include/asm/kvm_host.h +2 -1
@@ -596,7 +596,8 @@ int emulator_set_dr(struct x86_emulate_ctxt *ctxt, int dr,void kvm_get_segment (struct kvm_vcpu *vcpu,struct kvm_segment *var,int seg);int kvm_load_segment_descriptor (struct kvm_vcpu *vcpu,u16 selector,int seg); -int kvm_task_switch (struct kvm_vcpu *vcpu,u16 tss_selector,int reason); +int kvm_task_switch (struct kvm_vcpu *vcpu,u16 tss_selector,int reason, +bool has_error_code,u32 error_code);void kvm_set_cr0 (struct kvm_vcpu *vcpu,unsigned long cr0);void kvm_set_cr3 (struct kvm_vcpu *vcpu,unsigned long cr3);
arch/x86/include/asm/svm.h +1 -0
@@ -242,6 +242,7 @@ struct __attribute__ ((__packed__)) vmcb {#define SVM_EXITINFOSHIFT_TS_REASON_IRET 36#define SVM_EXITINFOSHIFT_TS_REASON_JMP 38 +#define SVM_EXITINFOSHIFT_TS_HAS_ERROR_CODE 44#define SVM_EXIT_READ_CR0 0x000#define SVM_EXIT_READ_CR3 0x003
arch/x86/kvm/emulate.c +17 -4
@@ -2344,8 +2344,9 @@ static int task_switch_32(struct x86_emulate_ctxt *ctxt, }static int emulator_do_task_switch (struct x86_emulate_ctxt *ctxt, -struct x86_emulate_ops *ops, -u16 tss_selector,int reason) +struct x86_emulate_ops *ops, +u16 tss_selector,int reason, +bool has_error_code,u32 error_code) {struct desc_struct curr_tss_desc, next_tss_desc;int ret;
@@ -2416,12 +2417,23 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, ops->set_cached_descriptor (&next_tss_desc,VCPU_SREG_TR , ctxt->vcpu); ops->set_segment_selector (tss_selector,VCPU_SREG_TR , ctxt->vcpu); +if (ret ==X86EMUL_CONTINUE && has_error_code) { +struct decode_cache *c = &ctxt->decode; + + c->op_bytes = c->ad_bytes = (next_tss_desc.type &8 ) ?4 :2 ; + c->lock_prefix =0 ; + c->src.val = (unsigned long ) error_code; +emulate_push (ctxt); + ret =writeback (ctxt, ops); + } +return ret; }int emulator_task_switch (struct x86_emulate_ctxt *ctxt,struct x86_emulate_ops *ops, -u16 tss_selector,int reason) +u16 tss_selector,int reason, +bool has_error_code,u32 error_code) {struct decode_cache *c = &ctxt->decode;int rc;
@@ -2430,7 +2442,8 @@ int emulator_task_switch(struct x86_emulate_ctxt *ctxt, c->eip = ctxt->eip;memcpy (c->regs, ctxt->vcpu->arch.regs,sizeof c->regs); - rc =emulator_do_task_switch (ctxt, ops, tss_selector, reason); + rc =emulator_do_task_switch (ctxt, ops, tss_selector, reason, + has_error_code, error_code);if (rc ==X86EMUL_CONTINUE ) {memcpy (ctxt->vcpu->arch.regs, c->regs,sizeof c->regs);
arch/x86/kvm/svm.c +10 -1
@@ -2277,6 +2277,8 @@ static int task_switch_interception(struct vcpu_svm *svm) svm->vmcb->control.exit_int_info &SVM_EXITINTINFO_TYPE_MASK ;uint32_t idt_v = svm->vmcb->control.exit_int_info &SVM_EXITINTINFO_VALID ; +bool has_error_code = false; +u32 error_code =0 ; tss_selector = (u16 )svm->vmcb->control.exit_info_1;
@@ -2297,6 +2299,12 @@ static int task_switch_interception(struct vcpu_svm *svm) svm->vcpu.arch.nmi_injected = false;break ;case SVM_EXITINTINFO_TYPE_EXEPT : +if (svm->vmcb->control.exit_info_2 & + (1ULL <<SVM_EXITINFOSHIFT_TS_HAS_ERROR_CODE )) { + has_error_code = true; + error_code = + (u32 )svm->vmcb->control.exit_info_2; + }kvm_clear_exception_queue (&svm->vcpu);break ;case SVM_EXITINTINFO_TYPE_INTR :
@@ -2313,7 +2321,8 @@ static int task_switch_interception(struct vcpu_svm *svm) (int_vec == OF_VECTOR || int_vec == BP_VECTOR)))skip_emulated_instruction (&svm->vcpu); -return kvm_task_switch (&svm->vcpu, tss_selector, reason); +return kvm_task_switch (&svm->vcpu, tss_selector, reason, + has_error_code, error_code); }static int cpuid_interception (struct vcpu_svm *svm)
arch/x86/kvm/vmx.c +11 -1
@@ -3327,6 +3327,8 @@ static int handle_task_switch(struct kvm_vcpu *vcpu) {struct vcpu_vmx *vmx =to_vmx (vcpu);unsigned long exit_qualification; +bool has_error_code = false; +u32 error_code =0 ;u16 tss_selector;int reason, type, idt_v;
@@ -3349,6 +3351,13 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)kvm_clear_interrupt_queue (vcpu);break ;case INTR_TYPE_HARD_EXCEPTION : +if (vmx->idt_vectoring_info & +VECTORING_INFO_DELIVER_CODE_MASK ) { + has_error_code = true; + error_code = +vmcs_read32 (IDT_VECTORING_ERROR_CODE ); + } +/* fall through */ case INTR_TYPE_SOFT_EXCEPTION :kvm_clear_exception_queue (vcpu);break ;
@@ -3363,7 +3372,8 @@ static int handle_task_switch(struct kvm_vcpu *vcpu) type != INTR_TYPE_NMI_INTR))skip_emulated_instruction (vcpu); -if (!kvm_task_switch (vcpu, tss_selector, reason)) +if (!kvm_task_switch (vcpu, tss_selector, reason, has_error_code, + error_code))return 0 ;/* clear all local breakpoint enable flags */
arch/x86/kvm/x86.c +4 -2
@@ -4717,7 +4717,8 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,return 0 ; } -int kvm_task_switch (struct kvm_vcpu *vcpu,u16 tss_selector,int reason) +int kvm_task_switch (struct kvm_vcpu *vcpu,u16 tss_selector,int reason, +bool has_error_code,u32 error_code) {int cs_db, cs_l, ret;cache_all_regs (vcpu);
@@ -4735,7 +4736,8 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int reason) ? X86EMUL_MODE_PROT32 : X86EMUL_MODE_PROT16; ret =emulator_task_switch (&vcpu->arch.emulate_ctxt, &emulate_ops, - tss_selector, reason); + tss_selector, reason, has_error_code, + error_code);if (ret ==X86EMUL_CONTINUE ) kvm_x86_ops->set_rflags (vcpu, vcpu->arch.emulate_ctxt.eflags);
arch/x86/kvm/emulate.c static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, · line 2424
Quoted 13 lines · Jan Kiszka
When a fault triggers a task switch, the error code, if it exists, has to be pushed on the new task's stack. Implement the missing bits.
arch/x86/kvm/emulate.c +4
-0@@ -2416,12 +2417,23 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, ops->set_cached_descriptor (&next_tss_desc,VCPU_SREG_TR , ctxt->vcpu); ops->set_segment_selector (tss_selector,VCPU_SREG_TR , ctxt->vcpu); +if (ret ==X86EMUL_CONTINUE && has_error_code) { +struct decode_cache *c =&ctxt->decode; + + c->op_bytes = c->ad_bytes = (next_tss_desc.type&8 ) ?4 :2 ;
Don't these depend on the attributes of the segment as well?
Quoted 10 lines
arch/x86/kvm/emulate.c +6
-0+ c->lock_prefix =0 ; + c->src.val = (unsigned long ) error_code; +emulate_push (ctxt); + ret =writeback (ctxt, ops); + } +return ret; }
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 9 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
When a fault triggers a task switch, the error code, if it exists, has to be pushed on the new task's stack. Implement the missing bits.
arch/x86/kvm/emulate.c +0
-0@@ -2416,12 +2417,23 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, ops->set_cached_descriptor (&next_tss_desc,VCPU_SREG_TR , ctxt->vcpu); ops->set_segment_selector (tss_selector,VCPU_SREG_TR , ctxt->vcpu);
Quoted 7 lines
Review static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, · line 2424
Quoted 5 lines
(quoted diff) +4
-0+ if (ret == X86EMUL_CONTINUE&& has_error_code) { + struct decode_cache *c =&ctxt->decode; + + c->op_bytes = c->ad_bytes = (next_tss_desc.type& 8) ? 4 : 2;Don't these depend on the attributes of the segment as well?
Not on the segment, but actually on the gate size. Will fix.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 10 lines · Jan Kiszka
Quoted 9 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
When a fault triggers a task switch, the error code, if it exists, has to be pushed on the new task's stack. Implement the missing bits.
arch/x86/kvm/emulate.c +0
-0@@ -2416,12 +2417,23 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, ops->set_cached_descriptor (&next_tss_desc,VCPU_SREG_TR , ctxt->vcpu); ops->set_segment_selector (tss_selector,VCPU_SREG_TR , ctxt->vcpu);
Quoted 8 lines
Quoted 6 lines
Review static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, · line 2424
Quoted 5 lines
(quoted diff) +4
-0+ if (ret == X86EMUL_CONTINUE&& has_error_code) { + struct decode_cache *c =&ctxt->decode; + + c->op_bytes = c->ad_bytes = (next_tss_desc.type& 8) ? 4 : 2;Don't these depend on the attributes of the segment as well?
Not on the segment, but actually on the gate size. Will fix.
Err, non-sense. Nothing to fix, that's already the case.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 20 lines · Jan Kiszka
Quoted 10 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
When a fault triggers a task switch, the error code, if it exists, has to be pushed on the new task's stack. Implement the missing bits.
arch/x86/kvm/emulate.c +0
-0@@ -2416,12 +2417,23 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, ops->set_cached_descriptor (&next_tss_desc,VCPU_SREG_TR , ctxt->vcpu); ops->set_segment_selector (tss_selector,VCPU_SREG_TR , ctxt->vcpu);Quoted 8 lines
Review static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, · line 2425
Quoted 6 lines
(quoted diff) +4
-0+ if (ret == X86EMUL_CONTINUE&& has_error_code) { + struct decode_cache *c =&ctxt->decode; + + c->op_bytes = c->ad_bytes = (next_tss_desc.type& 8) ? 4 : 2;Don't these depend on the attributes of the segment as well?
Quoted 2 lines
Not on the segment, but actually on the gate size. Will fix.
The TSS descriptor (gate doesn't have a size). But isn't it possible to have a 32-bit TSS with a 16-bit CS/SS?
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 21 lines · Avi Kivity
Quoted 20 lines · Jan Kiszka
Quoted 18 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
When a fault triggers a task switch, the error code, if it exists, has to be pushed on the new task's stack. Implement the missing bits.
arch/x86/kvm/emulate.c static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, · line 2425
Quoted 9 lines
arch/x86/kvm/emulate.c +4
-0@@ -2416,12 +2417,23 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, ops->set_cached_descriptor (&next_tss_desc,VCPU_SREG_TR , ctxt->vcpu); ops->set_segment_selector (tss_selector,VCPU_SREG_TR , ctxt->vcpu); +if (ret ==X86EMUL_CONTINUE && has_error_code) { +struct decode_cache *c =&ctxt->decode; + + c->op_bytes = c->ad_bytes = (next_tss_desc.type&8 ) ?4 :2 ;Don't these depend on the attributes of the segment as well?
Quoted 5 lines
Quoted 2 lines
Not on the segment, but actually on the gate size. Will fix.
The TSS descriptor (gate doesn't have a size). But isn't it possible to have a 32-bit TSS with a 16-bit CS/SS?
Might be possible, but will cause troubles as the spec says:
"The error code is pushed on the stack as a doubleword or word (depending on the default interrupt, trap, or task gate size)."
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 9 lines · Jan Kiszka
Quoted 4 lines
The TSS descriptor (gate doesn't have a size). But isn't it possible to have a 32-bit TSS with a 16-bit CS/SS?
Might be possible, but will cause troubles as the spec says:
"The error code is pushed on the stack as a doubleword or word (depending on the default interrupt, trap, or task gate size)."
My guess is that this is an error and that the 32-bitness of a TSS only refers to the format of the TSS, and has nothing to do with the code that actually runs. I'll ask Intel about it. Meanwhile this can be applied, if there's a problem with 16-bit exception handlers running through a 32-bit task referenced by a task gate in the IDT, it can be fixed later.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 11 lines · Avi Kivity
Quoted 7 lines · Jan Kiszka
Quoted 3 lines
The TSS descriptor (gate doesn't have a size). But isn't it possible to have a 32-bit TSS with a 16-bit CS/SS?
Might be possible, but will cause troubles as the spec says:
"The error code is pushed on the stack as a doubleword or word (depending on the default interrupt, trap, or task gate size)."
My guess is that this is an error and that the 32-bitness of a TSS only refers to the format of the TSS, and has nothing to do with the code that actually runs. I'll ask Intel about it. Meanwhile this can be applied, if there's a problem with 16-bit exception handlers running through a 32-bit task referenced by a task gate in the IDT, it can be fixed later.
I meant, after Gleb's concerns are addressed.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 10 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
Quoted 3 lines
The TSS descriptor (gate doesn't have a size). But isn't it possible to have a 32-bit TSS with a 16-bit CS/SS?
Might be possible, but will cause troubles as the spec says:
"The error code is pushed on the stack as a doubleword or word (depending on the default interrupt, trap, or task gate size)."
Quoted 6 lines
My guess is that this is an error and that the 32-bitness of a TSS only refers to the format of the TSS, and has nothing to do with the code that actually runs. I'll ask Intel about it. Meanwhile this can be applied, if there's a problem with 16-bit exception handlers running through a 32-bit task referenced by a task gate in the IDT, it can be fixed later.
Go ahead. But architecturally this looks fairly consistent to me as the processor simply derives the error code width from the corresponding entry in the IDT.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 21 lines · Jan Kiszka
Quoted 12 lines · Avi Kivity
Quoted 10 lines · Jan Kiszka
Quoted 4 lines
The TSS descriptor (gate doesn't have a size). But isn't it possible to have a 32-bit TSS with a 16-bit CS/SS?
Might be possible, but will cause troubles as the spec says:
"The error code is pushed on the stack as a doubleword or word (depending on the default interrupt, trap, or task gate size)."
Quoted 7 lines
My guess is that this is an error and that the 32-bitness of a TSS only refers to the format of the TSS, and has nothing to do with the code that actually runs. I'll ask Intel about it. Meanwhile this can be applied, if there's a problem with 16-bit exception handlers running through a 32-bit task referenced by a task gate in the IDT, it can be fixed later.
Quoted 4 lines
Go ahead. But architecturally this looks fairly consistent to me as the processor simply derives the error code width from the corresponding entry in the IDT.
You are correct (though the entry isn't in the IDT!)
Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.
Quoted 22 lines · Avi Kivity
Quoted 21 lines · Jan Kiszka
Quoted 19 lines · Avi Kivity
Quoted 10 lines · Jan Kiszka
Quoted 4 lines
The TSS descriptor (gate doesn't have a size). But isn't it possible to have a 32-bit TSS with a 16-bit CS/SS?
Might be possible, but will cause troubles as the spec says:
"The error code is pushed on the stack as a doubleword or word (depending on the default interrupt, trap, or task gate size)."
My guess is that this is an error and that the 32-bitness of a TSS only refers to the format of the TSS, and has nothing to do with the code that actually runs. I'll ask Intel about it. Meanwhile this can be applied, if there's a problem with 16-bit exception handlers running through a 32-bit task referenced by a task gate in the IDT, it can be fixed later.
Quoted 6 lines
Quoted 4 lines
Go ahead. But architecturally this looks fairly consistent to me as the processor simply derives the error code width from the corresponding entry in the IDT.
You are correct (though the entry isn't in the IDT!)
Yeah, right (this stuff keeps on hurting my brain).
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 10 lines · Jan Kiszka
static int emulator_do_task_switch(struct
x86_emulate_ctxt*ctxt,arch/x86/kvm/emulate.c +3
-2-struct x86_emulate_ops *ops, - u16 tss_selector, int reason) +struct x86_emulate_ops *ops, + u16 tss_selector, int reason, + bool has_error_code, u32 error_code) { struct desc_struct curr_tss_desc, next_tss_desc;int ret;
arch/x86/kvm/emulate.c static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, · line 2420
Quoted 4 lines
arch/x86/kvm/emulate.c +1
-0@@ -2416,12 +2417,23 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, ops->set_cached_descriptor (&next_tss_desc,VCPU_SREG_TR , ctxt->vcpu); ops->set_segment_selector (tss_selector,VCPU_SREG_TR , ctxt->vcpu); +if (ret ==X86EMUL_CONTINUE && has_error_code) {
It looks like we shouldn't get here if ret != X86EMUL_CONTINUE in the first place. This check should be done just after call to task_switch_16/32. Not directly related to your patch, but still...
Quoted 7 lines
arch/x86/kvm/emulate.c +0
-0@@ -2416,12 +2417,23 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, ops->set_cached_descriptor (&next_tss_desc,VCPU_SREG_TR , ctxt->vcpu); ops->set_segment_selector (tss_selector,VCPU_SREG_TR , ctxt->vcpu);
arch/x86/kvm/emulate.c static int emulator_do_task_switch(struct · line 2432
Quoted 10 lines
arch/x86/kvm/emulate.c +9
-0+if (ret ==X86EMUL_CONTINUE && has_error_code) { +struct decode_cache *c = &ctxt->decode; + + c->op_bytes = c->ad_bytes = (next_tss_desc.type &8 ) ?4 :2 ; + c->lock_prefix =0 ; + c->src.val = (unsigned long ) error_code; +emulate_push (ctxt); + ret =writeback (ctxt, ops); + }
I would move writeback() to emulator_task_switch(). Just make c->dst.type = OP_NONE if writeback is not needed.
--
Gleb.
Quoted 14 lines · Gleb Natapov
Quoted 13 lines · Jan Kiszka
static int emulator_do_task_switch(struct
x86_emulate_ctxt*ctxt,arch/x86/kvm/emulate.c +3
-2-struct x86_emulate_ops *ops, - u16 tss_selector, int reason) +struct x86_emulate_ops *ops, + u16 tss_selector, int reason, + bool has_error_code, u32 error_code) { struct desc_struct curr_tss_desc, next_tss_desc;int ret;@@ -2416,12 +2417,23 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, ops->set_cached_descriptor (&next_tss_desc,VCPU_SREG_TR , ctxt->vcpu); ops->set_segment_selector (tss_selector,VCPU_SREG_TR , ctxt->vcpu);
Quoted 4 lines
arch/x86/kvm/emulate.c static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, · line 2420
Quoted 1 line
arch/x86/kvm/emulate.c +1
-0+if (ret ==X86EMUL_CONTINUE && has_error_code) {It looks like we shouldn't get here if ret !=
X86EMUL_CONTINUEin the first place. This check should be done just after call to task_switch_16/32. Not directly related to your patch, but still...
Will do this in a preparational patch.
Quoted 8 lines
Quoted 8 lines
(quoted diff) +0
-0@@ -2416,12 +2417,23 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, ops->set_cached_descriptor(&next_tss_desc, VCPU_SREG_TR, ctxt->vcpu); ops->set_segment_selector(tss_selector, VCPU_SREG_TR, ctxt->vcpu);
Quoted 12 lines
arch/x86/kvm/emulate.c static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, · line 2428
Quoted 10 lines
arch/x86/kvm/emulate.c +9
-0+if (ret ==X86EMUL_CONTINUE && has_error_code) { +struct decode_cache *c = &ctxt->decode; + + c->op_bytes = c->ad_bytes = (next_tss_desc.type &8 ) ?4 :2 ; + c->lock_prefix =0 ; + c->src.val = (unsigned long ) error_code; +emulate_push (ctxt); + ret =writeback (ctxt, ops); + }I would move
writeback()toemulator_task_switch(). Just makec->dst.type=OP_NONEif writeback is not needed.
I should dramatically increase the overhead for the common case. :) Yeah, can do so if preferred.
Thanks,
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 26 lines · Gleb Natapov
Quoted 10 lines · Jan Kiszka
static int emulator_do_task_switch(struct
x86_emulate_ctxt*ctxt,arch/x86/kvm/emulate.c +3
-2-struct x86_emulate_ops *ops, - u16 tss_selector, int reason) +struct x86_emulate_ops *ops, + u16 tss_selector, int reason, + bool has_error_code, u32 error_code) { struct desc_struct curr_tss_desc, next_tss_desc;int ret;arch/x86/kvm/emulate.c static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, · line 2420
Quoted 4 lines
arch/x86/kvm/emulate.c +1
-0@@ -2416,12 +2417,23 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, ops->set_cached_descriptor (&next_tss_desc,VCPU_SREG_TR , ctxt->vcpu); ops->set_segment_selector (tss_selector,VCPU_SREG_TR , ctxt->vcpu); +if (ret ==X86EMUL_CONTINUE && has_error_code) {It looks like we shouldn't get here if ret !=
X86EMUL_CONTINUEin the first place. This check should be done just after call to task_switch_16/32. Not directly related to your patch, but still...Quoted 7 lines
(quoted diff) +0
-0@@ -2416,12 +2417,23 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, ops->set_cached_descriptor(&next_tss_desc, VCPU_SREG_TR, ctxt->vcpu); ops->set_segment_selector(tss_selector, VCPU_SREG_TR, ctxt->vcpu);
Quoted 12 lines
arch/x86/kvm/emulate.c static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, · line 2428
Quoted 10 lines
arch/x86/kvm/emulate.c +9
-0+if (ret ==X86EMUL_CONTINUE && has_error_code) { +struct decode_cache *c = &ctxt->decode; + + c->op_bytes = c->ad_bytes = (next_tss_desc.type &8 ) ?4 :2 ; + c->lock_prefix =0 ; + c->src.val = (unsigned long ) error_code; +emulate_push (ctxt); + ret =writeback (ctxt, ops); + }I would move
writeback()toemulator_task_switch(). Just makec->dst.type=OP_NONEif writeback is not needed.
BTW, how is state rollback realized if one of the steps raises an exception? Or where are the new state bits saved until the whole operation has succeeded so that they can be applied?
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 40 lines · Jan Kiszka
Quoted 26 lines · Gleb Natapov
Quoted 10 lines · Jan Kiszka
static int emulator_do_task_switch(struct
x86_emulate_ctxt*ctxt,arch/x86/kvm/emulate.c +3
-2-struct x86_emulate_ops *ops, - u16 tss_selector, int reason) +struct x86_emulate_ops *ops, + u16 tss_selector, int reason, + bool has_error_code, u32 error_code) { struct desc_struct curr_tss_desc, next_tss_desc;int ret;arch/x86/kvm/emulate.c static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, · line 2420
Quoted 4 lines
arch/x86/kvm/emulate.c +1
-0@@ -2416,12 +2417,23 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, ops->set_cached_descriptor (&next_tss_desc,VCPU_SREG_TR , ctxt->vcpu); ops->set_segment_selector (tss_selector,VCPU_SREG_TR , ctxt->vcpu); +if (ret ==X86EMUL_CONTINUE && has_error_code) {It looks like we shouldn't get here if ret !=
X86EMUL_CONTINUEin the first place. This check should be done just after call to task_switch_16/32. Not directly related to your patch, but still...Quoted 7 lines
(quoted diff) +0
-0@@ -2416,12 +2417,23 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, ops->set_cached_descriptor(&next_tss_desc, VCPU_SREG_TR, ctxt->vcpu); ops->set_segment_selector(tss_selector, VCPU_SREG_TR, ctxt->vcpu);Quoted 12 lines
arch/x86/kvm/emulate.c static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, · line 2428
Quoted 10 lines
arch/x86/kvm/emulate.c +9
-0+if (ret ==X86EMUL_CONTINUE && has_error_code) { +struct decode_cache *c = &ctxt->decode; + + c->op_bytes = c->ad_bytes = (next_tss_desc.type &8 ) ?4 :2 ; + c->lock_prefix =0 ; + c->src.val = (unsigned long ) error_code; +emulate_push (ctxt); + ret =writeback (ctxt, ops); + }I would move
writeback()toemulator_task_switch(). Just makec->dst.type=OP_NONEif writeback is not needed.
Quoted 4 lines
BTW, how is state rollback realized if one of the steps raises an exception? Or where are the new state bits saved until the whole operation has succeeded so that they can be applied?
Currently task switch code doesn't handle exception during task switch absolutely correct. Task switch has three distinct steps. During first one exceptions are delivered in a context of an old task, if exception happens during second step arch state is undefined and finally if exception happens during third step they are delivered in the context of a new task.
--
Gleb.