| Message ID | 20240905130657.4075906-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/trampoline: Header cleanup | expand |
On 05.09.2024 15:06, Andrew Cooper wrote: > kbd_shift_flags seems especially dubious. It's a snapshot of the keyboard > state when Xen happened to pass through the trampoline, and surely cannot be > useful for dom0 in the slightest... No more or less than if the kernel takes such a snapshot while booting natively. Whatever this is used for there (it's been too long - I don't recall). > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -102,9 +102,6 @@ static void __init efi_arch_relocate_image(unsigned long delta) > } > } > > -extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[]; > -extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[]; I'd prefer if these stayed here, leaving the 4 symbols as minimally exposed as possible. Recall that efi-boot.h isn't really a header in that sense, but rather a .c file. Elsewhere we keep decls in .c files when they're used in just one CU. Jan
On 05/09/2024 4:42 pm, Jan Beulich wrote: > On 05.09.2024 15:06, Andrew Cooper wrote: >> --- a/xen/arch/x86/efi/efi-boot.h >> +++ b/xen/arch/x86/efi/efi-boot.h >> @@ -102,9 +102,6 @@ static void __init efi_arch_relocate_image(unsigned long delta) >> } >> } >> >> -extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[]; >> -extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[]; > I'd prefer if these stayed here, leaving the 4 symbols as minimally exposed as > possible. Recall that efi-boot.h isn't really a header in that sense, but > rather a .c file. Elsewhere we keep decls in .c files when they're used in just > one CU. See Frediano's RFC series, which needs to change this in order to move the 32bit relocation logic from asm to C. The only reason efi-boot.h can get away with this right now is because the other logic is written entirely in asm. Scope-limiting linker section boundaries more than regular variables is weird to me. It's not as if they magically take more care to use than regular variables, and trampoline.h is not a wide scope by any means. ~Andrew
On Thu, Sep 5, 2024 at 5:10 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 05/09/2024 4:42 pm, Jan Beulich wrote: > > On 05.09.2024 15:06, Andrew Cooper wrote: > >> --- a/xen/arch/x86/efi/efi-boot.h > >> +++ b/xen/arch/x86/efi/efi-boot.h > >> @@ -102,9 +102,6 @@ static void __init efi_arch_relocate_image(unsigned > long delta) > >> } > >> } > >> > >> -extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[]; > >> -extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[]; > > I'd prefer if these stayed here, leaving the 4 symbols as minimally > exposed as > > possible. Recall that efi-boot.h isn't really a header in that sense, but > > rather a .c file. Elsewhere we keep decls in .c files when they're used > in just > > one CU. > > See Frediano's RFC series, which needs to change this in order to move > the 32bit relocation logic from asm to C. > > Not strictly necessary, I can declare in the C file as they were declared in efi-boot.h (which is more a .c file as an header as Jan said). I think the idea of declaring into a source file is that if another file wants to use it has to declare it again, so a bit more friction. But any access to trampoline variables should be considered as something to limit in any case, so having in a separate header helps (this does not mean that removing from the header is still increasing the friction). Personally, I'm not strong about the 2 options here. Slightly prefer having all variable declared in a single header. The only reason efi-boot.h can get away with this right now is because > the other logic is written entirely in asm. > > > Scope-limiting linker section boundaries more than regular variables is > weird to me. It's not as if they magically take more care to use than > regular variables, and trampoline.h is not a wide scope by any means. > > Frediano
On 05/09/2024 5:34 pm, Frediano Ziglio wrote: > On Thu, Sep 5, 2024 at 5:10 PM Andrew Cooper > <andrew.cooper3@citrix.com> wrote: > > On 05/09/2024 4:42 pm, Jan Beulich wrote: > > On 05.09.2024 15:06, Andrew Cooper wrote: > >> --- a/xen/arch/x86/efi/efi-boot.h > >> +++ b/xen/arch/x86/efi/efi-boot.h > >> @@ -102,9 +102,6 @@ static void __init > efi_arch_relocate_image(unsigned long delta) > >> } > >> } > >> > >> -extern const s32 __trampoline_rel_start[], > __trampoline_rel_stop[]; > >> -extern const s32 __trampoline_seg_start[], > __trampoline_seg_stop[]; > > I'd prefer if these stayed here, leaving the 4 symbols as > minimally exposed as > > possible. Recall that efi-boot.h isn't really a header in that > sense, but > > rather a .c file. Elsewhere we keep decls in .c files when > they're used in just > > one CU. > > See Frediano's RFC series, which needs to change this in order to move > the 32bit relocation logic from asm to C. > > Not strictly necessary, I can declare in the C file as they were > declared in efi-boot.h (which is more a .c file as an header as Jan said). > I think the idea of declaring into a source file is that if another > file wants to use it has to declare it again, so a bit more friction. > But any access to trampoline variables should be considered as > something to limit in any case, so having in a separate header helps > (this does not mean that removing from the header is still increasing > the friction). > > Personally, I'm not strong about the 2 options here. Slightly prefer > having all variable declared in a single header. Declaring the same symbol in multiple places is a hard MISRA failure now. Hence why I was trying to clean this up to simplify your patches. ~Andrew
On 05.09.2024 18:10, Andrew Cooper wrote: > On 05/09/2024 4:42 pm, Jan Beulich wrote: >> On 05.09.2024 15:06, Andrew Cooper wrote: >>> --- a/xen/arch/x86/efi/efi-boot.h >>> +++ b/xen/arch/x86/efi/efi-boot.h >>> @@ -102,9 +102,6 @@ static void __init efi_arch_relocate_image(unsigned long delta) >>> } >>> } >>> >>> -extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[]; >>> -extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[]; >> I'd prefer if these stayed here, leaving the 4 symbols as minimally exposed as >> possible. Recall that efi-boot.h isn't really a header in that sense, but >> rather a .c file. Elsewhere we keep decls in .c files when they're used in just >> one CU. > > See Frediano's RFC series, which needs to change this in order to move > the 32bit relocation logic from asm to C. And it moves the declarations to the new .c file. Visibility still limited to that one file. And (afaics) no Misra violation, contrary to what your later reply to Frediano suggests. > The only reason efi-boot.h can get away with this right now is because > the other logic is written entirely in asm. > > > Scope-limiting linker section boundaries more than regular variables is > weird to me. It's not as if they magically take more care to use than > regular variables, and trampoline.h is not a wide scope by any means. It's not "more than", it's "like" (i.e. no matter whether a linker script or assembly is the origin of the symbol). Jan
On 06/09/2024 6:58 am, Jan Beulich wrote: > On 05.09.2024 18:10, Andrew Cooper wrote: >> On 05/09/2024 4:42 pm, Jan Beulich wrote: >>> On 05.09.2024 15:06, Andrew Cooper wrote: >>>> --- a/xen/arch/x86/efi/efi-boot.h >>>> +++ b/xen/arch/x86/efi/efi-boot.h >>>> @@ -102,9 +102,6 @@ static void __init efi_arch_relocate_image(unsigned long delta) >>>> } >>>> } >>>> >>>> -extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[]; >>>> -extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[]; >>> I'd prefer if these stayed here, leaving the 4 symbols as minimally exposed as >>> possible. Recall that efi-boot.h isn't really a header in that sense, but >>> rather a .c file. Elsewhere we keep decls in .c files when they're used in just >>> one CU. >> See Frediano's RFC series, which needs to change this in order to move >> the 32bit relocation logic from asm to C. > And it moves the declarations to the new .c file. Visibility still limited > to that one file. And (afaics) no Misra violation, contrary to what your > later reply to Frediano suggests. If only there were an easy way to answer the question. https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/7766305370 Failure: 4 regressions found for clean guidelines service MC3R1.R8.5: (required) An external object or function shall be declared once in one and only one file: violation: 4 >> The only reason efi-boot.h can get away with this right now is because >> the other logic is written entirely in asm. >> >> >> Scope-limiting linker section boundaries more than regular variables is >> weird to me. It's not as if they magically take more care to use than >> regular variables, and trampoline.h is not a wide scope by any means. > It's not "more than", it's "like" (i.e. no matter whether a linker script > or assembly is the origin of the symbol). I'm asking why linker-section-boundary symbols should be treated specially, and not seeing an answer. I assert they do not warrant special treatment, and should live in header files like every other extern symbol we use. ~Andrew
On 06.09.2024 21:46, Andrew Cooper wrote: > On 06/09/2024 6:58 am, Jan Beulich wrote: >> On 05.09.2024 18:10, Andrew Cooper wrote: >>> On 05/09/2024 4:42 pm, Jan Beulich wrote: >>>> On 05.09.2024 15:06, Andrew Cooper wrote: >>>>> --- a/xen/arch/x86/efi/efi-boot.h >>>>> +++ b/xen/arch/x86/efi/efi-boot.h >>>>> @@ -102,9 +102,6 @@ static void __init efi_arch_relocate_image(unsigned long delta) >>>>> } >>>>> } >>>>> >>>>> -extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[]; >>>>> -extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[]; >>>> I'd prefer if these stayed here, leaving the 4 symbols as minimally exposed as >>>> possible. Recall that efi-boot.h isn't really a header in that sense, but >>>> rather a .c file. Elsewhere we keep decls in .c files when they're used in just >>>> one CU. >>> See Frediano's RFC series, which needs to change this in order to move >>> the 32bit relocation logic from asm to C. >> And it moves the declarations to the new .c file. Visibility still limited >> to that one file. And (afaics) no Misra violation, contrary to what your >> later reply to Frediano suggests. > > If only there were an easy way to answer the question. > > https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/7766305370 > > Failure: 4 regressions found for clean guidelines > service MC3R1.R8.5: (required) An external object or function shall be > declared once in one and only one file: > violation: 4 I'm afraid I'm having trouble locating, in that .log file, where the actual regressions are pointed out. I guess I'm simply not used to reading such logs yet, and hence I just don't know what to search for. In any event, I think there's a set of issues here: - Eclair apparently considered efi-boot.h a header file, which (as said earlier) isn't quite right. - Declarations there are thus deemed okay (when they shouldn't, unless deviated). - Movement to a proper .c file points out that those decls may have been missing "asmlinkage" already before. >>> The only reason efi-boot.h can get away with this right now is because >>> the other logic is written entirely in asm. >>> >>> >>> Scope-limiting linker section boundaries more than regular variables is >>> weird to me. It's not as if they magically take more care to use than >>> regular variables, and trampoline.h is not a wide scope by any means. >> It's not "more than", it's "like" (i.e. no matter whether a linker script >> or assembly is the origin of the symbol). > > I'm asking why linker-section-boundary symbols should be treated > specially, and not seeing an answer. IOW you're not taking "they're no different from symbols defined in .S files, and hence shouldn't be treated differently" as a possible position to take? See __page_tables_{start,end}, cpu0_stack[], or multiboot_ptr as examples. > I assert they do not warrant special treatment, and should live in > header files like every other extern symbol we use. Then the same would also apply to symbols coming from .S files. Which in turn were deliberately deviated (rather than moved) in the course of dealing with the Misra rule relevant here. Jan
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 23e510c77e2e..833e343a475e 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -102,9 +102,6 @@ static void __init efi_arch_relocate_image(unsigned long delta) } } -extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[]; -extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[]; - static void __init relocate_trampoline(unsigned long phys) { const s32 *trampoline_ptr; diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h index e71dbb8d3fbf..b8d8127e2dc3 100644 --- a/xen/arch/x86/include/asm/processor.h +++ b/xen/arch/x86/include/asm/processor.h @@ -97,8 +97,6 @@ extern void ctxt_switch_levelling(const struct vcpu *next); extern void (*ctxt_switch_masking)(const struct vcpu *next); extern bool opt_cpu_info; -extern u32 trampoline_efer; -extern u64 trampoline_misc_enable_off; /* Maximum width of physical addresses supported by the hardware. */ extern unsigned int paddr_bits; diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h index d75589178b91..4d88503fd2e6 100644 --- a/xen/arch/x86/include/asm/setup.h +++ b/xen/arch/x86/include/asm/setup.h @@ -40,8 +40,6 @@ int remove_xen_ranges(struct rangeset *r); int cf_check stub_selftest(void); -extern uint8_t kbd_shift_flags; - #ifdef NDEBUG # define highmem_start 0 #else diff --git a/xen/arch/x86/include/asm/trampoline.h b/xen/arch/x86/include/asm/trampoline.h index cc3420ba3530..dc2c47946be4 100644 --- a/xen/arch/x86/include/asm/trampoline.h +++ b/xen/arch/x86/include/asm/trampoline.h @@ -49,6 +49,13 @@ /* SAF-0-safe */ extern char trampoline_start[], trampoline_end[]; +/* + * Relocations for the trampoline. Generated by the bootsym_{seg,}rel() + * macros, and collected by the linker. + */ +extern const int32_t __trampoline_rel_start[], __trampoline_rel_stop[]; +extern const int32_t __trampoline_seg_start[], __trampoline_seg_stop[]; + /* * The physical address of trampoline_start[] in low memory. It must be below * the 1M boundary (as the trampoline contains 16-bit code), and must be 4k @@ -87,9 +94,24 @@ extern uint32_t trampoline_xen_phys_start; /* A semaphore to indicate signs-of-life at the start of the AP boot path. */ extern uint8_t trampoline_cpu_started; +/* + * Extra MSR_EFER settings when activating Long Mode. EFER_NXE is necessary + * for APs to boot if the BSP found and activated support. + */ +extern uint32_t trampoline_efer; + +/* + * When nonzero, clear the specified bits in MSR_MISC_ENABLE. This is + * necessary to clobber XD_DISABLE before trying to set MSR_EFER.NXE. + */ +extern uint64_t trampoline_misc_enable_off; + /* Quirks about video mode-setting on S3 resume. */ extern uint8_t video_flags; +/* BIOS Int 16h, Fn 02h. The keyboard shift status. */ +extern uint8_t kbd_shift_flags; + /* Extended Display Identification Data, gathered from the BIOS. */ extern uint16_t boot_edid_caps; extern uint8_t boot_edid_info[128];
... and document them too. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Frediano Ziglio <frediano.ziglio@cloud.com> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com> video.h, edd.h and e820.h also contain trampoline symbols, but they're pretty well contained headers already. kbd_shift_flags seems especially dubious. It's a snapshot of the keyboard state when Xen happened to pass through the trampoline, and surely cannot be useful for dom0 in the slightest... --- xen/arch/x86/efi/efi-boot.h | 3 --- xen/arch/x86/include/asm/processor.h | 2 -- xen/arch/x86/include/asm/setup.h | 2 -- xen/arch/x86/include/asm/trampoline.h | 22 ++++++++++++++++++++++ 4 files changed, 22 insertions(+), 7 deletions(-)