Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 18 Aug 2022 16:16:32 +0530
From:   "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Subject: Re: [PATCH 01/16] powerpc: Replace unreachable() with it's builtin
 variant in WARN_ON()
To:     Christophe Leroy <christophe.leroy@csgroup.eu>,
        "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
        Sathvika Vasireddy <sv@linux.ibm.com>
Cc:     "aik@ozlabs.ru" <aik@ozlabs.ru>,
        "chenzhongjin@huawei.com" <chenzhongjin@huawei.com>,
        "jpoimboe@redhat.com" <jpoimboe@redhat.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "mbenes@suse.cz" <mbenes@suse.cz>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "mpe@ellerman.id.au" <mpe@ellerman.id.au>,
        "npiggin@gmail.com" <npiggin@gmail.com>,
        "peterz@infradead.org" <peterz@infradead.org>,
        "rostedt@goodmis.org" <rostedt@goodmis.org>
References: <20220808114908.240813-1-sv@linux.ibm.com>
        <20220808114908.240813-2-sv@linux.ibm.com>
        <82eec792-b71f-17cc-d905-368fd5ca62f2@csgroup.eu>
In-Reply-To: <82eec792-b71f-17cc-d905-368fd5ca62f2@csgroup.eu>
MIME-Version: 1.0
User-Agent: astroid/4d6b06ad (https://github.com/astroidmail/astroid)
Message-Id: <1660817468.4x4re2ul0k.naveen@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Christophe Leroy wrote:
>=20
>=20
> Le 08/08/2022 =C3=A0 13:48, Sathvika Vasireddy a =C3=A9crit=C2=A0:
>> 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().
>=20
> I think it is necessary to explain why using unreachable() is not=20
> necessary for powerpc, or even why using unreachable() is wrong.
>=20
> Allthough we are getting rid of the problem here by replacing=20
> unreachable() by __builtin_unreachable(), it might still be a problem in=20
> core parts of kernel which still use unreachable.

I did a kernel build with this series applied, with a variant of=20
ppc64le_defconfig. I then did another build with the same config, but=20
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=20
codegen relating to the annotation, like we do with using unreachable()=20
in __WARN_FLAGS().

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

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

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

If I add a 'nop' between WARN_ENTRY() and unreachable() in=20
__WARN_FLAGS(), or convert WARN_ENTRY to BUG_ENTRY thereby removing use=20
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=20
an issue with its usage after WARN_ENTRY() due to use of=20
asm_volatile_goto. Other uses of unreachable() don't seem to exhibit=20
this problem.

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

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


- Naveen