[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 3/5] x86: Force proper gdt_boot_base setting
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |