[PATCH 0/4] livepatch: fixes for patch generation

[PATCH 0/4] livepatch: fixes for patch generation

[Roger Pau Monne]

Hello,

THe following series contains the minimum set of fixes require to get
the livepatch tools to work again with current Xen version.

Note the last patch depends on:

https://lore.kernel.org/xen-devel/20220302134425.38465-1-roger.pau@citrix.com/T/#u

If that's not considered acceptable a different way to add
-f{function,data}-sections will need to be discussed.

Thanks, Roger.

Roger Pau Monne (4):
  livepatch: update readme to mention --xen-depends
  livepatch: improve rune for fetching of Build ID
  livepatch: do the initial build using CROSS_COMPILE
  livepatch: differentiate between old and new build systems

 README.md       |  4 ++--
 livepatch-build | 25 ++++++++++++++++---------
 2 files changed, 18 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH 1/4] livepatch: update readme to mention --xen-depends

[Roger Pau Monne]

Fixes: b19df7b2c05e ('livepatch-build: Embed hypervisor build id into every hotpatch')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 README.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/README.md b/README.md
index 653c624..b48a3df 100644
--- a/README.md
+++ b/README.md
@@ -24,7 +24,7 @@ Next, build a live patch, using a patch and the source, build ID, and
 ```
 $ cd ~/src/livepatch-build
 $ ./livepatch-build -s ~/src/xenbuild -p ~/src/xsa.patch -o out \
-    -c ~/src/xen/xen/.config --depends $BUILDID
+    -c ~/src/xen/xen/.config --depends $BUILDID --xen-depends $BUILDID
 Building LivePatch patch: xsa
 
 Xen directory: /home/ross/src/xenbuild
-- 
2.34.1

[PATCH 2/4] livepatch: improve rune for fetching of Build ID

[Roger Pau Monne]

The current one is broken with my version of readelf and returns
'NT_GNU_BUILD_ID'.

Signed-off-by: Roger Pau Monné <roger.pau@citrrix.com>
---
 README.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/README.md b/README.md
index b48a3df..948a7de 100644
--- a/README.md
+++ b/README.md
@@ -16,7 +16,7 @@ $ cp -r ~/src/xen ~/src/xenbuild
 $ cd ~/src/xen/xen
 $ make nconfig # Make sure to set CONFIG_LIVEPATCH=y
 $ make
-$ BUILDID=$(readelf -Wn xen-syms | awk '/Build ID:/ {print $3}')
+$ BUILDID=$(readelf -Wn xen-syms | sed -n -e 's/^.*Build ID: //p')
 ```
 
 Next, build a live patch, using a patch and the source, build ID, and
-- 
2.34.1

[PATCH 3/4] livepatch: do the initial build using CROSS_COMPILE

[Roger Pau Monne]

Setting it afterwards for further builds will cause the build logic to
detect a change and thus force a rebuild of all sources.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 livepatch-build | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/livepatch-build b/livepatch-build
index e1715ea..38a92be 100755
--- a/livepatch-build
+++ b/livepatch-build
@@ -92,7 +92,6 @@ function build_special()
     cd "${SRCDIR}" || die
 
     # Capture .o files from the patched build
-    export CROSS_COMPILE="${TOOLSDIR}/livepatch-gcc "
     export LIVEPATCH_BUILD_DIR="$(pwd)/"
     export LIVEPATCH_CAPTURE_DIR="$OUTPUT/${name}"
     mkdir -p "$LIVEPATCH_CAPTURE_DIR"
@@ -408,6 +407,8 @@ if [ "${SKIP}" != "build" ]; then
         XEN_DEBUG="debug=$XEN_DEBUG"
     fi
 
+    export CROSS_COMPILE="${TOOLSDIR}/livepatch-gcc "
+
     echo "Perform full initial build with ${CPUS} CPU(s)..."
     build_full
 
-- 
2.34.1

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

[Roger Pau Monne]

Do not attempt to modify the build system if CFLAGS are not set in
Rules.mk, and instead rely on CONFIG_LIVEPATCH already setting
-f{function,data}-sections.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
This depends on getting the patch to add -f{function,data}-sections
when using CONFIG_LIVEPATCH accepted.
---
 livepatch-build | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

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
 
     unset LIVEPATCH_BUILD_DIR
     unset LIVEPATCH_CAPTURE_DIR
-- 
2.34.1

Re: [PATCH 1/4] livepatch: update readme to mention --xen-depends

[Doebel, Bjoern]

    Fixes: b19df7b2c05e ('livepatch-build: Embed hypervisor build id into every hotpatch')
    Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
    ---
     README.md | 2 +-
     1 file changed, 1 insertion(+), 1 deletion(-)

    diff --git a/README.md b/README.md
    index 653c624..b48a3df 100644
    --- a/README.md
    +++ b/README.md
    @@ -24,7 +24,7 @@ Next, build a live patch, using a patch and the source, build ID, and
     ```
     $ cd ~/src/livepatch-build
     $ ./livepatch-build -s ~/src/xenbuild -p ~/src/xsa.patch -o out \
    -    -c ~/src/xen/xen/.config --depends $BUILDID
    +    -c ~/src/xen/xen/.config --depends $BUILDID --xen-depends $BUILDID
     Building LivePatch patch: xsa


Reviewed-by: Bjoern Doebel <doebel@amazon.de>




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH 2/4] livepatch: improve rune for fetching of Build ID

[Doebel, Bjoern]

    The current one is broken with my version of readelf and returns
    'NT_GNU_BUILD_ID'.

    Signed-off-by: Roger Pau Monné <roger.pau@citrrix.com>
    ---
     README.md | 2 +-
     1 file changed, 1 insertion(+), 1 deletion(-)

    diff --git a/README.md b/README.md
    index b48a3df..948a7de 100644
    --- a/README.md
    +++ b/README.md
    @@ -16,7 +16,7 @@ $ cp -r ~/src/xen ~/src/xenbuild
     $ cd ~/src/xen/xen
     $ make nconfig # Make sure to set CONFIG_LIVEPATCH=y
     $ make
    -$ BUILDID=$(readelf -Wn xen-syms | awk '/Build ID:/ {print $3}')
    +$ BUILDID=$(readelf -Wn xen-syms | sed -n -e 's/^.*Build ID: //p')
     ```

     Next, build a live patch, using a patch and the source, build ID, and
 

Reviewed-by: Bjoern Doebel <doebel@amazon.de>




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

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

[Doebel, Bjoern]

On 02.03.22 15:27, Roger Pau Monne wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> Do not attempt to modify the build system if CFLAGS are not set in
> Rules.mk, and instead rely on CONFIG_LIVEPATCH already setting
> -f{function,data}-sections.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> This depends on getting the patch to add -f{function,data}-sections
> when using CONFIG_LIVEPATCH accepted.
> ---
>   livepatch-build | 22 ++++++++++++++--------
>   1 file changed, 14 insertions(+), 8 deletions(-)
> 
> 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

This means we are still breaking livepatch builds for all Xen versions 
between your recent patch and the original patch that moved CFLAGS to 
xen/Makefile (commit 2740d96efdd3009f8adb40aacdbcf05cfe8d1bbb, Fri Apr 
24 14:57:10 2020 +0200).

Is this acceptable? (I mean, no one seems to have noticed...)

> +         # 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
> 
>       unset LIVEPATCH_BUILD_DIR
>       unset LIVEPATCH_CAPTURE_DIR

Bjoern



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

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

[Roger Pau Monné]

On Wed, Mar 02, 2022 at 04:41:36PM +0100, Doebel, Bjoern wrote:
> On 02.03.22 15:27, Roger Pau Monne wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > 
> > 
> > 
> > Do not attempt to modify the build system if CFLAGS are not set in
> > Rules.mk, and instead rely on CONFIG_LIVEPATCH already setting
> > -f{function,data}-sections.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > This depends on getting the patch to add -f{function,data}-sections
> > when using CONFIG_LIVEPATCH accepted.
> > ---
> >   livepatch-build | 22 ++++++++++++++--------
> >   1 file changed, 14 insertions(+), 8 deletions(-)
> > 
> > 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
> 
> This means we are still breaking livepatch builds for all Xen versions
> between your recent patch and the original patch that moved CFLAGS to
> xen/Makefile (commit 2740d96efdd3009f8adb40aacdbcf05cfe8d1bbb, Fri Apr 24
> 14:57:10 2020 +0200).
> 
> Is this acceptable? (I mean, no one seems to have noticed...)

I've also wondered. I think a reasonable approach would be for
interested parties (ie: using livepatch) to backport the commit that
sets -f{function,data}-sections in their Xen build for that broken
window.

I think we could also consider it as a candidate for backport to
stable branches, but that won't fix versions we have already
shipped.

Thanks, Roger.

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

[Andrew Cooper]

On 02/03/2022 15:41, Doebel, Bjoern wrote:
> On 02.03.22 15:27, Roger Pau Monne wrote:
>> CAUTION: This email originated from outside of the organization. Do
>> not click links or open attachments unless you can confirm the sender
>> and know the content is safe.
>>
>>
>>
>> Do not attempt to modify the build system if CFLAGS are not set in
>> Rules.mk, and instead rely on CONFIG_LIVEPATCH already setting
>> -f{function,data}-sections.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> This depends on getting the patch to add -f{function,data}-sections
>> when using CONFIG_LIVEPATCH accepted.
>> ---
>>   livepatch-build | 22 ++++++++++++++--------
>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> 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
>
> This means we are still breaking livepatch builds for all Xen versions
> between your recent patch and the original patch that moved CFLAGS to
> xen/Makefile (commit 2740d96efdd3009f8adb40aacdbcf05cfe8d1bbb, Fri Apr
> 24 14:57:10 2020 +0200).
>
> Is this acceptable? (I mean, no one seems to have noticed...)

So yeah - not putting this into OSSTest has had exactly the forseen outcome.

The steps now are to unbreak it by whatever means are necessary, then
get CI in place so we can't regress it again.

The tip of livepatch-build-tools should work with all versions of Xen. 
If this means we need to backport fixes into the staging-* branches,
then so be it.

The downstream users of livepatching have patchqueues anyway, and these
breakages are not the kind of thing which could slip though even a token
testing attempt.

As far as this issue goes, I suggest that we do what we can to avoid
livepatch-build-tools needing to do surgery like that on the build
system, then backport it everywhere.  The above is the kind of fragility
which really shouldn't exist :-/

~Andrew

Re: [PATCH 3/4] livepatch: do the initial build using CROSS_COMPILE

[Doebel, Bjoern]

On 02.03.22 15:27, Roger Pau Monne wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> Setting it afterwards for further builds will cause the build logic to
> detect a change and thus force a rebuild of all sources.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>   livepatch-build | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/livepatch-build b/livepatch-build
> index e1715ea..38a92be 100755
> --- a/livepatch-build
> +++ b/livepatch-build
> @@ -92,7 +92,6 @@ function build_special()
>       cd "${SRCDIR}" || die
> 
>       # Capture .o files from the patched build
> -    export CROSS_COMPILE="${TOOLSDIR}/livepatch-gcc "
>       export LIVEPATCH_BUILD_DIR="$(pwd)/"
>       export LIVEPATCH_CAPTURE_DIR="$OUTPUT/${name}"
>       mkdir -p "$LIVEPATCH_CAPTURE_DIR"
> @@ -408,6 +407,8 @@ if [ "${SKIP}" != "build" ]; then
>           XEN_DEBUG="debug=$XEN_DEBUG"
>       fi
> 
> +    export CROSS_COMPILE="${TOOLSDIR}/livepatch-gcc "
> +
>       echo "Perform full initial build with ${CPUS} CPU(s)..."
>       build_full
> 

Reviewed-by: Bjoern Doebel <doebel@amazon.de>



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

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

[Doebel, Bjoern]

On 02.03.22 15:27, Roger Pau Monne wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> Do not attempt to modify the build system if CFLAGS are not set in
> Rules.mk, and instead rely on CONFIG_LIVEPATCH already setting
> -f{function,data}-sections.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> This depends on getting the patch to add -f{function,data}-sections
> when using CONFIG_LIVEPATCH accepted.
> ---
>   livepatch-build | 22 ++++++++++++++--------
>   1 file changed, 14 insertions(+), 8 deletions(-)
> 
> 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
> 
>       unset LIVEPATCH_BUILD_DIR
>       unset LIVEPATCH_CAPTURE_DIR

Reviewed-by: Bjoern Doebel <doebel@amazon.de>

Confirming that I can build a livepatch against upstream master with 
your series and the other CONFIG_LIVEPATCH patch.



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH 1/4] livepatch: update readme to mention --xen-depends

[Ross Lagerwall]

> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Wednesday, March 2, 2022 2:27 PM
> To: xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>; konrad.wilk@oracle.com <konrad.wilk@oracle.com>; doebel@amazon.de <doebel@amazon.de>; julien@xen.org <julien@xen.org>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>
> Subject: [PATCH 1/4] livepatch: update readme to mention --xen-depends 
>  
> Fixes: b19df7b2c05e ('livepatch-build: Embed hypervisor build id into every hotpatch')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  README.md | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/README.md b/README.md
> index 653c624..b48a3df 100644
> --- a/README.md
> +++ b/README.md
> @@ -24,7 +24,7 @@ Next, build a live patch, using a patch and the source, build ID, and
>  ```
>  $ cd ~/src/livepatch-build
>  $ ./livepatch-build -s ~/src/xenbuild -p ~/src/xsa.patch -o out \
> -    -c ~/src/xen/xen/.config --depends $BUILDID
> +    -c ~/src/xen/xen/.config --depends $BUILDID --xen-depends $BUILDID
>  Building LivePatch patch: xsa
>  
>  Xen directory: /home/ross/src/xenbuild
> -- 
> 2.34.1

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Re: [PATCH 2/4] livepatch: improve rune for fetching of Build ID

[Ross Lagerwall]

> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Wednesday, March 2, 2022 2:27 PM
> To: xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>; konrad.wilk@oracle.com <konrad.wilk@oracle.com>; doebel@amazon.de <doebel@amazon.de>; julien@xen.org <julien@xen.org>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; Roger Pau Monné <roger.pau@citrrix.com>
> Subject: [PATCH 2/4] livepatch: improve rune for fetching of Build ID 
>  
> The current one is broken with my version of readelf and returns
> 'NT_GNU_BUILD_ID'.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrrix.com>
> ---
>  README.md | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/README.md b/README.md
> index b48a3df..948a7de 100644
> --- a/README.md
> +++ b/README.md
> @@ -16,7 +16,7 @@ $ cp -r ~/src/xen ~/src/xenbuild
>  $ cd ~/src/xen/xen
>  $ make nconfig # Make sure to set CONFIG_LIVEPATCH=y
>  $ make
> -$ BUILDID=$(readelf -Wn xen-syms | awk '/Build ID:/ {print $3}')
> +$ BUILDID=$(readelf -Wn xen-syms | sed -n -e 's/^.*Build ID: //p')
>  ```
>  
>  Next, build a live patch, using a patch and the source, build ID, and
> -- 
> 2.34.1

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Re: [PATCH 3/4] livepatch: do the initial build using CROSS_COMPILE

[Ross Lagerwall]

> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Wednesday, March 2, 2022 2:27 PM
> To: xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>; konrad.wilk@oracle.com <konrad.wilk@oracle.com>; doebel@amazon.de <doebel@amazon.de>; julien@xen.org <julien@xen.org>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>
> Subject: [PATCH 3/4] livepatch: do the initial build using CROSS_COMPILE 
>  
> Setting it afterwards for further builds will cause the build logic to
> detect a change and thus force a rebuild of all sources.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  livepatch-build | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/livepatch-build b/livepatch-build
> index e1715ea..38a92be 100755
> --- a/livepatch-build
> +++ b/livepatch-build
> @@ -92,7 +92,6 @@ function build_special()
>      cd "${SRCDIR}" || die
>  
>      # Capture .o files from the patched build
> -    export CROSS_COMPILE="${TOOLSDIR}/livepatch-gcc "
>      export LIVEPATCH_BUILD_DIR="$(pwd)/"
>      export LIVEPATCH_CAPTURE_DIR="$OUTPUT/${name}"
>      mkdir -p "$LIVEPATCH_CAPTURE_DIR"
> @@ -408,6 +407,8 @@ if [ "${SKIP}" != "build" ]; then
>          XEN_DEBUG="debug=$XEN_DEBUG"
>      fi
>  
> +    export CROSS_COMPILE="${TOOLSDIR}/livepatch-gcc "
> +
>      echo "Perform full initial build with ${CPUS} CPU(s)..."
>      build_full
>  
> -- 
> 2.34.1

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

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

[Ross Lagerwall]

> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Wednesday, March 2, 2022 2:27 PM
> To: xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>; konrad.wilk@oracle.com <konrad.wilk@oracle.com>; doebel@amazon.de <doebel@amazon.de>; julien@xen.org <julien@xen.org>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>
> Subject: [PATCH 4/4] livepatch: differentiate between old and new build systems 
>  
> Do not attempt to modify the build system if CFLAGS are not set in
> Rules.mk, and instead rely on CONFIG_LIVEPATCH already setting
> -f{function,data}-sections.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> This depends on getting the patch to add -f{function,data}-sections
> when using CONFIG_LIVEPATCH accepted.
> ---
>  livepatch-build | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> 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
>  
>      unset LIVEPATCH_BUILD_DIR
>      unset LIVEPATCH_CAPTURE_DIR
> -- 
> 2.34.1

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

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

[Andrew Cooper]

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:

if grep -q CC_SPLIT_SECTIONS Kconfig; then
    # -f{function,data}-sections set by CONFIG_LIVEPATCH
    make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log"
|| die
else
    # Support for old build system, attempt to set
-f{function,data}-sections and rebuild
    ...
fi

?
~Andrew

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

[Roger Pau Monné]

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.

Thanks, Roger.

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

[Andrew Cooper]

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.

~Andrew

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

[Roger Pau Monné]

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.

Thanks, Roger.

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

[Roger Pau Monné]

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.