Re: AMD DC graphics display code enables -mhard-float, -msse, -msse2 without any visible FPU state protection

From: Masami Hiramatsu
Date: Sat Apr 04 2020 - 23:40:14 EST

On Sat, 4 Apr 2020 16:36:20 +0200
Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> On Sat, Apr 04, 2020 at 12:08:08PM +0900, Masami Hiramatsu wrote:
> > From c609be0b6403245612503fca1087628655bab96c Mon Sep 17 00:00:00 2001
> > From: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> > Date: Fri, 3 Apr 2020 16:58:22 +0900
> > Subject: [PATCH] x86: insn: Add insn_is_fpu()
> >
> > Add insn_is_fpu(insn) which tells that the insn is
> > whether touch the MMX/XMM/YMM register or the instruction
> > of FP coprocessor.
> >
> > Signed-off-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
>
> With that I get a lot of warnings:
>
> FPU instruction outside of kernel_fpu_{begin,end}()
>
> two random examples (x86-64-allmodconfig build):
>
> arch/x86/xen/enlighten.o: warning: objtool: xen_vcpu_restore()+0x341: FPU instruction outside of kernel_fpu_{begin,end}()
>
> $ ./objdump-func.sh defconfig-build/arch/x86/xen/enlighten.o xen_vcpu_restore | grep 341
> 0341 841: 0f 92 c3 setb %bl
>
> arch/x86/events/core.o: warning: objtool: x86_pmu_stop()+0x6d: FPU instruction outside of kernel_fpu_{begin,end}()
>
> $ ./objdump-func.sh defconfig-build/arch/x86/events/core.o x86_pmu_stop | grep 6d
> 006d 23ad: 41 0f 92 c6 setb %r14b
>
> Which seems to suggest something goes wobbly with SETB, but I'm not
> seeing what in a hurry.

Yes, I also got same issue, please try the new one.

Thank you!

