Messages in this thread | | | Subject | Re: AMD DC graphics display code enables -mhard-float, -msse, -msse2 without any visible FPU state protection | From | Christian König <> | Date | Fri, 10 Apr 2020 16:31:39 +0200 |
| |
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.
| |