[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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 parametersmopar_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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |