KVM: x86: Push potential exception error code on task switches

KVM: x86: Push potential exception error code on task switches

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

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 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.

diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index a1319c8..0b2729b 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -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 */
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 0c49c88..d767882 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -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);
diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index b26a38d..4ccd546 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -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
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 083b269..153750e 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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);
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index d04c7ad..df831c8 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -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)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 2bd30b9..b75152a 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -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 */
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index bffd049..79025c2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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);

Re: KVM: x86: Push potential exception error code on task switches

[Avi Kivity]

On 04/14/2010 03:11 PM, Jan Kiszka wrote:
> 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.
>
>
> @@ -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?

> +		c->lock_prefix = 0;
> +		c->src.val = (unsigned long) error_code;
> +		emulate_push(ctxt);
> +		ret = writeback(ctxt, ops);
> +	}
> +
>   	return ret;
>   }
>
>    

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

Re: KVM: x86: Push potential exception error code on task switches

[Jan Kiszka]

Avi Kivity wrote:
> On 04/14/2010 03:11 PM, Jan Kiszka wrote:
>> 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.
>>
>>
>> @@ -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?

Not on the segment, but actually on the gate size. Will fix.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: KVM: x86: Push potential exception error code on task switches

[Jan Kiszka]

Jan Kiszka wrote:
> Avi Kivity wrote:
>> On 04/14/2010 03:11 PM, Jan Kiszka wrote:
>>> 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.
>>>
>>>
>>> @@ -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?
> 
> Not on the segment, but actually on the gate size. Will fix.

Err, non-sense. Nothing to fix, that's already the case.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: KVM: x86: Push potential exception error code on task switches

[Avi Kivity]

On 04/14/2010 03:37 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 04/14/2010 03:11 PM, Jan Kiszka wrote:
>>      
>>> 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.
>>>
>>>
>>> @@ -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?
>>      
> 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?

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

Re: KVM: x86: Push potential exception error code on task switches

[Jan Kiszka]

Avi Kivity wrote:
> On 04/14/2010 03:37 PM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>    
>>> On 04/14/2010 03:11 PM, Jan Kiszka wrote:
>>>      
>>>> 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.
>>>>
>>>>
>>>> @@ -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?
>>>      
>> 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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: KVM: x86: Push potential exception error code on task switches

[Avi Kivity]

On 04/14/2010 03:58 PM, Jan Kiszka wrote:
>
>> 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.

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

Re: KVM: x86: Push potential exception error code on task switches

[Avi Kivity]

On 04/14/2010 04:07 PM, Avi Kivity wrote:
> On 04/14/2010 03:58 PM, Jan Kiszka wrote:
>>
>>> 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.

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

Re: KVM: x86: Push potential exception error code on task switches

[Jan Kiszka]

Avi Kivity wrote:
> On 04/14/2010 03:58 PM, Jan Kiszka wrote:
>>> 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.

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: KVM: x86: Push potential exception error code on task switches

[Avi Kivity]

On 04/14/2010 04:19 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 04/14/2010 03:58 PM, Jan Kiszka wrote:
>>      
>>>> 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.
>>      
> 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!)

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: KVM: x86: Push potential exception error code on task switches

[Jan Kiszka]

Avi Kivity wrote:
> On 04/14/2010 04:19 PM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>    
>>> On 04/14/2010 03:58 PM, Jan Kiszka wrote:
>>>      
>>>>> 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.
>>>      
>> 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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: KVM: x86: Push potential exception error code on task switches

[Gleb Natapov]

On Wed, Apr 14, 2010 at 02:11:39PM +0200, Jan Kiszka wrote:
>  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) {
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...

> @@ -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);
> +     }
I would move writeback() to emulator_task_switch(). Just make
c->dst.type = OP_NONE if writeback is not needed.

--
			Gleb.

Re: KVM: x86: Push potential exception error code on task switches

[Jan Kiszka]

Gleb Natapov wrote:
> On Wed, Apr 14, 2010 at 02:11:39PM +0200, Jan Kiszka wrote:
>>  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) {
> 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...

Will do this in a preparational patch.

> 
>> @@ -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);
>> +     }
> I would move writeback() to emulator_task_switch(). Just make
> c->dst.type = OP_NONE if writeback is not needed.

I should dramatically increase the overhead for the common case. :)
Yeah, can do so if preferred.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: KVM: x86: Push potential exception error code on task switches

[Jan Kiszka]

Gleb Natapov wrote:
> On Wed, Apr 14, 2010 at 02:11:39PM +0200, Jan Kiszka wrote:
>>  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) {
> 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...
> 
>> @@ -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);
>> +     }
> I would move writeback() to emulator_task_switch(). Just make
> c->dst.type = OP_NONE if 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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: KVM: x86: Push potential exception error code on task switches

[Gleb Natapov]

On Wed, Apr 14, 2010 at 03:00:18PM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Wed, Apr 14, 2010 at 02:11:39PM +0200, Jan Kiszka wrote:
> >>  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) {
> > 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...
> > 
> >> @@ -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);
> >> +     }
> > I would move writeback() to emulator_task_switch(). Just make
> > c->dst.type = OP_NONE if 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?
> 
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.