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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel