Re: [PATCH] livepatch-build-tools: allow livepatching version.c

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

On Wed, Dec 06, 2023 at 12:11:39PM +0000, Ross Lagerwall wrote:
> On Tue, Dec 5, 2023 at 2:57 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Tue, Dec 05, 2023 at 02:15:05PM +0000, Andrew Cooper wrote:
> > > On 05/12/2023 12:34 pm, Roger Pau Monne wrote:
> > > > Currently version.o is explicitly ignored as the file would change as a result
> > > > of the orignal and the patched build having possibly different dates and
> > > > times.
> > > >
> > > > Fix such difference by exporting the date and time from the build script, so
> > > > that both builds share the same build time.  This allows checking for changes
> > > > in version.c, since the rest of fields need to be manually changed in order to
> > > > produce different output.
> > > >
> > > > Setting XEN_BUILD_{DATE,TIME} as an environment variable has been supported
> > > > since before livepatch support was added to Xen, so it's safe to export those
> > > > variables unconditionally.
> > > >
> > > > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > > ---
> > > >  livepatch-build | 4 ++++
> > > >  livepatch-gcc   | 2 --
> > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/livepatch-build b/livepatch-build
> > > > index e2ccce4f7fd7..f622683fc56c 100755
> > > > --- a/livepatch-build
> > > > +++ b/livepatch-build
> > > > @@ -417,6 +417,10 @@ if [ "${SKIP}" != "build" ]; then
> > > >
> > > >      export CROSS_COMPILE="${TOOLSDIR}/livepatch-gcc "
> > > >
> > > > +    # Force same date and time to prevent unwanted changes in version.c
> > > > +    export XEN_BUILD_DATE=`LC_ALL=C date`
> > > > +    export XEN_BUILD_TIME=`LC_ALL=C date +%T`
> > >
> > > Date is the one that goes wrong every time, but everything else in
> > > compile.h can go wrong in a way that causes version.o to change.
> >
> > I've attempted to reflect that in "since the rest of fields need to be
> > manually changed in order to produce different output".
> >
> > For those to change there must be some kind of environment change
> > between the original and the patched version build, and hence I don't
> > think that would be supported.
> 
> In general, yes. However, with this patch changes to the
> hostname/domain/username would result in version.o being marked
> as changed even though it is entirely fine to build the live patch
> on a different build host from the original Xen.

Keep in mind livepatch-build-tools builds it's base version of Xen and
then a patched version, and that's how the diff is performed.  For the
hostname/domain/username changes to appear on the livepatch payload
such change would need to happen in the muddle of the execution of
livepatch-build.

This change doesn't prevent building the original Xen on a different
host than the one building the livepatch, and the
hostname/domain/username changes won't be part of the livepatch
payload.

> >
> > > Ideally, the pristine source for building livepatches would include a
> > > generated compile.h, and livepatch would have a way to force no
> > > regeneration of the header.  But I've got no idea how nice that would be
> > > to arrange.
> >
> > Yes, no idea how fragile that would be either.  IMO the proposed
> > approach is not that bad.
> >
> > > That way, you're using the same details as the Xen being patched, rather
> > > than hoping that two identical different details will cancel out in the
> > > binary diff.
> >
> > Another option is to set all the env variables to disable any
> > build time probing.  However things like compiler or version changing
> > between the original and the patched builds likely point out to issues
> > elsewhere, unless it's intentional modification of the helpers.
> >
> > > > +
> > > >      echo "Perform full initial build with ${CPUS} CPU(s)..."
> > > >      build_full
> > > >
> > > > diff --git a/livepatch-gcc b/livepatch-gcc
> > > > index fcad80551aa0..e4cb6fb59029 100755
> > > > --- a/livepatch-gcc
> > > > +++ b/livepatch-gcc
> > > > @@ -33,7 +33,6 @@ if [[ "$TOOLCHAINCMD" =~ $GCC_RE ]] ; then
> > > >              obj=$2
> > > >              [[ $2 = */.tmp_*.o ]] && obj=${2/.tmp_/}
> > > >              case "$(basename $obj)" in
> > > > -            version.o|\
> > > >              debug.o|\
> > > >              check.o|\
> > >
> > > Tangential question.  check.o is excluded because it's a toolchain test,
> > > but any idea what debug.o is doing in this list?
> > >
> > > It can't possibly be the debug.c I've recently added to x86 (which we'll
> > > want to be able to livepatch), so I guess it's got something to do the
> > > ARM debug.S's, but I can't see anything in those that are worthy of
> > > exemption either...
> >
> > Hm, that comes from the first commit that imported the wrapper to the
> > repository, and at that point only x86 had livepatch support.
> >
> > I'm tempted to think this was inherited from the original xsplice
> > tooling, and so debug.o needs to be removed from the list.
> >
> 
> livepatch-build-tools is derived from the kpatch build tooling and
> debug.o has never been present there so it was added here for a
> reason. AFAICT the gdbsx code used to live in debug.o. I can't
> recall why it was being marked as changed unnecessarily but since
> that is no longer an issue and the code lives elsewhere, the debug.o
> lines can be dropped.

Will someone send a patch for this, or should I do it?

Thanks, Roger.