Regression problem with commit 5045b46803

Regression problem with commit 5045b46803

[Nadav Amit]

Commit 5045b46803 added a check that cs.dpl equals cs.rpl during
task-switch. Unfortunately, is causes some of my tests that run well on
bare-metal to fail.

Although this check is mentioned in table 7-1 of the SDM as causing a
#TSS exception, it is not mentioned in table 6-6 that lists "invalid TSS
conditions" which cause #TSS exceptions.

Thus, I recommend on reverting commit 5045b46803, or alternatively
rechecking task-switch behavior on bare-metal.

Nadav

Re: Regression problem with commit 5045b46803

[Paolo Bonzini]

Il 13/08/2014 19:14, Nadav Amit ha scritto:
> Commit 5045b46803 added a check that cs.dpl equals cs.rpl during
> task-switch. Unfortunately, is causes some of my tests that run well on
> bare-metal to fail.
> 
> Although this check is mentioned in table 7-1 of the SDM as causing a
> #TSS exception, it is not mentioned in table 6-6 that lists "invalid TSS
> conditions" which cause #TSS exceptions.
> 
> Thus, I recommend on reverting commit 5045b46803, or alternatively
> rechecking task-switch behavior on bare-metal.

Can you provide a kvm-unit-tests test for this?

Paolo

Re: Regression problem with commit 5045b46803

[Nadav Amit]

On Aug 17, 2014, at 9:17 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 13/08/2014 19:14, Nadav Amit ha scritto:
>> Commit 5045b46803 added a check that cs.dpl equals cs.rpl during
>> task-switch. Unfortunately, is causes some of my tests that run well on
>> bare-metal to fail.
>> 
>> Although this check is mentioned in table 7-1 of the SDM as causing a
>> #TSS exception, it is not mentioned in table 6-6 that lists "invalid TSS
>> conditions" which cause #TSS exceptions.
>> 
>> Thus, I recommend on reverting commit 5045b46803, or alternatively
>> rechecking task-switch behavior on bare-metal.
> 
> Can you provide a kvm-unit-tests test for this?

I need to put time into writing one. However, the unit test would prove nothing.
The question is how bare-metal behaves. According to what I see, it does not perform this check.

Nadav

Re: Regression problem with commit 5045b46803

[Paolo Bonzini]

Il 17/08/2014 08:23, Nadav Amit ha scritto:
>>> >> Although this check is mentioned in table 7-1 of the SDM as causing a
>>> >> #TSS exception, it is not mentioned in table 6-6 that lists "invalid TSS
>>> >> conditions" which cause #TSS exceptions.
>>> >> 
>>> >> Thus, I recommend on reverting commit 5045b46803, or alternatively
>>> >> rechecking task-switch behavior on bare-metal.
>> > 
>> > Can you provide a kvm-unit-tests test for this?
> I need to put time into writing one. However, the unit test would prove nothing.
> The question is how bare-metal behaves. According to what I see, it does not perform this check.

Yes, but I still prefer to have a test, in case I find some time to fix
kvm-unit-tests to work on bare metal (or on Bochs).

There are already a few task switch tests (32-bit only).

Paolo

Re: Regression problem with commit 5045b46803

[Nadav Amit]

On Aug 17, 2014, at 9:28 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 17/08/2014 08:23, Nadav Amit ha scritto:
>>>>>> Although this check is mentioned in table 7-1 of the SDM as causing a
>>>>>> #TSS exception, it is not mentioned in table 6-6 that lists "invalid TSS
>>>>>> conditions" which cause #TSS exceptions.
>>>>>> 
>>>>>> Thus, I recommend on reverting commit 5045b46803, or alternatively
>>>>>> rechecking task-switch behavior on bare-metal.
>>>> 
>>>> Can you provide a kvm-unit-tests test for this?
>> 
>> The question is how bare-metal behaves. According to what I see, it does not perform this check.
> 
> Yes, but I still prefer to have a test, in case I find some time to fix
> kvm-unit-tests to work on bare metal (or on Bochs).
> 
> There are already a few task switch tests (32-bit only).
> 

Can you please assist in running the existing task-switch tests first?

In x86_64 the task-switch tests are skipped. If I configure the tests to use the i386 architecture, the build fails.
Even after fixing the compilation errors, I don’t manage to get the task-switch tests to pass.

Nadav

Re: Regression problem with commit 5045b46803

[Paolo Bonzini]

Il 17/08/2014 09:19, Nadav Amit ha scritto:
> Can you please assist in running the existing task-switch tests first?
> 
> In x86_64 the task-switch tests are skipped. If I configure the tests to use the i386 architecture, the build fails.
> Even after fixing the compilation errors, I don’t manage to get the task-switch tests to pass.

Bitrot...  I sent a patch to fix this.  BTW you might be using the wrong
account to send email from? :)

Paolo

[PATCH] KVM: x86: Revert "check CS.DPL against RPL during task switch"

[Nadav Amit]

This reverts commit 5045b468037dfe1c848827ce10e99d87f5669160.  Although the
cs.dpl=cs.rpl check is mentioned in table 7-1 of the SDM as causing a #TSS
exception, it is not mentioned in table 6-6 that lists "invalid TSS conditions"
which cause #TSS exceptions. As it causes some tests, which pass on bare-metal,
to fail - it should be reverted.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 56657b0..b73c9e8 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1415,7 +1415,7 @@ static int write_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 
 /* Does not support long mode */
 static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
-				     u16 selector, int seg, u8 cpl, bool in_task_switch)
+				     u16 selector, int seg, u8 cpl)
 {
 	struct desc_struct seg_desc, old_desc;
 	u8 dpl, rpl;
@@ -1491,9 +1491,6 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 			goto exception;
 		break;
 	case VCPU_SREG_CS:
-		if (in_task_switch && rpl != dpl)
-			goto exception;
-
 		if (!(seg_desc.type & 8))
 			goto exception;
 
@@ -1560,7 +1557,7 @@ static int load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 				   u16 selector, int seg)
 {
 	u8 cpl = ctxt->ops->cpl(ctxt);
-	return __load_segment_descriptor(ctxt, selector, seg, cpl, false);
+	return __load_segment_descriptor(ctxt, selector, seg, cpl);
 }
 
 static void write_register_operand(struct operand *op)
