Re: [PATCH] x86/livepatch: enable livepatching assembly source files

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.04.2023 11:24, Roger Pau Monne wrote:
> Some of the assembly entry points cannot be safely patched until it's
> safe to use jmp, as livepatch can replace a whole block with a jmp to
> a new address, and that won't be safe until speculative mitigations
> have been applied.

Isn't the issue only with indirect JMP, whereas livepatch uses only
direct ones?

> --- a/xen/arch/x86/include/asm/config.h
> +++ b/xen/arch/x86/include/asm/config.h
> @@ -44,6 +44,20 @@
>  /* Linkage for x86 */
>  #ifdef __ASSEMBLY__
>  #define ALIGN .align 16,0x90
> +#ifdef CONFIG_LIVEPATCH
> +#define START_LP(name)                          \
> +  jmp name;                                     \
> +  .pushsection .text.name, "ax", @progbits;     \

In how far is livepatch susceptible to two .text.* sections of the same
name? This can result here and perhaps also for static C functions.

> +  name:
> +#define END_LP(name)                            \
> +  .size name, . - name;                         \
> +  .type name, @function;                        \
> +  .popsection
> +#else
> +#define START_LP(name)                          \
> +  name:
> +#define END_LP(name)
> +#endif
>  #define ENTRY(name)                             \
>    .globl name;                                  \
>    ALIGN;                                        \

Do these really need to go into config.h, instead of e.g. asm_defns.h?
I'd prefer if stuff like this was moved out of here, rather than more
things accumulating. (Perhaps these also would better be assembler
macros, in which case asm-defns.h might be the place to put them, but I
guess that's fairly much a matter of taste.)

Couldn't END_LP() set type and size unconditionally? (But see also
below.)

> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -660,7 +660,7 @@ ENTRY(early_page_fault)
>  
>          ALIGN
>  /* No special register assumptions. */
> -restore_all_xen:
> +START_LP(restore_all_xen)
>          /*
>           * Check whether we need to switch to the per-CPU page tables, in
>           * case we return to late PV exit code (from an NMI or #MC).
> @@ -677,6 +677,7 @@ UNLIKELY_END(exit_cr3)
>  
>          RESTORE_ALL adj=8
>          iretq
> +END_LP(restore_all_xen)

While I'm fine with this conversion, ...

>  ENTRY(common_interrupt)
>          ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
> @@ -687,6 +688,7 @@ ENTRY(common_interrupt)
>          SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs, %r14=end, %rdx=0, Clob: 
> acd */
>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>  
> +START_LP(common_interrupt_lp)
>          mov   STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx
>          mov   STACK_CPUINFO_FIELD(use_pv_cr3)(%r14), %bl
>          mov   %rcx, %r15
> @@ -707,6 +709,7 @@ ENTRY(common_interrupt)
>          mov   %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
>          mov   %bl, STACK_CPUINFO_FIELD(use_pv_cr3)(%r14)
>          jmp ret_from_intr
> +END_LP(common_interrupt_lp)

... this one's odd, as it doesn't cover the entire "function". How would
you envision we sensibly add ELF metadata also for common_interrupt?

Jan