On Fri, 2019-08-30 at 17:43 +0200, Jan Beulich wrote: > On 21.08.2019 18:35, David Woodhouse wrote: > > From: David Woodhouse > > > > Ditch the bootsym() access from C code for the variables populated by > > 16-bit boot code. As well as being cleaner this also paves the way for > > not having the 16-bit boot code in low memory for no-real-mode or EFI > > loader boots at all. > > > > These variables are put into a separate .data.boot16 section and > > accessed in low memory during the real-mode boot, then copied back to > > their native location in the Xen image when real mode has finished. > > > > Fix the limit in gdt_48 to admit that trampoline_gdt actually includes > > 7 entries, since we do now use the seventh (BOOT_FS) in late code so it > > matters. Andrew has a patch to further tidy up the GDT and initialise > > accessed bits etc., so I won't go overboard with more than the trivial > > size fix for now. > > > > The bootsym() macro remains in C code purely for the variables which > > are written for the later AP startup and wakeup trampoline to use. > > > > Signed-off-by: David Woodhouse > > --- > > xen/arch/x86/boot/edd.S | 2 ++ > > xen/arch/x86/boot/head.S | 16 +++++++++++++++ > > xen/arch/x86/boot/mem.S | 2 ++ > > xen/arch/x86/boot/trampoline.S | 33 ++++++++++++++++++++++++++++--- > > xen/arch/x86/boot/video.S | 30 +++++++++++++++------------- > > xen/arch/x86/platform_hypercall.c | 18 ++++++++--------- > > xen/arch/x86/setup.c | 22 ++++++++++----------- > > xen/arch/x86/xen.lds.S | 9 ++++++++- > > xen/include/asm-x86/edd.h | 1 - > > 9 files changed, 94 insertions(+), 39 deletions(-) > > > > diff --git a/xen/arch/x86/boot/edd.S b/xen/arch/x86/boot/edd.S > > index 434bbbd960..138d04c964 100644 > > --- a/xen/arch/x86/boot/edd.S > > +++ b/xen/arch/x86/boot/edd.S > > @@ -163,6 +163,7 @@ edd_done: > > .Ledd_mbr_sig_skip: > > ret > > > > + .pushsection .data.boot16, "aw", @progbits > > GLOBAL(boot_edd_info_nr) > > .byte 0 > > GLOBAL(boot_mbr_signature_nr) > > @@ -171,3 +172,4 @@ GLOBAL(boot_mbr_signature) > > .fill EDD_MBR_SIG_MAX*8,1,0 > > GLOBAL(boot_edd_info) > > .fill EDD_INFO_MAX * (EDDEXTSIZE + EDDPARMSIZE), 1, 0 > > + .popsection > > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > > index 4118f73683..6d315020d2 100644 > > --- a/xen/arch/x86/boot/head.S > > +++ b/xen/arch/x86/boot/head.S > > @@ -725,6 +725,17 @@ trampoline_setup: > > cmp $sym_offs(__bootsym_seg_stop),%edi > > jb 1b > > > > + /* Relocations for the boot data section. */ > > + mov sym_fs(trampoline_phys),%edx > > + add $(boot_trampoline_end - boot_trampoline_start),%edx > > + mov $sym_offs(__bootdatasym_rel_start),%edi > > +1: > > + mov %fs:(%edi),%eax > > + add %edx,%fs:(%edi,%eax) > > + add $4,%edi > > + cmp $sym_offs(__bootdatasym_rel_stop),%edi > > + jb 1b > > + > > /* Do not parse command line on EFI platform here. */ > > cmpb $0,sym_fs(efi_platform) > > jnz 1f > > @@ -762,6 +773,11 @@ trampoline_setup: > > mov $((boot_trampoline_end - boot_trampoline_start) / 4),%ecx > > rep movsl %fs:(%esi),%es:(%edi) > > > > + /* Copy boot data template to low memory. */ > > + mov $sym_offs(bootdata_start),%esi > > + mov $((bootdata_end - bootdata_start) / 4),%ecx > > + rep movsl %fs:(%esi),%es:(%edi) > > Afaict neither bootdata_start nor bootdata_end are aligned, and so > the difference isn't necessarily a multiple of 4. In fact the > other (preexisting) movsl looks to have the same issue; I wonder > if we propagate bad EDID data for that reason on certain builds / > in certain versions. Hm, I'm not sure I quite realised the distinction between bootdata_start and __bootdata_start (and likewise _end). Now that things are placed in the .data.boot16 section by .pushsection/.popsection can we rely on the ordering, and that the globals in the .S files are actually at the start and end? I thought we *needed* to use the ones in the linker script, and what I should probably do here is kill bootdata_start/bootdata_end completely and rely only on the ones from the linker script? Either that or I should kill the ones in the linker script completely. > > --- a/xen/arch/x86/boot/trampoline.S > > +++ b/xen/arch/x86/boot/trampoline.S > > @@ -47,11 +47,15 @@ > > .long 111b - (off) - .; \ > > .popsection > > > > -#define bootdatasym(s) ((s)-boot_trampoline_start) > > + .pushsection .data.boot16, "aw", @progbits > > +GLOBAL(bootdata_start) > > + .popsection > > + > > +#define bootdatasym(s) ((s)-bootdata_start+(boot_trampoline_end-boot_trampoline_start)) > > Please can you add the missing blanks around the binary operators > here? (I should perhaps asked this already on the earlier patch > adding this #define.) Also it looks like the line might be overly > long. Ack. > > --- a/xen/arch/x86/boot/video.S > > +++ b/xen/arch/x86/boot/video.S > > @@ -15,10 +15,10 @@ > > > > #include "video.h" > > > > -/* Scratch space layout: boot_trampoline_end to boot_trampoline_end+0x1000. */ > > -#define modelist bootsym(boot_trampoline_end) /* 2kB (256 entries) */ > > -#define vesa_glob_info (modelist + 0x800) /* 1kB */ > > -#define vesa_mode_info (vesa_glob_info + 0x400) /* 1kB */ > > +/* Scratch space layout: bootdata_end to bootdata_end+0x1000. */ > > +#define modelist(t) bootdatasym_rel(bootdata_end,2,t) /* 2KiB (256 entries) */ > > +#define vesa_glob_info(t) bootdatasym_rel((bootdata_end+0x800),2,t) /* 1KiB */ > > +#define vesa_mode_info(t) bootdatasym_rel((bootdata_end+0xc00),2,t) /* 1KiB */ > > Didn't you agree to extend the comment to warn about the risk resulting > from the literal 2-s in here? I think I didn't explicitly respond to that paragraph, and thus I missed it when I went back through the emails to check I'd caught everything. Will do it this time; apologies for missing it. > > @@ -290,6 +292,11 @@ SECTIONS > > DECL_SECTION(.data) { > > *(.data.page_aligned) > > *(.data) > > + . = ALIGN(4); > > + __bootdata_start = .; > > + *(.data.boot16) > > + . = ALIGN(4); > > + __bootdata_end = .; > > What do you need the labels for here? And once they're gone the ALIGN() > won't belong here anymore either - suitable alignment should be enforced > by the contributions to the section. See above. Am I right to be concerned about the fragility of putting the symbols in the .S files? Doing it in the linker script is more robust, isn't it? I know we *currently* build everything with #include from one huge .S file and thus we do know that they'll end up first/last as we desire, and it doesn't depend on link order or any crap like that. But I don't like depending on that.