[PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns

[PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns

[Denys Vlasenko]

Only compile-tested.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: Ingo Molnar <mingo@kernel.org>
CC: Borislav Petkov <bp@alien8.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Kees Cook <keescook@chromium.org>
CC: x86@kernel.org
CC: linux-kernel@vger.kernel.org
---
 arch/x86/math-emu/fpu_aux.c     |  70 ++++++++++++++++++++++
 arch/x86/math-emu/fpu_entry.c   |  14 +++--
 arch/x86/math-emu/fpu_proto.h   |  12 ++++
 arch/x86/math-emu/reg_compare.c | 128 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 218 insertions(+), 6 deletions(-)

diff --git a/arch/x86/math-emu/fpu_aux.c b/arch/x86/math-emu/fpu_aux.c
index dd76a05..6539cb2 100644
--- a/arch/x86/math-emu/fpu_aux.c
+++ b/arch/x86/math-emu/fpu_aux.c
@@ -169,6 +169,76 @@ void fxch_i(void)
 	fpu_tag_word = tag_word;
 }
 
+static void fcmovCC(void)
+{
+	/* fcmovCC st(i) */
+	int i = FPU_rm;
+	FPU_REG *st0_ptr = &st(0);
+	FPU_REG *sti_ptr = &st(i);
+	long tag_word = fpu_tag_word;
+	int regnr = top & 7;
+	int regnri = (top + i) & 7;
+	u_char sti_tag = (tag_word >> (regnri * 2)) & 3;
+
+	if (sti_tag == TAG_Empty) {
+		FPU_stack_underflow();
+		clear_C1();
+		return;
+	}
+	reg_copy(sti_ptr, st0_ptr);
+	tag_word &= ~(3 << (regnr * 2));
+	tag_word |= (sti_tag << (regnr * 2));
+	fpu_tag_word = tag_word;
+}
+
+void fcmovb(void)
+{
+	if (FPU_EFLAGS & X86_EFLAGS_CF)
+		fcmovCC();
+}
+
+void fcmove(void)
+{
+	if (FPU_EFLAGS & X86_EFLAGS_ZF)
+		fcmovCC();
+}
+
+void fcmovbe(void)
+{
+	if (FPU_EFLAGS & (X86_EFLAGS_CF|X86_EFLAGS_ZF))
+		fcmovCC();
+}
+
+void fcmovu(void)
+{
+	if (FPU_EFLAGS & X86_EFLAGS_PF)
+		fcmovCC();
+}
+
+void fcmovnb(void)
+{
+	if (!(FPU_EFLAGS & X86_EFLAGS_CF))
+		fcmovCC();
+}
+
+void fcmovne(void)
+{
+	if (!(FPU_EFLAGS & X86_EFLAGS_ZF))
+		fcmovCC();
+}
+
+void fcmovnbe(void)
+{
+	if (!(FPU_EFLAGS & (X86_EFLAGS_CF|X86_EFLAGS_ZF)))
+		fcmovCC();
+}
+
+void fcmovnu(void)
+{
+	if (!(FPU_EFLAGS & X86_EFLAGS_PF))
+		fcmovCC();
+}
+
 void ffree_(void)
 {
 	/* ffree st(i) */
diff --git a/arch/x86/math-emu/fpu_entry.c b/arch/x86/math-emu/fpu_entry.c
index f37e84a..c5dfd59 100644
--- a/arch/x86/math-emu/fpu_entry.c
+++ b/arch/x86/math-emu/fpu_entry.c
@@ -58,14 +58,16 @@
 #define _df_d0_ fstp_i		/* unofficial code (17) */
 #define _df_d8_ fstp_i		/* unofficial code (1f) */
 
+/* fcmovCC and f(u)comi(p) are enabled if CPUID(1).EDX(15) "cmov" is set */
+
 static FUNC const st_instr_table[64] = {
-	fadd__, fld_i_, __BAD__, __BAD__, fadd_i, ffree_, faddp_, _df_c0_,
-	fmul__, fxch_i, __BAD__, __BAD__, fmul_i, _dd_c8_, fmulp_, _df_c8_,
-	fcom_st, fp_nop, __BAD__, __BAD__, _dc_d0_, fst_i_, _de_d0_, _df_d0_,
-	fcompst, _d9_d8_, __BAD__, __BAD__, _dc_d8_, fstp_i, fcompp, _df_d8_,
+	fadd__,  fld_i_,  fcmovb,   fcmovnb,  fadd_i,  ffree_,  faddp_,  _df_c0_,
+	fmul__,  fxch_i,  fcmove,   fcmovne,  fmul_i,  _dd_c8_, fmulp_,  _df_c8_,
+	fcom_st, fp_nop,  fcmovbe,  fcmovnbe, _dc_d0_, fst_i_,  _de_d0_, _df_d0_,
+	fcompst, _d9_d8_, fcmovu,   fcmovnu,  _dc_d8_, fstp_i,  fcompp,  _df_d8_,
 	fsub__, FPU_etc, __BAD__, finit_, fsubri, fucom_, fsubrp, fstsw_,
-	fsubr_, fconst, fucompp, __BAD__, fsub_i, fucomp, fsubp_, __BAD__,
-	fdiv__, FPU_triga, __BAD__, __BAD__, fdivri, __BAD__, fdivrp, __BAD__,
+	fsubr_, fconst,   fucompp,  fucomi_,  fsub_i,  fucomp,  fsubp_,  fucomip,
+	fdiv__, FPU_triga, __BAD__, fcomi_,   fdivri,  __BAD__, fdivrp,  fcomip,
 	fdivr_, FPU_trigb, __BAD__, __BAD__, fdiv_i, __BAD__, fdivp_, __BAD__,
 };
 
diff --git a/arch/x86/math-emu/fpu_proto.h b/arch/x86/math-emu/fpu_proto.h
index 9779df4..caff438 100644
--- a/arch/x86/math-emu/fpu_proto.h
+++ b/arch/x86/math-emu/fpu_proto.h
@@ -46,6 +46,14 @@ extern void fstsw_(void);
 extern void fp_nop(void);
 extern void fld_i_(void);
 extern void fxch_i(void);
+extern void fcmovb(void);
+extern void fcmove(void);
+extern void fcmovbe(void);
+extern void fcmovu(void);
+extern void fcmovnb(void);
+extern void fcmovne(void);
+extern void fcmovnbe(void);
+extern void fcmovnu(void);
 extern void ffree_(void);
 extern void ffreep(void);
 extern void fst_i_(void);
@@ -108,6 +116,10 @@ extern void fcompp(void);
 extern void fucom_(void);
 extern void fucomp(void);
 extern void fucompp(void);
+extern void fcomi_(void);
+extern void fcomip(void);
+extern void fucomi_(void);
+extern void fucomip(void);
 /* reg_constant.c */
 extern void fconst(void);
 /* reg_ld_str.c */
diff --git a/arch/x86/math-emu/reg_compare.c b/arch/x86/math-emu/reg_compare.c
index ecce55f..b77360f 100644
--- a/arch/x86/math-emu/reg_compare.c
+++ b/arch/x86/math-emu/reg_compare.c
@@ -249,6 +249,54 @@ static int compare_st_st(int nr)
 	return 0;
 }
 
+static int compare_i_st_st(int nr)
+{
+	int f, c;
+	FPU_REG *st_ptr;
+
+	if (!NOT_EMPTY(0) || !NOT_EMPTY(nr)) {
+		FPU_EFLAGS |= (X86_EFLAGS_ZF | X86_EFLAGS_PF | X86_EFLAGS_CF);
+		/* Stack fault */
+		EXCEPTION(EX_StackUnder);
+		return !(control_word & CW_Invalid);
+	}
+
+	partial_status &= ~SW_C0;
+	st_ptr = &st(nr);
+	c = compare(st_ptr, FPU_gettagi(nr));
+	if (c & COMP_NaN) {
+		FPU_EFLAGS |= (X86_EFLAGS_ZF | X86_EFLAGS_PF | X86_EFLAGS_CF);
+		EXCEPTION(EX_Invalid);
+		return !(control_word & CW_Invalid);
+	}
+
+	switch (c & 7) {
+	case COMP_A_lt_B:
+		f = X86_EFLAGS_CF;
+		break;
+	case COMP_A_eq_B:
+		f = X86_EFLAGS_ZF;
+		break;
+	case COMP_A_gt_B:
+		f = 0;
+		break;
+	case COMP_No_Comp:
+		f = X86_EFLAGS_ZF | X86_EFLAGS_PF | X86_EFLAGS_CF;
+		break;
+#ifdef PARANOID
+	default:
+		EXCEPTION(EX_INTERNAL | 0x122);
+		f = 0;
+		break;
+#endif /* PARANOID */
+	}
+	FPU_EFLAGS = (FPU_EFLAGS & ~(X86_EFLAGS_ZF | X86_EFLAGS_PF | X86_EFLAGS_CF)) | f;
+	if (c & COMP_Denormal) {
+		return denormal_operand() < 0;
+	}
+	return 0;
+}
+
 static int compare_u_st_st(int nr)
 {
 	int f = 0, c;
@@ -299,6 +347,58 @@ static int compare_u_st_st(int nr)
 	return 0;
 }
 
+static int compare_ui_st_st(int nr)
+{
+	int f = 0, c;
+	FPU_REG *st_ptr;
+
+	if (!NOT_EMPTY(0) || !NOT_EMPTY(nr)) {
+		FPU_EFLAGS |= (X86_EFLAGS_ZF | X86_EFLAGS_PF | X86_EFLAGS_CF);
+		/* Stack fault */
+		EXCEPTION(EX_StackUnder);
+		return !(control_word & CW_Invalid);
+	}
+
+	partial_status &= ~SW_C0;
+	st_ptr = &st(nr);
+	c = compare(st_ptr, FPU_gettagi(nr));
+	if (c & COMP_NaN) {
+		FPU_EFLAGS |= (X86_EFLAGS_ZF | X86_EFLAGS_PF | X86_EFLAGS_CF);
+		if (c & COMP_SNaN) {	/* This is the only difference between
+					   un-ordered and ordinary comparisons */
+			EXCEPTION(EX_Invalid);
+			return !(control_word & CW_Invalid);
+		}
+		return 0;
+	}
+
+	switch (c & 7) {
+	case COMP_A_lt_B:
+		f = X86_EFLAGS_CF;
+		break;
+	case COMP_A_eq_B:
+		f = X86_EFLAGS_ZF;
+		break;
+	case COMP_A_gt_B:
+		f = 0;
+		break;
+	case COMP_No_Comp:
+		f = X86_EFLAGS_ZF | X86_EFLAGS_PF | X86_EFLAGS_CF;
+		break;
+#ifdef PARANOID
+	default:
+		EXCEPTION(EX_INTERNAL | 0x123);
+		f = 0;
+		break;
+#endif /* PARANOID */
+	}
+	FPU_EFLAGS = (FPU_EFLAGS & ~(X86_EFLAGS_ZF | X86_EFLAGS_PF | X86_EFLAGS_CF)) | f;
+	if (c & COMP_Denormal) {
+		return denormal_operand() < 0;
+	}
+	return 0;
+}
+
 /*---------------------------------------------------------------------------*/
 
 void fcom_st(void)
@@ -348,3 +448,31 @@ void fucompp(void)
 	} else
 		FPU_illegal();
 }
+
+/* P6+ compare-to-EFLAGS ops */
+
+void fcomi_(void)
+{
+	/* fcomi st(i) */
+	compare_i_st_st(FPU_rm);
+}
+
+void fcomip(void)
+{
+	/* fcomip st(i) */
+	if (!compare_i_st_st(FPU_rm))
+		FPU_pop();
+}
+
+void fucomi_(void)
+{
+	/* fucomi st(i) */
+	compare_ui_st_st(FPU_rm);
+}
+
+void fucomip(void)
+{
+	/* fucomip st(i) */
+	if (!compare_ui_st_st(FPU_rm))
+		FPU_pop();
+}
-- 
1.8.1.4

Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns

[Ingo Molnar]

* Denys Vlasenko <dvlasenk@redhat.com> wrote:

> Only compile-tested.
> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: Ingo Molnar <mingo@kernel.org>
> CC: Borislav Petkov <bp@alien8.de>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Kees Cook <keescook@chromium.org>
> CC: x86@kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
>  arch/x86/math-emu/fpu_aux.c     |  70 ++++++++++++++++++++++
>  arch/x86/math-emu/fpu_entry.c   |  14 +++--
>  arch/x86/math-emu/fpu_proto.h   |  12 ++++
>  arch/x86/math-emu/reg_compare.c | 128 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 218 insertions(+), 6 deletions(-)

Very nice!

I tried out this patch (had to fix two math-emu bugs first), but it does not seem 
to make any difference, I still get a SIGILL in FUCOMIP:

  4dd76371:       df e9                   fucomip %st(1),%st

Thanks,

	Ingo

Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns

[Ingo Molnar]

* Denys Vlasenko <dvlasenk@redhat.com> wrote:

> +/* fcmovCC and f(u)comi(p) are enabled if CPUID(1).EDX(15) "cmov" is set */
> +
>  static FUNC const st_instr_table[64] = {
> -	fadd__, fld_i_, __BAD__, __BAD__, fadd_i, ffree_, faddp_, _df_c0_,
> -	fmul__, fxch_i, __BAD__, __BAD__, fmul_i, _dd_c8_, fmulp_, _df_c8_,
> -	fcom_st, fp_nop, __BAD__, __BAD__, _dc_d0_, fst_i_, _de_d0_, _df_d0_,
> -	fcompst, _d9_d8_, __BAD__, __BAD__, _dc_d8_, fstp_i, fcompp, _df_d8_,
> +	fadd__,  fld_i_,  fcmovb,   fcmovnb,  fadd_i,  ffree_,  faddp_,  _df_c0_,
> +	fmul__,  fxch_i,  fcmove,   fcmovne,  fmul_i,  _dd_c8_, fmulp_,  _df_c8_,
> +	fcom_st, fp_nop,  fcmovbe,  fcmovnbe, _dc_d0_, fst_i_,  _de_d0_, _df_d0_,
> +	fcompst, _d9_d8_, fcmovu,   fcmovnu,  _dc_d8_, fstp_i,  fcompp,  _df_d8_,
>  	fsub__, FPU_etc, __BAD__, finit_, fsubri, fucom_, fsubrp, fstsw_,
> -	fsubr_, fconst, fucompp, __BAD__, fsub_i, fucomp, fsubp_, __BAD__,
> -	fdiv__, FPU_triga, __BAD__, __BAD__, fdivri, __BAD__, fdivrp, __BAD__,
> +	fsubr_, fconst,   fucompp,  fucomi_,  fsub_i,  fucomp,  fsubp_,  fucomip,
> +	fdiv__, FPU_triga, __BAD__, fcomi_,   fdivri,  __BAD__, fdivrp,  fcomip,
>  	fdivr_, FPU_trigb, __BAD__, __BAD__, fdiv_i, __BAD__, fdivp_, __BAD__,
>  };

