[kvm-unit-tests PATCH 0/2] x86/emulator: Add some tests for loading segment descriptor in emulator

[kvm-unit-tests PATCH 0/2] x86/emulator: Add some tests for loading segment descriptor in emulator

[Hou Wenlong]

Add some lret and ljmp tests for the related x86 emulator bugs[*]. Those tests
would be tested both on hardware and emulator. Enable kvm.force_emulation_prefix
to test them on emulator.

[*] https://lore.kernel.org/kvm/cover.1642669684.git.houwenlong.hwl@antgroup.com

Hou Wenlong (2):
  x86/emulator: Add some tests for lret instruction emulation
  x86/emulator: Add some tests for ljmp instruction emulation

 x86/emulator.c | 283 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 270 insertions(+), 13 deletions(-)

--
2.31.1

[kvm-unit-tests PATCH 1/2] x86/emulator: Add some tests for lret instruction emulation

[Hou Wenlong]

Per Intel's SDM on the "Instruction Set Reference", when
loading segment descriptor for far return, not-present segment
check should be after all type and privilege checks. However,
__load_segment_descriptor() in x86's emulator does not-present
segment check first, so it would trigger #NP instead of #GP
if type or privilege checks fail and the segment is not present.

And if RPL < CPL, it should trigger #GP, but the check is missing
in emulator.

So add some tests for lret instruction, and it will test
those tests in hardware and emulator. Enable
kvm.force_emulation_prefix when try to test them in emulator.

Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
---
 x86/emulator.c | 181 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 181 insertions(+)

diff --git a/x86/emulator.c b/x86/emulator.c
index c5f584a9d8cc..480333a40eba 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -19,6 +19,88 @@ static int exceptions;
 #define KVM_FEP "ud2; .byte 'k', 'v', 'm';"
 #define KVM_FEP_LENGTH 5
 static int fep_available = 1;
+static unsigned int fep_vector = -1;
+static unsigned int fep_error_code = -1;
+
+struct fep_test_case {
+	uint16_t rpl;
+	uint16_t type;
+	uint16_t dpl;
+	uint16_t p;
+	unsigned int vector;
+	unsigned int error_code;
+	const char *msg;
+};
+
+enum fep_test_inst_type {
+	FEP_TEST_LRET,
+};
+
+struct fep_test {
+	enum fep_test_inst_type type;
+	unsigned long rip_advance;
+	struct fep_test_case *kernel_testcases;
+	unsigned int kernel_testcases_count;
+	struct fep_test_case *user_testcases;
+	unsigned int user_testcases_count;
+};
+
+#define NON_CONFORM_CS_TYPE	0xb
+#define CONFORM_CS_TYPE		0xf
+#define DS_TYPE			0x3
+
+static struct fep_test_case lret_kernel_testcases[] = {
+	{0, DS_TYPE, 0, 0, GP_VECTOR, FIRST_SPARE_SEL, "lret desc.type!=code && desc.p=0"},
+	{0, NON_CONFORM_CS_TYPE, 3, 0, GP_VECTOR, FIRST_SPARE_SEL, "lret non-conforming && dpl!=rpl && desc.p=0"},
+	{0, CONFORM_CS_TYPE, 3, 0, GP_VECTOR, FIRST_SPARE_SEL, "lret conforming && dpl>rpl && desc.p=0"},
+	{0, NON_CONFORM_CS_TYPE, 0, 0, NP_VECTOR, FIRST_SPARE_SEL, "lret desc.p=0"},
+};
+
+static struct fep_test_case lret_user_testcases[] = {
+	{0, NON_CONFORM_CS_TYPE, 3, 1, GP_VECTOR, FIRST_SPARE_SEL, "lret rpl<cpl"},
+};
+
+static struct fep_test fep_test_lret = {
+	.type = FEP_TEST_LRET,
+	.kernel_testcases = lret_kernel_testcases,
+	.kernel_testcases_count = sizeof(lret_kernel_testcases) / sizeof(struct fep_test_case),
+	.user_testcases = lret_user_testcases,
+	.user_testcases_count = sizeof(lret_user_testcases) / sizeof(struct fep_test_case),
+};
+
+static void test_in_user(bool emulate, uint16_t rpl, enum fep_test_inst_type type);
+
+#define TEST_LRET_ASM(seg, prefix)		\
+	asm volatile("pushq %[asm_seg]\n\t"	\
+		     "pushq $1f\n\t"		\
+		      prefix "lretq\n\t"	\
+		     "addq $16, %%rsp\n\t"	\
+		     "1:"			\
+		     : : [asm_seg]"r"(seg)	\
+		     : "memory");
+
+#define TEST_FEP_RESULT(vector, error_code, msg)	\
+	report(fep_vector == vector &&			\
+	       fep_error_code == error_code,msg);	\
+	fep_vector = -1;				\
+	fep_error_code = -1;
+
+#define TEST_FEP_INST(emulate, inst, seg, vector, error_code, msg) \
+	do {							   \
+		if (emulate) {					   \
+			TEST_##inst##_ASM(seg, KVM_FEP);	   \
+		} else {					   \
+			TEST_##inst##_ASM(seg, "");		   \
+		}						   \
+		TEST_FEP_RESULT(vector, error_code, msg);	   \
+	} while (0)
+
+#define TEST_FEP_INST_IN_USER(inst, emulate, rpl, dummy, vector, error_code, msg)\
+	do {								\
+		run_in_user((usermode_func)test_in_user, UD_VECTOR,	\
+			     emulate, rpl, FEP_TEST_##inst, 0, dummy);	\
+		TEST_FEP_RESULT(vector, error_code, msg);		\
+	} while (0)
 
 struct regs {
 	u64 rax, rbx, rcx, rdx;
@@ -890,6 +972,103 @@ static void test_mov_dr(uint64_t *mem)
 	report(rax == dr6_fixed_1, "mov_dr6");
 }
 
+static void fep_exception_handler(struct ex_regs *regs)
+{
+	fep_vector = regs->vector;
+	fep_error_code = regs->error_code;
+	regs->rip += rip_advance;
+}
+
+static void test_in_user(bool emulate, uint16_t rpl, enum fep_test_inst_type type)
+{
+	uint16_t seg = FIRST_SPARE_SEL | rpl;
+
+	switch (type) {
+	case FEP_TEST_LRET:
+		if (emulate) {
+			TEST_LRET_ASM(seg, KVM_FEP);
+		} else {
+			TEST_LRET_ASM(seg, "");
+		}
+		break;
+	}
+}
+
+static void test_fep_common(bool emulate, struct fep_test *test)
+{
+	int i;
+	bool dummy;
+	struct fep_test_case *t;
+	uint16_t seg = FIRST_SPARE_SEL;
+
+	handle_exception(GP_VECTOR, fep_exception_handler);
+	handle_exception(NP_VECTOR, fep_exception_handler);
+	rip_advance = test->rip_advance;
+
+	gdt[seg / 8] = gdt[KERNEL_CS / 8];
+	t = test->kernel_testcases;
+	for (i = 0; i < test->kernel_testcases_count; i++) {
+		seg = FIRST_SPARE_SEL | t[i].rpl;
+		gdt[seg / 8].type = t[i].type;
+		gdt[seg / 8].dpl = t[i].dpl;
+		gdt[seg / 8].p = t[i].p;
+
+		switch (test->type) {
+		case FEP_TEST_LRET:
+			TEST_FEP_INST(emulate, LRET, seg, t[i].vector,
+				      t[i].error_code, t[i].msg);
+			break;
+		}
+	}
+
+	gdt[seg / 8] = gdt[USER_CS64 / 8];
+	t = test->user_testcases;
+	for (i = 0; i < test->user_testcases_count; i++) {
+		gdt[seg / 8].type = t[i].type;
+		gdt[seg / 8].dpl = t[i].dpl;
+		gdt[seg / 8].p = t[i].p;
+
+		switch (test->type) {
+		case FEP_TEST_LRET:
+			TEST_FEP_INST_IN_USER(LRET, emulate, t[i].rpl,
+					      &dummy, t[i].vector,
+				              t[i].error_code, t[i].msg);
+			break;
+		}
+	}
+
+	handle_exception(GP_VECTOR, 0);
+	handle_exception(NP_VECTOR, 0);
+}
+
+static unsigned long get_lret_rip_advance(void)
+{
+	extern char lret_start, lret_end;
+	unsigned long lret_rip_advance = &lret_end - &lret_start;
+
+	asm volatile("data16 mov %%cs, %%rax\n\t"
+		     "pushq %%rax\n\t"
+		     "pushq $1f\n\t"
+		     "lret_start: lretq; lret_end:\n\t"
+		     "1:\n\t"
+		     : : : "ax", "memory");
+
+	return lret_rip_advance;
+}
+
+static void test_lret(uint64_t *mem)
+{
+	printf("test lret in hw\n");
+	fep_test_lret.rip_advance = get_lret_rip_advance();
+	test_fep_common(false, &fep_test_lret);
+}
+
+static void test_em_lret(uint64_t *mem)
+{
+	printf("test lret in emulator\n");
+	test_fep_common(true, &fep_test_lret);
+}
+
 static void test_push16(uint64_t *mem)
 {
 	uint64_t rsp1, rsp2;
@@ -1164,6 +1343,7 @@ int main(void)
 	test_smsw(mem);
 	test_lmsw();
 	test_ljmp(mem);
+	test_lret(mem);
 	test_stringio();
 	test_incdecnotneg(mem);
 	test_btc(mem);
@@ -1188,6 +1368,7 @@ int main(void)
 		test_smsw_reg(mem);
 		test_nop(mem);
 		test_mov_dr(mem);
+		test_em_lret(mem);
 	} else {
 		report_skip("skipping register-only tests, "
 			    "use kvm.force_emulation_prefix=1 to enable");
-- 
2.31.1

[kvm-unit-tests PATCH 2/2] x86/emulator: Add some tests for ljmp instruction emulation

[Hou Wenlong]

Per Intel's SDM on the "Instruction Set Reference", when
loading segment descriptor for ljmp, not-present segment
check should be after all type and privilege checks. However,
__load_segment_descriptor() in x86's emulator does not-present
segment check first, so it would trigger #NP instead of #GP
if type or privilege checks fail and the segment is not present.

So add some tests for ljmp instruction, and it will test
those tests in hardware and emulator. Enable
kvm.force_emulation_prefix when try to test them in emulator.

Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
---
 x86/emulator.c | 102 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 89 insertions(+), 13 deletions(-)

diff --git a/x86/emulator.c b/x86/emulator.c
index 480333a40eba..c80e2cf8374e 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -34,6 +34,7 @@ struct fep_test_case {
 
 enum fep_test_inst_type {
 	FEP_TEST_LRET,
+	FEP_TEST_LJMP,
 };
 
 struct fep_test {
@@ -68,6 +69,29 @@ static struct fep_test fep_test_lret = {
 	.user_testcases_count = sizeof(lret_user_testcases) / sizeof(struct fep_test_case),
 };
 
+static struct fep_test_case ljmp_kernel_testcases[] = {
+	{0, DS_TYPE, 0, 0, GP_VECTOR, FIRST_SPARE_SEL, "ljmp desc.type!=code && desc.p=0"},
+	{0, NON_CONFORM_CS_TYPE, 3, 0, GP_VECTOR, FIRST_SPARE_SEL, "jmp non-conforming && dpl!=cpl && desc.p=0"},
+	{3, NON_CONFORM_CS_TYPE, 0, 0, GP_VECTOR, FIRST_SPARE_SEL, "ljmp conforming && rpl>cpl && desc.p=0"},
+	{0, CONFORM_CS_TYPE, 3, 0, GP_VECTOR, FIRST_SPARE_SEL, "ljmp conforming && dpl>cpl && desc.p=0"},
+	{0, NON_CONFORM_CS_TYPE, 0, 0, NP_VECTOR, FIRST_SPARE_SEL, "ljmp desc.p=0"},
+};
+
+static struct fep_test_case ljmp_user_testcases[] = {
+	{3, CONFORM_CS_TYPE, 0, 1, -1, -1, "ljmp dpl<cpl"},
+};
+
+static struct fep_test fep_test_ljmp = {
+	.type = FEP_TEST_LJMP,
+	.kernel_testcases = ljmp_kernel_testcases,
+	.kernel_testcases_count = sizeof(ljmp_kernel_testcases) / sizeof(struct fep_test_case),
+	.user_testcases = ljmp_user_testcases,
+	.user_testcases_count = sizeof(ljmp_user_testcases) / sizeof(struct fep_test_case),
+};
+
+static unsigned long fep_jmp_buf[2];
+static unsigned long *fep_jmp_buf_ptr = &fep_jmp_buf[0];
+
 static void test_in_user(bool emulate, uint16_t rpl, enum fep_test_inst_type type);
 
 #define TEST_LRET_ASM(seg, prefix)		\
@@ -79,6 +103,14 @@ static void test_in_user(bool emulate, uint16_t rpl, enum fep_test_inst_type typ
 		     : : [asm_seg]"r"(seg)	\
 		     : "memory");
 
+#define TEST_LJMP_ASM(seg, prefix)			\
+	*(uint16_t *)(&fep_jmp_buf[1]) = seg;		\
+	asm volatile("movq $1f, (%[mem])\n\t"		\
+		     prefix "rex64 ljmp *(%[mem])\n\t"	\
+		     "1:"				\
+		     : : [mem]"a"(fep_jmp_buf_ptr)	\
+		     : "memory"); \
+
 #define TEST_FEP_RESULT(vector, error_code, msg)	\
 	report(fep_vector == vector &&			\
 	       fep_error_code == error_code,msg);	\
@@ -383,19 +415,6 @@ static void test_pop(void *mem)
 	       "enter");
 }
 
-static void test_ljmp(void *mem)
-{
-    unsigned char *m = mem;
-    volatile int res = 1;
-
-    *(unsigned long**)m = &&jmpf;
-    asm volatile ("data16 mov %%cs, %0":"=m"(*(m + sizeof(unsigned long))));
-    asm volatile ("rex64 ljmp *%0"::"m"(*m));
-    res = 0;
-jmpf:
-    report(res, "ljmp");
-}
-
 static void test_incdecnotneg(void *mem)
 {
     unsigned long *m = mem, v = 1234;
@@ -991,6 +1010,13 @@ static void test_in_user(bool emulate, uint16_t rpl, enum fep_test_inst_type typ
 			TEST_LRET_ASM(seg, "");
 		}
 		break;
+	case FEP_TEST_LJMP:
+		if (emulate) {
+			TEST_LJMP_ASM(seg, KVM_FEP);
+		} else {
+			TEST_LJMP_ASM(seg, "");
+		}
+		break;
 	}
 }
 
@@ -1018,6 +1044,10 @@ static void test_fep_common(bool emulate, struct fep_test *test)
 			TEST_FEP_INST(emulate, LRET, seg, t[i].vector,
 				      t[i].error_code, t[i].msg);
 			break;
+		case FEP_TEST_LJMP:
+			TEST_FEP_INST(emulate, LJMP, seg, t[i].vector,
+				      t[i].error_code, t[i].msg);
+			break;
 		}
 	}
 
@@ -1034,6 +1064,11 @@ static void test_fep_common(bool emulate, struct fep_test *test)
 					      &dummy, t[i].vector,
 				              t[i].error_code, t[i].msg);
 			break;
+		case FEP_TEST_LJMP:
+			TEST_FEP_INST_IN_USER(LJMP, emulate, t[i].rpl,
+					      &dummy, t[i].vector,
+				              t[i].error_code, t[i].msg);
+			break;
 		}
 	}
 
@@ -1041,6 +1076,46 @@ static void test_fep_common(bool emulate, struct fep_test *test)
 	handle_exception(NP_VECTOR, 0);
 }
 
+static unsigned long get_ljmp_rip_advance(void)
+{
+	extern char ljmp_start, ljmp_end;
+	unsigned long ljmp_rip_advance = &ljmp_end - &ljmp_start;
+
+	fep_jmp_buf[0] = (unsigned long)&ljmp_end;
+	*(uint16_t *)(&fep_jmp_buf[1]) = KERNEL_CS;
+	asm volatile ("ljmp_start: rex64 ljmp *(%0); ljmp_end:\n\t"
+		      ::"a"(fep_jmp_buf_ptr) : "memory");
+
+	return ljmp_rip_advance;
+}
+
+static void test_ljmp(void *mem)
+{
+	unsigned char *m = mem;
+	volatile int res = 1;
+
+	*(unsigned long**)m = &&jmpf;
+	asm volatile ("data16 mov %%cs, %0":"=m"(*(m + sizeof(unsigned long))));
+	asm volatile ("rex64 ljmp *%0"::"m"(*m));
+	res = 0;
+jmpf:
+	report(res, "ljmp");
+
+	printf("test ljmp in hw\n");
+	fep_test_ljmp.rip_advance = get_ljmp_rip_advance();
+	test_fep_common(false, &fep_test_ljmp);
+	/* reset CS to KERNEL_CS */
+	(void)get_ljmp_rip_advance();
+}
+
+static void test_em_ljmp(void *mem)
+{
+	printf("test ljmp in emulator\n");
+	test_fep_common(true, &fep_test_ljmp);
+	/* reset CS to KERNEL_CS */
+	(void)get_ljmp_rip_advance();
+}
+
 static unsigned long get_lret_rip_advance(void)
 {
 	extern char lret_start, lret_end;
@@ -1368,6 +1443,7 @@ int main(void)
 		test_smsw_reg(mem);
 		test_nop(mem);
 		test_mov_dr(mem);
+		test_em_ljmp(mem);
 		test_em_lret(mem);
 	} else {
 		report_skip("skipping register-only tests, "
-- 
2.31.1

Re: [kvm-unit-tests PATCH 1/2] x86/emulator: Add some tests for lret instruction emulation

[Sean Christopherson]

On Thu, Jan 20, 2022, Hou Wenlong wrote:
> Per Intel's SDM on the "Instruction Set Reference", when
> loading segment descriptor for far return, not-present segment
> check should be after all type and privilege checks. However,
> __load_segment_descriptor() in x86's emulator does not-present
> segment check first, so it would trigger #NP instead of #GP
> if type or privilege checks fail and the segment is not present.
> 
> And if RPL < CPL, it should trigger #GP, but the check is missing
> in emulator.
> 
> So add some tests for lret instruction, and it will test
> those tests in hardware and emulator. Enable
> kvm.force_emulation_prefix when try to test them in emulator.
> 
> Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> ---
>  x86/emulator.c | 181 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 181 insertions(+)
> 
> diff --git a/x86/emulator.c b/x86/emulator.c
> index c5f584a9d8cc..480333a40eba 100644
> --- a/x86/emulator.c
> +++ b/x86/emulator.c
> @@ -19,6 +19,88 @@ static int exceptions;
>  #define KVM_FEP "ud2; .byte 'k', 'v', 'm';"
>  #define KVM_FEP_LENGTH 5
>  static int fep_available = 1;
> +static unsigned int fep_vector = -1;
> +static unsigned int fep_error_code = -1;

I'd prefer we keep "Forced Emulation Prefix" out of the enums, macros, etc...
It's an implementation detail that has no impact on the functionality being tested.
Even more confusing is that e.g. test_lret() doesn't utilize the force prefix,
which makes all the "fep" nomenclature wrong.

Maybe far_xfer_* for the prefix?

> +struct fep_test_case {
> +	uint16_t rpl;
> +	uint16_t type;

Maybe s/type/insn?  "type" might be misconstrued as the type of segment, e.g. a
call gate, as opposed to the instruction.

> +	uint16_t dpl;
> +	uint16_t p;
> +	unsigned int vector;
> +	unsigned int error_code;
> +	const char *msg;
> +};
> +
> +enum fep_test_inst_type {

"insn" is generally preferred over "inst".

> +	FEP_TEST_LRET,

I strongly prefer FAR_RET over LRET to better match the SDM.  Ditto for FAR_JMP or LJMP.

> +};
> +
> +struct fep_test {
> +	enum fep_test_inst_type type;
> +	unsigned long rip_advance;
> +	struct fep_test_case *kernel_testcases;
> +	unsigned int kernel_testcases_count;
> +	struct fep_test_case *user_testcases;
> +	unsigned int user_testcases_count;

Add a flag to struct far_jmp_test_case to track user vs. kernel.  More below.

> +};
> +
> +#define NON_CONFORM_CS_TYPE	0xb
> +#define CONFORM_CS_TYPE		0xf
> +#define DS_TYPE			0x3
> +
> +static struct fep_test_case lret_kernel_testcases[] = {
> +	{0, DS_TYPE, 0, 0, GP_VECTOR, FIRST_SPARE_SEL, "lret desc.type!=code && desc.p=0"},
> +	{0, NON_CONFORM_CS_TYPE, 3, 0, GP_VECTOR, FIRST_SPARE_SEL, "lret non-conforming && dpl!=rpl && desc.p=0"},
> +	{0, CONFORM_CS_TYPE, 3, 0, GP_VECTOR, FIRST_SPARE_SEL, "lret conforming && dpl>rpl && desc.p=0"},
> +	{0, NON_CONFORM_CS_TYPE, 0, 0, NP_VECTOR, FIRST_SPARE_SEL, "lret desc.p=0"},
> +};
> +
> +static struct fep_test_case lret_user_testcases[] = {
> +	{0, NON_CONFORM_CS_TYPE, 3, 1, GP_VECTOR, FIRST_SPARE_SEL, "lret rpl<cpl"},
> +};
> +
> +static struct fep_test fep_test_lret = {
> +	.type = FEP_TEST_LRET,
> +	.kernel_testcases = lret_kernel_testcases,
> +	.kernel_testcases_count = sizeof(lret_kernel_testcases) / sizeof(struct fep_test_case),
> +	.user_testcases = lret_user_testcases,
> +	.user_testcases_count = sizeof(lret_user_testcases) / sizeof(struct fep_test_case),
> +};
> +
> +static void test_in_user(bool emulate, uint16_t rpl, enum fep_test_inst_type type);
> +
> +#define TEST_LRET_ASM(seg, prefix)		\
> +	asm volatile("pushq %[asm_seg]\n\t"	\
> +		     "pushq $1f\n\t"		\
> +		      prefix "lretq\n\t"	\
> +		     "addq $16, %%rsp\n\t"	\
> +		     "1:"			\
> +		     : : [asm_seg]"r"(seg)	\
> +		     : "memory");
> +
> +#define TEST_FEP_RESULT(vector, error_code, msg)	\
> +	report(fep_vector == vector &&			\
> +	       fep_error_code == error_code,msg);	\
> +	fep_vector = -1;				\
> +	fep_error_code = -1;

These should really be set _before_ running a test, not after.

> +#define TEST_FEP_INST(emulate, inst, seg, vector, error_code, msg) \
> +	do {							   \
> +		if (emulate) {					   \
> +			TEST_##inst##_ASM(seg, KVM_FEP);	   \
> +		} else {					   \
> +			TEST_##inst##_ASM(seg, "");		   \
> +		}						   \
> +		TEST_FEP_RESULT(vector, error_code, msg);	   \
> +	} while (0)
> +
> +#define TEST_FEP_INST_IN_USER(inst, emulate, rpl, dummy, vector, error_code, msg)\
> +	do {								\
> +		run_in_user((usermode_func)test_in_user, UD_VECTOR,	\
> +			     emulate, rpl, FEP_TEST_##inst, 0, dummy);	\
> +		TEST_FEP_RESULT(vector, error_code, msg);		\
> +	} while (0)

I love macro frameworks, but this is gratutious and makes the code hard to follow.
Low level macros to emit the assembly is necessary to deal with the prefix, but
everything else can be handled by refactoring the test loop.

> +static void test_in_user(bool emulate, uint16_t rpl, enum fep_test_inst_type type)

Here and elsewhere, s/emulate/force_emulation to clarity who/what is doing the
emulation.

> +{
> +	uint16_t seg = FIRST_SPARE_SEL | rpl;
> +
> +	switch (type) {
> +	case FEP_TEST_LRET:
> +		if (emulate) {
> +			TEST_LRET_ASM(seg, KVM_FEP);
> +		} else {
> +			TEST_LRET_ASM(seg, "");
> +		}

		TEST_FAR_RET_ASM(seg, emulate ? KVM_FEP : "");


Actually, assuming there are no string concatenation tricks we can play, even
better would be to make the current TEST_LRET_ASM __TEST_FAR_RET_ASM, and then
have TEST_FAR_RET_ASM() be:

#define TEST_FAR_RET_ASM(seg, force_emulation)	\
	__TEST_FAR_RET_ASM(seg, (force_emulation) ? KVM_FEP : "" )


> +		break;

This would ideally have a "default" path to fire on unknown instructions.

> +	}
> +}
> +
> +static void test_fep_common(bool emulate, struct fep_test *test)
> +{
> +	int i;
> +	bool dummy;
> +	struct fep_test_case *t;
> +	uint16_t seg = FIRST_SPARE_SEL;
> +
> +	handle_exception(GP_VECTOR, fep_exception_handler);
> +	handle_exception(NP_VECTOR, fep_exception_handler);
> +	rip_advance = test->rip_advance;
> +
> +	gdt[seg / 8] = gdt[KERNEL_CS / 8];
> +	t = test->kernel_testcases;
> +	for (i = 0; i < test->kernel_testcases_count; i++) {
> +		seg = FIRST_SPARE_SEL | t[i].rpl;
> +		gdt[seg / 8].type = t[i].type;
> +		gdt[seg / 8].dpl = t[i].dpl;
> +		gdt[seg / 8].p = t[i].p;
> +
> +		switch (test->type) {
> +		case FEP_TEST_LRET:
> +			TEST_FEP_INST(emulate, LRET, seg, t[i].vector,
> +				      t[i].error_code, t[i].msg);
> +			break;
> +		}
> +	}
> +
> +	gdt[seg / 8] = gdt[USER_CS64 / 8];
> +	t = test->user_testcases;
> +	for (i = 0; i < test->user_testcases_count; i++) {
> +		gdt[seg / 8].type = t[i].type;
> +		gdt[seg / 8].dpl = t[i].dpl;
> +		gdt[seg / 8].p = t[i].p;
> +
> +		switch (test->type) {
> +		case FEP_TEST_LRET:
> +			TEST_FEP_INST_IN_USER(LRET, emulate, t[i].rpl,
> +					      &dummy, t[i].vector,
> +				              t[i].error_code, t[i].msg);
> +			break;
> +		}
> +	}
> +
> +	handle_exception(GP_VECTOR, 0);
> +	handle_exception(NP_VECTOR, 0);
> +}

To avoid a bunch of macros, this can be refactored to (completely untested):

static void __test_far_xfer(enum far_xfer_insn insn, uint16_t seg,
			    bool force_emulation)
{
	switch (insn) {
	case FAR_XFER_RET:
		TEST_FAR_RET_ASM(seg, force_emulation);
		break;
	case FAR_XFER_JMP:
		TEST_FAR_JMP_ASM(seg, force_emulation);
		break;
	default:
		report_fail(...);
		break;
	}
}

static void test_far_xfer(bool force_emulation, struct far_xfer_test *test)
{
	struct far_xfer_test_case *t;
	uint16_t seg;
	bool ign;
	int i;

	handle_exception(GP_VECTOR, far_xfer_exception_handler);
	handle_exception(NP_VECTOR, far_xfer_exception_handler);

	for (i = 0; i < test->nr_testcases; i++) {
		t = test->testases[i];

		seg = FIRST_SPARE_SEL | t->rpl;
		gdt[seg / 8] = gdt[(t->usermode ? USER_CS64 : KERNEL_CS) / 8];
		gdt[seg / 8].type = t->type;
		gdt[seg / 8].dpl = t->dpl;
		gdt[seg / 8].p = t->p;

		far_xfer_vector = -1;
		far_error_code = -1;

		if (t->usermode)
			run_in_user((usermode_func)__test_far_xfer, UD_VECTOR,
				    t->insn, seg, force_emulation, 0, &ign);
		else
			__test_far_xfer(t->insn, seg, force_emulation);

		report(far_xfer_vector == vector &&
		       far_xfer_error_code == error_code, t->msg);
	}

	handle_exception(GP_VECTOR, 0);
	handle_exception(NP_VECTOR, 0);
}

> +
> +static unsigned long get_lret_rip_advance(void)
> +{
> +	extern char lret_start, lret_end;
> +	unsigned long lret_rip_advance = &lret_end - &lret_start;
> +
> +	asm volatile("data16 mov %%cs, %%rax\n\t"
> +		     "pushq %%rax\n\t"
> +		     "pushq $1f\n\t"
> +		     "lret_start: lretq; lret_end:\n\t"
> +		     "1:\n\t"
> +		     : : : "ax", "memory");

Hrm, this is probably fine in practice, but I don't love assuming that the compiler
will emit identical instructions.  I think we can instead use a hardcoded GPR to
store the target, e.g.

static void far_xfer_exception_handler(struct ex_regs *regs)
{
	far_xfer_vector = regs->vector;
	far_xfer_error_code = regs->error_code;
	regs->rip = regs->rax;
}

and then in the asm

#define __TEST_FAR_RET_ASM(seg, prefix)		\
	asm volatile("lea 1f(%%rip), %%rax\n\t"	\
		     "pushq %[asm_seg]\n\t"	\
		     "pushq $1f\n\t"		\
		      prefix "lretq\n\t"	\
		     "addq $16, %%rsp\n\t"	\
		     "1:"			\
		     : : [asm_seg]"r"(seg)	\
		     : "eax", "memory");

> +
> +	return lret_rip_advance;
> +}
> +
> +static void test_lret(uint64_t *mem)
> +{
> +	printf("test lret in hw\n");
> +	fep_test_lret.rip_advance = get_lret_rip_advance();
> +	test_fep_common(false, &fep_test_lret);
> +}
> +
> +static void test_em_lret(uint64_t *mem)
> +{
> +	printf("test lret in emulator\n");

This will fail if run in isolation, no?  I.e. fep_test_lret.rip_advance won't be
set.  Test should really be standalone, though KUT does a poor job on that front :-(

Moot point if we can find a way to avoid using a global for the post-fault RIP.

> +	test_fep_common(true, &fep_test_lret);
> +}
> +
>  static void test_push16(uint64_t *mem)
>  {
>  	uint64_t rsp1, rsp2;
> @@ -1164,6 +1343,7 @@ int main(void)
>  	test_smsw(mem);
>  	test_lmsw();
>  	test_ljmp(mem);
> +	test_lret(mem);
>  	test_stringio();
>  	test_incdecnotneg(mem);
>  	test_btc(mem);
> @@ -1188,6 +1368,7 @@ int main(void)
>  		test_smsw_reg(mem);
>  		test_nop(mem);
>  		test_mov_dr(mem);
> +		test_em_lret(mem);
>  	} else {
>  		report_skip("skipping register-only tests, "
>  			    "use kvm.force_emulation_prefix=1 to enable");
> -- 
> 2.31.1
> 

[kvm-unit-tests PATCH v2 0/2] x86/emulator: Add some tests for loading segment descriptor in emulator

[Hou Wenlong]

Add some lret and ljmp tests for the related x86 emulator bugs[*]. Those tests
would be tested both on hardware and emulator. Enable kvm.force_emulation_prefix
to test them on emulator.

Changed from v1:
- As Sean suggested, refactor the test loop to make the code simple.

[*] https://lore.kernel.org/kvm/cover.1644292363.git.houwenlong.hwl@antgroup.com

Hou Wenlong (2):
  x86/emulator: Add some tests for lret instruction emulation
  x86/emulator: Add some tests for ljmp instruction emulation

 x86/emulator.c | 205 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 192 insertions(+), 13 deletions(-)

--
2.31.1

[kvm-unit-tests PATCH v2 1/2] x86/emulator: Add some tests for lret instruction emulation

[Hou Wenlong]

Per Intel's SDM on the "Instruction Set Reference", when
loading segment descriptor for far return, not-present segment
check should be after all type and privilege checks. However,
__load_segment_descriptor() in x86's emulator does not-present
segment check first, so it would trigger #NP instead of #GP
if type or privilege checks fail and the segment is not present.

And if RPL < CPL, it should trigger #GP, but the check is missing
in emulator.

So add some tests for lret instruction, and it will test
those tests in hardware and emulator. Enable
kvm.force_emulation_prefix when try to test them in emulator.

Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
---
 x86/emulator.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 130 insertions(+)

diff --git a/x86/emulator.c b/x86/emulator.c
index 22a518f4ad65..a68debaabef0 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -19,6 +19,66 @@ static int exceptions;
 #define KVM_FEP "ud2; .byte 'k', 'v', 'm';"
 #define KVM_FEP_LENGTH 5
 static int fep_available = 1;
+static unsigned int far_xfer_vector = -1;
+static unsigned int far_xfer_error_code = -1;
+
+struct far_xfer_test_case {
+	uint16_t rpl;
+	uint16_t type;
+	uint16_t dpl;
+	uint16_t p;
+	bool usermode;
+	unsigned int vector;
+	unsigned int error_code;
+	const char *msg;
+};
+
+enum far_xfer_insn {
+	FAR_XFER_RET,
+};
+
+struct far_xfer_test {
+	enum far_xfer_insn insn;
+	struct far_xfer_test_case *testcases;
+	unsigned int nr_testcases;
+};
+
+#define NON_CONFORM_CS_TYPE	0xb
+#define CONFORM_CS_TYPE		0xf
+#define DS_TYPE			0x3
+
+static struct far_xfer_test_case far_ret_testcases[] = {
+	{0, DS_TYPE, 0, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret desc.type!=code && desc.p=0"},
+	{0, NON_CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret non-conforming && dpl!=rpl && desc.p=0"},
+	{0, CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret conforming && dpl>rpl && desc.p=0"},
+	{0, NON_CONFORM_CS_TYPE, 0, 0, false, NP_VECTOR, FIRST_SPARE_SEL, "lret desc.p=0"},
+	{0, NON_CONFORM_CS_TYPE, 3, 1, true, GP_VECTOR, FIRST_SPARE_SEL, "lret rpl<cpl"},
+};
+
+static struct far_xfer_test far_ret_test = {
+	.insn = FAR_XFER_RET,
+	.testcases = &far_ret_testcases[0],
+	.nr_testcases = sizeof(far_ret_testcases) / sizeof(struct far_xfer_test_case),
+};
+
+#define TEST_FAR_RET_ASM(seg, prefix)		\
+	asm volatile("lea 1f(%%rip), %%rax\n\t" \
+		     "pushq %[asm_seg]\n\t"	\
+		     "pushq $2f\n\t"		\
+		      prefix "lretq\n\t"	\
+		     "1: addq $16, %%rsp\n\t"	\
+		     "2:"			\
+		     : : [asm_seg]"r"(seg)	\
+		     : "eax", "memory");
+
+static inline void test_far_ret_asm(uint16_t seg, bool force_emulation)
+{
+	if (force_emulation) {
+		TEST_FAR_RET_ASM(seg, KVM_FEP);
+	} else {
+		TEST_FAR_RET_ASM(seg, "");
+	}
+}
 
 struct regs {
 	u64 rax, rbx, rcx, rdx;
@@ -891,6 +951,74 @@ static void test_mov_dr(uint64_t *mem)
 	report(rax == dr6_fixed_1, "mov_dr6");
 }
 
+static void far_xfer_exception_handler(struct ex_regs *regs)
+{
+	far_xfer_vector = regs->vector;
+	far_xfer_error_code = regs->error_code;
+	regs->rip = regs->rax;;
+}
+
+static void __test_far_xfer(enum far_xfer_insn insn, uint16_t seg,
+			    bool force_emulation)
+{
+	switch (insn) {
+	case FAR_XFER_RET:
+		test_far_ret_asm(seg, force_emulation);
+		break;
+	default:
+		report_fail("unknown instructions");
+		break;
+	}
+}
+
+static void test_far_xfer(bool force_emulation, struct far_xfer_test *test)
+{
+	struct far_xfer_test_case *t;
+	uint16_t seg;
+	bool ign;
+	int i;
+
+	handle_exception(GP_VECTOR, far_xfer_exception_handler);
+	handle_exception(NP_VECTOR, far_xfer_exception_handler);
+
+	for (i = 0; i < test->nr_testcases; i++) {
+		t = &test->testcases[i];
+
+		seg = FIRST_SPARE_SEL | t->rpl;
+		gdt[seg / 8] = gdt[(t->usermode ? USER_CS64 : KERNEL_CS) / 8];
+		gdt[seg / 8].type = t->type;
+		gdt[seg / 8].dpl = t->dpl;
+		gdt[seg / 8].p = t->p;
+
+		far_xfer_vector = -1;
+		far_xfer_error_code = -1;
+
+		if (t->usermode)
+			run_in_user((usermode_func)__test_far_xfer, UD_VECTOR,
+				    test->insn, seg, force_emulation, 0, &ign);
+		else
+			__test_far_xfer(test->insn, seg, force_emulation);
+
+		report(far_xfer_vector == t->vector &&
+		       far_xfer_error_code == t->error_code, t->msg);
+	}
+
+	handle_exception(GP_VECTOR, 0);
+	handle_exception(NP_VECTOR, 0);
+}
+
+static void test_lret(uint64_t *mem)
+{
+	printf("test lret in hw\n");
+	test_far_xfer(false, &far_ret_test);
+}
+
+static void test_em_lret(uint64_t *mem)
+{
+	printf("test lret in emulator\n");
+	test_far_xfer(true, &far_ret_test);
+}
+
 static void test_push16(uint64_t *mem)
 {
 	uint64_t rsp1, rsp2;
@@ -1165,6 +1293,7 @@ int main(void)
 	test_smsw(mem);
 	test_lmsw();
 	test_ljmp(mem);
+	test_lret(mem);
 	test_stringio();
 	test_incdecnotneg(mem);
 	test_btc(mem);
@@ -1189,6 +1318,7 @@ int main(void)
 		test_smsw_reg(mem);
 		test_nop(mem);
 		test_mov_dr(mem);
+		test_em_lret(mem);
 	} else {
 		report_skip("skipping register-only tests, "
 			    "use kvm.force_emulation_prefix=1 to enable");
-- 
2.31.1

[kvm-unit-tests PATCH v2 2/2] x86/emulator: Add some tests for ljmp instruction emulation

[Hou Wenlong]

Per Intel's SDM on the "Instruction Set Reference", when
loading segment descriptor for ljmp, not-present segment
check should be after all type and privilege checks. However,
__load_segment_descriptor() in x86's emulator does not-present
segment check first, so it would trigger #NP instead of #GP
if type or privilege checks fail and the segment is not present.

So add some tests for ljmp instruction, and it will test
those tests in hardware and emulator. Enable
kvm.force_emulation_prefix when try to test them in emulator.

Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
---
 x86/emulator.c | 75 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 62 insertions(+), 13 deletions(-)

diff --git a/x86/emulator.c b/x86/emulator.c
index a68debaabef0..b4e474356ff7 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -35,6 +35,7 @@ struct far_xfer_test_case {
 
 enum far_xfer_insn {
 	FAR_XFER_RET,
+	FAR_XFER_JMP,
 };
 
 struct far_xfer_test {
@@ -61,6 +62,24 @@ static struct far_xfer_test far_ret_test = {
 	.nr_testcases = sizeof(far_ret_testcases) / sizeof(struct far_xfer_test_case),
 };
 
+static struct far_xfer_test_case far_jmp_testcases[] = {
+	{0, DS_TYPE, 0, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "ljmp desc.type!=code && desc.p=0"},
+	{0, NON_CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "jmp non-conforming && dpl!=cpl && desc.p=0"},
+	{3, NON_CONFORM_CS_TYPE, 0, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "ljmp conforming && rpl>cpl && desc.p=0"},
+	{0, CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "ljmp conforming && dpl>cpl && desc.p=0"},
+	{0, NON_CONFORM_CS_TYPE, 0, 0, false, NP_VECTOR, FIRST_SPARE_SEL, "ljmp desc.p=0"},
+	{3, CONFORM_CS_TYPE, 0, 1, true, -1, -1, "ljmp dpl<cpl"},
+};
+
+static struct far_xfer_test far_jmp_test = {
+	.insn = FAR_XFER_JMP,
+	.testcases = &far_jmp_testcases[0],
+	.nr_testcases = sizeof(far_jmp_testcases) / sizeof(struct far_xfer_test_case),
+};
+
+static unsigned long fep_jmp_buf[2];
+static unsigned long *fep_jmp_buf_ptr = &fep_jmp_buf[0];
+
 #define TEST_FAR_RET_ASM(seg, prefix)		\
 	asm volatile("lea 1f(%%rip), %%rax\n\t" \
 		     "pushq %[asm_seg]\n\t"	\
@@ -80,6 +99,24 @@ static inline void test_far_ret_asm(uint16_t seg, bool force_emulation)
 	}
 }
 
+#define TEST_FAR_JMP_ASM(seg, prefix)		\
+	*(uint16_t *)(&fep_jmp_buf[1]) = seg;	\
+	asm volatile("lea 1f(%%rip), %%rax\n\t" \
+		     "movq $1f, (%[mem])\n\t"	\
+		      prefix "rex64 ljmp *(%[mem])\n\t"\
+		     "1:"			\
+		     : : [mem]"r"(fep_jmp_buf_ptr)\
+		     : "eax", "memory");
+
+static inline void test_far_jmp_asm(uint16_t seg, bool force_emulation)
+{
+	if (force_emulation) {
+		TEST_FAR_JMP_ASM(seg, KVM_FEP);
+	} else {
+		TEST_FAR_JMP_ASM(seg, "");
+	}
+}
+
 struct regs {
 	u64 rax, rbx, rcx, rdx;
 	u64 rsi, rdi, rsp, rbp;
@@ -362,19 +399,6 @@ static void test_pop(void *mem)
 	       "enter");
 }
 
-static void test_ljmp(void *mem)
-{
-    unsigned char *m = mem;
-    volatile int res = 1;
-
-    *(unsigned long**)m = &&jmpf;
-    asm volatile ("data16 mov %%cs, %0":"=m"(*(m + sizeof(unsigned long))));
-    asm volatile ("rex64 ljmp *%0"::"m"(*m));
-    res = 0;
-jmpf:
-    report(res, "ljmp");
-}
-
 static void test_incdecnotneg(void *mem)
 {
     unsigned long *m = mem, v = 1234;
@@ -965,6 +989,9 @@ static void __test_far_xfer(enum far_xfer_insn insn, uint16_t seg,
 	case FAR_XFER_RET:
 		test_far_ret_asm(seg, force_emulation);
 		break;
+	case FAR_XFER_JMP:
+		test_far_jmp_asm(seg, force_emulation);
+		break;
 	default:
 		report_fail("unknown instructions");
 		break;
@@ -1007,6 +1034,27 @@ static void test_far_xfer(bool force_emulation, struct far_xfer_test *test)
 	handle_exception(NP_VECTOR, 0);
 }
 
+static void test_ljmp(uint64_t *mem)
+{
+	unsigned char *m = (unsigned char *)mem;
+	volatile int res = 1;
+
+	*(unsigned long**)m = &&jmpf;
+	asm volatile ("data16 mov %%cs, %0":"=m"(*(m + sizeof(unsigned long))));
+	asm volatile ("rex64 ljmp *%0"::"m"(*m));
+	res = 0;
+jmpf:
+	report(res, "ljmp");
+
+	printf("test ljmp in hw\n");
+	test_far_xfer(false, &far_jmp_test);
+}
+
+static void test_em_ljmp(uint64_t *mem)
+{
+	printf("test ljmp in emulator\n");
+	test_far_xfer(true, &far_jmp_test);
+}
 static void test_lret(uint64_t *mem)
 {
 	printf("test lret in hw\n");
@@ -1318,6 +1366,7 @@ int main(void)
 		test_smsw_reg(mem);
 		test_nop(mem);
 		test_mov_dr(mem);
+		test_em_ljmp(mem);
 		test_em_lret(mem);
 	} else {
 		report_skip("skipping register-only tests, "
-- 
2.31.1

Re: [kvm-unit-tests PATCH v2 1/2] x86/emulator: Add some tests for lret instruction emulation

[Sean Christopherson]

Nit, "far ret" instead of "lret" in the shortlog.

On Tue, Feb 08, 2022, Hou Wenlong wrote:
> Per Intel's SDM on the "Instruction Set Reference", when
> loading segment descriptor for far return, not-present segment
> check should be after all type and privilege checks. However,
> __load_segment_descriptor() in x86's emulator does not-present
> segment check first, so it would trigger #NP instead of #GP
> if type or privilege checks fail and the segment is not present.
> 
> And if RPL < CPL, it should trigger #GP, but the check is missing
> in emulator.
> 
> So add some tests for lret instruction, and it will test
> those tests in hardware and emulator. Enable
> kvm.force_emulation_prefix when try to test them in emulator.
> 
> Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> ---

Was this tested?  There are multiple compilation errors with gcc...

x86/emulator.c: In function ‘test_far_xfer’:
x86/emulator.c:1034:10: error: format not a string literal and no format arguments [-Werror=format-security]
 1034 |          far_xfer_error_code == t->error_code, t->msg);
      |          ^~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [<builtin>: x86/emulator.o] Error 1

x86/emulator.c: Assembler messages:
x86/emulator.c:81: Error: incorrect register `%si' used with `q' suffix
x86/emulator.c:83: Error: incorrect register `%si' used with `q' suffix
make: *** [<builtin>: x86/emulator.o] Error 1

> +static struct far_xfer_test_case far_ret_testcases[] = {
> +	{0, DS_TYPE, 0, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret desc.type!=code && desc.p=0"},
> +	{0, NON_CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret non-conforming && dpl!=rpl && desc.p=0"},
> +	{0, CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret conforming && dpl>rpl && desc.p=0"},
> +	{0, NON_CONFORM_CS_TYPE, 0, 0, false, NP_VECTOR, FIRST_SPARE_SEL, "lret desc.p=0"},
> +	{0, NON_CONFORM_CS_TYPE, 3, 1, true, GP_VECTOR, FIRST_SPARE_SEL, "lret rpl<cpl"},

Since the framework is responsible for specifying the selector and the expected
error code is the same for all subtests that expect an exception, just drop the
error code.  E.g. if the framework decides to use a different selector than these
would need to be updated too, for no real benefit.

It'd also be helpful to align everything.

s/lret/far ret.  Actually to cut down on copy+paste, and because we need formatting
anyways (see below), how moving the instruction name into the far_xfer_test?

> +};
> +
> +static struct far_xfer_test far_ret_test = {
> +	.insn = FAR_XFER_RET,
> +	.testcases = &far_ret_testcases[0],
> +	.nr_testcases = sizeof(far_ret_testcases) / sizeof(struct far_xfer_test_case),
> +};
> +
> +#define TEST_FAR_RET_ASM(seg, prefix)		\
> +	asm volatile("lea 1f(%%rip), %%rax\n\t" \
> +		     "pushq %[asm_seg]\n\t"	\
> +		     "pushq $2f\n\t"		\
> +		      prefix "lretq\n\t"	\
> +		     "1: addq $16, %%rsp\n\t"	\
> +		     "2:"			\
> +		     : : [asm_seg]"r"(seg)	\

Because @seg is a u16, gcc emites "pushq SI".  Kinda dumb, but...

@@ -72,7 +71,7 @@ static struct far_xfer_test far_ret_test = {
                      prefix "lretq\n\t"        \
                     "1: addq $16, %%rsp\n\t"   \
                     "2:"                       \
-                    : : [asm_seg]"r"(seg)      \
+                    : : [asm_seg]"r"((u64)seg) \
                     : "eax", "memory");

> +		     : "eax", "memory");
> +
> +static inline void test_far_ret_asm(uint16_t seg, bool force_emulation)
> +{
> +	if (force_emulation) {

No need for braces.  Ah, the macro is missing ({ ... }) to force it to be evaluated
as a single statement.  It should be.

Side topic, a ternary operator would be nice, but I don't know how to force string
concatentation for this case...

#define TEST_FAR_RET_ASM(seg, prefix)		\
({						\
	asm volatile("lea 1f(%%rip), %%rax\n\t" \
		     "pushq %[asm_seg]\n\t"	\
		     "pushq $2f\n\t"		\
		      prefix "lretq\n\t"	\
		     "1: addq $16, %%rsp\n\t"	\
		     "2:"			\
		     : : [asm_seg]"r"((u64)seg)	\
		     : "eax", "memory");	\
})

> +		TEST_FAR_RET_ASM(seg, KVM_FEP);
> +	} else {
> +		TEST_FAR_RET_ASM(seg, "");
> +	}
> +}

This only has a single caller, just open code the macros in the case statements
of __test_far_xfer().  (My apologies if I suggested this in the last version).

>  struct regs {
>  	u64 rax, rbx, rcx, rdx;
> @@ -891,6 +951,74 @@ static void test_mov_dr(uint64_t *mem)
>  	report(rax == dr6_fixed_1, "mov_dr6");
>  }
>  
> +static void far_xfer_exception_handler(struct ex_regs *regs)
> +{
> +	far_xfer_vector = regs->vector;
> +	far_xfer_error_code = regs->error_code;
> +	regs->rip = regs->rax;;

Double semi-colon.

> +}
> +
> +static void __test_far_xfer(enum far_xfer_insn insn, uint16_t seg,
> +			    bool force_emulation)
> +{
> +	switch (insn) {
> +	case FAR_XFER_RET:
> +		test_far_ret_asm(seg, force_emulation);
> +		break;
> +	default:
> +		report_fail("unknown instructions");

It's worth spitting out the insn, it might save someone a few seconds/minutes, e.g.

		report_fail("Unexpected insn enum = %d\n", insn);

> +		break;
> +	}
> +}
> +
> +static void test_far_xfer(bool force_emulation, struct far_xfer_test *test)
> +{
> +	struct far_xfer_test_case *t;
> +	uint16_t seg;
> +	bool ign;
> +	int i;
> +
> +	handle_exception(GP_VECTOR, far_xfer_exception_handler);
> +	handle_exception(NP_VECTOR, far_xfer_exception_handler);
> +
> +	for (i = 0; i < test->nr_testcases; i++) {
> +		t = &test->testcases[i];
> +
> +		seg = FIRST_SPARE_SEL | t->rpl;
> +		gdt[seg / 8] = gdt[(t->usermode ? USER_CS64 : KERNEL_CS) / 8];
> +		gdt[seg / 8].type = t->type;
> +		gdt[seg / 8].dpl = t->dpl;
> +		gdt[seg / 8].p = t->p;
> +
> +		far_xfer_vector = -1;
> +		far_xfer_error_code = -1;
> +
> +		if (t->usermode)
> +			run_in_user((usermode_func)__test_far_xfer, UD_VECTOR,
> +				    test->insn, seg, force_emulation, 0, &ign);
> +		else
> +			__test_far_xfer(test->insn, seg, force_emulation);
> +
> +		report(far_xfer_vector == t->vector &&
> +		       far_xfer_error_code == t->error_code, t->msg);

To avoid having to specify the error code, just do:

		       (far_xfer_vector < 0 || far_xfer_error_code == FIRST_SPARE_SEL),

far_xfer_vector and t->vector need to be signed ints, but that's perfectly ok,
vector fits in a u8.  Printing out the expecte vs. actual is also helpful for
debug (pet peeve of mine in KUT...).  I doesn't have to be super fancy formatting,
just enough so that the user doesn't have to modify the test to figure out what
went wrong.

And with the insn_name + msg + target (see below) formatting:


		report(far_xfer_vector == t->vector &&
		       (far_xfer_vector < 0 || far_xfer_error_code == FIRST_SPARE_SEL),
		       "%s on %s, %s: wanted %d (%d), got %d (%d)",
		       test->insn_name, force_emulation ? "emuator" : "hardware", t->msg,
		       t->vector, t->vector < 0 ? -1 : FIRST_SPARE_SEL,
		       far_xfer_vector, far_xfer_error_code);

> +	}
> +
> +	handle_exception(GP_VECTOR, 0);
> +	handle_exception(NP_VECTOR, 0);
> +}
> +
> +static void test_lret(uint64_t *mem)

test_far_ret()

> +{
> +	printf("test lret in hw\n");

Rather than print this out before, just spit out the "target" in the report.

> +	test_far_xfer(false, &far_ret_test);
> +}

All of the above (I think) feeback:

---
 x86/emulator.c | 57 +++++++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/x86/emulator.c b/x86/emulator.c
index 2a35caf..d28e36c 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -21,7 +21,7 @@ static int exceptions;
 #define KVM_FEP "ud2; .byte 'k', 'v', 'm';"
 #define KVM_FEP_LENGTH 5
 static int fep_available = 1;
-static unsigned int far_xfer_vector = -1;
+static int far_xfer_vector = -1;
 static unsigned int far_xfer_error_code = -1;

 struct far_xfer_test_case {
@@ -30,8 +30,7 @@ struct far_xfer_test_case {
 	uint16_t dpl;
 	uint16_t p;
 	bool usermode;
-	unsigned int vector;
-	unsigned int error_code;
+	int vector;
 	const char *msg;
 };

@@ -41,6 +40,7 @@ enum far_xfer_insn {

 struct far_xfer_test {
 	enum far_xfer_insn insn;
+	const char *insn_name;
 	struct far_xfer_test_case *testcases;
 	unsigned int nr_testcases;
 };
@@ -50,37 +50,31 @@ struct far_xfer_test {
 #define DS_TYPE			0x3

 static struct far_xfer_test_case far_ret_testcases[] = {
-	{0, DS_TYPE, 0, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret desc.type!=code && desc.p=0"},
-	{0, NON_CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret non-conforming && dpl!=rpl && desc.p=0"},
-	{0, CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret conforming && dpl>rpl && desc.p=0"},
-	{0, NON_CONFORM_CS_TYPE, 0, 0, false, NP_VECTOR, FIRST_SPARE_SEL, "lret desc.p=0"},
-	{0, NON_CONFORM_CS_TYPE, 3, 1, true, GP_VECTOR, FIRST_SPARE_SEL, "lret rpl<cpl"},
+	{0, DS_TYPE,		 0, 0, false, GP_VECTOR, "desc.type!=code && desc.p=0"},
+	{0, NON_CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, "non-conforming && dpl!=rpl && desc.p=0"},
+	{0, CONFORM_CS_TYPE,     3, 0, false, GP_VECTOR, "conforming && dpl>rpl && desc.p=0"},
+	{0, NON_CONFORM_CS_TYPE, 0, 0, false, NP_VECTOR, "desc.p=0"},
+	{0, NON_CONFORM_CS_TYPE, 3, 1, true,  GP_VECTOR, "rpl<cpl"},
 };

 static struct far_xfer_test far_ret_test = {
 	.insn = FAR_XFER_RET,
+	.insn_name = "far ret",
 	.testcases = &far_ret_testcases[0],
 	.nr_testcases = sizeof(far_ret_testcases) / sizeof(struct far_xfer_test_case),
 };

 #define TEST_FAR_RET_ASM(seg, prefix)		\
+({						\
 	asm volatile("lea 1f(%%rip), %%rax\n\t" \
 		     "pushq %[asm_seg]\n\t"	\
 		     "pushq $2f\n\t"		\
 		      prefix "lretq\n\t"	\
 		     "1: addq $16, %%rsp\n\t"	\
 		     "2:"			\
-		     : : [asm_seg]"r"(seg)	\
-		     : "eax", "memory");
-
-static inline void test_far_ret_asm(uint16_t seg, bool force_emulation)
-{
-	if (force_emulation) {
-		TEST_FAR_RET_ASM(seg, KVM_FEP);
-	} else {
-		TEST_FAR_RET_ASM(seg, "");
-	}
-}
+		     : : [asm_seg]"r"((u64)seg)	\
+		     : "eax", "memory");	\
+})

 struct regs {
 	u64 rax, rbx, rcx, rdx;
@@ -959,7 +953,7 @@ static void far_xfer_exception_handler(struct ex_regs *regs)
 {
 	far_xfer_vector = regs->vector;
 	far_xfer_error_code = regs->error_code;
-	regs->rip = regs->rax;;
+	regs->rip = regs->rax;
 }

 static void __test_far_xfer(enum far_xfer_insn insn, uint16_t seg,
@@ -967,10 +961,13 @@ static void __test_far_xfer(enum far_xfer_insn insn, uint16_t seg,
 {
 	switch (insn) {
 	case FAR_XFER_RET:
-		test_far_ret_asm(seg, force_emulation);
+		if (force_emulation)
+			TEST_FAR_RET_ASM(seg, KVM_FEP);
+		else
+			TEST_FAR_RET_ASM(seg, "");
 		break;
 	default:
-		report_fail("unknown instructions");
+		report_fail("Unexpected insn enum = %d\n", insn);
 		break;
 	}
 }
@@ -1004,22 +1001,24 @@ static void test_far_xfer(bool force_emulation, struct far_xfer_test *test)
 			__test_far_xfer(test->insn, seg, force_emulation);

 		report(far_xfer_vector == t->vector &&
-		       far_xfer_error_code == t->error_code, "%s", t->msg);
+		       (far_xfer_vector < 0 || far_xfer_error_code == FIRST_SPARE_SEL),
+		       "%s on %s, %s: wanted %d (%d), got %d (%d)",
+		       test->insn_name, force_emulation ? "emuator" : "hardware", t->msg,
+		       t->vector, t->vector < 0 ? -1 : FIRST_SPARE_SEL,
+		       far_xfer_vector, far_xfer_error_code);
 	}

 	handle_exception(GP_VECTOR, 0);
 	handle_exception(NP_VECTOR, 0);
 }

-static void test_lret(uint64_t *mem)
+static void test_far_ret(uint64_t *mem)
 {
-	printf("test lret in hw\n");
 	test_far_xfer(false, &far_ret_test);
 }

-static void test_em_lret(uint64_t *mem)
+static void test_em_far_ret(uint64_t *mem)
 {
-	printf("test lret in emulator\n");
 	test_far_xfer(true, &far_ret_test);
 }

@@ -1297,7 +1296,7 @@ int main(void)
 	test_smsw(mem);
 	test_lmsw();
 	test_ljmp(mem);
-	test_lret(mem);
+	test_far_ret(mem);
 	test_stringio();
 	test_incdecnotneg(mem);
 	test_btc(mem);
@@ -1322,7 +1321,7 @@ int main(void)
 		test_smsw_reg(mem);
 		test_nop(mem);
 		test_mov_dr(mem);
-		test_em_lret(mem);
+		test_em_far_ret(mem);
 	} else {
 		report_skip("skipping register-only tests, "
 			    "use kvm.force_emulation_prefix=1 to enable");

base-commit: b56a1a58abfcc6abc16e782f13505f4495cf59e8
--

Re: [kvm-unit-tests PATCH v2 2/2] x86/emulator: Add some tests for ljmp instruction emulation

[Sean Christopherson]

On Tue, Feb 08, 2022, Hou Wenlong wrote:
> Per Intel's SDM on the "Instruction Set Reference", when
> loading segment descriptor for ljmp, not-present segment
> check should be after all type and privilege checks. However,
> __load_segment_descriptor() in x86's emulator does not-present
> segment check first, so it would trigger #NP instead of #GP
> if type or privilege checks fail and the segment is not present.
> 
> So add some tests for ljmp instruction, and it will test
> those tests in hardware and emulator. Enable
> kvm.force_emulation_prefix when try to test them in emulator.

Many of the same comments from patch 01 apply here.  A few more below.

> @@ -1007,6 +1034,27 @@ static void test_far_xfer(bool force_emulation, struct far_xfer_test *test)
>  	handle_exception(NP_VECTOR, 0);
>  }
>  
> +static void test_ljmp(uint64_t *mem)

test_far_jmp(), the existing code is wrong ;-)

> +{
> +	unsigned char *m = (unsigned char *)mem;
> +	volatile int res = 1;
> +
> +	*(unsigned long**)m = &&jmpf;
> +	asm volatile ("data16 mov %%cs, %0":"=m"(*(m + sizeof(unsigned long))));
> +	asm volatile ("rex64 ljmp *%0"::"m"(*m));
> +	res = 0;
> +jmpf:
> +	report(res, "ljmp");

It'd be helfup to explain what this is doing (took me a while to decipher...), e.g.

	report(res, "far jmp, via emulated MMIO");

And as a delta patch...

---
 x86/emulator.c | 55 ++++++++++++++++++++++++--------------------------
 1 file changed, 26 insertions(+), 29 deletions(-)

diff --git a/x86/emulator.c b/x86/emulator.c
index 4e7c6d1..110d10d 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -66,16 +66,17 @@ static struct far_xfer_test far_ret_test = {
 };

 static struct far_xfer_test_case far_jmp_testcases[] = {
-	{0, DS_TYPE, 0, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "ljmp desc.type!=code && desc.p=0"},
-	{0, NON_CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "jmp non-conforming && dpl!=cpl && desc.p=0"},
-	{3, NON_CONFORM_CS_TYPE, 0, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "ljmp conforming && rpl>cpl && desc.p=0"},
-	{0, CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "ljmp conforming && dpl>cpl && desc.p=0"},
-	{0, NON_CONFORM_CS_TYPE, 0, 0, false, NP_VECTOR, FIRST_SPARE_SEL, "ljmp desc.p=0"},
-	{3, CONFORM_CS_TYPE, 0, 1, true, -1, -1, "ljmp dpl<cpl"},
+	{0, DS_TYPE,		 0, 0, false, GP_VECTOR, "desc.type!=code && desc.p=0"},
+	{0, NON_CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, "non-conforming && dpl!=cpl && desc.p=0"},
+	{3, NON_CONFORM_CS_TYPE, 0, 0, false, GP_VECTOR, "conforming && rpl>cpl && desc.p=0"},
+	{0, CONFORM_CS_TYPE,	 3, 0, false, GP_VECTOR, "conforming && dpl>cpl && desc.p=0"},
+	{0, NON_CONFORM_CS_TYPE, 0, 0, false, NP_VECTOR, "desc.p=0"},
+	{3, CONFORM_CS_TYPE,	 0, 1, true, -1,	 "dpl<cpl"},
 };

 static struct far_xfer_test far_jmp_test = {
 	.insn = FAR_XFER_JMP,
+	.insn_name = "far jmp",
 	.testcases = &far_jmp_testcases[0],
 	.nr_testcases = sizeof(far_jmp_testcases) / sizeof(struct far_xfer_test_case),
 };
@@ -95,23 +96,16 @@ static unsigned long *fep_jmp_buf_ptr = &fep_jmp_buf[0];
 		     : "eax", "memory");	\
 })

-#define TEST_FAR_JMP_ASM(seg, prefix)		\
-	*(uint16_t *)(&fep_jmp_buf[1]) = seg;	\
-	asm volatile("lea 1f(%%rip), %%rax\n\t" \
-		     "movq $1f, (%[mem])\n\t"	\
-		      prefix "rex64 ljmp *(%[mem])\n\t"\
-		     "1:"			\
-		     : : [mem]"r"(fep_jmp_buf_ptr)\
-		     : "eax", "memory");
-
-static inline void test_far_jmp_asm(uint16_t seg, bool force_emulation)
-{
-	if (force_emulation) {
-		TEST_FAR_JMP_ASM(seg, KVM_FEP);
-	} else {
-		TEST_FAR_JMP_ASM(seg, "");
-	}
-}
+#define TEST_FAR_JMP_ASM(seg, prefix)			\
+({							\
+	*(uint16_t *)(&fep_jmp_buf[1]) = seg;		\
+	asm volatile("lea 1f(%%rip), %%rax\n\t"		\
+		     "movq $1f, (%[mem])\n\t"		\
+		      prefix "rex64 ljmp *(%[mem])\n\t"	\
+		     "1:"				\
+		     : : [mem]"r"(fep_jmp_buf_ptr)	\
+		     : "eax", "memory");		\
+})

 struct regs {
 	u64 rax, rbx, rcx, rdx;
@@ -991,7 +985,10 @@ static void __test_far_xfer(enum far_xfer_insn insn, uint16_t seg,
 			TEST_FAR_RET_ASM(seg, "");
 		break;
 	case FAR_XFER_JMP:
-		test_far_jmp_asm(seg, force_emulation);
+		if (force_emulation)
+			TEST_FAR_JMP_ASM(seg, KVM_FEP);
+		else
+			TEST_FAR_JMP_ASM(seg, "");
 		break;
 	default:
 		report_fail("Unexpected insn enum = %d\n", insn);
@@ -1039,7 +1036,7 @@ static void test_far_xfer(bool force_emulation, struct far_xfer_test *test)
 	handle_exception(NP_VECTOR, 0);
 }

-static void test_ljmp(uint64_t *mem)
+static void test_far_jmp(uint64_t *mem)
 {
 	unsigned char *m = (unsigned char *)mem;
 	volatile int res = 1;
@@ -1049,12 +1046,12 @@ static void test_ljmp(uint64_t *mem)
 	asm volatile ("rex64 ljmp *%0"::"m"(*m));
 	res = 0;
 jmpf:
-	report(res, "far jmp, self-modifying code");
+	report(res, "far jmp, via emulated MMIO");

 	test_far_xfer(false, &far_jmp_test);
 }

-static void test_em_ljmp(uint64_t *mem)
+static void test_em_far_jmp(uint64_t *mem)
 {
 	test_far_xfer(true, &far_jmp_test);
 }
@@ -1341,7 +1338,7 @@ int main(void)

 	test_smsw(mem);
 	test_lmsw();
-	test_ljmp(mem);
+	test_far_jmp(mem);
 	test_far_ret(mem);
 	test_stringio();
 	test_incdecnotneg(mem);
@@ -1367,7 +1364,7 @@ int main(void)
 		test_smsw_reg(mem);
 		test_nop(mem);
 		test_mov_dr(mem);
-		test_em_ljmp(mem);
+		test_em_far_jmp(mem);
 		test_em_far_ret(mem);
 	} else {
 		report_skip("skipping register-only tests, "

base-commit: 57a0e341f906f09df1ce45ef7bf080e9737eeff2
--

Re: [kvm-unit-tests PATCH v2 1/2] x86/emulator: Add some tests for lret instruction emulation

[Hou Wenlong]

Thanks for your kind review.

On Thu, Feb 10, 2022 at 06:07:11AM +0800, Sean Christopherson wrote:
> Nit, "far ret" instead of "lret" in the shortlog.
> 
> On Tue, Feb 08, 2022, Hou Wenlong wrote:
> > Per Intel's SDM on the "Instruction Set Reference", when
> > loading segment descriptor for far return, not-present segment
> > check should be after all type and privilege checks. However,
> > __load_segment_descriptor() in x86's emulator does not-present
> > segment check first, so it would trigger #NP instead of #GP
> > if type or privilege checks fail and the segment is not present.
> > 
> > And if RPL < CPL, it should trigger #GP, but the check is missing
> > in emulator.
> > 
> > So add some tests for lret instruction, and it will test
> > those tests in hardware and emulator. Enable
> > kvm.force_emulation_prefix when try to test them in emulator.
> > 
> > Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> > ---
> 
> Was this tested?  There are multiple compilation errors with gcc...
> 
> x86/emulator.c: In function ‘test_far_xfer’:
> x86/emulator.c:1034:10: error: format not a string literal and no format arguments [-Werror=format-security]
>  1034 |          far_xfer_error_code == t->error_code, t->msg);
>       |          ^~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> make: *** [<builtin>: x86/emulator.o] Error 1
>
> x86/emulator.c: Assembler messages:
> x86/emulator.c:81: Error: incorrect register `%si' used with `q' suffix
> x86/emulator.c:83: Error: incorrect register `%si' used with `q' suffix
> make: *** [<builtin>: x86/emulator.o] Error 1
> 
The compilation was OK when I tested on my machine. But my gcc is old and
customted, -Wformat-security is not enabled by default. After switching
to higher version of gcc, I get compilation errors too.

> > +static struct far_xfer_test_case far_ret_testcases[] = {
> > +	{0, DS_TYPE, 0, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret desc.type!=code && desc.p=0"},
> > +	{0, NON_CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret non-conforming && dpl!=rpl && desc.p=0"},
> > +	{0, CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret conforming && dpl>rpl && desc.p=0"},
> > +	{0, NON_CONFORM_CS_TYPE, 0, 0, false, NP_VECTOR, FIRST_SPARE_SEL, "lret desc.p=0"},
> > +	{0, NON_CONFORM_CS_TYPE, 3, 1, true, GP_VECTOR, FIRST_SPARE_SEL, "lret rpl<cpl"},
> 
> Since the framework is responsible for specifying the selector and the expected
> error code is the same for all subtests that expect an exception, just drop the
> error code.  E.g. if the framework decides to use a different selector than these
> would need to be updated too, for no real benefit.
> 
> It'd also be helpful to align everything.
> 
> s/lret/far ret.  Actually to cut down on copy+paste, and because we need formatting
> anyways (see below), how moving the instruction name into the far_xfer_test?
> 
> > +};
> > +
> > +static struct far_xfer_test far_ret_test = {
> > +	.insn = FAR_XFER_RET,
> > +	.testcases = &far_ret_testcases[0],
> > +	.nr_testcases = sizeof(far_ret_testcases) / sizeof(struct far_xfer_test_case),
> > +};
> > +
> > +#define TEST_FAR_RET_ASM(seg, prefix)		\
> > +	asm volatile("lea 1f(%%rip), %%rax\n\t" \
> > +		     "pushq %[asm_seg]\n\t"	\
> > +		     "pushq $2f\n\t"		\
> > +		      prefix "lretq\n\t"	\
> > +		     "1: addq $16, %%rsp\n\t"	\
> > +		     "2:"			\
> > +		     : : [asm_seg]"r"(seg)	\
> 
> Because @seg is a u16, gcc emites "pushq SI".  Kinda dumb, but...
> 
> @@ -72,7 +71,7 @@ static struct far_xfer_test far_ret_test = {
>                       prefix "lretq\n\t"        \
>                      "1: addq $16, %%rsp\n\t"   \
>                      "2:"                       \
> -                    : : [asm_seg]"r"(seg)      \
> +                    : : [asm_seg]"r"((u64)seg) \
>                      : "eax", "memory");
> 
> > +		     : "eax", "memory");
> > +
> > +static inline void test_far_ret_asm(uint16_t seg, bool force_emulation)
> > +{
> > +	if (force_emulation) {
> 
> No need for braces.  Ah, the macro is missing ({ ... }) to force it to be evaluated
> as a single statement.  It should be.
> 
> Side topic, a ternary operator would be nice, but I don't know how to force string
> concatentation for this case...
>
I don't know too, so I use if/else statement here.

> #define TEST_FAR_RET_ASM(seg, prefix)		\
> ({						\
> 	asm volatile("lea 1f(%%rip), %%rax\n\t" \
> 		     "pushq %[asm_seg]\n\t"	\
> 		     "pushq $2f\n\t"		\
> 		      prefix "lretq\n\t"	\
> 		     "1: addq $16, %%rsp\n\t"	\
> 		     "2:"			\
> 		     : : [asm_seg]"r"((u64)seg)	\
> 		     : "eax", "memory");	\
> })
> 
> > +		TEST_FAR_RET_ASM(seg, KVM_FEP);
> > +	} else {
> > +		TEST_FAR_RET_ASM(seg, "");
> > +	}
> > +}
> 
> This only has a single caller, just open code the macros in the case statements
> of __test_far_xfer().  (My apologies if I suggested this in the last version).
> 
> >  struct regs {
> >  	u64 rax, rbx, rcx, rdx;
> > @@ -891,6 +951,74 @@ static void test_mov_dr(uint64_t *mem)
> >  	report(rax == dr6_fixed_1, "mov_dr6");
> >  }
> >  
> > +static void far_xfer_exception_handler(struct ex_regs *regs)
> > +{
> > +	far_xfer_vector = regs->vector;
> > +	far_xfer_error_code = regs->error_code;
> > +	regs->rip = regs->rax;;
> 
> Double semi-colon.
> 
> > +}
> > +
> > +static void __test_far_xfer(enum far_xfer_insn insn, uint16_t seg,
> > +			    bool force_emulation)
> > +{
> > +	switch (insn) {
> > +	case FAR_XFER_RET:
> > +		test_far_ret_asm(seg, force_emulation);
> > +		break;
> > +	default:
> > +		report_fail("unknown instructions");
> 
> It's worth spitting out the insn, it might save someone a few seconds/minutes, e.g.
> 
> 		report_fail("Unexpected insn enum = %d\n", insn);
> 
> > +		break;
> > +	}
> > +}
> > +
> > +static void test_far_xfer(bool force_emulation, struct far_xfer_test *test)
> > +{
> > +	struct far_xfer_test_case *t;
> > +	uint16_t seg;
> > +	bool ign;
> > +	int i;
> > +
> > +	handle_exception(GP_VECTOR, far_xfer_exception_handler);
> > +	handle_exception(NP_VECTOR, far_xfer_exception_handler);
> > +
> > +	for (i = 0; i < test->nr_testcases; i++) {
> > +		t = &test->testcases[i];
> > +
> > +		seg = FIRST_SPARE_SEL | t->rpl;
> > +		gdt[seg / 8] = gdt[(t->usermode ? USER_CS64 : KERNEL_CS) / 8];
> > +		gdt[seg / 8].type = t->type;
> > +		gdt[seg / 8].dpl = t->dpl;
> > +		gdt[seg / 8].p = t->p;
> > +
> > +		far_xfer_vector = -1;
> > +		far_xfer_error_code = -1;
> > +
> > +		if (t->usermode)
> > +			run_in_user((usermode_func)__test_far_xfer, UD_VECTOR,
> > +				    test->insn, seg, force_emulation, 0, &ign);
> > +		else
> > +			__test_far_xfer(test->insn, seg, force_emulation);
> > +
> > +		report(far_xfer_vector == t->vector &&
> > +		       far_xfer_error_code == t->error_code, t->msg);
> 
> To avoid having to specify the error code, just do:
> 
> 		       (far_xfer_vector < 0 || far_xfer_error_code == FIRST_SPARE_SEL),
> 
> far_xfer_vector and t->vector need to be signed ints, but that's perfectly ok,
> vector fits in a u8.  Printing out the expecte vs. actual is also helpful for
> debug (pet peeve of mine in KUT...).  I doesn't have to be super fancy formatting,
> just enough so that the user doesn't have to modify the test to figure out what
> went wrong.
> 
> And with the insn_name + msg + target (see below) formatting:
> 
> 
> 		report(far_xfer_vector == t->vector &&
> 		       (far_xfer_vector < 0 || far_xfer_error_code == FIRST_SPARE_SEL),
> 		       "%s on %s, %s: wanted %d (%d), got %d (%d)",
> 		       test->insn_name, force_emulation ? "emuator" : "hardware", t->msg,
> 		       t->vector, t->vector < 0 ? -1 : FIRST_SPARE_SEL,
> 		       far_xfer_vector, far_xfer_error_code);
> 
> > +	}
> > +
> > +	handle_exception(GP_VECTOR, 0);
> > +	handle_exception(NP_VECTOR, 0);
> > +}
> > +
> > +static void test_lret(uint64_t *mem)
> 
> test_far_ret()
> 
> > +{
> > +	printf("test lret in hw\n");
> 
> Rather than print this out before, just spit out the "target" in the report.
> 
> > +	test_far_xfer(false, &far_ret_test);
> > +}
> 
> All of the above (I think) feeback:
> 
> ---
>  x86/emulator.c | 57 +++++++++++++++++++++++++-------------------------
>  1 file changed, 28 insertions(+), 29 deletions(-)
> 
> diff --git a/x86/emulator.c b/x86/emulator.c
> index 2a35caf..d28e36c 100644
> --- a/x86/emulator.c
> +++ b/x86/emulator.c
> @@ -21,7 +21,7 @@ static int exceptions;
>  #define KVM_FEP "ud2; .byte 'k', 'v', 'm';"
>  #define KVM_FEP_LENGTH 5
>  static int fep_available = 1;
> -static unsigned int far_xfer_vector = -1;
> +static int far_xfer_vector = -1;
>  static unsigned int far_xfer_error_code = -1;
> 
>  struct far_xfer_test_case {
> @@ -30,8 +30,7 @@ struct far_xfer_test_case {
>  	uint16_t dpl;
>  	uint16_t p;
>  	bool usermode;
> -	unsigned int vector;
> -	unsigned int error_code;
> +	int vector;
>  	const char *msg;
>  };
> 
> @@ -41,6 +40,7 @@ enum far_xfer_insn {
> 
>  struct far_xfer_test {
>  	enum far_xfer_insn insn;
> +	const char *insn_name;
>  	struct far_xfer_test_case *testcases;
>  	unsigned int nr_testcases;
>  };
> @@ -50,37 +50,31 @@ struct far_xfer_test {
>  #define DS_TYPE			0x3
> 
>  static struct far_xfer_test_case far_ret_testcases[] = {
> -	{0, DS_TYPE, 0, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret desc.type!=code && desc.p=0"},
> -	{0, NON_CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret non-conforming && dpl!=rpl && desc.p=0"},
> -	{0, CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret conforming && dpl>rpl && desc.p=0"},
> -	{0, NON_CONFORM_CS_TYPE, 0, 0, false, NP_VECTOR, FIRST_SPARE_SEL, "lret desc.p=0"},
> -	{0, NON_CONFORM_CS_TYPE, 3, 1, true, GP_VECTOR, FIRST_SPARE_SEL, "lret rpl<cpl"},
> +	{0, DS_TYPE,		 0, 0, false, GP_VECTOR, "desc.type!=code && desc.p=0"},
> +	{0, NON_CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, "non-conforming && dpl!=rpl && desc.p=0"},
> +	{0, CONFORM_CS_TYPE,     3, 0, false, GP_VECTOR, "conforming && dpl>rpl && desc.p=0"},
> +	{0, NON_CONFORM_CS_TYPE, 0, 0, false, NP_VECTOR, "desc.p=0"},
> +	{0, NON_CONFORM_CS_TYPE, 3, 1, true,  GP_VECTOR, "rpl<cpl"},
>  };
> 
>  static struct far_xfer_test far_ret_test = {
>  	.insn = FAR_XFER_RET,
> +	.insn_name = "far ret",
>  	.testcases = &far_ret_testcases[0],
>  	.nr_testcases = sizeof(far_ret_testcases) / sizeof(struct far_xfer_test_case),
>  };
> 
>  #define TEST_FAR_RET_ASM(seg, prefix)		\
> +({						\
>  	asm volatile("lea 1f(%%rip), %%rax\n\t" \
>  		     "pushq %[asm_seg]\n\t"	\
>  		     "pushq $2f\n\t"		\
>  		      prefix "lretq\n\t"	\
>  		     "1: addq $16, %%rsp\n\t"	\
>  		     "2:"			\
> -		     : : [asm_seg]"r"(seg)	\
> -		     : "eax", "memory");
> -
> -static inline void test_far_ret_asm(uint16_t seg, bool force_emulation)
> -{
> -	if (force_emulation) {
> -		TEST_FAR_RET_ASM(seg, KVM_FEP);
> -	} else {
> -		TEST_FAR_RET_ASM(seg, "");
> -	}
> -}
> +		     : : [asm_seg]"r"((u64)seg)	\
> +		     : "eax", "memory");	\
> +})
> 
>  struct regs {
>  	u64 rax, rbx, rcx, rdx;
> @@ -959,7 +953,7 @@ static void far_xfer_exception_handler(struct ex_regs *regs)
>  {
>  	far_xfer_vector = regs->vector;
>  	far_xfer_error_code = regs->error_code;
> -	regs->rip = regs->rax;;
> +	regs->rip = regs->rax;
>  }
> 
>  static void __test_far_xfer(enum far_xfer_insn insn, uint16_t seg,
> @@ -967,10 +961,13 @@ static void __test_far_xfer(enum far_xfer_insn insn, uint16_t seg,
>  {
>  	switch (insn) {
>  	case FAR_XFER_RET:
> -		test_far_ret_asm(seg, force_emulation);
> +		if (force_emulation)
> +			TEST_FAR_RET_ASM(seg, KVM_FEP);
> +		else
> +			TEST_FAR_RET_ASM(seg, "");
>  		break;
>  	default:
> -		report_fail("unknown instructions");
> +		report_fail("Unexpected insn enum = %d\n", insn);
>  		break;
>  	}
>  }
> @@ -1004,22 +1001,24 @@ static void test_far_xfer(bool force_emulation, struct far_xfer_test *test)
>  			__test_far_xfer(test->insn, seg, force_emulation);
> 
>  		report(far_xfer_vector == t->vector &&
> -		       far_xfer_error_code == t->error_code, "%s", t->msg);
> +		       (far_xfer_vector < 0 || far_xfer_error_code == FIRST_SPARE_SEL),
> +		       "%s on %s, %s: wanted %d (%d), got %d (%d)",
> +		       test->insn_name, force_emulation ? "emuator" : "hardware", t->msg,
> +		       t->vector, t->vector < 0 ? -1 : FIRST_SPARE_SEL,
> +		       far_xfer_vector, far_xfer_error_code);
>  	}
> 
>  	handle_exception(GP_VECTOR, 0);
>  	handle_exception(NP_VECTOR, 0);
>  }
> 
> -static void test_lret(uint64_t *mem)
> +static void test_far_ret(uint64_t *mem)
>  {
> -	printf("test lret in hw\n");
>  	test_far_xfer(false, &far_ret_test);
>  }
> 
> -static void test_em_lret(uint64_t *mem)
> +static void test_em_far_ret(uint64_t *mem)
>  {
> -	printf("test lret in emulator\n");
>  	test_far_xfer(true, &far_ret_test);
>  }
> 
> @@ -1297,7 +1296,7 @@ int main(void)
>  	test_smsw(mem);
>  	test_lmsw();
>  	test_ljmp(mem);
> -	test_lret(mem);
> +	test_far_ret(mem);
>  	test_stringio();
>  	test_incdecnotneg(mem);
>  	test_btc(mem);
> @@ -1322,7 +1321,7 @@ int main(void)
>  		test_smsw_reg(mem);
>  		test_nop(mem);
>  		test_mov_dr(mem);
> -		test_em_lret(mem);
> +		test_em_far_ret(mem);
>  	} else {
>  		report_skip("skipping register-only tests, "
>  			    "use kvm.force_emulation_prefix=1 to enable");
> 
> base-commit: b56a1a58abfcc6abc16e782f13505f4495cf59e8
> --