2023-06-20 06:29:14

by Naveen N Rao

[permalink] [raw]
Subject: Re: [RFC PATCH v1 2/3] powerpc: Mark all .S files invalid for objtool

Christophe Leroy wrote:
> A lot of work is required in .S files in order to get them ready
> for objtool checks.
>
> For the time being, exclude them from the checks.
>
> This is done with the script below:
>
> #!/bin/sh
> DIRS=`find arch/powerpc -name "*.S" -exec dirname {} \; | sort | uniq`
> for d in $DIRS
> do
> pushd $d
> echo >> Makefile
> for f in *.S
> do
> echo "OBJECT_FILES_NON_STANDARD_$f := y" | sed s/"\.S"/".o"/g
> done >> Makefile
> popd
> done
>
> Signed-off-by: Christophe Leroy <[email protected]>
> ---
> arch/powerpc/boot/Makefile | 17 +++++++++
> arch/powerpc/crypto/Makefile | 13 +++++++
> arch/powerpc/kernel/Makefile | 44 ++++++++++++++++++++++
> arch/powerpc/kernel/trace/Makefile | 4 ++
> arch/powerpc/kernel/vdso/Makefile | 11 ++++++
> arch/powerpc/kexec/Makefile | 2 +
> arch/powerpc/kvm/Makefile | 13 +++++++
> arch/powerpc/lib/Makefile | 25 ++++++++++++
> arch/powerpc/mm/book3s32/Makefile | 3 ++
> arch/powerpc/mm/nohash/Makefile | 3 ++
> arch/powerpc/perf/Makefile | 2 +
> arch/powerpc/platforms/44x/Makefile | 2 +
> arch/powerpc/platforms/52xx/Makefile | 3 ++
> arch/powerpc/platforms/83xx/Makefile | 2 +
> arch/powerpc/platforms/cell/spufs/Makefile | 3 ++
> arch/powerpc/platforms/pasemi/Makefile | 2 +
> arch/powerpc/platforms/powermac/Makefile | 3 ++
> arch/powerpc/platforms/powernv/Makefile | 3 ++
> arch/powerpc/platforms/ps3/Makefile | 2 +
> arch/powerpc/platforms/pseries/Makefile | 2 +
> arch/powerpc/purgatory/Makefile | 3 ++
> arch/powerpc/sysdev/Makefile | 3 ++
> arch/powerpc/xmon/Makefile | 3 ++
> 23 files changed, 168 insertions(+)
>

I think it might be better to have a config option so that architectures
can opt-in to skip objtool on asm files. We can then do:

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 9f94fc83f08652..878027cf4faf37 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -359,7 +359,11 @@ $(obj)/%.s: $(src)/%.S FORCE
$(call if_changed_dep,cpp_s_S)

quiet_cmd_as_o_S = AS $(quiet_modtag) $@
+ifndef CONFIG_ARCH_OBJTOOL_SKIP_ASM
cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $< $(cmd_objtool)
+else
+ cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $<
+endif

ifdef CONFIG_ASM_MODVERSIONS



- Naveen



2023-06-20 06:48:26

by Christophe Leroy

[permalink] [raw]
Subject: Re: [RFC PATCH v1 2/3] powerpc: Mark all .S files invalid for objtool



Le 20/06/2023 à 08:04, Naveen N Rao a écrit :
> Christophe Leroy wrote:
>> A lot of work is required in .S files in order to get them ready
>> for objtool checks.
>>
>> For the time being, exclude them from the checks.
>>
>> This is done with the script below:
>>
>>     #!/bin/sh
>>     DIRS=`find arch/powerpc -name "*.S" -exec dirname {} \; | sort |
>> uniq`
>>     for d in $DIRS
>>     do
>>         pushd $d
>>         echo >> Makefile
>>         for f in *.S
>>         do
>>             echo "OBJECT_FILES_NON_STANDARD_$f := y" | sed s/"\.S"/".o"/g
>>         done >> Makefile
>>         popd
>>     done
>>
>> Signed-off-by: Christophe Leroy <[email protected]>
>> ---
>>  arch/powerpc/boot/Makefile                 | 17 +++++++++
>>  arch/powerpc/crypto/Makefile               | 13 +++++++
>>  arch/powerpc/kernel/Makefile               | 44 ++++++++++++++++++++++
>>  arch/powerpc/kernel/trace/Makefile         |  4 ++
>>  arch/powerpc/kernel/vdso/Makefile          | 11 ++++++
>>  arch/powerpc/kexec/Makefile                |  2 +
>>  arch/powerpc/kvm/Makefile                  | 13 +++++++
>>  arch/powerpc/lib/Makefile                  | 25 ++++++++++++
>>  arch/powerpc/mm/book3s32/Makefile          |  3 ++
>>  arch/powerpc/mm/nohash/Makefile            |  3 ++
>>  arch/powerpc/perf/Makefile                 |  2 +
>>  arch/powerpc/platforms/44x/Makefile        |  2 +
>>  arch/powerpc/platforms/52xx/Makefile       |  3 ++
>>  arch/powerpc/platforms/83xx/Makefile       |  2 +
>>  arch/powerpc/platforms/cell/spufs/Makefile |  3 ++
>>  arch/powerpc/platforms/pasemi/Makefile     |  2 +
>>  arch/powerpc/platforms/powermac/Makefile   |  3 ++
>>  arch/powerpc/platforms/powernv/Makefile    |  3 ++
>>  arch/powerpc/platforms/ps3/Makefile        |  2 +
>>  arch/powerpc/platforms/pseries/Makefile    |  2 +
>>  arch/powerpc/purgatory/Makefile            |  3 ++
>>  arch/powerpc/sysdev/Makefile               |  3 ++
>>  arch/powerpc/xmon/Makefile                 |  3 ++
>>  23 files changed, 168 insertions(+)
>>
>
> I think it might be better to have a config option so that architectures
> can opt-in to skip objtool on asm files. We can then do:

