Christophe Leroy wrote:

Le 08/08/2022 à 13:48, Sathvika Vasireddy a écrit :

quoted

objtool is throwing *unannotated intra-function call*
warnings with a few instructions that are marked
unreachable. Replace unreachable() with __builtin_unreachable()
to fix these warnings, as the codegen remains same
with unreachable() and __builtin_unreachable().

I think it is necessary to explain why using unreachable() is not 
necessary for powerpc, or even why using unreachable() is wrong.

Allthough we are getting rid of the problem here by replacing 
unreachable() by __builtin_unreachable(), it might still be a problem in 
core parts of kernel which still use unreachable.

I did a kernel build with this series applied, with a variant of 
ppc64le_defconfig. I then did another build with the same config, but 
with the below hunk to disable objtool:

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 6be2e68fa9eb64..4c466acdc70d4c 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig

@@ -237,8 +237,6 @@ config PPC
        select HAVE_MOD_ARCH_SPECIFIC
        select HAVE_NMI                         if PERF_EVENTS || (PPC64 && PPC_BOOK3S)
        select HAVE_OPTPROBES
-       select HAVE_OBJTOOL                     if PPC32 || MPROFILE_KERNEL
-       select HAVE_OBJTOOL_MCOUNT              if HAVE_OBJTOOL
        select HAVE_PERF_EVENTS
        select HAVE_PERF_EVENTS_NMI             if PPC64
        select HAVE_PERF_REGS

This has the effect of disabling annotations for unreachable().

When I compared the resulting object files, I did not see changes in 
codegen relating to the annotation, like we do with using unreachable() 
in __WARN_FLAGS().

More specifically, arch/powerpc/kvm/book3s.o:kvmppc_h_logical_ci_load() 
uses BUG(), and the generated code remains the same with/without the 
unreachable() annotation.

This suggests that the bad codegen we are seeing with the annotation in 
unreachable() is limited to its use in __WARN_FLAGS(), which I suspect 
is due to an interaction with the use of asm_volatile_goto() for 
WARN_ENTRY().

If I revert this patch (patch 01/16), gcc seems to add a label 8 bytes 
before _some_ function in this object file, which happens to hold a 
relocation against .TOC., and emits a bl to that symbol. Otherwise, gcc 
either emits no new instruction for the annotation, or a 'nop' in some 
cases.

If I add a 'nop' between WARN_ENTRY() and unreachable() in 
__WARN_FLAGS(), or convert WARN_ENTRY to BUG_ENTRY thereby removing use 
of asm_volatile_goto(), the problem goes away and no bl is emitted:

diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index 61a4736355c244..88e0027c20ba5c 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h

@@ -99,6 +99,7 @@
        __label__ __label_warn_on;                              \
                                                                \
        WARN_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags), __label_warn_on); \
+       __asm__ __volatile__("nop");                            \
        unreachable();                                          \
                                                                \
 __label_warn_on:

In summary, I think the annotation itself is fine and we are only seeing 
an issue with its usage after WARN_ENTRY() due to use of 
asm_volatile_goto. Other uses of unreachable() don't seem to exhibit 
this problem.

As such, I think this patch is appropriate for this series, though I 
think we should capture some of this information in the changelog.

Note also that if and when we start utlizing the annotation, if we 
classify twui as INSN_BUG, this change will continue to be appropriate.


- Naveen


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel