| Message ID | 36d62ca62e1344d17b17b1979a627bb304e82396.1484683034.git-series.cardoe@cardoe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 17.01.17 at 21:07, <cardoe@cardoe.com> wrote: > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -519,6 +519,7 @@ trampoline_setup: > 1: > /* Switch to low-memory stack. */ > mov sym_phys(trampoline_phys),%edi > + /* The stack base is 64kb after the location of trampoline_phys */ > lea 0x10000(%edi),%esp I'm sorry for being picky, but the stack _base_ isn't where the stack pointer should point initially, or else there would be no room on the stack at all. Perhaps you had a reason for not using the wording I did suggest, but whatever wording is chosen should not risk to cause confusion (otherwise I think we'd be better off not adding any comment here). > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -146,8 +146,6 @@ static void __init > efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, > { > struct e820entry *e; > unsigned int i; > - /* Check for extra mem for mbi data if Xen is loaded via multiboot2 protocol. */ > - UINTN extra_mem = efi_enabled(EFI_LOADER) ? 0 : (64 << 10); > > /* Populate E820 table and check trampoline area availability. */ > e = e820map - 1; > @@ -170,8 +168,7 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, > /* fall through */ > case EfiConventionalMemory: > if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 && > - len >= cfg.size + extra_mem && > - desc->PhysicalStart + len > cfg.addr ) > + len >= cfg.size && desc->PhysicalStart + len > cfg.addr ) > cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK; > /* fall through */ > case EfiLoaderCode: > @@ -686,6 +683,10 @@ paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTa > setup_efi_pci(); > efi_variables(); > > + /* This is the maximum size of our trampoline + our low memory stack */ > + cfg.size = max_t(UINTN, 64 << 10, > + (trampoline_end - trampoline_start) + 4096); Considering the consuming code further up, I don't understand the reason for the addition of 4096 here. And if there is a reason, I'm pretty sure you actually mean PAGE_SIZE. Jan
On 1/18/17 4:52 AM, Jan Beulich wrote: >>>> On 17.01.17 at 21:07, <cardoe@cardoe.com> wrote: >> --- a/xen/arch/x86/boot/head.S >> +++ b/xen/arch/x86/boot/head.S >> @@ -519,6 +519,7 @@ trampoline_setup: >> 1: >> /* Switch to low-memory stack. */ >> mov sym_phys(trampoline_phys),%edi >> + /* The stack base is 64kb after the location of trampoline_phys */ >> lea 0x10000(%edi),%esp > > I'm sorry for being picky, but the stack _base_ isn't where the stack > pointer should point initially, or else there would be no room on the > stack at all. Perhaps you had a reason for not using the wording I > did suggest, but whatever wording is chosen should not risk to cause > confusion (otherwise I think we'd be better off not adding any > comment here). So this was my perception. Maybe I'm totally wrong here. |----------|---------|------| a b c d example values a = 0x0 (0) b = 0x8c000 (trampoline_phys) c = 0x9c000 (base of stack as it grows towards 0x8c000) d = 0x100000 (1mb) mov sym_phys(trampoline_phys),%edi lea 0x10000(%edi),%esp I would think that 0x10000 + %edi would be the base or start of the stack since its growing downwards towards b. I'll use "ends" however and resubmit. > >> --- a/xen/arch/x86/efi/efi-boot.h >> +++ b/xen/arch/x86/efi/efi-boot.h >> @@ -146,8 +146,6 @@ static void __init >> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, >> { >> struct e820entry *e; >> unsigned int i; >> - /* Check for extra mem for mbi data if Xen is loaded via multiboot2 protocol. */ >> - UINTN extra_mem = efi_enabled(EFI_LOADER) ? 0 : (64 << 10); >> >> /* Populate E820 table and check trampoline area availability. */ >> e = e820map - 1; >> @@ -170,8 +168,7 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, >> /* fall through */ >> case EfiConventionalMemory: >> if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 && >> - len >= cfg.size + extra_mem && >> - desc->PhysicalStart + len > cfg.addr ) >> + len >= cfg.size && desc->PhysicalStart + len > cfg.addr ) >> cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK; >> /* fall through */ >> case EfiLoaderCode: >> @@ -686,6 +683,10 @@ paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTa >> setup_efi_pci(); >> efi_variables(); >> >> + /* This is the maximum size of our trampoline + our low memory stack */ >> + cfg.size = max_t(UINTN, 64 << 10, >> + (trampoline_end - trampoline_start) + 4096); > > Considering the consuming code further up, I don't understand the > reason for the addition of 4096 here. And if there is a reason, I'm > pretty sure you actually mean PAGE_SIZE. > > Jan > You are correct. Given that the assembly is hardcoded at 64k there is no reason for this. I had kicked around doing a similar max() call in assembly instead of hardcoding the value but didn't do it. So I should just remove this.
>>> On 18.01.17 at 14:33, <cardoe@cardoe.com> wrote: > On 1/18/17 4:52 AM, Jan Beulich wrote: >>>>> On 17.01.17 at 21:07, <cardoe@cardoe.com> wrote: >>> --- a/xen/arch/x86/boot/head.S >>> +++ b/xen/arch/x86/boot/head.S >>> @@ -519,6 +519,7 @@ trampoline_setup: >>> 1: >>> /* Switch to low-memory stack. */ >>> mov sym_phys(trampoline_phys),%edi >>> + /* The stack base is 64kb after the location of trampoline_phys */ >>> lea 0x10000(%edi),%esp >> >> I'm sorry for being picky, but the stack _base_ isn't where the stack >> pointer should point initially, or else there would be no room on the >> stack at all. Perhaps you had a reason for not using the wording I >> did suggest, but whatever wording is chosen should not risk to cause >> confusion (otherwise I think we'd be better off not adding any >> comment here). > > So this was my perception. Maybe I'm totally wrong here. > > |----------|---------|------| > a b c d > > example values > a = 0x0 (0) > b = 0x8c000 (trampoline_phys) > c = 0x9c000 (base of stack as it grows towards 0x8c000) > d = 0x100000 (1mb) > > mov sym_phys(trampoline_phys),%edi > lea 0x10000(%edi),%esp > > I would think that 0x10000 + %edi would be the base or start of the > stack since its growing downwards towards b. Well, to me "start" and "base" are the lowest address and "end" the highest (or one byte beyond). >>> @@ -686,6 +683,10 @@ paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTa >>> setup_efi_pci(); >>> efi_variables(); >>> >>> + /* This is the maximum size of our trampoline + our low memory stack */ >>> + cfg.size = max_t(UINTN, 64 << 10, >>> + (trampoline_end - trampoline_start) + 4096); >> >> Considering the consuming code further up, I don't understand the >> reason for the addition of 4096 here. And if there is a reason, I'm >> pretty sure you actually mean PAGE_SIZE. > > You are correct. Given that the assembly is hardcoded at 64k there is no > reason for this. I had kicked around doing a similar max() call in > assembly instead of hardcoding the value but didn't do it. So I should > just remove this. Well, I don't mind the max() (albeit it's not very useful, especially if trampoline size would ever get pretty close to 64k). And as said in reply to the earlier version - I think this would better be checked at build time (see the various ASSERT()s at the end of xen.lds.S). Jan
On 1/18/17 8:41 AM, Jan Beulich wrote: >>>> On 18.01.17 at 14:33, <cardoe@cardoe.com> wrote: >> On 1/18/17 4:52 AM, Jan Beulich wrote: >>>>>> On 17.01.17 at 21:07, <cardoe@cardoe.com> wrote: >>>> @@ -686,6 +683,10 @@ paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTa >>>> setup_efi_pci(); >>>> efi_variables(); >>>> >>>> + /* This is the maximum size of our trampoline + our low memory stack */ >>>> + cfg.size = max_t(UINTN, 64 << 10, >>>> + (trampoline_end - trampoline_start) + 4096); >>> >>> Considering the consuming code further up, I don't understand the >>> reason for the addition of 4096 here. And if there is a reason, I'm >>> pretty sure you actually mean PAGE_SIZE. >> >> You are correct. Given that the assembly is hardcoded at 64k there is no >> reason for this. I had kicked around doing a similar max() call in >> assembly instead of hardcoding the value but didn't do it. So I should >> just remove this. > > Well, I don't mind the max() (albeit it's not very useful, especially > if trampoline size would ever get pretty close to 64k). And as said > in reply to the earlier version - I think this would better be checked > at build time (see the various ASSERT()s at the end of xen.lds.S). > > Jan > Thank you. I had looked for BUILD_ASSERT() and didn't see it so I didn't know what mechanism we had which is why I didn't add it.
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index ac93df0..d288959 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -519,6 +519,7 @@ trampoline_setup: 1: /* Switch to low-memory stack. */ mov sym_phys(trampoline_phys),%edi + /* The stack base is 64kb after the location of trampoline_phys */ lea 0x10000(%edi),%esp lea trampoline_boot_cpu_entry-trampoline_start(%edi),%eax pushl $BOOT_CS32 diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index dc857d8..a73134c 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -146,8 +146,6 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, { struct e820entry *e; unsigned int i; - /* Check for extra mem for mbi data if Xen is loaded via multiboot2 protocol. */ - UINTN extra_mem = efi_enabled(EFI_LOADER) ? 0 : (64 << 10); /* Populate E820 table and check trampoline area availability. */ e = e820map - 1; @@ -170,8 +168,7 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, /* fall through */ case EfiConventionalMemory: if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 && - len >= cfg.size + extra_mem && - desc->PhysicalStart + len > cfg.addr ) + len >= cfg.size && desc->PhysicalStart + len > cfg.addr ) cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK; /* fall through */ case EfiLoaderCode: @@ -686,6 +683,10 @@ paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTa setup_efi_pci(); efi_variables(); + /* This is the maximum size of our trampoline + our low memory stack */ + cfg.size = max_t(UINTN, 64 << 10, + (trampoline_end - trampoline_start) + 4096); + if ( gop ) efi_set_gop_mode(gop, gop_mode); diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c index 6ea6aa1..b81adc0 100644 --- a/xen/arch/x86/efi/stub.c +++ b/xen/arch/x86/efi/stub.c @@ -33,7 +33,7 @@ paddr_t __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, * not be directly supported by C compiler. */ asm volatile( - " call %2 \n" + " call *%2 \n" "0: hlt \n" " jmp 0b \n" : "+c" (StdErr), "+d" (err) : "g" (StdErr->OutputString)