On Mon, 2019-08-12 at 12:24 +0200, Jan Beulich wrote:
> 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.

Ack.

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

I don't grow it but... count it ☺.

I do start using sym_fs() here in places that it wasn't before, so the
incorrect size started to *matter* because the BOOT_FS selector wasn't
included in the limit.

I will make sure I explicitly comment on that in the commit message; no
need for a code comment to explain why the limit actually *does* match
the size of the table.

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

Yeah. I file that one under "don't touch the VESA code unless you want
your brain to dribble out of your ears". Which was basically true
before I touched it too, in my defence ☺.

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

Er... not sure. I think this (and the end) can be 4 as you suggest
elsewhere. Will make that change and retest.

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

In the subsequent patch (6/6), bootsym() is indeed gone from C code,
and only trampsym() is left. The latter is for the permanent (not boot
time) trampoline used wakeup and for AP startup. As noted in the commit
message of that patch, the physical location of the Xen image isn't
mapped when those code paths run. So anything they need must be
relocated with them.