From: Ard Biesheuvel <ardb@kernel.org>

Calling C code via a different mapping than it was linked at is
problematic, because the compiler assumes that RIP-relative and absolute
symbol references are interchangeable. GCC in particular may use
RIP-relative per-CPU variable references even when not using -fpic.

So call xen_prepare_pvh() via its kernel virtual mapping on x86_64, so
that those RIP-relative references produce the correct values. This
matches the pre-existing behavior for i386, which also invokes
xen_prepare_pvh() via the kernel virtual mapping before invoking
startup_32 with paging disabled again.

Fixes: 7243b93345f7 ("xen/pvh: Bootstrap PVH guest")
Tested-by: Jason Andryuk <jason.andryuk@amd.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/platform/pvh/head.S | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/platform/pvh/head.S b/arch/x86/platform/pvh/head.S
index 64fca49cd88f..98ddd552885a 100644
--- a/arch/x86/platform/pvh/head.S
+++ b/arch/x86/platform/pvh/head.S
@@ -172,7 +172,13 @@ SYM_CODE_START_LOCAL(pvh_start_xen)
 	movq %rbp, %rbx
 	subq $_pa(pvh_start_xen), %rbx
 	movq %rbx, phys_base(%rip)
-	call xen_prepare_pvh
+
+	/* Call xen_prepare_pvh() via the kernel virtual mapping */
+	leaq xen_prepare_pvh(%rip), %rax
+	addq $__START_KERNEL_map, %rax
+	ANNOTATE_RETPOLINE_SAFE
+	call *%rax
+
 	/*
 	 * Clear phys_base.  __startup_64 will *add* to its value,
 	 * so reset to 0.
-- 
2.46.0.792.g87dc391469-goog

On Thu, 26 Sept 2024 at 12:41, Ard Biesheuvel <ardb+git@google.com> wrote:
>
> From: Ard Biesheuvel <ardb@kernel.org>
>
> Calling C code via a different mapping than it was linked at is
> problematic, because the compiler assumes that RIP-relative and absolute
> symbol references are interchangeable. GCC in particular may use
> RIP-relative per-CPU variable references even when not using -fpic.
>
> So call xen_prepare_pvh() via its kernel virtual mapping on x86_64, so
> that those RIP-relative references produce the correct values. This
> matches the pre-existing behavior for i386, which also invokes
> xen_prepare_pvh() via the kernel virtual mapping before invoking
> startup_32 with paging disabled again.
>
> Fixes: 7243b93345f7 ("xen/pvh: Bootstrap PVH guest")
> Tested-by: Jason Andryuk <jason.andryuk@amd.com>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/x86/platform/pvh/head.S | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/platform/pvh/head.S b/arch/x86/platform/pvh/head.S
> index 64fca49cd88f..98ddd552885a 100644
> --- a/arch/x86/platform/pvh/head.S
> +++ b/arch/x86/platform/pvh/head.S
> @@ -172,7 +172,13 @@ SYM_CODE_START_LOCAL(pvh_start_xen)
>         movq %rbp, %rbx
>         subq $_pa(pvh_start_xen), %rbx
>         movq %rbx, phys_base(%rip)
> -       call xen_prepare_pvh
> +
> +       /* Call xen_prepare_pvh() via the kernel virtual mapping */
> +       leaq xen_prepare_pvh(%rip), %rax

Just realized that we probably need

+       subq phys_base(%rip), %rax

here (or grab phys_base from %rbx, but the above is more obvious and
less likely to get broken in the future)

> +       addq $__START_KERNEL_map, %rax
> +       ANNOTATE_RETPOLINE_SAFE
> +       call *%rax
> +
>         /*
>          * Clear phys_base.  __startup_64 will *add* to its value,
>          * so reset to 0.
> --
> 2.46.0.792.g87dc391469-goog
>

On 2024-09-26 06:55, Ard Biesheuvel wrote:
> On Thu, 26 Sept 2024 at 12:41, Ard Biesheuvel <ardb+git@google.com> wrote:
>>
>> From: Ard Biesheuvel <ardb@kernel.org>
>>
>> Calling C code via a different mapping than it was linked at is
>> problematic, because the compiler assumes that RIP-relative and absolute
>> symbol references are interchangeable. GCC in particular may use
>> RIP-relative per-CPU variable references even when not using -fpic.
>>
>> So call xen_prepare_pvh() via its kernel virtual mapping on x86_64, so
>> that those RIP-relative references produce the correct values. This
>> matches the pre-existing behavior for i386, which also invokes
>> xen_prepare_pvh() via the kernel virtual mapping before invoking
>> startup_32 with paging disabled again.
>>
>> Fixes: 7243b93345f7 ("xen/pvh: Bootstrap PVH guest")
>> Tested-by: Jason Andryuk <jason.andryuk@amd.com>
>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>> ---
>>   arch/x86/platform/pvh/head.S | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/platform/pvh/head.S b/arch/x86/platform/pvh/head.S
>> index 64fca49cd88f..98ddd552885a 100644
>> --- a/arch/x86/platform/pvh/head.S
>> +++ b/arch/x86/platform/pvh/head.S
>> @@ -172,7 +172,13 @@ SYM_CODE_START_LOCAL(pvh_start_xen)
>>          movq %rbp, %rbx
>>          subq $_pa(pvh_start_xen), %rbx
>>          movq %rbx, phys_base(%rip)
>> -       call xen_prepare_pvh
>> +
>> +       /* Call xen_prepare_pvh() via the kernel virtual mapping */
>> +       leaq xen_prepare_pvh(%rip), %rax
> 
> Just realized that we probably need
> 
> +       subq phys_base(%rip), %rax

Yes, this is necessary when phys_base is non-0.  I intended to test a 
non-0 case yesterday, but it turns out I didn't.  Re-testing, I have 
confirmed this subq is necessary.

Thanks,
Jason