x86/entry: Fix !PV build

[PATCH] x86/entry: Fix !PV build

Posted by Andrew Cooper 1 year, 8 months ago

early_page_fault() needs to outside of #ifdef CONFIG_PV

Spotted by Gitlab CI.

Fixes: fe3f50726e87 ("x86/entry: move .init.text section higher up in the code for readability")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/x86_64/entry.S | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 9b34150bc7ab..b0efe48192d5 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -22,6 +22,17 @@
 #endif
 .endm
 
+        .section .init.text, "ax", @progbits
+ENTRY(early_page_fault)
+        ENDBR64
+        movl  $TRAP_page_fault, 4(%rsp)
+        SAVE_ALL
+        movq  %rsp, %rdi
+        call  do_early_page_fault
+        jmp   restore_all_xen
+
+        .text
+
 #ifdef CONFIG_PV
 /* %rbx: struct vcpu */
 switch_to_kernel:
@@ -140,15 +151,6 @@ process_trap:
         call create_bounce_frame
         jmp  test_all_events
 
-        .section .init.text, "ax", @progbits
-ENTRY(early_page_fault)
-        ENDBR64
-        movl  $TRAP_page_fault, 4(%rsp)
-        SAVE_ALL
-        movq  %rsp, %rdi
-        call  do_early_page_fault
-        jmp   restore_all_xen
-
         .section .text.entry, "ax", @progbits
 
 /* %rbx: struct vcpu, interrupts disabled */
-- 
2.11.0

Re: [PATCH] x86/entry: Fix !PV build

Posted by Jan Beulich 1 year, 8 months ago

On 22.08.2022 15:12, Andrew Cooper wrote:
> early_page_fault() needs to outside of #ifdef CONFIG_PV
> 
> Spotted by Gitlab CI.
> 
> Fixes: fe3f50726e87 ("x86/entry: move .init.text section higher up in the code for readability")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Makes me wonder whether the original change then really was worth it.

> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -22,6 +22,17 @@
>  #endif
>  .endm
>  
> +        .section .init.text, "ax", @progbits
> +ENTRY(early_page_fault)
> +        ENDBR64
> +        movl  $TRAP_page_fault, 4(%rsp)
> +        SAVE_ALL
> +        movq  %rsp, %rdi
> +        call  do_early_page_fault
> +        jmp   restore_all_xen
> +
> +        .text
> +
>  #ifdef CONFIG_PV
>  /* %rbx: struct vcpu */
>  switch_to_kernel:

Rather than putting it at the very top of the file, may I suggest to put
it immediately after

/* --- CODE BELOW THIS LINE (MOSTLY) NOT GUEST RELATED --- */

or yet a few more lines down between continue_pv_domain and
restore_all_xen? Which, as a minor gain, then also doesn't require you
to add a new .text (or other section) directive. Preferably that way
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH] x86/entry: Fix !PV build

Posted by Andrew Cooper 1 year, 8 months ago

On 22/08/2022 14:32, Jan Beulich wrote:
> On 22.08.2022 15:12, Andrew Cooper wrote:
>> early_page_fault() needs to outside of #ifdef CONFIG_PV
>>
>> Spotted by Gitlab CI.
>>
>> Fixes: fe3f50726e87 ("x86/entry: move .init.text section higher up in the code for readability")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Makes me wonder whether the original change then really was worth it.

It was, IMO.  In it's previous location, it was a single area of non
.text.entry hidden in a large .text.entry block.

>
>> --- a/xen/arch/x86/x86_64/entry.S
>> +++ b/xen/arch/x86/x86_64/entry.S
>> @@ -22,6 +22,17 @@
>>  #endif
>>  .endm
>>  
>> +        .section .init.text, "ax", @progbits
>> +ENTRY(early_page_fault)
>> +        ENDBR64
>> +        movl  $TRAP_page_fault, 4(%rsp)
>> +        SAVE_ALL
>> +        movq  %rsp, %rdi
>> +        call  do_early_page_fault
>> +        jmp   restore_all_xen
>> +
>> +        .text
>> +
>>  #ifdef CONFIG_PV
>>  /* %rbx: struct vcpu */
>>  switch_to_kernel:
> Rather than putting it at the very top of the file, may I suggest to put
> it immediately after
>
> /* --- CODE BELOW THIS LINE (MOSTLY) NOT GUEST RELATED --- */
>
> or yet a few more lines down between continue_pv_domain and
> restore_all_xen? Which, as a minor gain, then also doesn't require you
> to add a new .text (or other section) directive. Preferably that way
> Acked-by: Jan Beulich <jbeulich@suse.com>

Done.  Thanks.

~Andrew