lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b64111a3-df3f-bf59-20ce-0af57715ad53@csgroup.eu>
Date:   Tue, 9 Mar 2021 10:57:44 +0100
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        amd-gfx list <amd-gfx@...ts.freedesktop.org>,
        Christian König <christian.koenig@....com>,
        Alex Deucher <alexdeucher@...il.com>
Subject: Re: [PATCH] powerpc: Fix missing declaration of
 [en/dis]able_kernel_vsx()



Le 09/03/2021 à 10:16, Geert Uytterhoeven a écrit :
> Hi Christophe,
> 
> On Tue, Mar 9, 2021 at 9:52 AM Christophe Leroy
> <christophe.leroy@...roup.eu> wrote:
>> Le 09/03/2021 à 09:45, Geert Uytterhoeven a écrit :
>>> On Tue, Mar 9, 2021 at 9:39 AM Christophe Leroy
>>> <christophe.leroy@...roup.eu> wrote:
>>>> Add stub instances of enable_kernel_vsx() and disable_kernel_vsx()
>>>> when CONFIG_VSX is not set, to avoid following build failure.
>>>>
>>>>     CC [M]  drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.o
>>>> In file included from ./drivers/gpu/drm/amd/amdgpu/../display/dc/dm_services_types.h:29,
>>>>                    from ./drivers/gpu/drm/amd/amdgpu/../display/dc/dm_services.h:37,
>>>>                    from drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.c:27:
>>>> drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.c: In function 'dcn_bw_apply_registry_override':
>>>> ./drivers/gpu/drm/amd/amdgpu/../display/dc/os_types.h:64:3: error: implicit declaration of function 'enable_kernel_vsx'; did you mean 'enable_kernel_fp'? [-Werror=implicit-function-declaration]
>>>>      64 |   enable_kernel_vsx(); \
>>>>         |   ^~~~~~~~~~~~~~~~~
>>>> drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.c:640:2: note: in expansion of macro 'DC_FP_START'
>>>>     640 |  DC_FP_START();
>>>>         |  ^~~~~~~~~~~
>>>> ./drivers/gpu/drm/amd/amdgpu/../display/dc/os_types.h:75:3: error: implicit declaration of function 'disable_kernel_vsx'; did you mean 'disable_kernel_fp'? [-Werror=implicit-function-declaration]
>>>>      75 |   disable_kernel_vsx(); \
>>>>         |   ^~~~~~~~~~~~~~~~~~
>>>> drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.c:676:2: note: in expansion of macro 'DC_FP_END'
>>>>     676 |  DC_FP_END();
>>>>         |  ^~~~~~~~~
>>>> cc1: some warnings being treated as errors
>>>> make[5]: *** [drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.o] Error 1
>>>>
>>>> Fixes: 16a9dea110a6 ("amdgpu: Enable initial DCN support on POWER")
>>>> Cc: stable@...r.kernel.org
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>
>>>
>>> Thanks for your patch!
>>>
>>>> --- a/arch/powerpc/include/asm/switch_to.h
>>>> +++ b/arch/powerpc/include/asm/switch_to.h
>>>> @@ -71,6 +71,16 @@ static inline void disable_kernel_vsx(void)
>>>>    {
>>>>           msr_check_and_clear(MSR_FP|MSR_VEC|MSR_VSX);
>>>>    }
>>>> +#else
>>>> +static inline void enable_kernel_vsx(void)
>>>> +{
>>>> +       BUILD_BUG();
>>>> +}
>>>> +
>>>> +static inline void disable_kernel_vsx(void)
>>>> +{
>>>> +       BUILD_BUG();
>>>> +}
>>>>    #endif
>>>
>>> I'm wondering how this is any better than the current situation: using
>>> BUILD_BUG() will still cause a build failure?
>>
>> No it won't cause a failure. In drivers/gpu/drm/amd/display/dc/os_types.h you have:
>>
>> #define DC_FP_START() { \
>>          if (cpu_has_feature(CPU_FTR_VSX_COMP)) { \
>>                  preempt_disable(); \
>>                  enable_kernel_vsx(); \
>>          } else if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) { \
>>                  preempt_disable(); \
>>                  enable_kernel_altivec(); \
>>          } else if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) { \
>>                  preempt_disable(); \
>>                  enable_kernel_fp(); \
>>          } \
>>
>> When CONFIG_VSX is not selected, cpu_has_feature(CPU_FTR_VSX_COMP) constant folds to 'false' so the
>> call to enable_kernel_vsx() is discarded and the build succeeds.
> 
> IC. So you might as well have an empty (dummy) function instead?
> 

But with an empty function, you take the risk that one day, someone calls it without checking that 
CONFIG_VSX is selected. Here if someone does that, build will fail.

Another solution is to declare a non static prototype of it, like __put_user_bad() for instance. In 
that case, the link will fail.

I prefer the BUILD_BUG() approach as I find it cleaner and more explicit, and also because it breaks 
the build at compile time, you don't have to wait link time to catch the error.

Christophe

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