[PATCH v2 09/11] x86/efi: avoid a relocation in efi_arch_post_exit_boot()

Roger Pau Monne posted 11 patches 7 months ago
[PATCH v2 09/11] x86/efi: avoid a relocation in efi_arch_post_exit_boot()
Posted by Roger Pau Monne 7 months ago
Instead of using the absolute __start_xen address, calculate it as an
offset from the current instruction pointer.  The relocation would be
problematic if the loader has acknowledged the Xen image section
attributes, and mapped .init.text with just read and execute permissions.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/efi/efi-boot.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 1d8902a9a724..c5cbf56cc0c4 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -266,7 +266,9 @@ static void __init noreturn efi_arch_post_exit_boot(void)
 
                    /* Jump to higher mappings. */
                    "mov    stack_start(%%rip), %%rsp\n\t"
-                   "movabs $__start_xen, %[rip]\n\t"
+                   "lea    __start_xen(%%rip), %[rip]\n\t"
+                   "add    %[offset], %[rip]\n\t"
+
                    "push   %[cs]\n\t"
                    "push   %[rip]\n\t"
                    "lretq"
@@ -274,7 +276,8 @@ static void __init noreturn efi_arch_post_exit_boot(void)
                      [cr4] "+&r" (cr4)
                    : [cr3] "r" (idle_pg_table),
                      [cs] "i" (__HYPERVISOR_CS),
-                     [ds] "r" (__HYPERVISOR_DS)
+                     [ds] "r" (__HYPERVISOR_DS),
+                     [offset] "r" (__XEN_VIRT_START - xen_phys_start)
                    : "memory" );
     unreachable();
 }
-- 
2.48.1


Re: [PATCH v2 09/11] x86/efi: avoid a relocation in efi_arch_post_exit_boot()
Posted by Jan Beulich 7 months ago
On 01.04.2025 15:08, Roger Pau Monne wrote:
> Instead of using the absolute __start_xen address, calculate it as an
> offset from the current instruction pointer.  The relocation would be
> problematic if the loader has acknowledged the Xen image section
> attributes, and mapped .init.text with just read and execute permissions.

Fine in principle, but ...

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -266,7 +266,9 @@ static void __init noreturn efi_arch_post_exit_boot(void)
>  
>                     /* Jump to higher mappings. */
>                     "mov    stack_start(%%rip), %%rsp\n\t"
> -                   "movabs $__start_xen, %[rip]\n\t"
> +                   "lea    __start_xen(%%rip), %[rip]\n\t"
> +                   "add    %[offset], %[rip]\n\t"
> +
>                     "push   %[cs]\n\t"
>                     "push   %[rip]\n\t"
>                     "lretq"
> @@ -274,7 +276,8 @@ static void __init noreturn efi_arch_post_exit_boot(void)
>                       [cr4] "+&r" (cr4)
>                     : [cr3] "r" (idle_pg_table),
>                       [cs] "i" (__HYPERVISOR_CS),
> -                     [ds] "r" (__HYPERVISOR_DS)
> +                     [ds] "r" (__HYPERVISOR_DS),
> +                     [offset] "r" (__XEN_VIRT_START - xen_phys_start)
>                     : "memory" );
>      unreachable();
>  }

... imo ought to come with a brief comment, to keep people from trying to
undo ("simplify") that again.

[offset]'s constraint could in principle be "rme", I think, as [rip] is
"&r" already. Just that the compiler (at least gcc) won't synthesize a
memory operand, and the value can't be expressed by an immediate. IOW -
probably all fine with just "r". Of course if/when we add further operands
here, we need to pay attention to the number of registers needed.

Jan