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, 14 Apr 2022 19:59:57 +0200
From:   Nicolas Schier <nicolas@fjasle.eu>
To:     Petr Mladek <pmladek@suse.com>
Cc:     Joe Lawrence <joe.lawrence@redhat.com>,
        live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-kbuild@vger.kernel.org
Subject: Re: [RFC PATCH v6 02/12] kbuild: Support for symbols.klp creation
Message-ID: <YlhhHSQIWpLG0Cgn@fjasle.eu>
References: <20220216163940.228309-1-joe.lawrence@redhat.com>
 <20220216163940.228309-3-joe.lawrence@redhat.com>
 <Ylfq7t0uOP7gCPEO@alley>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Ylfq7t0uOP7gCPEO@alley>
Precedence: bulk

On Thu, Apr 14, 2022 at 11:35:42AM +0200 Petr Mladek wrote:
> On Wed 2022-02-16 11:39:30, Joe Lawrence wrote:
> > From: Joao Moreira <jmoreira@suse.de>
> > 
> > For automatic resolution of livepatch relocations, a file called
> > symbols.klp is used. This file maps symbols within every compiled kernel
> > object allowing the identification of symbols whose name is unique, thus
> > relocation can be automatically inferred, or providing information that
> > helps developers when code annotation is required for solving the
> > matter.
> > 
> > Add support for creating symbols.klp in the main Makefile. First, ensure
> > that built-in is compiled when CONFIG_LIVEPATCH is enabled (as required
> > to achieve a complete symbols.klp file). Define the command to build
> > symbols.klp (cmd_klp_map) and hook it in the modules rule.
> > 
> > As it is undesirable to have symbols from livepatch objects inside
> > symbols.klp, make livepatches discernible by modifying
> > scripts/Makefile.build to create a .livepatch file for each livepatch in
> > $(MODVERDIR). This file then used by cmd_klp_map to identify and bypass
> > livepatches.
> >
> > For identifying livepatches during the build process, a flag variable
> > LIVEPATCH_$(basetarget).o is considered in scripts/Makefile.build. This
> > way, set this flag for the livepatch sample Makefile in
> > samples/livepatch/Makefile.
> 
> I do not see the related code in scripts/Makefile.build.
> 
> > Finally, Add a clean rule to ensure that symbols.klp is removed during
> > clean.
> > 
> > Notes:
> > 
> > To achieve a correct symbols.klp file, all kernel objects must be
> > considered, thus, its construction require these objects to be priorly
> > built. On the other hand, invoking scripts/Makefile.modpost without
> > having a complete symbols.klp in place would occasionally lead to
> > in-tree livepatches being post-processed incorrectly.
> 
> Honestly, I do not understand what it exactly means that "in-tree
> livepatches would occasionally be post-processed incorrectly".
> 
> Is it the problem that modpost is not able to handle the unresolved
> symbols that have to be updated by klp-convert?
> 
> > To prevent this
> > from becoming a circular dependency, the construction of symbols.klp
> > uses non-post-processed kernel objects and such does not cause harm as
> > the symbols normally referenced from within livepatches are visible at
> > this stage. Also due to these requirements, the spot in-between modules
> > compilation and the invocation of scripts/Makefile.modpost was picked
> > for hooking cmd_klp_map.
> > 
> > The approach based on .livepatch files was proposed as an alternative to
> > using MODULE_INFO statements. This approach was originally proposed by
> > Miroslav Benes as a workaround for identifying livepathes without
> > depending on modinfo during the modpost stage. It was moved to this
> > patch as the approach also shown to be useful while building
> > symbols.klp.
> 
> All the tricky code is removed in the 5th patch. My understanding is
> that the problem causing the cyclic dependency is solved by modifying
> modpost.
> 
> It looks like this patch is outdated and mostly obsoleted. On the
> other hand, the commit message in 5th patch is too short.
> 
> What about merging the two patches and updating the commit message?

+1

Yes, please merge those patches.  These '$(shell ...)' side-effect lines in the
definition of 'cmd_klp_map' are quite confusing.

Kind regards,
Nicolas