* Re: [PATCH 4/4] livepatch: differentiate between old and new build systems
2022-03-02 14:27 ` [PATCH 4/4] livepatch: differentiate between old and new build systems Roger Pau Monne
@ 2022-03-02 15:41 ` Doebel, Bjoern
2022-03-02 15:51 ` Roger Pau Monné
2022-03-02 15:55 ` Andrew Cooper
2022-03-02 16:25 ` Doebel, Bjoern
` (2 subsequent siblings)
3 siblings, 2 replies; 21+ messages in thread
From: Doebel, Bjoern @ 2022-03-02 15:41 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel
Cc: ross.lagerwall, konrad.wilk, julien, Andrew.Cooper3
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] livepatch: differentiate between old and new build systems
2022-03-02 15:41 ` Doebel, Bjoern
@ 2022-03-02 15:51 ` Roger Pau Monné
2022-03-02 15:55 ` Andrew Cooper
1 sibling, 0 replies; 21+ messages in thread
From: Roger Pau Monné @ 2022-03-02 15:51 UTC (permalink / raw)
To: Doebel, Bjoern
Cc: xen-devel, ross.lagerwall, konrad.wilk, julien, Andrew.Cooper3
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.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] livepatch: differentiate between old and new build systems
2022-03-02 15:41 ` Doebel, Bjoern
2022-03-02 15:51 ` Roger Pau Monné
@ 2022-03-02 15:55 ` Andrew Cooper
1 sibling, 0 replies; 21+ messages in thread
From: Andrew Cooper @ 2022-03-02 15:55 UTC (permalink / raw)
To: Doebel, Bjoern, Roger Pau Monne, xen-devel
Cc: Ross Lagerwall, konrad.wilk, julien
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] livepatch: differentiate between old and new build systems
2022-03-02 14:27 ` [PATCH 4/4] livepatch: differentiate between old and new build systems Roger Pau Monne
2022-03-02 15:41 ` Doebel, Bjoern
@ 2022-03-02 16:25 ` Doebel, Bjoern
2022-03-08 13:58 ` Ross Lagerwall
2022-03-08 14:38 ` Andrew Cooper
3 siblings, 0 replies; 21+ messages in thread
From: Doebel, Bjoern @ 2022-03-02 16:25 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel
Cc: ross.lagerwall, konrad.wilk, julien, Andrew.Cooper3
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] livepatch: differentiate between old and new build systems
2022-03-02 14:27 ` [PATCH 4/4] livepatch: differentiate between old and new build systems Roger Pau Monne
2022-03-02 15:41 ` Doebel, Bjoern
2022-03-02 16:25 ` Doebel, Bjoern
@ 2022-03-08 13:58 ` Ross Lagerwall
2022-03-08 14:38 ` Andrew Cooper
3 siblings, 0 replies; 21+ messages in thread
From: Ross Lagerwall @ 2022-03-08 13:58 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel; +Cc: konrad.wilk, doebel, julien, Andrew Cooper
> 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>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] livepatch: differentiate between old and new build systems
2022-03-02 14:27 ` [PATCH 4/4] livepatch: differentiate between old and new build systems Roger Pau Monne
` (2 preceding siblings ...)
2022-03-08 13:58 ` Ross Lagerwall
@ 2022-03-08 14:38 ` Andrew Cooper
2022-03-08 14:52 ` Roger Pau Monné
3 siblings, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2022-03-08 14:38 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel; +Cc: Ross Lagerwall, konrad.wilk, doebel, julien
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] livepatch: differentiate between old and new build systems
2022-03-08 14:38 ` Andrew Cooper
@ 2022-03-08 14:52 ` Roger Pau Monné
2022-03-10 18:01 ` Andrew Cooper
0 siblings, 1 reply; 21+ messages in thread
From: Roger Pau Monné @ 2022-03-08 14:52 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Ross Lagerwall, konrad.wilk, doebel, julien
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.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] livepatch: differentiate between old and new build systems
2022-03-08 14:52 ` Roger Pau Monné
@ 2022-03-10 18:01 ` Andrew Cooper
2022-03-11 8:33 ` Roger Pau Monné
0 siblings, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2022-03-10 18:01 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel, Ross Lagerwall, konrad.wilk, doebel, julien
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] livepatch: differentiate between old and new build systems
2022-03-10 18:01 ` Andrew Cooper
@ 2022-03-11 8:33 ` Roger Pau Monné
2022-03-21 10:25 ` Roger Pau Monné
0 siblings, 1 reply; 21+ messages in thread
From: Roger Pau Monné @ 2022-03-11 8:33 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Ross Lagerwall, konrad.wilk, doebel, julien
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.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/4] livepatch: differentiate between old and new build systems
2022-03-11 8:33 ` Roger Pau Monné
@ 2022-03-21 10:25 ` Roger Pau Monné
0 siblings, 0 replies; 21+ messages in thread
From: Roger Pau Monné @ 2022-03-21 10:25 UTC (permalink / raw)
To: Andrew Cooper, Ross Lagerwall, konrad.wilk; +Cc: xen-devel, doebel, julien
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.
^ permalink raw reply [flat|nested] 21+ messages in thread