* [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns @ 2015-08-21 10:19 Denys Vlasenko 2015-08-22 8:13 ` Ingo Molnar 2015-08-22 8:54 ` Ingo Molnar 0 siblings, 2 replies; 10+ messages in thread From: Denys Vlasenko @ 2015-08-21 10:19 UTC (permalink / raw) To: Ingo Molnar Cc: Denys Vlasenko, Borislav Petkov, H. Peter Anvin, Andy Lutomirski, Kees Cook, x86, linux-kernel 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns 2015-08-21 10:19 [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns Denys Vlasenko @ 2015-08-22 8:13 ` Ingo Molnar 2015-08-22 8:54 ` Ingo Molnar 1 sibling, 0 replies; 10+ messages in thread From: Ingo Molnar @ 2015-08-22 8:13 UTC (permalink / raw) To: Denys Vlasenko Cc: Borislav Petkov, H. Peter Anvin, Andy Lutomirski, Kees Cook, x86, linux-kernel, Linus Torvalds * 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns 2015-08-21 10:19 [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns Denys Vlasenko 2015-08-22 8:13 ` Ingo Molnar @ 2015-08-22 8:54 ` Ingo Molnar 2015-08-22 18:57 ` Denys Vlasenko 1 sibling, 1 reply; 10+ messages in thread From: Ingo Molnar @ 2015-08-22 8:54 UTC (permalink / raw) To: Denys Vlasenko Cc: Borislav Petkov, H. Peter Anvin, Andy Lutomirski, Kees Cook, x86, linux-kernel * 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 */ ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns 2015-08-22 8:54 ` Ingo Molnar @ 2015-08-22 18:57 ` Denys Vlasenko 2015-08-23 6:15 ` Ingo Molnar 0 siblings, 1 reply; 10+ messages in thread From: Denys Vlasenko @ 2015-08-22 18:57 UTC (permalink / raw) To: Ingo Molnar Cc: Borislav Petkov, H. Peter Anvin, Andy Lutomirski, Kees Cook, x86, linux-kernel 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: ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns 2015-08-22 18:57 ` Denys Vlasenko @ 2015-08-23 6:15 ` Ingo Molnar 2015-08-23 15:47 ` Denys Vlasenko 0 siblings, 1 reply; 10+ messages in thread From: Ingo Molnar @ 2015-08-23 6:15 UTC (permalink / raw) To: Denys Vlasenko Cc: Borislav Petkov, H. Peter Anvin, Andy Lutomirski, Kees Cook, x86, linux-kernel * 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns 2015-08-23 6:15 ` Ingo Molnar @ 2015-08-23 15:47 ` Denys Vlasenko 2015-08-24 6:46 ` Ingo Molnar 0 siblings, 1 reply; 10+ messages in thread From: Denys Vlasenko @ 2015-08-23 15:47 UTC (permalink / raw) To: Ingo Molnar Cc: Borislav Petkov, H. Peter Anvin, Andy Lutomirski, Kees Cook, x86, linux-kernel 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__, ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns 2015-08-23 15:47 ` Denys Vlasenko @ 2015-08-24 6:46 ` Ingo Molnar 2015-08-24 6:50 ` Ingo Molnar 0 siblings, 1 reply; 10+ messages in thread From: Ingo Molnar @ 2015-08-24 6:46 UTC (permalink / raw) To: Denys Vlasenko Cc: Borislav Petkov, H. Peter Anvin, Andy Lutomirski, Kees Cook, x86, linux-kernel * 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns 2015-08-24 6:46 ` Ingo Molnar @ 2015-08-24 6:50 ` Ingo Molnar 2015-08-24 14:15 ` Denys Vlasenko 0 siblings, 1 reply; 10+ messages in thread From: Ingo Molnar @ 2015-08-24 6:50 UTC (permalink / raw) To: Denys Vlasenko Cc: Borislav Petkov, H. Peter Anvin, Andy Lutomirski, Kees Cook, x86, linux-kernel * 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns 2015-08-24 6:50 ` Ingo Molnar @ 2015-08-24 14:15 ` Denys Vlasenko 2015-08-25 8:36 ` Ingo Molnar 0 siblings, 1 reply; 10+ messages in thread From: Denys Vlasenko @ 2015-08-24 14:15 UTC (permalink / raw) To: Ingo Molnar Cc: Borislav Petkov, H. Peter Anvin, Andy Lutomirski, Kees Cook, x86, linux-kernel 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__, ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns 2015-08-24 14:15 ` Denys Vlasenko @ 2015-08-25 8:36 ` Ingo Molnar 0 siblings, 0 replies; 10+ messages in thread From: Ingo Molnar @ 2015-08-25 8:36 UTC (permalink / raw) To: Denys Vlasenko Cc: Borislav Petkov, H. Peter Anvin, Andy Lutomirski, Kees Cook, x86, linux-kernel * 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-08-25 8:36 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-08-21 10:19 [PATCH] x86/math-emu: Add support for FCMOVcc and F[U]COMI[P] insns Denys Vlasenko 2015-08-22 8:13 ` Ingo Molnar 2015-08-22 8:54 ` Ingo Molnar 2015-08-22 18:57 ` Denys Vlasenko 2015-08-23 6:15 ` Ingo Molnar 2015-08-23 15:47 ` Denys Vlasenko 2015-08-24 6:46 ` Ingo Molnar 2015-08-24 6:50 ` Ingo Molnar 2015-08-24 14:15 ` Denys Vlasenko 2015-08-25 8:36 ` Ingo Molnar
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.