@@ -2460,19 +2457,19 @@ static int load_state_from_tss16(struct x86_emulate_ctxt *ctxt,
 	 * Now load segment descriptors. If fault happens at this stage
 	 * it is handled in a context of new task
 	 */
-	ret = __load_segment_descriptor(ctxt, tss->ldt, VCPU_SREG_LDTR, cpl, true);
+	ret = __load_segment_descriptor(ctxt, tss->ldt, VCPU_SREG_LDTR, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->es, VCPU_SREG_ES, cpl, true);
+	ret = __load_segment_descriptor(ctxt, tss->es, VCPU_SREG_ES, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->cs, VCPU_SREG_CS, cpl, true);
+	ret = __load_segment_descriptor(ctxt, tss->cs, VCPU_SREG_CS, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->ss, VCPU_SREG_SS, cpl, true);
+	ret = __load_segment_descriptor(ctxt, tss->ss, VCPU_SREG_SS, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->ds, VCPU_SREG_DS, cpl, true);
+	ret = __load_segment_descriptor(ctxt, tss->ds, VCPU_SREG_DS, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
 
@@ -2597,25 +2594,26 @@ static int load_state_from_tss32(struct x86_emulate_ctxt *ctxt,
 	 * Now load segment descriptors. If fault happenes at this stage
 	 * it is handled in a context of new task
 	 */
-	ret = __load_segment_descriptor(ctxt, tss->ldt_selector, VCPU_SREG_LDTR, cpl, true);
+	ret = __load_segment_descriptor(ctxt, tss->ldt_selector, VCPU_SREG_LDTR,
+					cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->es, VCPU_SREG_ES, cpl, true);
+	ret = __load_segment_descriptor(ctxt, tss->es, VCPU_SREG_ES, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->cs, VCPU_SREG_CS, cpl, true);
+	ret = __load_segment_descriptor(ctxt, tss->cs, VCPU_SREG_CS, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->ss, VCPU_SREG_SS, cpl, true);
+	ret = __load_segment_descriptor(ctxt, tss->ss, VCPU_SREG_SS, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->ds, VCPU_SREG_DS, cpl, true);
+	ret = __load_segment_descriptor(ctxt, tss->ds, VCPU_SREG_DS, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->fs, VCPU_SREG_FS, cpl, true);
+	ret = __load_segment_descriptor(ctxt, tss->fs, VCPU_SREG_FS, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->gs, VCPU_SREG_GS, cpl, true);
+	ret = __load_segment_descriptor(ctxt, tss->gs, VCPU_SREG_GS, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
 
-- 
1.9.1

[PATCH kvm-unit-tests] x86: Test task-switch with cs.rpl != cs.dpl

[Nadav Amit]

Commit 5045b46803 added a check that cs.dpl equals cs.rpl during task-switch.
This is a wrong check, and this test introduces a test in which cs.dpl !=
cs.rpl. To do so, it configures tss.cs to be conforming with rpl=3 and dpl=0.
Since the cpl after calling is 3, it does not make any prints in the callee.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 x86/taskswitch2.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/x86/taskswitch2.c b/x86/taskswitch2.c
index 92fc941..d96853f 100644
--- a/x86/taskswitch2.c
+++ b/x86/taskswitch2.c
@@ -7,6 +7,8 @@
 
 #define MAIN_TSS_SEL (FIRST_SPARE_SEL + 0)
 #define VM86_TSS_SEL (FIRST_SPARE_SEL + 8)
+#define USER_CS_SEL  (FIRST_SPARE_SEL + 16)
+#define USER_DS_SEL  (FIRST_SPARE_SEL + 24)
 
 static volatile int test_count;
 static volatile unsigned int test_divider;
@@ -102,6 +104,14 @@ start:
 	goto start;
 }
 
+static void user_tss(void)
+{
+start:
+	test_count++;
+	asm volatile ("iret");
+	goto start;
+}
+
 void test_kernel_mode_int()
 {
 	unsigned int res;
@@ -201,6 +211,18 @@ void test_kernel_mode_int()
 	asm volatile ("ljmp $" xstr(TSS_INTR) ", $0xf4f4f4f4");
 	printf("Jump back succeeded\n");
 	report("ljmp", test_count == 1);
+
+	/* test lcall with conforming segment, cs.dpl != cs.rpl */
+	test_count = 0;
+	set_intr_task_gate(0, user_tss);
+
+	tss_intr.cs = USER_CS_SEL | 3;
+	tss_intr.ss = USER_DS_SEL | 3;
+	tss_intr.ds = tss_intr.gs = tss_intr.fs = tss_intr.ss;
+	set_gdt_entry(USER_CS_SEL, 0, 0xffffffff, 0x9f, 0xc0);
+	set_gdt_entry(USER_DS_SEL, 0, 0xffffffff, 0xf3, 0xc0);
+	asm volatile("lcall $" xstr(TSS_INTR) ", $0xf4f4f4f4");
+	report("lcall when cs.rpl != cs.dpl", test_count == 1);
 }
 
 void test_vm86_switch(void)
-- 
1.9.1

Re: [PATCH kvm-unit-tests] x86: Test task-switch with cs.rpl != cs.dpl

[Paolo Bonzini]

Il 17/08/2014 21:34, Nadav Amit ha scritto:
> Commit 5045b46803 added a check that cs.dpl equals cs.rpl during task-switch.
> This is a wrong check, and this test introduces a test in which cs.dpl !=
> cs.rpl. To do so, it configures tss.cs to be conforming with rpl=3 and dpl=0.
> Since the cpl after calling is 3, it does not make any prints in the callee.
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  x86/taskswitch2.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/x86/taskswitch2.c b/x86/taskswitch2.c
> index 92fc941..d96853f 100644
> --- a/x86/taskswitch2.c
> +++ b/x86/taskswitch2.c
> @@ -7,6 +7,8 @@
>  
>  #define MAIN_TSS_SEL (FIRST_SPARE_SEL + 0)
>  #define VM86_TSS_SEL (FIRST_SPARE_SEL + 8)
> +#define USER_CS_SEL  (FIRST_SPARE_SEL + 16)

Please call it CONFORM_CS_SEL since it's not really a user-mode selector
(DPL=0), it's just used as one (RPL=3).

> +#define USER_DS_SEL  (FIRST_SPARE_SEL + 24)

Not needed, see below.

>  
>  static volatile int test_count;
>  static volatile unsigned int test_divider;
> @@ -102,6 +104,14 @@ start:
>  	goto start;
>  }
>  
> +static void user_tss(void)
> +{
> +start:

Please add a printf and print_current_tss_info() here.

> +	test_count++;
> +	asm volatile ("iret");
> +	goto start;
> +}
> +
>  void test_kernel_mode_int()
>  {
>  	unsigned int res;
> @@ -201,6 +211,18 @@ void test_kernel_mode_int()
>  	asm volatile ("ljmp $" xstr(TSS_INTR) ", $0xf4f4f4f4");
>  	printf("Jump back succeeded\n");
>  	report("ljmp", test_count == 1);
> +
> +	/* test lcall with conforming segment, cs.dpl != cs.rpl */
> +	test_count = 0;
> +	set_intr_task_gate(0, user_tss);
> +
> +	tss_intr.cs = USER_CS_SEL | 3;
> +	tss_intr.ss = USER_DS_SEL | 3;
> +	tss_intr.ds = tss_intr.gs = tss_intr.fs = tss_intr.ss;
> +	set_gdt_entry(USER_CS_SEL, 0, 0xffffffff, 0x9f, 0xc0);
> +	set_gdt_entry(USER_DS_SEL, 0, 0xffffffff, 0xf3, 0xc0);

You can use USER_DS here.  Also, please put the test in a separate
function and call it last (after test_vm86_switch), because a failure in
this test breaks test_vm86_switch too.

Paolo

> +	asm volatile("lcall $" xstr(TSS_INTR) ", $0xf4f4f4f4");
> +	report("lcall when cs.rpl != cs.dpl", test_count == 1);
>  }
>  
>  void test_vm86_switch(void)
> 

[PATCH kvm-unit-tests v2] x86: Test task-switch with cs.rpl != cs.dpl

[Nadav Amit]

Commit 5045b46803 added a check that cs.dpl equals cs.rpl during task-switch.
This is a wrong check, and this patch introduces a test in which cs.dpl !=
cs.rpl. To do so, it configures tss.cs to be conforming with rpl=3 and dpl=0.
Since the cpl after calling is 3, it does not make any prints in the callee.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 x86/taskswitch2.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/x86/taskswitch2.c b/x86/taskswitch2.c
index 92fc941..1fe833e 100644
--- a/x86/taskswitch2.c
+++ b/x86/taskswitch2.c
@@ -7,6 +7,7 @@
 
 #define MAIN_TSS_SEL (FIRST_SPARE_SEL + 0)
 #define VM86_TSS_SEL (FIRST_SPARE_SEL + 8)
+#define CONFORM_CS_SEL  (FIRST_SPARE_SEL + 16)
 
 static volatile int test_count;
 static volatile unsigned int test_divider;
@@ -102,6 +103,14 @@ start:
 	goto start;
 }
 
+static void user_tss(void)
+{
+start:
+	test_count++;
+	asm volatile ("iret");
+	goto start;
+}
+
 void test_kernel_mode_int()
 {
 	unsigned int res;
@@ -248,6 +257,19 @@ void test_vm86_switch(void)
     report("VM86", 1);
 }
 
+void test_conforming_switch(void)
+{
+	/* test lcall with conforming segment, cs.dpl != cs.rpl */
+	test_count = 0;
+	set_intr_task_gate(0, user_tss);
+
+	tss_intr.cs = CONFORM_CS_SEL | 3;
+	tss_intr.ds = tss_intr.gs = tss_intr.fs = tss_intr.ss = USER_DS;
+	set_gdt_entry(CONFORM_CS_SEL, 0, 0xffffffff, 0x9f, 0xc0);
+	asm volatile("lcall $" xstr(TSS_INTR) ", $0xf4f4f4f4");
+	report("lcall with cs.rpl != cs.dpl", test_count == 1);
+}
+
 int main()
 {
 	setup_vm();
@@ -256,6 +278,7 @@ int main()
 
 	test_kernel_mode_int();
 	test_vm86_switch();
+	test_conforming_switch();
 
 	return report_summary();
 }
-- 
1.9.1

Re: [PATCH kvm-unit-tests v2] x86: Test task-switch with cs.rpl != cs.dpl

[Paolo Bonzini]

Il 19/08/2014 15:04, Nadav Amit ha scritto:
> Commit 5045b46803 added a check that cs.dpl equals cs.rpl during task-switch.
> This is a wrong check, and this patch introduces a test in which cs.dpl !=
> cs.rpl. To do so, it configures tss.cs to be conforming with rpl=3 and dpl=0.
> Since the cpl after calling is 3, it does not make any prints in the callee.
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  x86/taskswitch2.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/x86/taskswitch2.c b/x86/taskswitch2.c
> index 92fc941..1fe833e 100644
> --- a/x86/taskswitch2.c
> +++ b/x86/taskswitch2.c
> @@ -7,6 +7,7 @@
>  
>  #define MAIN_TSS_SEL (FIRST_SPARE_SEL + 0)
>  #define VM86_TSS_SEL (FIRST_SPARE_SEL + 8)
> +#define CONFORM_CS_SEL  (FIRST_SPARE_SEL + 16)
>  
>  static volatile int test_count;
>  static volatile unsigned int test_divider;
> @@ -102,6 +103,14 @@ start:
>  	goto start;
>  }
>  
> +static void user_tss(void)
> +{
> +start:
> +	test_count++;
> +	asm volatile ("iret");
> +	goto start;
> +}
> +
>  void test_kernel_mode_int()
>  {
>  	unsigned int res;
> @@ -248,6 +257,19 @@ void test_vm86_switch(void)
>      report("VM86", 1);
>  }
>  
> +void test_conforming_switch(void)
> +{
> +	/* test lcall with conforming segment, cs.dpl != cs.rpl */
> +	test_count = 0;
> +	set_intr_task_gate(0, user_tss);

No need to use set_intr_task_gate, since the IDT is not involved here.

        tss_intr.eip = (u32)user_tss;

is enough.

I fixed this up and applied the patch.  Thanks!

Paolo

> +
> +	tss_intr.cs = CONFORM_CS_SEL | 3;
> +	tss_intr.ds = tss_intr.gs = tss_intr.fs = tss_intr.ss = USER_DS;
> +	set_gdt_entry(CONFORM_CS_SEL, 0, 0xffffffff, 0x9f, 0xc0);
> +	asm volatile("lcall $" xstr(TSS_INTR) ", $0xf4f4f4f4");
> +	report("lcall with cs.rpl != cs.dpl", test_count == 1);
> +}
> +
>  int main()
>  {
>  	setup_vm();
> @@ -256,6 +278,7 @@ int main()
>  
>  	test_kernel_mode_int();
>  	test_vm86_switch();
> +	test_conforming_switch();
>  
>  	return report_summary();
>  }
> 