>
>
> ---
> --- a/arch/x86/include/asm/fpu/api.h
> +++ b/arch/x86/include/asm/fpu/api.h
> @@ -12,6 +12,13 @@
> #define _ASM_X86_FPU_API_H
> #include <linux/bottom_half.h>
>
> +#define annotate_fpu() ({ \
> + asm volatile("%c0:\n\t" \
> + ".pushsection .discard.fpu_safe\n\t" \
> + ".long %c0b - .\n\t" \
> + ".popsection\n\t" : : "i" (__COUNTER__)); \
> +})
> +
> /*
> * Use kernel_fpu_begin/end() if you intend to use FPU in kernel context. It
> * disables preemption so be careful if you intend to use it for long periods
> --- a/arch/x86/include/asm/fpu/internal.h
> +++ b/arch/x86/include/asm/fpu/internal.h
> @@ -437,6 +437,7 @@ static inline int copy_fpregs_to_fpstate
> * Legacy FPU register saving, FNSAVE always clears FPU registers,
> * so we have to mark them inactive:
> */
> + annotate_fpu();
> asm volatile("fnsave %[fp]; fwait" : [fp] "=m" (fpu->state.fsave));
>
> return 0;
> @@ -462,6 +463,7 @@ static inline void copy_kernel_to_fpregs
> * "m" is a random variable that should be in L1.
> */
> if (unlikely(static_cpu_has_bug(X86_BUG_FXSAVE_LEAK))) {
> + annotate_fpu();
> asm volatile(
> "fnclex\n\t"
> "emms\n\t"
> --- a/arch/x86/kernel/fpu/init.c
> +++ b/arch/x86/kernel/fpu/init.c
> @@ -38,7 +38,10 @@ static void fpu__init_cpu_generic(void)
> fpstate_init_soft(&current->thread.fpu.state.soft);
> else
> #endif
> + {
> + annotate_fpu();
> asm volatile ("fninit");
> + }
> }
>
> /*
> @@ -61,6 +64,7 @@ static bool fpu__probe_without_cpuid(voi
> cr0 &= ~(X86_CR0_TS | X86_CR0_EM);
> write_cr0(cr0);
>
> + annotate_fpu();
> asm volatile("fninit ; fnstsw %0 ; fnstcw %1" : "+m" (fsw), "+m" (fcw));
>
> pr_info("x86/fpu: Probing for FPU: FSW=0x%04hx FCW=0x%04hx\n", fsw, fcw);
> --- a/tools/objtool/arch.h
> +++ b/tools/objtool/arch.h
> @@ -27,6 +27,7 @@ enum insn_type {
> INSN_CLAC,
> INSN_STD,
> INSN_CLD,
> + INSN_FPU,
> INSN_OTHER,
> };
>
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -92,6 +92,11 @@ int arch_decode_instruction(struct elf *
> *len = insn.length;
> *type = INSN_OTHER;
>
> + if (insn_is_fpu(&insn)) {
> + *type = INSN_FPU;
> + return 0;
> + }
> +
> if (insn.vex_prefix.nbytes)
> return 0;
>
> @@ -357,48 +362,54 @@ int arch_decode_instruction(struct elf *
>
> case 0x0f:
>
> - if (op2 == 0x01) {
> -
> + switch (op2) {
> + case 0x01:
> if (modrm == 0xca)
> *type = INSN_CLAC;
> else if (modrm == 0xcb)
> *type = INSN_STAC;
> + break;
>
> - } else if (op2 >= 0x80 && op2 <= 0x8f) {
> -
> + case 0x80 ... 0x8f: /* Jcc */
> *type = INSN_JUMP_CONDITIONAL;
> + break;
>
> - } else if (op2 == 0x05 || op2 == 0x07 || op2 == 0x34 ||
> - op2 == 0x35) {
> -
> - /* sysenter, sysret */
> + case 0x05: /* syscall */
> + case 0x07: /* sysret */
> + case 0x34: /* sysenter */
> + case 0x35: /* sysexit */
> *type = INSN_CONTEXT_SWITCH;
> + break;
>
> - } else if (op2 == 0x0b || op2 == 0xb9) {
> -
> - /* ud2 */
> + case 0xff: /* ud0 */
> + case 0xb9: /* ud1 */
> + case 0x0b: /* ud2 */
> *type = INSN_BUG;
> + break;
>
> - } else if (op2 == 0x0d || op2 == 0x1f) {
> -
> + case 0x0d:
> + case 0x1f:
> /* nopl/nopw */
> *type = INSN_NOP;
> + break;
>
> - } else if (op2 == 0xa0 || op2 == 0xa8) {
> -
> - /* push fs/gs */
> + case 0xa0: /* push fs */
> + case 0xa8: /* push gs */
> *type = INSN_STACK;
> op->src.type = OP_SRC_CONST;
> op->dest.type = OP_DEST_PUSH;
> + break;
>
> - } else if (op2 == 0xa1 || op2 == 0xa9) {
> -
> - /* pop fs/gs */
> + case 0xa1: /* pop fs */
> + case 0xa9: /* pop gs */
> *type = INSN_STACK;
> op->src.type = OP_SRC_POP;
> op->dest.type = OP_DEST_MEM;
> - }
> + break;
>
> + default:
> + break;
> + }
> break;
>
> case 0xc9:
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -1316,6 +1316,43 @@ static int read_unwind_hints(struct objt
> return 0;
> }
>
> +static int read_fpu_hints(struct objtool_file *file)
> +{
> + struct section *sec;
> + struct instruction *insn;
> + struct rela *rela;
> +
> + sec = find_section_by_name(file->elf, ".rela.discard.fpu_safe");
> + if (!sec)
> + return 0;
> +
> + list_for_each_entry(rela, &sec->rela_list, list) {
> + if (rela->sym->type != STT_SECTION) {
> + WARN("unexpected relocation symbol type in %s", sec->name);
> + return -1;
> + }
> +
> + insn = find_insn(file, rela->sym->sec, rela->addend);
> + if (!insn) {
> + WARN("bad .discard.fpu_safe entry");
> + return -1;
> + }
> +
> + if (insn->type != INSN_FPU) {
> + WARN_FUNC("fpu_safe hint not an FPU instruction",
> + insn->sec, insn->offset);
> +// return -1;
> + }
> +
> + while (insn && insn->type == INSN_FPU) {
> + insn->fpu_safe = true;
> + insn = next_insn_same_func(file, insn);
> + }
> + }
> +
> + return 0;
> +}
> +
> static int read_retpoline_hints(struct objtool_file *file)
> {
> struct section *sec;
> @@ -1422,6 +1459,10 @@ static int decode_sections(struct objtoo
> if (ret)
> return ret;
>
> + ret = read_fpu_hints(file);
> + if (ret)
> + return ret;
> +
> return 0;
> }
>
> @@ -2167,6 +2208,16 @@ static int validate_branch(struct objtoo
> if (dead_end_function(file, insn->call_dest))
> return 0;
>
> + if (insn->call_dest) {
> + if (!strcmp(insn->call_dest->name, "kernel_fpu_begin") ||
> + !strcmp(insn->call_dest->name, "emulator_get_fpu"))
> + state.fpu = true;
> +
> + if (!strcmp(insn->call_dest->name, "kernel_fpu_end") ||
> + !strcmp(insn->call_dest->name, "emulator_put_fpu"))
> + state.fpu = false;
> + }
> +
> break;
>
> case INSN_JUMP_CONDITIONAL:
> @@ -2275,6 +2326,13 @@ static int validate_branch(struct objtoo
> state.df = false;
> break;
>
> + case INSN_FPU:
> + if (!state.fpu && !insn->fpu_safe) {
> + WARN_FUNC("FPU instruction outside of kernel_fpu_{begin,end}()", sec, insn->offset);
> + return 1;
> + }
> + break;
> +
> default:
> break;
> }
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -20,6 +20,7 @@ struct insn_state {
> unsigned char type;
> bool bp_scratch;
> bool drap, end, uaccess, df;
> + bool fpu;
> unsigned int uaccess_stack;
> int drap_reg, drap_offset;
> struct cfi_reg vals[CFI_NUM_REGS];
> @@ -34,7 +35,7 @@ struct instruction {
> enum insn_type type;
> unsigned long immediate;
> bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
> - bool retpoline_safe;
> + bool retpoline_safe, fpu_safe;
> u8 visited;
> struct symbol *call_dest;
> struct instruction *jump_dest;
>


--
Masami Hiramatsu <mhiramat@xxxxxxxxxx>