On Tue, Feb 27, 2024 at 09:53:24AM +0100, Jan Beulich wrote: > On 27.02.2024 09:15, Roger Pau Monné wrote: > > On Mon, Feb 26, 2024 at 01:36:32PM +0100, Jan Beulich wrote: > >> On 26.02.2024 12:32, Roger Pau Monné wrote: > >>> On Tue, Feb 13, 2024 at 04:58:38PM +0100, Jan Beulich wrote: > >>>> On 07.02.2024 15:55, Roger Pau Monne wrote: > >>>>> The minimal function size requirements for an x86 livepatch are either > >>>>> 5 bytes > >>>>> (for jmp) or 9 bytes (for endbr + jmp), and always 4 bytes on Arm. > >>>>> Ensure that > >>>>> distance between functions entry points is always at least of the > >>>>> minimal > >>>>> required size for livepatch instruction replacement to be successful. > >>>>> > >>>>> Add an additional align directive to the linker scripts, in order to > >>>>> ensure that > >>>>> the next section placed after the .text.* (per-function sections) is > >>>>> also > >>>>> aligned to the required boundary, so that the distance of the last > >>>>> function > >>>>> entry point with the next symbol is also of minimal size. > >>>> > >>>> Perhaps "... minimal required size"? > >>> > >>> Yes. > >>> > >>>>> --- a/xen/common/Kconfig > >>>>> +++ b/xen/common/Kconfig > >>>>> @@ -395,8 +395,11 @@ config CRYPTO > >>>>> config LIVEPATCH > >>>>> bool "Live patching support" > >>>>> default X86 > >>>>> - depends on "$(XEN_HAS_BUILD_ID)" = "y" > >>>>> + depends on "$(XEN_HAS_BUILD_ID)" = "y" && > >>>>> CC_HAS_FUNCTION_ALIGNMENT > >>>>> select CC_SPLIT_SECTIONS > >>>>> + select FUNCTION_ALIGNMENT_16B if XEN_IBT > >>>>> + select FUNCTION_ALIGNMENT_8B if X86 > >>>>> + select FUNCTION_ALIGNMENT_4B if ARM > >>>> > >>>> This isn't strictly needed, is it? Would be nice to avoid re-selection > >>>> of what the default for an arch is anyway, as otherwise this will start > >>>> looking clumsy when a couple more architectures are added. > >>> > >>> My worry was that the default per-arch could change, ie: for example > >>> x86 moving from 16 to 8 and then it would hamper livepatch support if > >>> IBT is also enabled. I however think it's very unlikely to reduce the > >>> default alignment, and in any case we would hit a build time assert if > >>> that ever happens. > >>> > >>> So yes, I'm fine with dropping those. > >> > >> Oh, no - not "those", only "that", i.e. only the last (Arm) one. > > > > Oh, I see what you mean, even x86 selects the default one when IBT is > > enabled, and when not the requirement for livepatch is < than the > > default anyway. That's why I said that we could even drop all of them > > and just rely on the build time assert to catch any changes here. > > Just to clarify: The default I mean is the architecture imposed one. > Leaving aside Thumb mode, Arm instructions are all 32-bit words, and > hence less than 4-byte alignment makes no sense (and may even be > disallowed by the architecture). Whereas for x86 what you're talking > about is just a compiler default, which isn't really guaranteed to > never be lower (with -Os for example I'd expect it to be perhaps as > low as 1).
Right, it's a compiler default, but in patch 1 we already set the default alignment for x86 to be 16 bytes. When in your first comment you mentioned "... default for an arch is anyway" I assumed you mean the default in the arch Kconfig file, not what the ISA mandates. Thanks, Roger.
