[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2] livepatch: set -f{function,data}-sections compiler option


  • To: Julien Grall <julien@xxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 8 Mar 2022 09:13:30 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=GmdmBT8xwaAp+5PvvSMSf2AOX8Aw+VXBSXod15KL0aQ=; b=HSEg/dNeOWQE/Teq97y7urPxKdjLFTDq9UMtDFO9JSV410K0ebubsaw7YgZ4mQ/l2NT/HT5zXlTvGWiQamFGaczR7FoUcsjsFaq5bYUzvhUVmdCdV+A013vtENjbxfeBhPWKwMnvF8CCt/Crqla1Vbr5CPk/KRyAWhjTY4ZmY5c6qpAlW+bTD4v//jrFY0nn//cLx13dqrt9HgkTahtJwd909ehqzy5EET0LEwCVJ7xOtNzLBJJ/bWCRg5tpPC805TVAe2im439HvncTNhh8xh5VSruYEOmB51dZz4j+faHNFUOLh5P+63ZOsnSMvWqO5czNGTjiUkdLTRlLKOIafw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Bbj/nbEoXhRgsGx+G/bIQ0jXvlhx93gB/5xHR96gX5nz04594OKf2aIP+SnsQTPcgWTgOUjbUQKLPD+kKAyGUOUdQSLMJfAGMDzYX6H4TSNP14AoTF0LBTcieJXgialYZkUs0L8zynt/qlx8asE7rdkGidrTFKpd2nKsWogmjgHt6lxIrYbl8wPkshcQ0xwYvSxNXr2oV87AE9bjbRCijV1rFUsc/q6KSdPBUcDwKO7yxCWtSAj+Y3Lg7xi3sEb4R9IOUJMTRJuDD5Qs+sgognzDs1A2HAjid8CZXYYIM7tghp+DwWwr8tB4jR+yCdtw7FKm9XT2Ejg+qN2IqOYKMA==
  • Authentication-results: esa2.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Tue, 08 Mar 2022 08:13:51 +0000
  • Ironport-data: A9a23:q4sBOqs8pvv8O6UNnZFg8ffAL+fnVKJeMUV32f8akzHdYApBsoF/q tZmKWqAaKneNGv2eYt1Pti2pB4O6pfXyYJgTVNv+C9gESpG+JbJXdiXEBz9bniYRiHhoOOLz Cm8hv3odp1coqr0/0/1WlTZhSAgk/nOHNIQMcacUsxLbVYMpBwJ1FQyw4bVvqYy2YLjW1jV5 oupyyHiEATNNwBcYzp8B52r8HuDjNyq0N/PlgVjDRzjlAa2e0g9VPrzF4noR5fLatA88tqBb /TC1NmEElbxpH/BPD8HfoHTKSXmSpaKVeSHZ+E/t6KK2nCurQRquko32WZ1he66RFxlkvgoo Oihu6BcRi8CHfT8peZeeSIHFg9iMu4c35/1HEmW5Jn7I03uKxMAwt1rBUAye4YZ5vx2ESdF8 vlwxDIlN07ZwbjsmfTiF7cq1p9LwMrDZevzvll6yj7UF7A+SI3rSKTW/95Imjw3g6iiGN6AO pRIMWM+MHwsZTVoMEs4VLIfnNymg3jOXXp08HOvprM4tj27IAtZj+G2bYu9lsaxbdpRtlaVo CTB5WuRKhsXLsCFwDyJtHelnPbSnDjTUZgXUra/85ZCvlqXwWACDQwMYnGyq/K5l02WVspWL gof/S9GhbMp6EWhQ935Xhu5iH2JpBgRX5xXCeJSwAOQzqvZ5S6JC25CSSROAPQ2uclzSTE02 1uhm9LyGScpoLCTUWia9LqfsXW1Iyd9BW0IaDIATAAFy8L+u4x1hRXKJuuPC4bs0IezQ2uph WnX8m5u3N3/kPLnyY2Q3gn3vjDvjKOWSyAp6UaNeUKX0zJQMdvNi5OT1XDX6vNJLYC8R1aHv WQZl8X20N3iHa1hhwTWHrxTQejBC+KtdWSF3AUxR8VJGyGFpib7Fb289g2SM6uA3iwsXTbyK HHetgpKjHO4FCv7NPQnC25d5ilD8EQBKTgHfq2MBjatSsIoHONiwM2ITRTIt4wKuBJw+ZzTw b/BLa6R4Y8yUMyLNgaeSeYHyqMMzSsj327VTp2T5035jeTAOy7IEu5VbQDmggUFAEWs+l69H zF3bZfi9vmieLemPnm/HXA7dzjm0kTX9bip8pcKJ4Zv0yJtGX07Cu+5/F/SU9cNokihrc+Rp ivVchYBkDLX3CSbQS3XOiELQO6+Bv5X8CNkVRHAyH71ghDPl670t/xBH3b2FJF6nNFeIQlcE 6FVK5/eU64UFlwqOV01NPHAkWCrTzzy7SqmNCu5ejkvOZlmQg3C4Nj/eQXzsiIJC0KKWQEW+ tVMCiuzrUI/ejlf
  • Ironport-hdrordr: A9a23:GagXMq8mAdZxiCzh639uk+AJI+orL9Y04lQ7vn2ZLiYlFfBw9v re+MjzsCWftN9/Yh8dcLy7WZVoIkmskKKdg7NhXotKNTOO0ADEQO5fBODZsl/d8kPFltJ15O NYaK55B8T3DV9myejHwCTQKadH/PC3tJmyg+HQ1nFsShwvTZpBwUNWNia3e3cGPTWvI/ICZe GhDw581kKdRUg=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Mon, Mar 07, 2022 at 05:19:53PM +0000, Julien Grall wrote:
> Hi Roger,
> 
> On 07/03/2022 15:55, Roger Pau Monne wrote:
> > If livepatching support is enabled build the hypervisor with
> > -f{function,data}-sections compiler options, which is required by the
> > livepatching tools to detect changes and create livepatches.
> > 
> > This shouldn't result in any functional change on the hypervisor
> > binary image, but does however require some changes in the linker
> > script in order to handle that each function and data item will now be
> > placed into its own section in object files. As a result add catch-all
> > for .text, .data and .bss in order to merge each individual item
> > section into the final image.
> > 
> > The main difference will be that .text.startup will end up being part
> > of .text rather than .init, and thus won't be freed. .text.exit will
> > also be part of .text rather than dropped. Overall this could make the
> > image bigger, and package some .text code in a sub-optimal way.
> > 
> > Note that placement of the sections inside of .text is also slightly
> > adjusted to be more similar to the position found in the default GNU
> > ld linker script. This requires having a separate section for the
> > header in order to place it at the begging of the output image,
> > followed with the unlikely and related sections, and finally the main
> > .text section.
> > 
> > On Arm the .data.read_mostly needs to be moved ahead of the .data
> > section like it's already done on x86, and the alignment needs to be
> > set to PAGE_SIZE so it doesn't end up being placed at the tail of a
> > read-only page from the previous section. While there move the
> > alignment of the .data section ahead of the section declaration, like
> > it's done for other sections.
> 
> This sounds like a bug not related to this patch. Can this be split
> separately?

No, .data.read_mostly needs to be moved before .data so that the catch
all .data.* introduced to .data to account for -fdata-sections doesn't
end up also including .data.read_mostly.

> > 
> > The benefit of having CONFIG_LIVEPATCH enable those compiler option
> > is that the livepatch build tools no longer need to fiddle with the
> > build system in order to enable them. Note the current livepatch tools
> > are broken after the recent build changes due to the way they
> > attempt to set  -f{function,data}-sections.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > Changes since v1:
> >   - Introduce CC_SPLIT_SECTIONS for selecting the compiler options.
> >   - Drop check for compiler options, all supported versions have them.
> >   - Re-arrange section placement in .text, to match the default linker
> >     script.
> >   - Introduce .text.header to contain the headers bits that must appear
> >     first in the final binary.
> > ---
> > Given that now the header is explicitly placed in .text.header, it's
> > likely possible to simplify some of the ordering of the object files
> > for the prelink.o generation, as arch/x86/boot/built_in.o no longer
> > needs to be the first object file in the list.
> > 
> > It also seems on Arm the schedulers and hypfs .data sections should be
> > moved into read_mostly.
> > ---
> > Tested by gitlab in order to assert I didn't introduce any regression
> > on Arm specially.
> > ---
> >   xen/Makefile              |  2 ++
> >   xen/arch/arm/arm32/head.S |  1 +
> >   xen/arch/arm/arm64/head.S |  1 +
> >   xen/arch/arm/xen.lds.S    | 49 +++++++++++++++++++++------------------
> >   xen/arch/x86/boot/head.S  |  2 +-
> >   xen/arch/x86/xen.lds.S    | 22 +++++++++++-------
> >   xen/common/Kconfig        |  4 ++++
> >   7 files changed, 49 insertions(+), 32 deletions(-)
> > 
> > diff --git a/xen/Makefile b/xen/Makefile
> > index 5c21492d6f..18a4f7e101 100644
> > --- a/xen/Makefile
> > +++ b/xen/Makefile
> > @@ -273,6 +273,8 @@ else
> >   CFLAGS += -fomit-frame-pointer
> >   endif
> > +CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections
> > +
> >   CFLAGS += -nostdinc -fno-builtin -fno-common
> >   CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
> >   $(call cc-option-add,CFLAGS,CC,-Wvla)
> > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> > index 7a906167ef..c837d3054c 100644
> > --- a/xen/arch/arm/arm32/head.S
> > +++ b/xen/arch/arm/arm32/head.S
> > @@ -120,6 +120,7 @@
> >   #endif /* !CONFIG_EARLY_PRINTK */
> > +        .section .text.header, "ax", %progbits
> >           .arm
> >           /*
> > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > index 66d862fc81..e62c48ec1c 100644
> > --- a/xen/arch/arm/arm64/head.S
> > +++ b/xen/arch/arm/arm64/head.S
> > @@ -133,6 +133,7 @@
> >           add \xb, \xb, x20
> >   .endm
> > +        .section .text.header, "ax", %progbits
> >           /*.aarch64*/
> >           /*
> > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> > index 08016948ab..836da880c3 100644
> > --- a/xen/arch/arm/xen.lds.S
> > +++ b/xen/arch/arm/xen.lds.S
> > @@ -30,9 +30,16 @@ SECTIONS
> >     _start = .;
> >     .text : {
> >           _stext = .;            /* Text section */
> > +       *(.text.header)
> 
> With this change, the changes in arch/*/arch.mk to order head.o looks
> unnecessary. Can we remove it at the same time? (The .text.header and the
> update of arch.mk may want to be done together in a separate patch).

I had a note below the commit message about this, as I didn't think it
was strictly necessary to get this accepted. I will do a pre-patch
then with those added.

Thanks, Roger.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.