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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.08.2019 17:02, David Woodhouse wrote:
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -733,6 +733,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
> @@ -770,6 +781,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 + 3) / 4),%ecx
> +        rep movsl %fs:(%esi),%es:(%edi)
The new data arrangement should be described in the commit message.
Also just like for the trampoline copying I think it would be better
if you suitable aligned bootdata_start and bootdata_end, such that
you wouldn't need to add 3 here before dividing by 4.

> @@ -227,7 +231,7 @@ start64:
>          .word   0
>  idt_48: .word   0, 0, 0 # base = limit = 0
>          .word   0
> -gdt_48: .word   6*8-1
> +gdt_48: .word   7*8-1
>          .long   tramp32sym_rel(trampoline_gdt,4)
You don't grow trampoline_gdt here, so I think this change is
wrong. And if a change was needed at all (perhaps in the next
patch), then I think it would be better to replace the use of
literal numbers, using the difference of two labels instead
(the "end" lable preferably being a .L-prefixed one).

> --- 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 */
>   
>   /* Retrieve Extended Display Identification Data. */
>   #define CONFIG_FIRMWARE_EDID
> @@ -113,7 +113,7 @@ mopar2: movb    %al, _param(PARAM_VIDEO_LINES)
>   
>   # Fetching of VESA frame buffer parameters
>   mopar_gr:
> -        leaw    vesa_mode_info, %di
> +        leaw    vesa_mode_info(%di)
Just as a note, as I can't really see how to improve the situation:
The embedding of the relocation offset (2) in the macros is making
this code even more fragile, as they're now not usable anymore in
an arbitrary way (consider e.g. their use for the memory operand if
an insn which also requires an immediate). I think you want to at
least warn about this restriction in the comment above.

> @@ -291,6 +293,10 @@ SECTIONS
>     DECL_SECTION(.data) {
>          *(.data.page_aligned)
>          *(.data)
> +       . = ALIGN(16);
> +       __bootdata_start = .;
> +       *(.data.boot16)
> +       __bootdata_end = .;
Why 16-byte alignment?

Having reached the end of the patch without seeing the C-level
bootsym() go away (and as a result noticing that you didn't remove
all uses) - could you please explain in the commit message what
the replacement (or not) criteria are?

Jan

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