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

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>, Roger Pau Monne <roger.pau@xxxxxxxxxx>
  • From: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
  • Date: Wed, 9 Mar 2022 19:06:33 +0000
  • Accept-language: en-GB, en-US
  • 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=3ko7aNu7ldzqTgsm0KgJdKRF90vvCA4Ha2JKLEi48ZE=; b=Yp3f9ISL6JZUSHRdncHh6e4J78bPs4FfO3YfM504Jx3lErQn+RWG82jWv7X8ymHzIkDnRwIxg8x7c6X0e6PrQ5cnRxMzSGRLOPCCFY6U6gwLPW3fS4Asl5VFBKqgK88N6pywOoAlON1kyE/FyENANSNGTHI0ptbt3ey0RpQZQ66L8kQFFWV4jusw02HQ+6zv+Wo5k2SZLVJ7I5TlkHhC6W0n5PO694H2O8pNx/05WsYSPFOfC2XxjNgjVj1tRxYmAbqiwAjMFu942lsFWqcYc1dFyauJqJZ65DHMPkh4kSAxsb3ybhf5riDlktgwbUjpMKWZ4RBmQfP08fmoaPwXXg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=bLz1yH1M9QwkzrcxDTXDzNJLrB7pxODPdKWchjmYieCbNh3UVfffMEBh9p+aVGh2UU0p+8L2XCiExsbKaEIkL6BzT/HzeFbn4/jEZydSZUsnC4x9hfRRHpLnGb0B97AbtyXoe94x9D9HvpWieKZCNCqxV3ifSL/pC82f7gV8CemEI/WOw+86fAWhITEDFjctW1udIyE7C8kMURHLcFMSqY+pudmNj6z1QHVEDqrgC/1L+Az6JTCJsp3x7wFues2XFTkb/vkvxKKIviVctH/IZmDRphGstkN7C24vSNq4GNQot3jEmYJEpV0vJbTvxMxgxhbEKd1LCiQmkWWU3NQeKA==
  • Authentication-results: esa3.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: George Dunlap <George.Dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, "Bertrand Marquis" <bertrand.marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Wed, 09 Mar 2022 19:06:47 +0000
  • Ironport-data: A9a23:/PBd5q3Q4BEDl+oDC/bD5SBxkn2cJEfYwER7XKvMYLTBsI5bpzYPz mBLXD+OOa7fZ2TxfIh1a4mxpkwC65OHnYU1S1Q+pC1hF35El5HIVI+TRqvS04J+DSFhoGZPt Zh2hgzodZhsJpPkjk7xdOCn9xGQ7InQLlbGILes1htZGEk1EE/NtTo5w7Rj2tUw0IDia++wk YiaT/P3aQfNNwFcagr424rbwP+4lK2v0N+wlgVWicFj5DcypVFMZH4sDfjZw0/DaptVBoaHq 9Prl9lVyI97EyAFUbtJmp6jGqEDryW70QKm0hK6UID66vROS7BbPg/W+5PwZG8O4whlkeydx /1SrbqIezUwMJbwidsRdwYJDRlEMfZJreqvzXiX6aR/zmXDenrohf5vEFs3LcsT/eMf7WNmr KJCbmpXN1ba2rzwkOnTpupE36zPKOHCOo8Ft24m5jbeFfs8GrjIQrnQ5M8e1zA17ixLNaiDN 5ZIMGQ3BPjGSx1SE3AxLY4Hp96L22XZeBlEl16Z/INitgA/yyQuieOwYbI5YOeiT8hPglyRo G6A+m3jGwwbL/SW0z/D+XWp7sfxmif8VJMXBaeP3Pdgi12OxUQeEBQTE1C8pJGRmkO4Ht5SN UEQ0i4vtrQpslymSMHnWB+1q2LCuQQTM/JPF8Uq5QfLzbDbiy6bDGUZSj9KaPQ9qdQ7Azct0 zehnc7tBDFpmK2YTzSa7Lj8hSipJSEfIGsGZCkFZQgI+d/upMc0lB2nczp4OPfr1JuvQ2i2m m3U6nhl71kOsSIV/7qj22j1sSuinaTYcQIR+VTJZliptQwsMeZJeLeUwVTc6P9BKqOQQV+Ao GUIlqCi0QweMX2evHfTGbtQRdlF897AaWSB2gA3Q/HN4hzwoybLQGxG3N1pyK6F2O4gcCShX kLcsBg5CHR7bCrzNv8fj25c5q0XIUnc+TbNC6i8gjlmOMEZmOq7EMdGPxD4M4fFyhRErE3HE c3HGftA9F5DYUid8BK4Rv0GzZggzT0kyGXYSPjTlkr7j+XAOCTFFetZbjNii9zVCove8G05F P4Fa6O3J+h3CrWiMkE7D6ZIRbz1EZTLLc+v8JEGHgJyCgFnBHsgG5fsLUAJIORYc1Buvr6Qp BmVAxYAoHKm3CGvAVjaOxhLNeK0Nb4i/C1TAMDZFQvxs5TVSd30t/l3mlpeVeRPydGPOtYvF qhbIZrcWqoTItkFkhxEBaTAQEVZXE3DrSqFPja/YSh5eJhlRgfT/cTjcBep/y4LZhdbf+Nly 1F8/ms3maY+ejk=
  • Ironport-hdrordr: A9a23:vUc6zqjvS8kC4MCXvKvrhW8aX3BQX/p13DAbv31ZSRFFG/FwyP rBoB1L73DJYWgqNE3I+Orwc5VoLkmsk6KdjbNhXotKPzOW8ldATrsSlLcKqgeIc0aVm4886U 4JSdk9NDSaNykesS+O2njeLz9W+qjizEnHv5a9855Fd3AQV4hQqyNCTiqLGEx/QwdLQbAjEo CH28ZBrz28PVwKc8WSHBA+LqX+juyOsKijTQ8NBhYh5gXLpyiv8qTGHx+R2Qpbey9TwI0l7X POn2XCl+meWrCAu1DhPl3ontVrcejau5t+7fm3+4Yowm+FsHfTWG0uYczAgNl/mpDW1L9jqq iwn/5nBbU315qZRBDInfO5szOQrgoG+jvsz0SVjmDkptG8TDUmC9BZjYYcaRfB7VE81esMmJ 6j8ljpwaa/Nymw1RgVJuK4Ii1Chw6xuz4vgOQTh3tQXc8Xb6JQt5UW+AdPHJIJDEvBmfca+M EHNrCs2B5+GWnqEEwxflMftOBEck5DbStuGHJyyvB9+wIm7kxE8w==
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHYMvNxmieyqH8SX0SDccReVIf6jKy1hkuAgAAKagCAAAelgIAAGJiAgAAEt4CAARULAIAADp6AgACSWgA=
  • Thread-topic: [PATCH v3 2/2] livepatch: set -f{function,data}-sections compiler option

On 09/03/2022 10:22, Jan Beulich wrote:
> On 09.03.2022 10:30, Roger Pau Monné wrote:
>> On Tue, Mar 08, 2022 at 05:58:49PM +0100, Jan Beulich wrote:
>>> On 08.03.2022 17:41, Roger Pau Monné wrote:
>>>> On Tue, Mar 08, 2022 at 04:13:55PM +0100, Jan Beulich wrote:
>>>>> On 08.03.2022 15:46, Roger Pau Monné wrote:
>>>>>> On Tue, Mar 08, 2022 at 03:09:17PM +0100, Jan Beulich wrote:
>>>>>>> On 08.03.2022 14:49, 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.
>>>>>>>>
>>>>>>>> On Arm the .data.read_mostly needs to be moved ahead of the .data
>>>>>>>> section like it's already done on x86, so the .data.* catch-all
>>>>>>>> doesn't also include .data.read_mostly. The alignment of
>>>>>>>> .data.read_mostly also 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.
>>>>>>>>
>>>>>>>> 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>
>>>>>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>>>>
>>>>>>>> --- a/xen/arch/x86/xen.lds.S
>>>>>>>> +++ b/xen/arch/x86/xen.lds.S
>>>>>>>> @@ -88,6 +88,9 @@ SECTIONS
>>>>>>>>         *(.text.unlikely .text.*_unlikely .text.unlikely.*)
>>>>>>>>  
>>>>>>>>         *(.text)
>>>>>>>> +#ifdef CONFIG_CC_SPLIT_SECTIONS
>>>>>>>> +       *(.text.*)
>>>>>>>> +#endif
>>>>>>>>         *(.text.__x86_indirect_thunk_*)
>>>>>>>>         *(.text.page_aligned)
>>>>>>> These last two now will not have any effect anymore when
>>>>>>> CC_SPLIT_SECTIONS=y. This may have undesirable effects on the
>>>>>>> overall size when there is more than one object with a
>>>>>>> .text.page_aligned contribution. In .data ...
>>>>>> Agreed. I wondered whether to move those ahead of the main text
>>>>>> section, so likely:
>>>>>>
>>>>>>        *(.text.unlikely .text.*_unlikely .text.unlikely.*)
>>>>>>
>>>>>>        *(.text.page_aligned)
>>>>>>        *(.text.__x86_indirect_thunk_*)
>>>>>>        *(.text)
>>>>>> #ifdef CONFIG_CC_SPLIT_SECTIONS
>>>>>>        *(.text.*)
>>>>>> #endif
>>>>> Perhaps; I'm not really worried of .text.__x86_indirect_thunk_*,
>>>>> though. When adding .text.* that one can likely go away.
>>>>>
>>>>>> FWIW, Linux seems fine to package .text.page_aligned together with the
>>>>>> rest of .text using the .text.[0-9a-zA-Z_]* catch-all.
>>>>> There's no question this is functionally fine. The question is how
>>>>> many extra padding areas are inserted because of this.
>>>>>
>>>>>>>> @@ -292,9 +295,7 @@ SECTIONS
>>>>>>>>  
>>>>>>>>    DECL_SECTION(.data) {
>>>>>>>>         *(.data.page_aligned)
>>>>>>>> -       *(.data)
>>>>>>>> -       *(.data.rel)
>>>>>>>> -       *(.data.rel.*)
>>>>>>>> +       *(.data .data.*)
>>>>>>>>    } PHDR(text)
>>>>>>> ... this continues to be named first. I wonder whether we wouldn't
>>>>>>> want to use SORT_BY_ALIGNMENT (if available) instead in both places.
>>>>>> We could use the command line option if available
>>>>>> (--sort-section=alignment) to sort all wildcard sections?
>>>>> Depends on the scope of the sorting that would result when enabled
>>>>> globally like this.
>>>> I'm not sure I'm following. Don't we generally want to sort by
>>>> alignment in order to avoid adding unnecessary padding as much as
>>>> possible?
>>>>
>>>> For any wildcard sections we really don't care anymore how they are
>>>> sorted?
>>> Sure. Question is whether sorting is limited to within any single
>>> *(...) construct, or whether it could extend to adjacent ones. IOW
>>> whether the command line option strictly is a replacement of adding
>>> SORT_BY_ALIGNMENT to every one of these constructs.
>> AFAICT the command line option will have the effect of setting the
>> sorting of any wildcard containing sections to use SORT_BY_ALIGNMENT.
>> Ie: .data.* would become SORT_BY_ALIGNMENT(.data.*):
>>
>> *(.data SORT_BY_ALIGNMENT(.data.*))
>>
>> I've taken a look at the binutils ld source and that seems to be the
>> case, any wildcard containing enum will get it's sorting set to by
>> alignment (but I'm not familiar with ld code so I might be missing
>> pieces).
> Okay - why don't we try that then (in a separate patch, so it's going
> to be easy to revert)? For the patch here all I'd like to ask for is
> to keep .text.page_aligned enumerated explicitly (and the wildcard
> placed after it, obviously).

I'm not convinced this will be an improvement.  It will make a marginal
space saving, but cost runtime performance by breaking
locality-of-reference inside a TU.

What would make an improvement (if this isn't how it already works) is
having each TU sort by alignment on its own, then link in order.

~Andrew

 


Rackspace

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