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

[Christian KÃnig]

Am 09.04.20 um 22:01 schrieb Peter Zijlstra:
> On Thu, Apr 09, 2020 at 08:15:57PM +0200, Christian KÃnig wrote:
> > Am 09.04.20 um 19:09 schrieb Peter Zijlstra:
> > > On Thu, Apr 09, 2020 at 05:59:56PM +0200, Peter Zijlstra wrote:
> > > [SNIP]
> > > > I'll need another approach, let me consider.
> > > Christian; it says these files are generated, does that generator know
> > > which functions are wholly in FPU context and which are not?
> > Well that "generator" is still a human being :)
> >
> > It's just that the formulae for the calculation come from the hardware team
> > and we are not able to easily transcript them to fixed point calculations.
> Well, if it's a human, can this human respect the kernel coding style a
> bit more :-) Some of that stuff is atrocious.

Yes, I know. That's unfortunately something we still need to work on as 
well.

> > We are currently in the process of moving all the stuff which requires
> > floating point into a single C file(s) and then make sure that we only call
> > those within kernel_fpu_begin()/end() blocks.
> Can you make the build system stick all those .o files in a single
> archive? That's the only way I can do call validation; external
> relocatoin records do not contain the section.

Need to double check that with the display team responsible for the 
code, but I think that shouldn't be much of a problem.

> > Annotating those function with __fpu or even saying to gcc that all code of
> > those files should go into a special text.fpu segment shouldn't be much of a
> > problem.
> Guess what the __fpu attribute does ;-)

Good to know that my suspicion how this is implemented was correct :)

> With the below patch (which is on to of newer versions of the objtool
> patches send earlier, let me know if you want a full set

Getting a branch somewhere would be perfect.

> ) that only
> converts a few files, but fully converts:
>
>    drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.c
>
> But building it (and this is an absolute pain; when you're reworking
> this, can you pretty please also fix the Makefiles?), we get:
>
>    drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.o: warning: objtool: dcn_validate_bandwidth()+0x34fa: FPU instruction outside of kernel_fpu_{begin,end}()
>
> $ ./scripts/faddr2line defconfig-build/drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.o dcn_validate_bandwidth+0x34fa
> dcn_validate_bandwidth+0x34fa/0x57ce:
> dcn_validate_bandwidth at /usr/src/linux-2.6/defconfig-build/../drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.c:1293 (discriminator 5)
>
> # ./objdump-func.sh defconfig-build/drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.o dcn_validate_bandwidth | grep 34fa
> 34fa     50fa:  f2 0f 10 b5 60 ff ff    movsd  -0xa0(%rbp),%xmm6
>
> Which seems to indicate there's still problms with the current code.

Making an educated guess I would say the compiler has no idea that it 
shouldn't use instructions which touch fp registers outside of 
kernel_fpu_{begin,end}().

Going to talk with the display team about this whole topic internally 
once more. Since this discussion already raised attention in our 
technical management it shouldn't be to much of a problem to get 
manpower to get this fixed properly.

Can we put this new automated check will be behind a configuration flag 
initially? Or at least make it a warning and not a hard error.

Thanks,
Christian.