Message ID | 20220822131204.25814-1-andrew.cooper3@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86/entry: Fix !PV build | expand |
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
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
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 */
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(-)