So the problem is that you did not give an FPU register encoding type table entry 
for the new opcodes:

static u_char const type_table[64] = {
	_REGI_, _NONE_, _null_, _null_, _REGIi, _REGi_, _REGIp, _REGi_,
	_REGI_, _REGIn, _null_, _null_, _REGIi, _REGI_, _REGIp, _REGI_,
	_REGIc, _NONE_, _null_, _null_, _REGIc, _REG0_, _REGIc, _REG0_,
	_REGIc, _REG0_, _null_, _null_, _REGIc, _REG0_, _REGIc, _REG0_,
	_REGI_, _NONE_, _null_, _NONE_, _REGIi, _REGIc, _REGIp, _NONE_,
	_REGI_, _NONE_, _REGIc, _null_, _REGIi, _REGIc, _REGIp, _null_,
	_REGI_, _NONE_, _null_, _null_, _REGIi, _null_, _REGIp, _null_,
	_REGI_, _NONE_, _null_, _null_, _REGIi, _null_, _REGIp, _null_
};

Those _null_ entries must be filled in as well.

For FUCOMI[P] it's _REGIc I think, so I tried that - and the patch below on top of 
yours made those instructions appear to work - only to be caught in an MMX op:

0xb75eb3fb <bn_mul_add_words+59>:       movd   %ebp,%mm0                                                                                                                              

:-/

Arguably the way I tested it, user-space libraries see SSE and MMX capabilities:

flags           : vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat 
pse36 clflush mmx sse2 ht syscall nx mmxext lm 3dnowext 3dnow rep_good pni lahf_lm 
cmp_legacy 3dnowprl...

So I'll first turn those CPUID bits off, to (hopefully) not confuse user-space.

Thanks,

	Ingo

====================>

 arch/x86/math-emu/fpu_entry.c | 82 ++++++++++++++-----------------------------
 1 file changed, 27 insertions(+), 55 deletions(-)

diff --git a/arch/x86/math-emu/fpu_entry.c b/arch/x86/math-emu/fpu_entry.c
index d20c8f8420e2..4d91c0fc6bc3 100644
--- a/arch/x86/math-emu/fpu_entry.c
+++ b/arch/x86/math-emu/fpu_entry.c
@@ -40,12 +40,10 @@
 
 #define __BAD__ FPU_illegal	/* Illegal on an 80486, causes SIGILL */
 
-#ifndef NO_UNDOC_CODE		/* Un-documented FPU op-codes supported by default. */
-
-/* WARNING: These codes are not documented by Intel in their 80486 manual
+/* WARNING: These codes are not all documented by Intel in their 80486 manual
    and may not work on FPU clones or later Intel FPUs. */
 
-/* Changes to support the un-doc codes provided by Linus Torvalds. */
+/* Changes to support the un-documented instructions provided by Linus Torvalds. */
 
 #define _d9_d8_ fstp_i		/* unofficial code (19) */
 #define _dc_d0_ fcom_st		/* unofficial code (14) */
@@ -60,31 +58,24 @@
 /* fcmovCC and f(u)comi(p) are enabled if CPUID(1).EDX(15) "cmov" is set */
 
 static FUNC const st_instr_table[64] = {
-	fadd__,  fld_i_,  fcmovb,   fcmovnb,  fadd_i,  ffree_,  faddp_,  _df_c0_,
-	fmul__,  fxch_i,  fcmove,   fcmovne,  fmul_i,  _dd_c8_, fmulp_,  _df_c8_,
-	fcom_st, fp_nop,  fcmovbe,  fcmovnbe, _dc_d0_, fst_i_,  _de_d0_, _df_d0_,
-	fcompst, _d9_d8_, fcmovu,   fcmovnu,  _dc_d8_, fstp_i,  fcompp,  _df_d8_,
-	fsub__, FPU_etc, __BAD__, finit_, fsubri, fucom_, fsubrp, fstsw_,
-	fsubr_, fconst,   fucompp,  fucomi_,  fsub_i,  fucomp,  fsubp_,  fucomip,
-	fdiv__, FPU_triga, __BAD__, fcomi_,   fdivri,  __BAD__, fdivrp,  fcomip,
-	fdivr_, FPU_trigb, __BAD__, __BAD__, fdiv_i, __BAD__, fdivp_, __BAD__,
+/* 0x00 */	fadd__,		fld_i_,		fcmovb,		fcmovnb,
+/* 0x04 */	fadd_i,		ffree_,		faddp_,		_df_c0_,
+/* 0x08 */	fmul__,		fxch_i,		fcmove,		fcmovne,
+/* 0x0c */	fmul_i,		_dd_c8_,	fmulp_,		_df_c8_,
+/* 0x10 */	fcom_st,	fp_nop,		fcmovbe,	fcmovnbe,
+/* 0x14 */	_dc_d0_,	fst_i_,		_de_d0_,	_df_d0_,
+/* 0x18 */	fcompst,	_d9_d8_,	fcmovu,		fcmovnu,
+/* 0x1c */	_dc_d8_,	fstp_i,		fcompp,		_df_d8_,
+/* 0x20 */	fsub__,		FPU_etc,	__BAD__,	finit_,
+/* 0x24 */	fsubri,		fucom_,		fsubrp,		fstsw_,
+/* 0x28 */	fsubr_,		fconst,		fucompp,	fucomi_,
+/* 0x2c */	fsub_i,		fucomp,		fsubp_,		fucomip,
+/* 0x30 */	fdiv__,		FPU_triga,	__BAD__,	fcomi_,
+/* 0x34 */	fdivri,		__BAD__,	fdivrp,		fcomip,
+/* 0x38 */	fdivr_,		FPU_trigb,	__BAD__,	__BAD__,
+/* 0x3c */	fdiv_i,		__BAD__,	fdivp_,		__BAD__,
 };
 
-#else /* Support only documented FPU op-codes */
-
-static FUNC const st_instr_table[64] = {
-	fadd__, fld_i_, __BAD__, __BAD__, fadd_i, ffree_, faddp_, __BAD__,
-	fmul__, fxch_i, __BAD__, __BAD__, fmul_i, __BAD__, fmulp_, __BAD__,
-	fcom_st, fp_nop, __BAD__, __BAD__, __BAD__, fst_i_, __BAD__, __BAD__,
-	fcompst, __BAD__, __BAD__, __BAD__, __BAD__, fstp_i, fcompp, __BAD__,
-	fsub__, FPU_etc, __BAD__, finit_, fsubri, fucom_, fsubrp, fstsw_,
-	fsubr_, fconst, fucompp, __BAD__, fsub_i, fucomp, fsubp_, __BAD__,
-	fdiv__, FPU_triga, __BAD__, __BAD__, fdivri, __BAD__, fdivrp, __BAD__,
-	fdivr_, FPU_trigb, __BAD__, __BAD__, fdiv_i, __BAD__, fdivp_, __BAD__,
-};
-
-#endif /* NO_UNDOC_CODE */
-
 #define _NONE_ 0		/* Take no special action */
 #define _REG0_ 1		/* Need to check for not empty st(0) */
 #define _REGI_ 2		/* Need to check for not empty st(0) and st(rm) */
