Re: [Xen-devel] [PATCH v2 5/6] x86/boot: Copy 16-bit boot variables back up to Xen image
> On 19.08.2019 17:25, David Woodhouse wrote: >> On Mon, 2019-08-12 at 12:24 +0200, Jan Beulich wrote: >>> On 09.08.2019 17:02, David Woodhouse wrote: @@ -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. > > Oh, I see - a (latent) bug introduced by b28044226e. Should perhaps > be a separate patch to fix this then (by, as suggested, using an > "end" label rather than hard coded numbers). Indeed. Andrew already posted a patch for that, which (along with his others) I have rebased on top of my tree at https://xenbits.xen.org/gitweb/?p=people/dwmw2/xen.git;a=shortlog;h=refs/heads/bootcleanup-andy -- dwmw2 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 5/6] x86/boot: Copy 16-bit boot variables back up to Xen image
On 19.08.2019 17:25, David Woodhouse wrote: On Mon, 2019-08-12 at 12:24 +0200, Jan Beulich wrote: On 09.08.2019 17:02, David Woodhouse wrote: @@ -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. Oh, I see - a (latent) bug introduced by b28044226e. Should perhaps be a separate patch to fix this then (by, as suggested, using an "end" label rather than hard coded numbers). Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 5/6] x86/boot: Copy 16-bit boot variables back up to Xen image
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:
> > -leawvesa_mode_info, %di
> > +leawvesa_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
Re: [Xen-devel] [PATCH v2 5/6] x86/boot: Copy 16-bit boot variables back up to Xen image
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:
-leawvesa_mode_info, %di
+leawvesa_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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 5/6] x86/boot: Copy 16-bit boot variables back up to Xen image
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. 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| 8 +++- xen/include/asm-x86/edd.h | 1 - 9 files changed, 93 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 556dab127f..104eb0eb3c 100644 --- 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) + /* Jump into the relocated trampoline. */ lret diff --git a/xen/arch/x86/boot/mem.S b/xen/arch/x86/boot/mem.S index aa39608442..86f0fa9af7 100644 --- a/xen/arch/x86/boot/mem.S +++ b/xen/arch/x86/boot/mem.S @@ -67,6 +67,7 @@ get_memory_map: ret .align 4 +.pushsection .data.boot16, "aw", @progbits GLOBAL(bios_e820map) .fill E820_BIOS_MAX*20,1,0 GLOBAL(bios_e820nr) @@ -75,3 +76,4 @@ GLOBAL(lowmem_kb) .long 0 GLOBAL(highmem_kb) .long 0 + .popsection diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S index 26af9c6beb..b5517a44bb 100644 --- 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)) #define bootdatasym_rel(sym, off, opnd...) \ bootdatasym(sym),##opnd; \ 111:; \ -.pushsection .bootdatasym_rel, "a";\ +.pushsection .bootsym_rel, "a";\ .long 111b - (off) - .;\ .popsection @@ -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) /* The first page of trampoline is permanent, the rest boot-time only. */ @@ -318,6 +322,23 @@ trampoline_boot_cpu_entry: mov %eax,%gs mov %eax,%ss +/* + * Copy locally-gathered data back up into the Xen physical image + */ +mov $BOOT_FS,%eax +mov %eax,%es + +mov $sym_offs(bootdata_end),%ecx +mov $sym_offs(bootdata_start),%edi +sub %edi,%ecx +mov $bootdatasym_rel(bootdata_start,4,%esi) +rep movsb %ds:(%esi),%es:(%edi) + +/* + * %es still points to BOOT_FS but trampoline_protmode_entry + * reloads it anyway. + */ + /* EBX == 0 indicates we are the BP (Boot Processor). */ xor %ebx,%ebx @@ -345,8 +366,10 @@ vesa_size: .word 0,0,0 /* width x depth
