Re: [Xen-devel] [PATCH v3 4/5] x86/boot: Copy 16-bit boot variables back up to Xen image

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> --- 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.

> --- 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?

> @@ -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.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel