Re: [PATCH 4/4] livepatch: differentiate between old and new build systems

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Mar 11, 2022 at 09:33:15AM +0100, Roger Pau Monné wrote:
> On Thu, Mar 10, 2022 at 06:01:48PM +0000, Andrew Cooper wrote:
> > On 08/03/2022 14:52, Roger Pau Monne wrote:
> > > On Tue, Mar 08, 2022 at 02:38:47PM +0000, Andrew Cooper wrote:
> > >> On 02/03/2022 14:27, Roger Pau Monne wrote:
> > >>> diff --git a/livepatch-build b/livepatch-build
> > >>> index 38a92be..656cdac 100755
> > >>> --- a/livepatch-build
> > >>> +++ b/livepatch-build
> > >>> @@ -98,14 +98,20 @@ function build_special()
> > >>>  
> > >>>      # Build with special GCC flags
> > >>>      cd "${SRCDIR}/xen" || die
> > >>> -    sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk
> > >>> -    cp -p arch/x86/Makefile arch/x86/Makefile.bak
> > >>> -    sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile
> > >>> -    # Restore timestamps to prevent spurious rebuilding
> > >>> -    touch --reference=arch/x86/Makefile.bak arch/x86/Makefile
> > >>> -    make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die
> > >>> -    sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk
> > >>> -    mv -f arch/x86/Makefile.bak arch/x86/Makefile
> > >>> +    if grep -q 'nostdinc' Rules.mk; then
> > >>> +         # Support for old build system, attempt to set -f{function,data}-sections and rebuild
> > >>> +        sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk
> > >>> +        cp -p arch/x86/Makefile arch/x86/Makefile.bak
> > >>> +        sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile
> > >>> +        # Restore timestamps to prevent spurious rebuilding
> > >>> +        touch --reference=arch/x86/Makefile.bak arch/x86/Makefile
> > >>> +        make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die
> > >>> +        sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk
> > >>> +        mv -f arch/x86/Makefile.bak arch/x86/Makefile
> > >>> +    else
> > >>> +        # -f{function,data}-sections set by CONFIG_LIVEPATCH
> > >>> +        make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die
> > >>> +    fi
> > >> This really ought to be the other way around, by spotting the thing we
> > >> know is good, and then falling back to the heuristics.  In light of the
> > >> updates to the Xen side, something like:
> > > I'm not sure I agree. I do prefer to spot the 'bad' one, and just
> > > fallback to expecting Xen to correctly set -f{function,data}-sections
> > > otherwise.
> > >
> > >> if grep -q CC_SPLIT_SECTIONS Kconfig; then
> > > Because this logic ties us to not moving CC_SPLIT_SECTIONS from being
> > > defined in xen/Kconfig (or even changing it's name), and gain ties the
> > > livepatch tools to internal details about the Xen build system.
> > 
> > It doesn't particularly matter which way around the if/else is.  It does
> > matter that we're choosing based on something relevant.
> > 
> > nostdinc in Rules.mk has exactly the same amount of "magic string in
> > magic file" as CC_SPLIT_SECTIONS in Kconfig, but has absolutely nothing
> > to do with the property we actually care about.
> > 
> > Really what you actually want is
> > 
> > if grep -q CC_SPLIT_SECTIONS Kconfig; then
> >     # Xen behaves sensibly
> > elif grep -q 'nostdinc' Rules.mk; then
> >     # Legacy mess with Rules.mk
> > else
> >     die "Help with build system divination"
> > fi
> > 
> > The "behaves sensibly" case is unlikely to change name and unlikely to
> > move locations, but each are easy to cope with via `grep -e FOO -e BAR
> > file1 file2`, and this approach avoids the problem of blindly (and
> > falsely) assuming that anything which is 4.14 and later splits sections
> > correctly, and that this will remain true even when someone adds "# use
> > to have -nostdinc here" to Rules.mk.
> 
> TBH, I don't find the proposed solution is much better to what's in
> this patch, and as said I really dislike tying the behavior of the
> livepatch build tools to heuristics against Xen internal build files -
> be it a Kconfig or a Makefile. Specially because your proposed
> approach adds heuristics to detect the 'good' case which should be the
> default one going forward.
> 
> A better option might be to just make the 'build adjustments' a
> command line option that the user can pass to the tools, ie:
> --build-adjust and let the user decide whether it needs the
> adjustments or not. If I was a livepatch user myself I would seriously
> consider picking the linker script changes and backport that to my
> production version.

Ping?

Is the proposed command line option an acceptable way to move this
forward?

Can I have an opinion from the maintainers?

Thanks, Roger.