Well, the idea here was to initially opt out every file in order to
quickly add support for objtool uaccess checking, and then opt-in back
files one by one once they are ready for it.

In most files,all we have to do is to replace all _GLOBAL() by
SYM_FUNC_START(), add a SYM_FUNC_END() at the end of the function, then
do the same with SYM_FUNC_START_LOCAL() with all non global functions.

That's easy to do and worth it but it takes time, hence the idea of an
incremental approach.

Christophe

2023-06-20 06:59:04

by Naveen N Rao

[permalink] [raw]
Subject: Re: [RFC PATCH v1 2/3] powerpc: Mark all .S files invalid for objtool

Christophe Leroy wrote:
>
>
> Le 20/06/2023 à 08:04, Naveen N Rao a écrit :
>> Christophe Leroy wrote:
>>> A lot of work is required in .S files in order to get them ready
>>> for objtool checks.
>>>
>>> For the time being, exclude them from the checks.
>>>
>>> This is done with the script below:
>>>
>>>     #!/bin/sh
>>>     DIRS=`find arch/powerpc -name "*.S" -exec dirname {} \; | sort |
>>> uniq`
>>>     for d in $DIRS
>>>     do
>>>         pushd $d
>>>         echo >> Makefile
>>>         for f in *.S
>>>         do
>>>             echo "OBJECT_FILES_NON_STANDARD_$f := y" | sed s/"\.S"/".o"/g
>>>         done >> Makefile
>>>         popd
>>>     done
>>>
>>> Signed-off-by: Christophe Leroy <[email protected]>
>>> ---
>>>  arch/powerpc/boot/Makefile                 | 17 +++++++++
>>>  arch/powerpc/crypto/Makefile               | 13 +++++++
>>>  arch/powerpc/kernel/Makefile               | 44 ++++++++++++++++++++++
>>>  arch/powerpc/kernel/trace/Makefile         |  4 ++
>>>  arch/powerpc/kernel/vdso/Makefile          | 11 ++++++
>>>  arch/powerpc/kexec/Makefile                |  2 +
>>>  arch/powerpc/kvm/Makefile                  | 13 +++++++
>>>  arch/powerpc/lib/Makefile                  | 25 ++++++++++++
>>>  arch/powerpc/mm/book3s32/Makefile          |  3 ++
>>>  arch/powerpc/mm/nohash/Makefile            |  3 ++
>>>  arch/powerpc/perf/Makefile                 |  2 +
>>>  arch/powerpc/platforms/44x/Makefile        |  2 +
>>>  arch/powerpc/platforms/52xx/Makefile       |  3 ++
>>>  arch/powerpc/platforms/83xx/Makefile       |  2 +
>>>  arch/powerpc/platforms/cell/spufs/Makefile |  3 ++
>>>  arch/powerpc/platforms/pasemi/Makefile     |  2 +
>>>  arch/powerpc/platforms/powermac/Makefile   |  3 ++
>>>  arch/powerpc/platforms/powernv/Makefile    |  3 ++
>>>  arch/powerpc/platforms/ps3/Makefile        |  2 +
>>>  arch/powerpc/platforms/pseries/Makefile    |  2 +
>>>  arch/powerpc/purgatory/Makefile            |  3 ++
>>>  arch/powerpc/sysdev/Makefile               |  3 ++
>>>  arch/powerpc/xmon/Makefile                 |  3 ++
>>>  23 files changed, 168 insertions(+)
>>>
>>
>> I think it might be better to have a config option so that architectures
>> can opt-in to skip objtool on asm files. We can then do:
>
> Well, the idea here was to initially opt out every file in order to
> quickly add support for objtool uaccess checking, and then opt-in back
> files one by one once they are ready for it.

That was my initial thought too.

>
> In most files,all we have to do is to replace all _GLOBAL() by
> SYM_FUNC_START(), add a SYM_FUNC_END() at the end of the function, then
> do the same with SYM_FUNC_START_LOCAL() with all non global functions.
>
> That's easy to do and worth it but it takes time, hence the idea of an
> incremental approach.

Right. But until that's done, I am not sure it is worth the churn to the
Makefiles.

Besides, it isn't clear to me that the existing features we are
targeting for objtool on powerpc need objtool to run on asm files (so,
an incremental approach may not really get us much). Objtool --mcount
doesn't care about .S files. I suppose uaccess validation also doesn't
need it. It's likely just stack validation (should we choose to enable
it) that needs objtool to be run on asm files.

The first patch series converting much of our .S files can drop the
config option and exclude the remaining files.


- Naveen