|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 4/5] x86/boot: Copy 16-bit boot variables back up to Xen image
On Fri, 2019-08-30 at 17:43 +0200, Jan Beulich wrote:
> On 21.08.2019 18:35, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@xxxxxxxxxxxx>
> >
> > 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 <dwmw@xxxxxxxxxxxx>
> > ---
> > 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.
Attachment:
smime.p7s _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |