| Message ID | 20240814083428.3012-4-frediano.ziglio@cloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support EFI multiboot loading using PE binary | expand |
On 14.08.2024 10:34, Frediano Ziglio wrote: > Instead of relocate the value at that position compute it > entirely and write it. > During EFI boots sym_offs(SYMBOL) are potentially relocated > causing the values to be corrupted. This requires quite a bit more explanation, also to understand why a lone specific sym_offs() is being dealt with here, leaving others untouched. I'm specifically puzzled by the two in the MB2 header: If the GDT one is a problem, why would those not be? Of course similarly for others, in particular those used to pre-fill page tables. I think an adjustment here needs to come with the addition of a comment next to the #define, to clarify where the use is appropriate and where it needs to be avoided. > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -132,8 +132,7 @@ multiboot2_header: > gdt_boot_descr: > .word .Ltrampoline_gdt_end - trampoline_gdt - 1 > gdt_boot_base: > - .long sym_offs(trampoline_gdt) > - .long 0 /* Needed for 64-bit lgdt */ > + .quad 0 /* Needed for 64-bit lgdt */ The comment is now somewhat odd: It's no longer the entire line that's there just because we want to use LGDT from 64-bit code, but just half of what the .quad produces. Therefore perhaps either keep as two longs (maybe with a different brief comment put on the former), or adjust the comment wording. > @@ -373,15 +372,16 @@ __efi64_mb2_start: > x86_32_switch: > mov %r15,%rdi > > - /* Store Xen image load base address in place accessible for 32-bit code. */ > - lea __image_base__(%rip),%esi > - > cli > > /* Initialize GDTR. */ > - add %esi,gdt_boot_base(%rip) > + lea trampoline_gdt(%rip), %esi > + mov %esi, gdt_boot_base(%rip) > lgdt gdt_boot_descr(%rip) > > + /* Store Xen image load base address in place accessible for 32-bit code. */ > + lea __image_base__(%rip),%esi What's the point in moving this code? Surely you could use another register for the LEA/MOV pair above? In any event - _if_ you move the code, please also add the blank missing after the comma. > @@ -439,7 +439,8 @@ __pvh_start: > movb $-1, sym_esi(opt_console_xen) > > /* Prepare gdt and segments */ > - add %esi, sym_esi(gdt_boot_base) > + lea sym_esi(trampoline_gdt), %ecx > + mov %ecx, sym_esi(gdt_boot_base) > lgdt sym_esi(gdt_boot_descr) > > mov $BOOT_DS, %ecx > @@ -543,7 +544,8 @@ trampoline_bios_setup: > * > * Initialize GDTR and basic data segments. > */ > - add %esi,sym_esi(gdt_boot_base) > + lea sym_esi(trampoline_gdt), %ecx > + mov %ecx, sym_esi(gdt_boot_base) > lgdt sym_esi(gdt_boot_descr) > > mov $BOOT_DS,%ecx While you mention that you make these changes for consistency, I'm afraid I don't really see the point. The paths are all different anyway - there's nothing wrong with leaving everything except x86_32_switch untouched. Far less code churn then. All it would take is extending the comment there a little to mention why the value is fully overwritten rather than merely adjusted. Jan
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index af598a60bf..666e341bc5 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -132,8 +132,7 @@ multiboot2_header: gdt_boot_descr: .word .Ltrampoline_gdt_end - trampoline_gdt - 1 gdt_boot_base: - .long sym_offs(trampoline_gdt) - .long 0 /* Needed for 64-bit lgdt */ + .quad 0 /* Needed for 64-bit lgdt */ vga_text_buffer: .long 0xb8000 @@ -373,15 +372,16 @@ __efi64_mb2_start: x86_32_switch: mov %r15,%rdi - /* Store Xen image load base address in place accessible for 32-bit code. */ - lea __image_base__(%rip),%esi - cli /* Initialize GDTR. */ - add %esi,gdt_boot_base(%rip) + lea trampoline_gdt(%rip), %esi + mov %esi, gdt_boot_base(%rip) lgdt gdt_boot_descr(%rip) + /* Store Xen image load base address in place accessible for 32-bit code. */ + lea __image_base__(%rip),%esi + /* Reload code selector. */ pushq $BOOT_CS32 lea cs32_switch(%rip),%edx @@ -439,7 +439,8 @@ __pvh_start: movb $-1, sym_esi(opt_console_xen) /* Prepare gdt and segments */ - add %esi, sym_esi(gdt_boot_base) + lea sym_esi(trampoline_gdt), %ecx + mov %ecx, sym_esi(gdt_boot_base) lgdt sym_esi(gdt_boot_descr) mov $BOOT_DS, %ecx @@ -543,7 +544,8 @@ trampoline_bios_setup: * * Initialize GDTR and basic data segments. */ - add %esi,sym_esi(gdt_boot_base) + lea sym_esi(trampoline_gdt), %ecx + mov %ecx, sym_esi(gdt_boot_base) lgdt sym_esi(gdt_boot_descr) mov $BOOT_DS,%ecx
Instead of relocate the value at that position compute it entirely and write it. During EFI boots sym_offs(SYMBOL) are potentially relocated causing the values to be corrupted. For PVH and BIOS the change won't be necessary but keep the code consistent. Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> --- xen/arch/x86/boot/head.S | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)