Re: [PATCH kvm-unit-tests v2] x86: Test task-switch with cs.rpl != cs.dpl

[Nadav Amit]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]


On Aug 19, 2014, at 4:28 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 19/08/2014 15:04, Nadav Amit ha scritto:
>> 
>> +void test_conforming_switch(void)
>> +{
>> +	/* test lcall with conforming segment, cs.dpl != cs.rpl */
>> +	test_count = 0;
>> +	set_intr_task_gate(0, user_tss);
> 
> No need to use set_intr_task_gate, since the IDT is not involved here.
> 
>        tss_intr.eip = (u32)user_tss;
> 
> is enough.
> 
> I fixed this up and applied the patch.  Thanks!

I know, but all the other ‘call' tests did. If there is an error, at least it should be consistent. ;-)

Thanks,
Nadav

[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH kvm-unit-tests v2] x86: Test task-switch with cs.rpl != cs.dpl

[Paolo Bonzini]

Il 19/08/2014 15:34, Nadav Amit ha scritto:
> I know, but all the other ‘call' tests did. If there is an error, at
> least it should be consistent. ;-)

Right, let's fix it up:

diff --git a/x86/taskswitch2.c b/x86/taskswitch2.c
index fd9a404..3cfb467 100644
--- a/x86/taskswitch2.c
+++ b/x86/taskswitch2.c
@@ -190,7 +190,7 @@ void test_kernel_mode_int()

 	/* test that calling a task by lcall works */
 	test_count = 0;
-	set_intr_task_gate(0, irq_tss);
+	tss_intr.eip = (u32)irq_tss;
 	printf("Calling task by lcall\n");
 	/* hlt opcode is 0xf4 I use destination IP 0xf4f4f4f4 to catch
 	   incorrect instruction length calculation */
@@ -205,7 +205,7 @@ void test_kernel_mode_int()

 	/* test that calling a task by ljmp works */
 	test_count = 0;
-	set_intr_task_gate(0, jmp_tss);
+	tss_intr.eip = (u32)jmp_tss;
 	printf("Jumping to a task by ljmp\n");
 	asm volatile ("ljmp $" xstr(TSS_INTR) ", $0xf4f4f4f4");
 	printf("Jump back succeeded\n");


Paolo

Re: [PATCH] KVM: x86: Revert "check CS.DPL against RPL during task switch"

[Paolo Bonzini]

Il 17/08/2014 21:32, Nadav Amit ha scritto:
> This reverts commit 5045b468037dfe1c848827ce10e99d87f5669160.  Although the
> cs.dpl=cs.rpl check is mentioned in table 7-1 of the SDM as causing a #TSS
> exception, it is not mentioned in table 6-6 that lists "invalid TSS conditions"
> which cause #TSS exceptions. As it causes some tests, which pass on bare-metal,
> to fail - it should be reverted.

Right.  However, I think reverting the patch is too big a hammer.  We 
still need in_task_switch to raise TS_VECTOR instead of GP_VECTOR, so I 
propose instead something like:

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 56657b0bb3bb..cd230b035514 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1468,7 +1468,7 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 		return ret;
 
 	err_code = selector & 0xfffc;
-	err_vec = GP_VECTOR;
+	err_vec = in_task_switch ? TS_VECTOR : GP_VECTOR;
 
 	/* can't load system descriptor into segment selector */
 	if (seg <= VCPU_SREG_GS && !seg_desc.s)
@@ -1491,9 +1491,6 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 			goto exception;
 		break;
 	case VCPU_SREG_CS:
-		if (in_task_switch && rpl != dpl)
-			goto exception;
-
 		if (!(seg_desc.type & 8))
 			goto exception;
 

either in a single patch or as two separate patch.  I'll test this against
your test case and repost (not before Tuesday).

Paolo

Re: [PATCH] KVM: x86: Revert "check CS.DPL against RPL during task switch"

[Paolo Bonzini]

Il 17/08/2014 23:09, Paolo Bonzini ha scritto:
> Il 17/08/2014 21:32, Nadav Amit ha scritto:
>> This reverts commit 5045b468037dfe1c848827ce10e99d87f5669160.  Although the
>> cs.dpl=cs.rpl check is mentioned in table 7-1 of the SDM as causing a #TSS
>> exception, it is not mentioned in table 6-6 that lists "invalid TSS conditions"
>> which cause #TSS exceptions. As it causes some tests, which pass on bare-metal,
>> to fail - it should be reverted.
> 
> Right.  However, I think reverting the patch is too big a hammer.  We 
> still need in_task_switch to raise TS_VECTOR instead of GP_VECTOR, so I 
> propose instead something like:
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 56657b0bb3bb..cd230b035514 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -1468,7 +1468,7 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
>  		return ret;
>  
>  	err_code = selector & 0xfffc;
> -	err_vec = GP_VECTOR;
> +	err_vec = in_task_switch ? TS_VECTOR : GP_VECTOR;
>  
>  	/* can't load system descriptor into segment selector */
>  	if (seg <= VCPU_SREG_GS && !seg_desc.s)
> @@ -1491,9 +1491,6 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
>  			goto exception;
>  		break;
>  	case VCPU_SREG_CS:
> -		if (in_task_switch && rpl != dpl)
> -			goto exception;
> -
>  		if (!(seg_desc.type & 8))
>  			goto exception;
>  

Also, what about the rpl > cpl test below, for non-conforming code
segments?  It is not mentioned in table 6-6 either.

Paolo

Re: [PATCH] KVM: x86: Revert "check CS.DPL against RPL during task switch"

[Nadav Amit]

On Aug 18, 2014, at 12:13 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 17/08/2014 23:09, Paolo Bonzini ha scritto:
>> Il 17/08/2014 21:32, Nadav Amit ha scritto:
>>> This reverts commit 5045b468037dfe1c848827ce10e99d87f5669160.  Although the
>>> cs.dpl=cs.rpl check is mentioned in table 7-1 of the SDM as causing a #TSS
>>> exception, it is not mentioned in table 6-6 that lists "invalid TSS conditions"
>>> which cause #TSS exceptions. As it causes some tests, which pass on bare-metal,
>>> to fail - it should be reverted.
>> 
>> Right.  However, I think reverting the patch is too big a hammer.  We 
>> still need in_task_switch to raise TS_VECTOR instead of GP_VECTOR, so I 
>> propose instead something like:
>> 
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index 56657b0bb3bb..cd230b035514 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -1468,7 +1468,7 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
>> 		return ret;
>> 
>> 	err_code = selector & 0xfffc;
>> -	err_vec = GP_VECTOR;
>> +	err_vec = in_task_switch ? TS_VECTOR : GP_VECTOR;
>> 
>> 	/* can't load system descriptor into segment selector */
>> 	if (seg <= VCPU_SREG_GS && !seg_desc.s)
>> @@ -1491,9 +1491,6 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
>> 			goto exception;
>> 		break;
>> 	case VCPU_SREG_CS:
>> -		if (in_task_switch && rpl != dpl)
>> -			goto exception;
>> -
>> 		if (!(seg_desc.type & 8))
>> 			goto exception;
>> 
> 
> Also, what about the rpl > cpl test below, for non-conforming code
> segments?  It is not mentioned in table 6-6 either.

As far as I understand, after task-switch cpl = cs.rpl. This is how the load_state_from_tss32 does it, and follows SDM 7.3, Task Switching: 
"The new task begins executing at the privilege level specified in the CPL field of the CS register, which is loaded from the TSS.”
As a result, this condition can never occur during task-switch.

Do I miss anything?

Nadav

Re: [PATCH] KVM: x86: Revert "check CS.DPL against RPL during task switch"

[Paolo Bonzini]

Il 17/08/2014 23:33, Nadav Amit ha scritto:
>> > Also, what about the rpl > cpl test below, for non-conforming code
>> > segments?  It is not mentioned in table 6-6 either.
> As far as I understand, after task-switch cpl = cs.rpl. This is how the load_state_from_tss32 does it, and follows SDM 7.3, Task Switching: 
> "The new task begins executing at the privilege level specified in the CPL field of the CS register, which is loaded from the TSS.”
> As a result, this condition can never occur during task-switch.
> 
> Do I miss anything?

No, you're right.

Paolo

Re: [PATCH] KVM: x86: Revert "check CS.DPL against RPL during task switch"

[Nadav Amit]

[-- Attachment #1: Type: text/plain, Size: 2009 bytes --]


On Aug 18, 2014, at 12:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 17/08/2014 21:32, Nadav Amit ha scritto:
>> This reverts commit 5045b468037dfe1c848827ce10e99d87f5669160.  Although the
>> cs.dpl=cs.rpl check is mentioned in table 7-1 of the SDM as causing a #TSS
>> exception, it is not mentioned in table 6-6 that lists "invalid TSS conditions"
>> which cause #TSS exceptions. As it causes some tests, which pass on bare-metal,
>> to fail - it should be reverted.
> 
> Right.  However, I think reverting the patch is too big a hammer.  We 
> still need in_task_switch to raise TS_VECTOR instead of GP_VECTOR, so I 
> propose instead something like:
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 56657b0bb3bb..cd230b035514 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -1468,7 +1468,7 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
>  		return ret;
> 
>  	err_code = selector & 0xfffc;
> -	err_vec = GP_VECTOR;
> +	err_vec = in_task_switch ? TS_VECTOR : GP_VECTOR;
> 
>  	/* can't load system descriptor into segment selector */
>  	if (seg <= VCPU_SREG_GS && !seg_desc.s)
> @@ -1491,9 +1491,6 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
>  			goto exception;
>  		break;
>  	case VCPU_SREG_CS:
> -		if (in_task_switch && rpl != dpl)
> -			goto exception;
> -
>  		if (!(seg_desc.type & 8))
>  			goto exception;
> 
> 
> either in a single patch or as two separate patch.  I'll test this against
> your test case and repost (not before Tuesday).
> 

I missed the TS_VECTOR thing. Yes, in that case, full revert makes no sense. I’m in no hurry with this patch, and I posted it during your vacation only because it is a recent bug.
Anyhow, you may want to look at another patch I sent, "KVM: x86: Avoid emulating instructions on #UD mistakenly”, ASAP. It fixes a bug that was introduced into 3.17-RC1 and is visible in userspace.

Nadav




[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] KVM: x86: Revert "check CS.DPL against RPL during task switch"

[Paolo Bonzini]

Il 17/08/2014 23:39, Nadav Amit ha scritto:
> I missed the TS_VECTOR thing. Yes, in that case, full revert makes no
> sense. I’m in no hurry with this patch, and I posted it during your
> vacation only because it is a recent bug. Anyhow, you may want to
> look at another patch I sent, "KVM: x86: Avoid emulating instructions
> on #UD mistakenly”, ASAP. It fixes a bug that was introduced into
> 3.17-RC1 and is visible in userspace.

Yes, it's on my list and will definitely get into one of the next 3.17
RCs.  Thanks,

Paolo