@@ -96,36 +87,17 @@ static FUNC const st_instr_table[64] = {
 #define _REGIc 0		/* Compare st(0) and st(rm) */
 #define _REGIn 0		/* Uses st(0) and st(rm), but handle checks later */
 
-#ifndef NO_UNDOC_CODE
-
-/* Un-documented FPU op-codes supported by default. (see above) */
-
-static u_char const type_table[64] = {
-	_REGI_, _NONE_, _null_, _null_, _REGIi, _REGi_, _REGIp, _REGi_,
-	_REGI_, _REGIn, _null_, _null_, _REGIi, _REGI_, _REGIp, _REGI_,
-	_REGIc, _NONE_, _null_, _null_, _REGIc, _REG0_, _REGIc, _REG0_,
-	_REGIc, _REG0_, _null_, _null_, _REGIc, _REG0_, _REGIc, _REG0_,
-	_REGI_, _NONE_, _null_, _NONE_, _REGIi, _REGIc, _REGIp, _NONE_,
-	_REGI_, _NONE_, _REGIc, _null_, _REGIi, _REGIc, _REGIp, _null_,
-	_REGI_, _NONE_, _null_, _null_, _REGIi, _null_, _REGIp, _null_,
-	_REGI_, _NONE_, _null_, _null_, _REGIi, _null_, _REGIp, _null_
-};
-
-#else /* Support only documented FPU op-codes */
-
 static u_char const type_table[64] = {
-	_REGI_, _NONE_, _null_, _null_, _REGIi, _REGi_, _REGIp, _null_,
-	_REGI_, _REGIn, _null_, _null_, _REGIi, _null_, _REGIp, _null_,
-	_REGIc, _NONE_, _null_, _null_, _null_, _REG0_, _null_, _null_,
-	_REGIc, _null_, _null_, _null_, _null_, _REG0_, _REGIc, _null_,
-	_REGI_, _NONE_, _null_, _NONE_, _REGIi, _REGIc, _REGIp, _NONE_,
-	_REGI_, _NONE_, _REGIc, _null_, _REGIi, _REGIc, _REGIp, _null_,
-	_REGI_, _NONE_, _null_, _null_, _REGIi, _null_, _REGIp, _null_,
-	_REGI_, _NONE_, _null_, _null_, _REGIi, _null_, _REGIp, _null_
+/* 0x00 */	_REGI_, _NONE_, _null_, _null_, _REGIi, _REGi_, _REGIp, _REGi_,
+/* 0x08 */	_REGI_, _REGIn, _null_, _null_, _REGIi, _REGI_, _REGIp, _REGI_,
+/* 0x10 */	_REGIc, _NONE_, _null_, _null_, _REGIc, _REG0_, _REGIc, _REG0_,
+/* 0x18 */	_REGIc, _REG0_, _null_, _null_, _REGIc, _REG0_, _REGIc, _REG0_,
+/* 0x20 */	_REGI_, _NONE_, _null_, _NONE_, _REGIi, _REGIc, _REGIp, _NONE_,
+/* 0x28 */	_REGI_, _NONE_, _REGIc, _REGIc, _REGIi, _REGIc, _REGIp, _REGIc,
+/* 0x30 */	_REGI_, _NONE_, _null_, _null_, _REGIi, _null_, _REGIp, _null_,
+/* 0x38 */	_REGI_, _NONE_, _null_, _null_, _REGIi, _null_, _REGIp, _null_
 };
 
-#endif /* NO_UNDOC_CODE */
-
 #ifdef RE_ENTRANT_CHECKING
 u_char emulating = 0;
 #endif /* RE_ENTRANT_CHECKING */

Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns

[Denys Vlasenko]

On 08/22/2015 10:54 AM, Ingo Molnar wrote:
> 
> * Denys Vlasenko <dvlasenk@redhat.com> wrote:
> 
>> +/* fcmovCC and f(u)comi(p) are enabled if CPUID(1).EDX(15) "cmov" is set */
>> +
>>  static FUNC const st_instr_table[64] = {
>> -	fadd__, fld_i_, __BAD__, __BAD__, fadd_i, ffree_, faddp_, _df_c0_,
>> -	fmul__, fxch_i, __BAD__, __BAD__, fmul_i, _dd_c8_, fmulp_, _df_c8_,
>> -	fcom_st, fp_nop, __BAD__, __BAD__, _dc_d0_, fst_i_, _de_d0_, _df_d0_,
>> -	fcompst, _d9_d8_, __BAD__, __BAD__, _dc_d8_, fstp_i, fcompp, _df_d8_,
>> +	fadd__,  fld_i_,  fcmovb,   fcmovnb,  fadd_i,  ffree_,  faddp_,  _df_c0_,
>> +	fmul__,  fxch_i,  fcmove,   fcmovne,  fmul_i,  _dd_c8_, fmulp_,  _df_c8_,
>> +	fcom_st, fp_nop,  fcmovbe,  fcmovnbe, _dc_d0_, fst_i_,  _de_d0_, _df_d0_,
>> +	fcompst, _d9_d8_, fcmovu,   fcmovnu,  _dc_d8_, fstp_i,  fcompp,  _df_d8_,
>>  	fsub__, FPU_etc, __BAD__, finit_, fsubri, fucom_, fsubrp, fstsw_,
>> -	fsubr_, fconst, fucompp, __BAD__, fsub_i, fucomp, fsubp_, __BAD__,
>> -	fdiv__, FPU_triga, __BAD__, __BAD__, fdivri, __BAD__, fdivrp, __BAD__,
>> +	fsubr_, fconst,   fucompp,  fucomi_,  fsub_i,  fucomp,  fsubp_,  fucomip,
>> +	fdiv__, FPU_triga, __BAD__, fcomi_,   fdivri,  __BAD__, fdivrp,  fcomip,
>>  	fdivr_, FPU_trigb, __BAD__, __BAD__, fdiv_i, __BAD__, fdivp_, __BAD__,
>>  };
> 
> So the problem is that you did not give an FPU register encoding type table entry 
> for the new opcodes:
> 
> static u_char const type_table[64] = {
> 	_REGI_, _NONE_, _null_, _null_, _REGIi, _REGi_, _REGIp, _REGi_,
> 	_REGI_, _REGIn, _null_, _null_, _REGIi, _REGI_, _REGIp, _REGI_,
> 	_REGIc, _NONE_, _null_, _null_, _REGIc, _REG0_, _REGIc, _REG0_,
> 	_REGIc, _REG0_, _null_, _null_, _REGIc, _REG0_, _REGIc, _REG0_,
> 	_REGI_, _NONE_, _null_, _NONE_, _REGIi, _REGIc, _REGIp, _NONE_,
> 	_REGI_, _NONE_, _REGIc, _null_, _REGIi, _REGIc, _REGIp, _null_,
> 	_REGI_, _NONE_, _null_, _null_, _REGIi, _null_, _REGIp, _null_,
> 	_REGI_, _NONE_, _null_, _null_, _REGIi, _null_, _REGIp, _null_
> };
> 
> Those _null_ entries must be filled in as well.
> 
> For FUCOMI[P] it's _REGIc I think, so I tried that - and the patch below on top of 
> yours made those instructions appear to work - only to be caught in an MMX op:
> 
> 0xb75eb3fb <bn_mul_add_words+59>:       movd   %ebp,%mm0                                                                                                                              
> 
> :-/
> 
> Arguably the way I tested it, user-space libraries see SSE and MMX capabilities:
> 
> flags           : vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat 
> pse36 clflush mmx sse2 ht syscall nx mmxext lm 3dnowext 3dnow rep_good pni lahf_lm 
> cmp_legacy 3dnowprl...
> 
> So I'll first turn those CPUID bits off, to (hopefully) not confuse user-space.
> 
> Thanks,
> 
> 	Ingo
> 
> ====================>
> 
>  arch/x86/math-emu/fpu_entry.c | 82 ++++++++++++++-----------------------------
>  1 file changed, 27 insertions(+), 55 deletions(-)
> 
> diff --git a/arch/x86/math-emu/fpu_entry.c b/arch/x86/math-emu/fpu_entry.c
> index d20c8f8420e2..4d91c0fc6bc3 100644
> --- a/arch/x86/math-emu/fpu_entry.c
> +++ b/arch/x86/math-emu/fpu_entry.c
> @@ -40,12 +40,10 @@
>  
>  #define __BAD__ FPU_illegal	/* Illegal on an 80486, causes SIGILL */
>  
> -#ifndef NO_UNDOC_CODE		/* Un-documented FPU op-codes supported by default. */
> -
> -/* WARNING: These codes are not documented by Intel in their 80486 manual
> +/* WARNING: These codes are not all documented by Intel in their 80486 manual
>     and may not work on FPU clones or later Intel FPUs. */
>  
> -/* Changes to support the un-doc codes provided by Linus Torvalds. */
> +/* Changes to support the un-documented instructions provided by Linus Torvalds. */
>  
>  #define _d9_d8_ fstp_i		/* unofficial code (19) */
>  #define _dc_d0_ fcom_st		/* unofficial code (14) */
> @@ -60,31 +58,24 @@
>  /* fcmovCC and f(u)comi(p) are enabled if CPUID(1).EDX(15) "cmov" is set */
>  
>  static FUNC const st_instr_table[64] = {
> -	fadd__,  fld_i_,  fcmovb,   fcmovnb,  fadd_i,  ffree_,  faddp_,  _df_c0_,
> -	fmul__,  fxch_i,  fcmove,   fcmovne,  fmul_i,  _dd_c8_, fmulp_,  _df_c8_,
> -	fcom_st, fp_nop,  fcmovbe,  fcmovnbe, _dc_d0_, fst_i_,  _de_d0_, _df_d0_,
> -	fcompst, _d9_d8_, fcmovu,   fcmovnu,  _dc_d8_, fstp_i,  fcompp,  _df_d8_,
> -	fsub__, FPU_etc, __BAD__, finit_, fsubri, fucom_, fsubrp, fstsw_,
> -	fsubr_, fconst,   fucompp,  fucomi_,  fsub_i,  fucomp,  fsubp_,  fucomip,
> -	fdiv__, FPU_triga, __BAD__, fcomi_,   fdivri,  __BAD__, fdivrp,  fcomip,
> -	fdivr_, FPU_trigb, __BAD__, __BAD__, fdiv_i, __BAD__, fdivp_, __BAD__,
> +/* 0x00 */	fadd__,		fld_i_,		fcmovb,		fcmovnb,
> +/* 0x04 */	fadd_i,		ffree_,		faddp_,		_df_c0_,
> +/* 0x08 */	fmul__,		fxch_i,		fcmove,		fcmovne,
> +/* 0x0c */	fmul_i,		_dd_c8_,	fmulp_,		_df_c8_,
> +/* 0x10 */	fcom_st,	fp_nop,		fcmovbe,	fcmovnbe,
> +/* 0x14 */	_dc_d0_,	fst_i_,		_de_d0_,	_df_d0_,
> +/* 0x18 */	fcompst,	_d9_d8_,	fcmovu,		fcmovnu,
> +/* 0x1c */	_dc_d8_,	fstp_i,		fcompp,		_df_d8_,
> +/* 0x20 */	fsub__,		FPU_etc,	__BAD__,	finit_,
> +/* 0x24 */	fsubri,		fucom_,		fsubrp,		fstsw_,
> +/* 0x28 */	fsubr_,		fconst,		fucompp,	fucomi_,
> +/* 0x2c */	fsub_i,		fucomp,		fsubp_,		fucomip,
> +/* 0x30 */	fdiv__,		FPU_triga,	__BAD__,	fcomi_,
> +/* 0x34 */	fdivri,		__BAD__,	fdivrp,		fcomip,
> +/* 0x38 */	fdivr_,		FPU_trigb,	__BAD__,	__BAD__,
> +/* 0x3c */	fdiv_i,		__BAD__,	fdivp_,		__BAD__,

The numeric comments added at the left don't look correct.
In this table, each _column_ corresponds to one 0xd? opcode.
Each row corresponds to a group of mod-reg-rm bytes
with only "rm" field chnaging. (These insns act on registers,
not memory, and "rm" value encodes register number, st(i).)

Something like this:

/*Opcode: d8      d9        da      db       dc      dd      de      df */
/*c0..7*/ fadd__, fld_i_,   fcmovb, fcmovnb, fadd_i, ffree_, faddp_, _df_c0_,
/*c8..f*/ fmul__, fxch_i,   fcmove, fcmovne, fmul_i, _dd_c8_,fmulp_, _df_c8_,
/*d0..7*/ fcom_st,fp_nop,   fcmovbe,fcmovnbe,_dc_d0_,fst_i_, _de_d0_,_df_d0_,
/*d8..f*/ fcompst,_d9_d8_,  fcmovu, fcmovnu, _dc_d8_,fstp_i, fcompp, _df_d8_,
/*e0..7*/ fsub__, FPU_etc,  __BAD__,finit_,  fsubri, fucom_, fsubrp, fstsw_,
/*e8..f*/ fsubr_, fconst,   fucompp,fucomi_, fsub_i, fucomp, fsubp_, fucomip,
/*f0..7*/ fdiv__, FPU_triga,__BAD__,fcomi_,  fdivri, __BAD__,fdivrp, fcomip,
/*f8..f*/ fdivr_, FPU_trigb,__BAD__,__BAD__, fdiv_i, __BAD__,fdivp_, __BAD__,


They should be:

Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns

[Ingo Molnar]

* Denys Vlasenko <dvlasenk@redhat.com> wrote:

> >  static FUNC const st_instr_table[64] = {
> > -	fadd__,  fld_i_,  fcmovb,   fcmovnb,  fadd_i,  ffree_,  faddp_,  _df_c0_,
> > -	fmul__,  fxch_i,  fcmove,   fcmovne,  fmul_i,  _dd_c8_, fmulp_,  _df_c8_,
> > -	fcom_st, fp_nop,  fcmovbe,  fcmovnbe, _dc_d0_, fst_i_,  _de_d0_, _df_d0_,
> > -	fcompst, _d9_d8_, fcmovu,   fcmovnu,  _dc_d8_, fstp_i,  fcompp,  _df_d8_,
> > -	fsub__, FPU_etc, __BAD__, finit_, fsubri, fucom_, fsubrp, fstsw_,
> > -	fsubr_, fconst,   fucompp,  fucomi_,  fsub_i,  fucomp,  fsubp_,  fucomip,
> > -	fdiv__, FPU_triga, __BAD__, fcomi_,   fdivri,  __BAD__, fdivrp,  fcomip,
> > -	fdivr_, FPU_trigb, __BAD__, __BAD__, fdiv_i, __BAD__, fdivp_, __BAD__,
> > +/* 0x00 */	fadd__,		fld_i_,		fcmovb,		fcmovnb,
> > +/* 0x04 */	fadd_i,		ffree_,		faddp_,		_df_c0_,
> > +/* 0x08 */	fmul__,		fxch_i,		fcmove,		fcmovne,
> > +/* 0x0c */	fmul_i,		_dd_c8_,	fmulp_,		_df_c8_,
> > +/* 0x10 */	fcom_st,	fp_nop,		fcmovbe,	fcmovnbe,
> > +/* 0x14 */	_dc_d0_,	fst_i_,		_de_d0_,	_df_d0_,
> > +/* 0x18 */	fcompst,	_d9_d8_,	fcmovu,		fcmovnu,
> > +/* 0x1c */	_dc_d8_,	fstp_i,		fcompp,		_df_d8_,
> > +/* 0x20 */	fsub__,		FPU_etc,	__BAD__,	finit_,
> > +/* 0x24 */	fsubri,		fucom_,		fsubrp,		fstsw_,
> > +/* 0x28 */	fsubr_,		fconst,		fucompp,	fucomi_,
> > +/* 0x2c */	fsub_i,		fucomp,		fsubp_,		fucomip,
> > +/* 0x30 */	fdiv__,		FPU_triga,	__BAD__,	fcomi_,
> > +/* 0x34 */	fdivri,		__BAD__,	fdivrp,		fcomip,
> > +/* 0x38 */	fdivr_,		FPU_trigb,	__BAD__,	__BAD__,
> > +/* 0x3c */	fdiv_i,		__BAD__,	fdivp_,		__BAD__,
> 
> The numeric comments added at the left don't look correct.

Yeah, so they are correct for the purpose I used it: I simply wanted to index the 
table by its natural index: 'instr_idx', which is derived from the opcode.

But you are right that indexing it by the opcode is more useful for future 
extensions:

> In this table, each _column_ corresponds to one 0xd? opcode.
> Each row corresponds to a group of mod-reg-rm bytes
> with only "rm" field chnaging. (These insns act on registers,
> not memory, and "rm" value encodes register number, st(i).)
> 
> Something like this:
> 
> /*Opcode: d8      d9        da      db       dc      dd      de      df */
> /*c0..7*/ fadd__, fld_i_,   fcmovb, fcmovnb, fadd_i, ffree_, faddp_, _df_c0_,
> /*c8..f*/ fmul__, fxch_i,   fcmove, fcmovne, fmul_i, _dd_c8_,fmulp_, _df_c8_,
> /*d0..7*/ fcom_st,fp_nop,   fcmovbe,fcmovnbe,_dc_d0_,fst_i_, _de_d0_,_df_d0_,
> /*d8..f*/ fcompst,_d9_d8_,  fcmovu, fcmovnu, _dc_d8_,fstp_i, fcompp, _df_d8_,
> /*e0..7*/ fsub__, FPU_etc,  __BAD__,finit_,  fsubri, fucom_, fsubrp, fstsw_,
> /*e8..f*/ fsubr_, fconst,   fucompp,fucomi_, fsub_i, fucomp, fsubp_, fucomip,
> /*f0..7*/ fdiv__, FPU_triga,__BAD__,fcomi_,  fdivri, __BAD__,fdivrp, fcomip,
> /*f8..f*/ fdivr_, FPU_trigb,__BAD__,__BAD__, fdiv_i, __BAD__,fdivp_, __BAD__,

(Assuming you also do the table alignment improvements I did, the current format 
is pretty hard to navigate.)

> 
> 
> They should be:

Hm, your mail seems to have been cut off at this point.

Thanks,

	Ingo

Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns

[Denys Vlasenko]

On 08/23/2015 08:15 AM, Ingo Molnar wrote:
> 
> * Denys Vlasenko <dvlasenk@redhat.com> wrote:
> 
>>>  static FUNC const st_instr_table[64] = {
>>> -	fadd__,  fld_i_,  fcmovb,   fcmovnb,  fadd_i,  ffree_,  faddp_,  _df_c0_,
>>> -	fmul__,  fxch_i,  fcmove,   fcmovne,  fmul_i,  _dd_c8_, fmulp_,  _df_c8_,
>>> -	fcom_st, fp_nop,  fcmovbe,  fcmovnbe, _dc_d0_, fst_i_,  _de_d0_, _df_d0_,
>>> -	fcompst, _d9_d8_, fcmovu,   fcmovnu,  _dc_d8_, fstp_i,  fcompp,  _df_d8_,
>>> -	fsub__, FPU_etc, __BAD__, finit_, fsubri, fucom_, fsubrp, fstsw_,
>>> -	fsubr_, fconst,   fucompp,  fucomi_,  fsub_i,  fucomp,  fsubp_,  fucomip,
>>> -	fdiv__, FPU_triga, __BAD__, fcomi_,   fdivri,  __BAD__, fdivrp,  fcomip,
>>> -	fdivr_, FPU_trigb, __BAD__, __BAD__, fdiv_i, __BAD__, fdivp_, __BAD__,
>>> +/* 0x00 */	fadd__,		fld_i_,		fcmovb,		fcmovnb,
>>> +/* 0x04 */	fadd_i,		ffree_,		faddp_,		_df_c0_,
>>> +/* 0x08 */	fmul__,		fxch_i,		fcmove,		fcmovne,
>>> +/* 0x0c */	fmul_i,		_dd_c8_,	fmulp_,		_df_c8_,
>>> +/* 0x10 */	fcom_st,	fp_nop,		fcmovbe,	fcmovnbe,
>>> +/* 0x14 */	_dc_d0_,	fst_i_,		_de_d0_,	_df_d0_,
>>> +/* 0x18 */	fcompst,	_d9_d8_,	fcmovu,		fcmovnu,
>>> +/* 0x1c */	_dc_d8_,	fstp_i,		fcompp,		_df_d8_,
>>> +/* 0x20 */	fsub__,		FPU_etc,	__BAD__,	finit_,
>>> +/* 0x24 */	fsubri,		fucom_,		fsubrp,		fstsw_,
>>> +/* 0x28 */	fsubr_,		fconst,		fucompp,	fucomi_,
>>> +/* 0x2c */	fsub_i,		fucomp,		fsubp_,		fucomip,
>>> +/* 0x30 */	fdiv__,		FPU_triga,	__BAD__,	fcomi_,
>>> +/* 0x34 */	fdivri,		__BAD__,	fdivrp,		fcomip,
>>> +/* 0x38 */	fdivr_,		FPU_trigb,	__BAD__,	__BAD__,
>>> +/* 0x3c */	fdiv_i,		__BAD__,	fdivp_,		__BAD__,
>>
>> The numeric comments added at the left don't look correct.
> 
> Yeah, so they are correct for the purpose I used it: I simply wanted to index the 
> table by its natural index: 'instr_idx', which is derived from the opcode.
> 
> But you are right that indexing it by the opcode is more useful for future 
> extensions:
> 
>> In this table, each _column_ corresponds to one 0xd? opcode.
>> Each row corresponds to a group of mod-reg-rm bytes
>> with only "rm" field chnaging. (These insns act on registers,
>> not memory, and "rm" value encodes register number, st(i).)
>>
>> Something like this:
>>
>> /*Opcode: d8      d9        da      db       dc      dd      de      df */
>> /*c0..7*/ fadd__, fld_i_,   fcmovb, fcmovnb, fadd_i, ffree_, faddp_, _df_c0_,
>> /*c8..f*/ fmul__, fxch_i,   fcmove, fcmovne, fmul_i, _dd_c8_,fmulp_, _df_c8_,
>> /*d0..7*/ fcom_st,fp_nop,   fcmovbe,fcmovnbe,_dc_d0_,fst_i_, _de_d0_,_df_d0_,
>> /*d8..f*/ fcompst,_d9_d8_,  fcmovu, fcmovnu, _dc_d8_,fstp_i, fcompp, _df_d8_,
>> /*e0..7*/ fsub__, FPU_etc,  __BAD__,finit_,  fsubri, fucom_, fsubrp, fstsw_,
>> /*e8..f*/ fsubr_, fconst,   fucompp,fucomi_, fsub_i, fucomp, fsubp_, fucomip,
>> /*f0..7*/ fdiv__, FPU_triga,__BAD__,fcomi_,  fdivri, __BAD__,fdivrp, fcomip,
>> /*f8..f*/ fdivr_, FPU_trigb,__BAD__,__BAD__, fdiv_i, __BAD__,fdivp_, __BAD__,
> 
> (Assuming you also do the table alignment improvements I did, the current format 
> is pretty hard to navigate.)
> 
>>
>>
>> They should be:
> 
> Hm, your mail seems to have been cut off at this point.

In fact, my mail was too long - I forgot to delete this last line,
"They should be"    :)

I propose the table to be commented like shown below:

/*Opcode: d8      d9        da      db       dc      dd      de      df */
/*c0..f*/ fadd__, fld_i_,   fcmovb, fcmovnb, fadd_i, ffree_, faddp_, _df_c0_,
/*c8..f*/ fmul__, fxch_i,   fcmove, fcmovne, fmul_i, _dd_c8_,fmulp_, _df_c8_,
/*d0..7*/ fcom_st,fp_nop,   fcmovbe,fcmovnbe,_dc_d0_,fst_i_, _de_d0_,_df_d0_,
/*d8..f*/ fcompst,_d9_d8_,  fcmovu, fcmovnu, _dc_d8_,fstp_i, fcompp, _df_d8_,
/*e0..7*/ fsub__, FPU_etc,  __BAD__,finit_,  fsubri, fucom_, fsubrp, fstsw_,
/*e8..f*/ fsubr_, fconst,   fucompp,fucomi_, fsub_i, fucomp, fsubp_, fucomip,
/*f0..7*/ fdiv__, FPU_triga,__BAD__,fcomi_,  fdivri, __BAD__,fdivrp, fcomip,
/*f8..f*/ fdivr_, FPU_trigb,__BAD__,__BAD__, fdiv_i, __BAD__,fdivp_, __BAD__,

Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns

[Ingo Molnar]

* Denys Vlasenko <dvlasenk@redhat.com> wrote:

> I propose the table to be commented like shown below:
> 
> /*Opcode: d8      d9        da      db       dc      dd      de      df */
> /*c0..f*/ fadd__, fld_i_,   fcmovb, fcmovnb, fadd_i, ffree_, faddp_, _df_c0_,
> /*c8..f*/ fmul__, fxch_i,   fcmove, fcmovne, fmul_i, _dd_c8_,fmulp_, _df_c8_,
> /*d0..7*/ fcom_st,fp_nop,   fcmovbe,fcmovnbe,_dc_d0_,fst_i_, _de_d0_,_df_d0_,
> /*d8..f*/ fcompst,_d9_d8_,  fcmovu, fcmovnu, _dc_d8_,fstp_i, fcompp, _df_d8_,
> /*e0..7*/ fsub__, FPU_etc,  __BAD__,finit_,  fsubri, fucom_, fsubrp, fstsw_,
> /*e8..f*/ fsubr_, fconst,   fucompp,fucomi_, fsub_i, fucomp, fsubp_, fucomip,
> /*f0..7*/ fdiv__, FPU_triga,__BAD__,fcomi_,  fdivri, __BAD__,fdivrp, fcomip,
> /*f8..f*/ fdivr_, FPU_trigb,__BAD__,__BAD__, fdiv_i, __BAD__,fdivp_, __BAD__,

I agree with that, but please fix the vertical alignment (like my patch did), as 
the table is pretty hard to navigate in this form.

Also, the first patch in the series should remove the 'undocumented' #ifdef. That 
define was fully justified ~20 years ago but let's not complicate new code with 
it.

Thanks,

	Ingo

Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Denys Vlasenko <dvlasenk@redhat.com> wrote:
> 
> > I propose the table to be commented like shown below:
> > 
> > /*Opcode: d8      d9        da      db       dc      dd      de      df */
> > /*c0..f*/ fadd__, fld_i_,   fcmovb, fcmovnb, fadd_i, ffree_, faddp_, _df_c0_,
> > /*c8..f*/ fmul__, fxch_i,   fcmove, fcmovne, fmul_i, _dd_c8_,fmulp_, _df_c8_,
> > /*d0..7*/ fcom_st,fp_nop,   fcmovbe,fcmovnbe,_dc_d0_,fst_i_, _de_d0_,_df_d0_,
> > /*d8..f*/ fcompst,_d9_d8_,  fcmovu, fcmovnu, _dc_d8_,fstp_i, fcompp, _df_d8_,
> > /*e0..7*/ fsub__, FPU_etc,  __BAD__,finit_,  fsubri, fucom_, fsubrp, fstsw_,
> > /*e8..f*/ fsubr_, fconst,   fucompp,fucomi_, fsub_i, fucomp, fsubp_, fucomip,
> > /*f0..7*/ fdiv__, FPU_triga,__BAD__,fcomi_,  fdivri, __BAD__,fdivrp, fcomip,
> > /*f8..f*/ fdivr_, FPU_trigb,__BAD__,__BAD__, fdiv_i, __BAD__,fdivp_, __BAD__,
> 
> I agree with that, but please fix the vertical alignment (like my patch did), as 
> the table is pretty hard to navigate in this form.

and by that I mean to split each line into two, to have groups of 4 instructions 
and enough tabs between them.

> Also, the first patch in the series should remove the 'undocumented' #ifdef. 
> That define was fully justified ~20 years ago but let's not complicate new code 
> with it.

The formerly undocumented opcodes could also grow real names.

All in nicely separate patches.

Thanks,

	Ingo

Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns

[Denys Vlasenko]

On 08/24/2015 08:50 AM, Ingo Molnar wrote:
> 
> * Ingo Molnar <mingo@kernel.org> wrote:
> 
>>
>> * Denys Vlasenko <dvlasenk@redhat.com> wrote:
>>
>>> I propose the table to be commented like shown below:
>>>
>>> /*Opcode: d8      d9        da      db       dc      dd      de      df */
>>> /*c0..f*/ fadd__, fld_i_,   fcmovb, fcmovnb, fadd_i, ffree_, faddp_, _df_c0_,
>>> /*c8..f*/ fmul__, fxch_i,   fcmove, fcmovne, fmul_i, _dd_c8_,fmulp_, _df_c8_,
>>> /*d0..7*/ fcom_st,fp_nop,   fcmovbe,fcmovnbe,_dc_d0_,fst_i_, _de_d0_,_df_d0_,
>>> /*d8..f*/ fcompst,_d9_d8_,  fcmovu, fcmovnu, _dc_d8_,fstp_i, fcompp, _df_d8_,
>>> /*e0..7*/ fsub__, FPU_etc,  __BAD__,finit_,  fsubri, fucom_, fsubrp, fstsw_,
>>> /*e8..f*/ fsubr_, fconst,   fucompp,fucomi_, fsub_i, fucomp, fsubp_, fucomip,
>>> /*f0..7*/ fdiv__, FPU_triga,__BAD__,fcomi_,  fdivri, __BAD__,fdivrp, fcomip,
>>> /*f8..f*/ fdivr_, FPU_trigb,__BAD__,__BAD__, fdiv_i, __BAD__,fdivp_, __BAD__,
>>
>> I agree with that, but please fix the vertical alignment (like my patch did), as 
>> the table is pretty hard to navigate in this form.
> 
> and by that I mean to split each line into two, to have groups of 4 instructions 
> and enough tabs between them.

How would you want it? Like this?

/*Opcode: d8       d9         da       db */
/*        dc       dd         de       df */
/*c0..f*/ fadd__,  fld_i_,    fcmovb,  fcmovnb,
/*c0..f*/ fadd_i,  ffree_,    faddp_,  _df_c0_,
/*c8..f*/ fmul__,  fxch_i,    fcmove,  fcmovne,
/*c8..f*/ fmul_i,  _dd_c8_,   fmulp_,  _df_c8_,
/*d0..7*/ fcom_st, fp_nop,    fcmovbe, fcmovnbe,
/*d0..7*/ _dc_d0_, fst_i_,    _de_d0_, _df_d0_,
/*d8..f*/ fcompst, _d9_d8_,   fcmovu,  fcmovnu,
/*d8..f*/ _dc_d8_, fstp_i,    fcompp,  _df_d8_,
/*e0..7*/ fsub__,  FPU_etc,   __BAD__, finit_,
/*e0..7*/ fsubri,  fucom_,    fsubrp,  fstsw_,
/*e8..f*/ fsubr_,  fconst,    fucompp, fucomi_,
/*e8..f*/ fsub_i,  fucomp,    fsubp_,  fucomip,
/*f0..7*/ fdiv__,  FPU_triga, __BAD__, fcomi_,
/*f0..7*/ fdivri,  __BAD__,   fdivrp,  fcomip,
/*f8..f*/ fdivr_,  FPU_trigb, __BAD__, __BAD__,
/*f8..f*/ fdiv_i,  __BAD__,   fdivp_,  __BAD__,

Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns

[Ingo Molnar]

* Denys Vlasenko <dvlasenk@redhat.com> wrote:

> On 08/24/2015 08:50 AM, Ingo Molnar wrote:
> > 
> > * Ingo Molnar <mingo@kernel.org> wrote:
> > 
> >>
> >> * Denys Vlasenko <dvlasenk@redhat.com> wrote:
> >>
> >>> I propose the table to be commented like shown below:
> >>>
> >>> /*Opcode: d8      d9        da      db       dc      dd      de      df */
> >>> /*c0..f*/ fadd__, fld_i_,   fcmovb, fcmovnb, fadd_i, ffree_, faddp_, _df_c0_,
> >>> /*c8..f*/ fmul__, fxch_i,   fcmove, fcmovne, fmul_i, _dd_c8_,fmulp_, _df_c8_,
> >>> /*d0..7*/ fcom_st,fp_nop,   fcmovbe,fcmovnbe,_dc_d0_,fst_i_, _de_d0_,_df_d0_,
> >>> /*d8..f*/ fcompst,_d9_d8_,  fcmovu, fcmovnu, _dc_d8_,fstp_i, fcompp, _df_d8_,
> >>> /*e0..7*/ fsub__, FPU_etc,  __BAD__,finit_,  fsubri, fucom_, fsubrp, fstsw_,
> >>> /*e8..f*/ fsubr_, fconst,   fucompp,fucomi_, fsub_i, fucomp, fsubp_, fucomip,
> >>> /*f0..7*/ fdiv__, FPU_triga,__BAD__,fcomi_,  fdivri, __BAD__,fdivrp, fcomip,
> >>> /*f8..f*/ fdivr_, FPU_trigb,__BAD__,__BAD__, fdiv_i, __BAD__,fdivp_, __BAD__,
> >>
> >> I agree with that, but please fix the vertical alignment (like my patch did), as 
> >> the table is pretty hard to navigate in this form.
> > 
> > and by that I mean to split each line into two, to have groups of 4 instructions 
> > and enough tabs between them.
> 
> How would you want it? Like this?
> 
> /*Opcode: d8       d9         da       db */
> /*        dc       dd         de       df */
> /*c0..f*/ fadd__,  fld_i_,    fcmovb,  fcmovnb,
> /*c0..f*/ fadd_i,  ffree_,    faddp_,  _df_c0_,
> /*c8..f*/ fmul__,  fxch_i,    fcmove,  fcmovne,
> /*c8..f*/ fmul_i,  _dd_c8_,   fmulp_,  _df_c8_,
> /*d0..7*/ fcom_st, fp_nop,    fcmovbe, fcmovnbe,
> /*d0..7*/ _dc_d0_, fst_i_,    _de_d0_, _df_d0_,
> /*d8..f*/ fcompst, _d9_d8_,   fcmovu,  fcmovnu,
> /*d8..f*/ _dc_d8_, fstp_i,    fcompp,  _df_d8_,
> /*e0..7*/ fsub__,  FPU_etc,   __BAD__, finit_,
> /*e0..7*/ fsubri,  fucom_,    fsubrp,  fstsw_,
> /*e8..f*/ fsubr_,  fconst,    fucompp, fucomi_,
> /*e8..f*/ fsub_i,  fucomp,    fsubp_,  fucomip,
> /*f0..7*/ fdiv__,  FPU_triga, __BAD__, fcomi_,
> /*f0..7*/ fdivri,  __BAD__,   fdivrp,  fcomip,
> /*f8..f*/ fdivr_,  FPU_trigb, __BAD__, __BAD__,
> /*f8..f*/ fdiv_i,  __BAD__,   fdivp_,  __BAD__,

Yeah, although we could afford a few more tabs and spaces to make it less 
claustrophobic, right?

Something like this:

 /* c0..f */		fadd__,		fld_i_,		fcmovb,		fcmovnb,
 /* c0..f */		fadd_i,		ffree_,		faddp_,		_df_c0_,
 /* c8..f */		fmul__,		fxch_i,		fcmove,		fcmovne,
 /* c8..f */		fmul_i,		_dd_c8_,	fmulp_,		_df_c8_,
 /* d0..7 */		fcom_st,	fp_nop,		fcmovbe,	fcmovnbe,
 /* d0..7 */		_dc_d0_,	fst_i_,		_de_d0_,	_df_d0_,
 /* d8..f */		fcompst,	_d9_d8_		fcmovu,		fcmovnu,
 /* d8..f */		_dc_d8_,	fstp_i,		fcompp,		_df_d8_,
 /* e0..7 */		fsub__,		FPU_etc,	__BAD__,	finit_,
 /* e0..7 */		fsubri,		fucom_,		fsubrp,		fstsw_,
 /* e8..f */		fsubr_,		fconst,		fucompp,	fucomi_,
 /* e8..f */		fsub_i,		fucomp,		fsubp_,		fucomip,
 /* f0..7 */		fdiv__,		FPU_triga,	__BAD__,	fcomi_,
 /* f0..7 */		fdivri,		__BAD__,	fdivrp,		fcomip,
 /* f8..f */		fdivr_,		FPU_trigb,	__BAD__,	__BAD__,
 /* f8..f */		fdiv_i,		__BAD__,	fdivp_,		__BAD__,

looks pretty good to me.

Thanks,

	Ingo