Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
On Tue, Mar 17, 2015 at 10:32:01AM +, Jan Beulich wrote: On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote: @@ -94,6 +111,17 @@ ENTRY(start) gdt_boot_descr: .word 6*8-1 .long sym_phys(trampoline_gdt) +.long 0 /* Needed for 64-bit lgdt */ + +cs32_switch_addr: +.long sym_phys(cs32_switch) +.long BOOT_CS32 + +efi_st: +.quad 0 + +efi_ih: +.quad 0 .Lbad_cpu_msg: .asciz ERR: Not a 64-bit CPU! .Lbad_ldr_msg: .asciz ERR: Not a Multiboot bootloader! @@ -124,6 +152,133 @@ print_err: .Lhalt: hlt jmp .Lhalt +.code64 + +__efi64_start: +cld + +/* Bootloaders may set multiboot[12].mem_lower to a nonzero value */ +xor %edx,%edx + +/* Check for Multiboot2 bootloader */ +cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax +je efi_multiboot2_proto + +jmp not_multiboot + +efi_multiboot2_proto: jne not_multiboot (and efi_multiboot2_proto dropped altogether) +/* Skip Multiboot2 information fixed part */ +lea MB2_fixed_sizeof(%ebx),%ecx Let's please not add more assumptions than necessary about stuff being below 4G. I am not sure what do you mean by that. + +0: +/* Get mem_lower from Multiboot2 information */ +cmpl$MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx) +jne 1f + +mov MB2_mem_lower(%ecx),%edx +jmp 4f + +1: +/* Get EFI SystemTable address from Multiboot2 information */ +cmpl$MULTIBOOT2_TAG_TYPE_EFI64,(%ecx) +jne 2f + +lea MB2_efi64_st(%ecx),%esi +lea efi_st(%rip),%edi +movsq A simple pair of mov-s please, assuming all of this really needs to be done in assembly in the first place. Also please use .Lname labels in this assembly coded switch statement to ease reading. + +/* Do not go into real mode on EFI platform */ +movb$1,skip_realmode(%rip) + +jmp 4f + +2: +/* Get EFI ImageHandle address from Multiboot2 information */ +cmpl$MULTIBOOT2_TAG_TYPE_EFI64_IH,(%ecx) +jne 3f + +lea MB2_efi64_ih(%ecx),%esi +lea efi_ih(%rip),%edi +movsq +jmp 4f + +3: +/* Is it the end of Multiboot2 information? */ +cmpl$MULTIBOOT2_TAG_TYPE_END,(%ecx) +je run_bs + +4: Please switch the order so that 2 can fall through into 4 (and you then won't need the run_bs label, which otherwise should also becom .Lrun_bs). +/* Go to next Multiboot2 information tag */ +add MB2_tag_size(%ecx),%ecx +add $(MULTIBOOT2_TAG_ALIGN-1),%ecx +and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx +jmp 0b + +run_bs: +push%rax +push%rdx + +/* Initialize BSS (no nasty surprises!) */ +lea __bss_start(%rip),%rdi +lea _end(%rip),%rcx +sub %rdi,%rcx +xor %rax,%rax +rep stosb rep stosb + +mov efi_ih(%rip),%rdi /* EFI ImageHandle */ +mov efi_st(%rip),%rsi /* EFI SystemTable */ +callefi_multiboot2 With efi_multiboot2 being a C function it really looks like much of the above doesn't need to be done in assembly. Potentially make sense. I will try to do that. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
On 27.03.15 at 14:06, daniel.ki...@oracle.com wrote: On Tue, Mar 17, 2015 at 10:32:01AM +, Jan Beulich wrote: On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote: +/* Skip Multiboot2 information fixed part */ +lea MB2_fixed_sizeof(%ebx),%ecx Let's please not add more assumptions than necessary about stuff being below 4G. I am not sure what do you mean by that. See the 32-bit register used for addressing here (and in many more places)? Jan ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
On 27.03.15 at 15:26, daniel.ki...@oracle.com wrote: On Fri, Mar 27, 2015 at 01:36:32PM +, Jan Beulich wrote: On 27.03.15 at 14:06, daniel.ki...@oracle.com wrote: On Tue, Mar 17, 2015 at 10:32:01AM +, Jan Beulich wrote: On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote: +/* Skip Multiboot2 information fixed part */ +lea MB2_fixed_sizeof(%ebx),%ecx Let's please not add more assumptions than necessary about stuff being below 4G. I am not sure what do you mean by that. See the 32-bit register used for addressing here (and in many more places)? This is what I expected but I was confused because you were referring only here to this problem. Anyway, is it possible to do this in different way? Should we care if image is always loaded at 0x10 right now? Even with Xen early boot code being relocatable loader could not load image higher than 0x - 14 MiB. I don't understand what you're alluding to. Just use 64-bit registers for memory accesses and LEAs, and be done. This will result in smaller code as a benefit. Jan ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
On Fri, Mar 27, 2015 at 01:36:32PM +, Jan Beulich wrote: On 27.03.15 at 14:06, daniel.ki...@oracle.com wrote: On Tue, Mar 17, 2015 at 10:32:01AM +, Jan Beulich wrote: On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote: +/* Skip Multiboot2 information fixed part */ +lea MB2_fixed_sizeof(%ebx),%ecx Let's please not add more assumptions than necessary about stuff being below 4G. I am not sure what do you mean by that. See the 32-bit register used for addressing here (and in many more places)? This is what I expected but I was confused because you were referring only here to this problem. Anyway, is it possible to do this in different way? Should we care if image is always loaded at 0x10 right now? Even with Xen early boot code being relocatable loader could not load image higher than 0x - 14 MiB. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
On Fri, Mar 27, 2015 at 02:34:19PM +, Jan Beulich wrote: On 27.03.15 at 15:26, daniel.ki...@oracle.com wrote: On Fri, Mar 27, 2015 at 01:36:32PM +, Jan Beulich wrote: On 27.03.15 at 14:06, daniel.ki...@oracle.com wrote: On Tue, Mar 17, 2015 at 10:32:01AM +, Jan Beulich wrote: On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote: +/* Skip Multiboot2 information fixed part */ +lea MB2_fixed_sizeof(%ebx),%ecx Let's please not add more assumptions than necessary about stuff being below 4G. I am not sure what do you mean by that. See the 32-bit register used for addressing here (and in many more places)? This is what I expected but I was confused because you were referring only here to this problem. Anyway, is it possible to do this in different way? Should we care if image is always loaded at 0x10 right now? Even with Xen early boot code being relocatable loader could not load image higher than 0x - 14 MiB. I don't understand what you're alluding to. Just use 64-bit registers for memory accesses and LEAs, and be done. This will result in smaller code as a benefit. Well... How can I do that here if processor is in 32-bit mode? Maybe, we could that things after switching to 64-bit mode. However, I think this requires separate patch to do these changes. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
On 27.03.15 at 15:57, daniel.ki...@oracle.com wrote: On Fri, Mar 27, 2015 at 02:34:19PM +, Jan Beulich wrote: On 27.03.15 at 15:26, daniel.ki...@oracle.com wrote: On Fri, Mar 27, 2015 at 01:36:32PM +, Jan Beulich wrote: On 27.03.15 at 14:06, daniel.ki...@oracle.com wrote: On Tue, Mar 17, 2015 at 10:32:01AM +, Jan Beulich wrote: On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote: +/* Skip Multiboot2 information fixed part */ +lea MB2_fixed_sizeof(%ebx),%ecx Let's please not add more assumptions than necessary about stuff being below 4G. I am not sure what do you mean by that. See the 32-bit register used for addressing here (and in many more places)? This is what I expected but I was confused because you were referring only here to this problem. Anyway, is it possible to do this in different way? Should we care if image is always loaded at 0x10 right now? Even with Xen early boot code being relocatable loader could not load image higher than 0x - 14 MiB. I don't understand what you're alluding to. Just use 64-bit registers for memory accesses and LEAs, and be done. This will result in smaller code as a benefit. Well... How can I do that here if processor is in 32-bit mode? Maybe, we could that things after switching to 64-bit mode. However, I think this requires separate patch to do these changes. No, if the processor is in 32-bit mode, then using 32-bit registers is fine of course. But I'm pretty certain I spotted at least some cases where it looked like you used 32-bit registers in 64-bit mode. Jan ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
On Fri, Mar 27, 2015 at 03:06:43PM +, Jan Beulich wrote: On 27.03.15 at 15:57, daniel.ki...@oracle.com wrote: On Fri, Mar 27, 2015 at 02:34:19PM +, Jan Beulich wrote: On 27.03.15 at 15:26, daniel.ki...@oracle.com wrote: On Fri, Mar 27, 2015 at 01:36:32PM +, Jan Beulich wrote: On 27.03.15 at 14:06, daniel.ki...@oracle.com wrote: On Tue, Mar 17, 2015 at 10:32:01AM +, Jan Beulich wrote: On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote: +/* Skip Multiboot2 information fixed part */ +lea MB2_fixed_sizeof(%ebx),%ecx Let's please not add more assumptions than necessary about stuff being below 4G. I am not sure what do you mean by that. See the 32-bit register used for addressing here (and in many more places)? This is what I expected but I was confused because you were referring only here to this problem. Anyway, is it possible to do this in different way? Should we care if image is always loaded at 0x10 right now? Even with Xen early boot code being relocatable loader could not load image higher than 0x - 14 MiB. I don't understand what you're alluding to. Just use 64-bit registers for memory accesses and LEAs, and be done. This will result in smaller code as a benefit. Well... How can I do that here if processor is in 32-bit mode? Maybe, we could that things after switching to 64-bit mode. However, I think this requires separate patch to do these changes. No, if the processor is in 32-bit mode, then using 32-bit registers is fine of course. But I'm pretty certain I spotted at least some cases where it looked like you used 32-bit registers in 64-bit mode. OK, I will double check. If I find something then I will fix it. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote: @@ -94,6 +111,17 @@ ENTRY(start) gdt_boot_descr: .word 6*8-1 .long sym_phys(trampoline_gdt) +.long 0 /* Needed for 64-bit lgdt */ + +cs32_switch_addr: +.long sym_phys(cs32_switch) +.long BOOT_CS32 + +efi_st: +.quad 0 + +efi_ih: +.quad 0 .Lbad_cpu_msg: .asciz ERR: Not a 64-bit CPU! .Lbad_ldr_msg: .asciz ERR: Not a Multiboot bootloader! @@ -124,6 +152,133 @@ print_err: .Lhalt: hlt jmp .Lhalt +.code64 + +__efi64_start: +cld + +/* Bootloaders may set multiboot[12].mem_lower to a nonzero value */ +xor %edx,%edx + +/* Check for Multiboot2 bootloader */ +cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax +je efi_multiboot2_proto + +jmp not_multiboot + +efi_multiboot2_proto: jne not_multiboot (and efi_multiboot2_proto dropped altogether) +/* Skip Multiboot2 information fixed part */ +lea MB2_fixed_sizeof(%ebx),%ecx Let's please not add more assumptions than necessary about stuff being below 4G. + +0: +/* Get mem_lower from Multiboot2 information */ +cmpl$MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx) +jne 1f + +mov MB2_mem_lower(%ecx),%edx +jmp 4f + +1: +/* Get EFI SystemTable address from Multiboot2 information */ +cmpl$MULTIBOOT2_TAG_TYPE_EFI64,(%ecx) +jne 2f + +lea MB2_efi64_st(%ecx),%esi +lea efi_st(%rip),%edi +movsq A simple pair of mov-s please, assuming all of this really needs to be done in assembly in the first place. Also please use .Lname labels in this assembly coded switch statement to ease reading. + +/* Do not go into real mode on EFI platform */ +movb$1,skip_realmode(%rip) + +jmp 4f + +2: +/* Get EFI ImageHandle address from Multiboot2 information */ +cmpl$MULTIBOOT2_TAG_TYPE_EFI64_IH,(%ecx) +jne 3f + +lea MB2_efi64_ih(%ecx),%esi +lea efi_ih(%rip),%edi +movsq +jmp 4f + +3: +/* Is it the end of Multiboot2 information? */ +cmpl$MULTIBOOT2_TAG_TYPE_END,(%ecx) +je run_bs + +4: Please switch the order so that 2 can fall through into 4 (and you then won't need the run_bs label, which otherwise should also becom .Lrun_bs). +/* Go to next Multiboot2 information tag */ +add MB2_tag_size(%ecx),%ecx +add $(MULTIBOOT2_TAG_ALIGN-1),%ecx +and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx +jmp 0b + +run_bs: +push%rax +push%rdx + +/* Initialize BSS (no nasty surprises!) */ +lea __bss_start(%rip),%rdi +lea _end(%rip),%rcx +sub %rdi,%rcx +xor %rax,%rax +rep stosb rep stosb + +mov efi_ih(%rip),%rdi /* EFI ImageHandle */ +mov efi_st(%rip),%rsi /* EFI SystemTable */ +callefi_multiboot2 With efi_multiboot2 being a C function it really looks like much of the above doesn't need to be done in assembly. + +pop %rcx +pop %rax + +shl $10-4,%rcx /* Convert multiboot2.mem_lower to bytes/16 */ + +cli + +/* Initialise GDT */ +lgdtgdt_boot_descr(%rip) + +/* Reload code selector */ +ljmpl *cs32_switch_addr(%rip) + +.code32 + +cs32_switch: +/* Initialise basic data segments */ +mov $BOOT_DS,%edx +mov %edx,%ds +mov %edx,%es +mov %edx,%fs +mov %edx,%gs +mov %edx,%ss + +mov $sym_phys(cpu0_stack)+1024,%esp + +/* Disable paging */ +mov %cr0,%edx +and $(~X86_CR0_PG),%edx +mov %edx,%cr0 + +push%eax +push%ecx + +/* Disable Long Mode */ +mov $MSR_EFER,%ecx +rdmsr +and $(~EFER_LME),%eax +wrmsr I don't think this is really needed. + +pop %ecx +pop %eax + +/* Turn off PAE */ +mov %cr4,%edx +and $(~X86_CR4_PAE),%edx +mov %edx,%cr4 Nor this. @@ -179,7 +334,7 @@ multiboot2_proto: and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx jmp 0b -trampoline_setup: +bios_platform: /* Set up trampoline segment 64k below EBDA */ This is still trampoline setup code, so it's not clear why you rename the label. If you need another named label ... @@ -195,12 +350,13 @@ trampoline_setup: * multiboot structure (if available) and use the smallest. */ cmp $0x100,%edx /* is the multiboot value too small? */ -
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
On Tue, Mar 17, 2015 at 10:32:01AM +, Jan Beulich wrote: On 30.01.15 at 18:54, daniel.ki...@oracle.com wrote: @@ -94,6 +111,17 @@ ENTRY(start) gdt_boot_descr: .word 6*8-1 .long sym_phys(trampoline_gdt) +.long 0 /* Needed for 64-bit lgdt */ + +cs32_switch_addr: +.long sym_phys(cs32_switch) +.long BOOT_CS32 + +efi_st: +.quad 0 + +efi_ih: +.quad 0 .Lbad_cpu_msg: .asciz ERR: Not a 64-bit CPU! .Lbad_ldr_msg: .asciz ERR: Not a Multiboot bootloader! @@ -124,6 +152,133 @@ print_err: .Lhalt: hlt jmp .Lhalt +.code64 + +__efi64_start: +cld + +/* Bootloaders may set multiboot[12].mem_lower to a nonzero value */ +xor %edx,%edx + +/* Check for Multiboot2 bootloader */ +cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax +je efi_multiboot2_proto + +jmp not_multiboot + +efi_multiboot2_proto: jne not_multiboot (and efi_multiboot2_proto dropped altogether) [...] Jan, thanks for your comments. Now I am working on relocatable early Xen code. I hope that I will finish that this week and start tests on this crazy UEFI platforms. Then I get back to this patch series and replay for your and other guys comments. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
On Sat, Feb 14, 2015 at 08:23:45PM +0300, Andrei Borzenkov wrote: В Wed, 11 Feb 2015 08:20:04 + Jan Beulich jbeul...@suse.com пишет: On 10.02.15 at 22:27, daniel.ki...@oracle.com wrote: After some testing we have found at least one machine on which this thing does not work. It is Dell PowerEdge R820 with latest firmware. Machine crashes/stops because early 32-bit code is not relocatable and must live under 0x10 address. (side note: I am surprised how it worked without any issue until now; Multiboot protocol, any version, does not guarantee that OS image will be loaded at specified/requested address; How does it not? It's an ELF binary without relocations that's being loaded - I can't see how such could be validly loaded anywhere but at the virtual address(es) its program header states (and I don't know whether grub [1 or 2] would correctly process relocations if there were any, but I doubt it). grub2 relocates own modules when loading them. It does not do Great! However, it also ignores MULTIBOOT_TAG_TYPE_EFI_BS flag and overwrite BS if it leaves in region requested by image. It is a bug which I have just discovered. I will fix it. relocation when loading Xen binary, but it does not sound impossible. Ugh... You are right. I was confused because multiboot2 command just allocate memory outside reserved regions. I thought that this is final destination. However, later if you execute boot command then image is relocated to final destination. Facepalm! Anyway, we must support relocatable images in multiboot2 protocol. I think that image (any format, e.g. PE) could inform loader that it can live anywhere below 4 GiB by setting special flag in multiboot2 header. Or ELF image could have relocation section which would be interpreted by loader. Both approaches make sense and are feasible. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
В Wed, 11 Feb 2015 08:20:04 + Jan Beulich jbeul...@suse.com пишет: On 10.02.15 at 22:27, daniel.ki...@oracle.com wrote: After some testing we have found at least one machine on which this thing does not work. It is Dell PowerEdge R820 with latest firmware. Machine crashes/stops because early 32-bit code is not relocatable and must live under 0x10 address. (side note: I am surprised how it worked without any issue until now; Multiboot protocol, any version, does not guarantee that OS image will be loaded at specified/requested address; How does it not? It's an ELF binary without relocations that's being loaded - I can't see how such could be validly loaded anywhere but at the virtual address(es) its program header states (and I don't know whether grub [1 or 2] would correctly process relocations if there were any, but I doubt it). grub2 relocates own modules when loading them. It does not do relocation when loading Xen binary, but it does not sound impossible. Now I see two solutions for these issues: 1) We can make early 32-bit code relocatable. We may use something similar to xen/arch/x86/boot/trampoline.S:bootsym_rel(). Additionally, I think that early code should not blindly map first 16 MiB of memory. It should map first 1 MiB of memory and then 16 MiB of memory starting from xen_phys_start. This way we also fix long standing bug in early code which I described earlier. 2) We can jump from EFI x86-64 mode directly into Xen x86-64 mode like it is done in case of EFI loader. However, then we must duplicate multiboot2 protocol implementation in x86-64 mode (if we wish that multiboot2 protocol can be used on legacy BIOS and EFI platforms; I think that we should support this protocol on both for users convenience). Additionally, we must use a workaround to relocate trampoline if boot services uses memory below 1 MiB (please check commit c1f2dfe8f6a559bc28935f24e31bb33d17d9713d, x86/EFI: make trampoline allocation more flexible, for more details). I prefer #1 because this way we do not duplicate multiboot2 protocol implementation (one for legacy BIOS and EFI) and we avoid issues with trampoline relocation when low memory is occupied by boot services and/or 1:1 EFI page tables. Between the two, 1 is certainly the preferable option. PS I have just realized that commit c1f2dfe8f6a559bc28935f24e31bb33d17d9713d will not work if trampoline code will overwrite some of EFI 1:1 page tables. Dell PowerEdge R820 store part of 1:1 page tables below 1 MiB. Xen loaded by native EFI loader boots but it is only lucky coincidence that it does not overwrite used entries. So, I tend to go and choose #1 even more. How awful a firmware implementation! On PC-like systems, _nothing_ that _absolutely_ has to be below 1Mb should be placed there. Jan ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
On 10.02.15 at 22:27, daniel.ki...@oracle.com wrote: After some testing we have found at least one machine on which this thing does not work. It is Dell PowerEdge R820 with latest firmware. Machine crashes/stops because early 32-bit code is not relocatable and must live under 0x10 address. (side note: I am surprised how it worked without any issue until now; Multiboot protocol, any version, does not guarantee that OS image will be loaded at specified/requested address; How does it not? It's an ELF binary without relocations that's being loaded - I can't see how such could be validly loaded anywhere but at the virtual address(es) its program header states (and I don't know whether grub [1 or 2] would correctly process relocations if there were any, but I doubt it). Now I see two solutions for these issues: 1) We can make early 32-bit code relocatable. We may use something similar to xen/arch/x86/boot/trampoline.S:bootsym_rel(). Additionally, I think that early code should not blindly map first 16 MiB of memory. It should map first 1 MiB of memory and then 16 MiB of memory starting from xen_phys_start. This way we also fix long standing bug in early code which I described earlier. 2) We can jump from EFI x86-64 mode directly into Xen x86-64 mode like it is done in case of EFI loader. However, then we must duplicate multiboot2 protocol implementation in x86-64 mode (if we wish that multiboot2 protocol can be used on legacy BIOS and EFI platforms; I think that we should support this protocol on both for users convenience). Additionally, we must use a workaround to relocate trampoline if boot services uses memory below 1 MiB (please check commit c1f2dfe8f6a559bc28935f24e31bb33d17d9713d, x86/EFI: make trampoline allocation more flexible, for more details). I prefer #1 because this way we do not duplicate multiboot2 protocol implementation (one for legacy BIOS and EFI) and we avoid issues with trampoline relocation when low memory is occupied by boot services and/or 1:1 EFI page tables. Between the two, 1 is certainly the preferable option. PS I have just realized that commit c1f2dfe8f6a559bc28935f24e31bb33d17d9713d will not work if trampoline code will overwrite some of EFI 1:1 page tables. Dell PowerEdge R820 store part of 1:1 page tables below 1 MiB. Xen loaded by native EFI loader boots but it is only lucky coincidence that it does not overwrite used entries. So, I tend to go and choose #1 even more. How awful a firmware implementation! On PC-like systems, _nothing_ that _absolutely_ has to be below 1Mb should be placed there. Jan ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
On Fri, Jan 30, 2015 at 06:54:22PM +0100, Daniel Kiper wrote: Signed-off-by: Daniel Kiper daniel.ki...@oracle.com --- xen/arch/x86/boot/head.S | 174 +++-- xen/arch/x86/efi/efi-boot.h | 29 +++ xen/arch/x86/setup.c | 23 ++--- xen/arch/x86/x86_64/asm-offsets.c |2 + xen/common/efi/boot.c | 11 +++ 5 files changed, 222 insertions(+), 17 deletions(-) After some testing we have found at least one machine on which this thing does not work. It is Dell PowerEdge R820 with latest firmware. Machine crashes/stops because early 32-bit code is not relocatable and must live under 0x10 address. (side note: I am surprised how it worked without any issue until now; Multiboot protocol, any version, does not guarantee that OS image will be loaded at specified/requested address; So, it looks that there are not any legacy BIOS machines with e.g. 1 MiB - 2 MiB region reserved in the wild or they are not very common; Am I missing something?). Sadly, this region is used by BS, so, GRUB2 loads Xen higher (at least above 64 MiB). Please look at memory map on this machine: Type StartEnd # Pages Attributes BS_Data0001-0009 0090 000F BS_Data0010-03FF 3F00 000F Available 0400-0FFFEFFF BFFF 000F BS_Code0000-1006CFFF 006E 000F Available 1006D000-B3E73FFF 000A3E07 000F [...] Additionally, early Xen boot code maps only first 16 MiB of memory. Hence, it means that jump into __high_start fails immediately. Now I see two solutions for these issues: 1) We can make early 32-bit code relocatable. We may use something similar to xen/arch/x86/boot/trampoline.S:bootsym_rel(). Additionally, I think that early code should not blindly map first 16 MiB of memory. It should map first 1 MiB of memory and then 16 MiB of memory starting from xen_phys_start. This way we also fix long standing bug in early code which I described earlier. 2) We can jump from EFI x86-64 mode directly into Xen x86-64 mode like it is done in case of EFI loader. However, then we must duplicate multiboot2 protocol implementation in x86-64 mode (if we wish that multiboot2 protocol can be used on legacy BIOS and EFI platforms; I think that we should support this protocol on both for users convenience). Additionally, we must use a workaround to relocate trampoline if boot services uses memory below 1 MiB (please check commit c1f2dfe8f6a559bc28935f24e31bb33d17d9713d, x86/EFI: make trampoline allocation more flexible, for more details). I prefer #1 because this way we do not duplicate multiboot2 protocol implementation (one for legacy BIOS and EFI) and we avoid issues with trampoline relocation when low memory is occupied by boot services and/or 1:1 EFI page tables. Daniel PS I have just realized that commit c1f2dfe8f6a559bc28935f24e31bb33d17d9713d will not work if trampoline code will overwrite some of EFI 1:1 page tables. Dell PowerEdge R820 store part of 1:1 page tables below 1 MiB. Xen loaded by native EFI loader boots but it is only lucky coincidence that it does not overwrite used entries. So, I tend to go and choose #1 even more. ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
On 10/02/2015 21:27, Daniel Kiper wrote:
On Fri, Jan 30, 2015 at 06:54:22PM +0100, Daniel Kiper wrote:
Signed-off-by: Daniel Kiper daniel.ki...@oracle.com
---
xen/arch/x86/boot/head.S | 174
+++--
xen/arch/x86/efi/efi-boot.h | 29 +++
xen/arch/x86/setup.c | 23 ++---
xen/arch/x86/x86_64/asm-offsets.c |2 +
xen/common/efi/boot.c | 11 +++
5 files changed, 222 insertions(+), 17 deletions(-)
After some testing we have found at least one machine on which this thing
does not work. It is Dell PowerEdge R820 with latest firmware. Machine
crashes/stops because early 32-bit code is not relocatable and must live
under 0x10 address. (side note: I am surprised how it worked without
any issue until now; Multiboot protocol, any version, does not guarantee
that OS image will be loaded at specified/requested address; So, it looks
that there are not any legacy BIOS machines with e.g. 1 MiB - 2 MiB region
reserved in the wild or they are not very common; Am I missing something?).
Sadly, this region is used by BS, so, GRUB2 loads Xen higher (at least
above 64 MiB). Please look at memory map on this machine:
Type StartEnd # Pages Attributes
BS_Data0001-0009 0090 000F
BS_Data0010-03FF 3F00 000F
Available 0400-0FFFEFFF BFFF 000F
BS_Code0000-1006CFFF 006E 000F
Available 1006D000-B3E73FFF 000A3E07 000F
[...]
Additionally, early Xen boot code maps only first 16 MiB of memory. Hence,
it means that jump into __high_start fails immediately.
Now I see two solutions for these issues:
1) We can make early 32-bit code relocatable. We may use something similar
to xen/arch/x86/boot/trampoline.S:bootsym_rel(). Additionally, I think
that early code should not blindly map first 16 MiB of memory. It should
map first 1 MiB of memory and then 16 MiB of memory starting from
xen_phys_start. This way we also fix long standing bug in early code
which I described earlier.
2) We can jump from EFI x86-64 mode directly into Xen x86-64 mode like
it is done in case of EFI loader. However, then we must duplicate
multiboot2
protocol implementation in x86-64 mode (if we wish that multiboot2 protocol
can be used on legacy BIOS and EFI platforms; I think that we should
support
this protocol on both for users convenience). Additionally, we must use
a workaround to relocate trampoline if boot services uses memory below 1
MiB
(please check commit c1f2dfe8f6a559bc28935f24e31bb33d17d9713d, x86/EFI:
make
trampoline allocation more flexible, for more details).
I prefer #1 because this way we do not duplicate multiboot2 protocol
implementation
(one for legacy BIOS and EFI) and we avoid issues with trampoline relocation
when
low memory is occupied by boot services and/or 1:1 EFI page tables.
Daniel
PS I have just realized that commit c1f2dfe8f6a559bc28935f24e31bb33d17d9713d
will not work if trampoline code will overwrite some of EFI 1:1 page
tables.
Dell PowerEdge R820 store part of 1:1 page tables below 1 MiB. Xen loaded
by native EFI loader boots but it is only lucky coincidence that it does
not overwrite used entries. So, I tend to go and choose #1 even more.
I have to admit to also being surprised at the fragility of the Xen, and
how it cannot function if _start isn't loaded on the 1MB boundary.
Given that there is provably one system in the wild which causes us to
fall over this limitation, it clearly needs fixing.
I would also agree that making the entry point relocatable is the
correct way forwards. However, you cannot use something like
bootsym_rel() as that requires infrastructure to patch the references
(observe the loop over __trampoline_rel_{start,end} just before the call
to cmdline_parse_early())
This can probably be achieved by having a small bit of hand-written PIC
which puts an appropriate offset into %ds.
~Andrew
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
On 30/01/15 17:54, Daniel Kiper wrote: Signed-off-by: Daniel Kiper daniel.ki...@oracle.com --- xen/arch/x86/boot/head.S | 174 +++-- xen/arch/x86/efi/efi-boot.h | 29 +++ xen/arch/x86/setup.c | 23 ++--- xen/arch/x86/x86_64/asm-offsets.c |2 + xen/common/efi/boot.c | 11 +++ 5 files changed, 222 insertions(+), 17 deletions(-) diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 7861057..89f5aa7 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -8,6 +8,7 @@ #include asm/page.h #include asm/msr.h #include asm/cpufeature.h +#include asm/processor.h .text .code32 @@ -57,6 +58,9 @@ ENTRY(start) .long .Lmultiboot2_info_req_end - .Lmultiboot2_info_req .long MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO .long MULTIBOOT2_TAG_TYPE_MMAP +.long MULTIBOOT2_TAG_TYPE_EFI_BS +.long MULTIBOOT2_TAG_TYPE_EFI64 +.long MULTIBOOT2_TAG_TYPE_EFI64_IH .Lmultiboot2_info_req_end: .align MULTIBOOT2_TAG_ALIGN @@ -80,6 +84,19 @@ ENTRY(start) .long 0 /* Number of the lines - no preference. */ .long 0 /* Number of bits per pixel - no preference. */ +/* Do not disable EFI boot services. */ +.align MULTIBOOT2_TAG_ALIGN +.short MULTIBOOT2_HEADER_TAG_EFI_BS +.short MULTIBOOT2_HEADER_TAG_OPTIONAL +.long 8 /* Tag size. */ + +/* EFI64 entry point. */ +.align MULTIBOOT2_TAG_ALIGN +.short MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS_EFI64 +.short MULTIBOOT2_HEADER_TAG_OPTIONAL +.long 12 /* Tag size. */ +.long sym_phys(__efi64_start) + /* Multiboot2 header end tag. */ .align MULTIBOOT2_TAG_ALIGN .short MULTIBOOT2_HEADER_TAG_END @@ -94,6 +111,17 @@ ENTRY(start) gdt_boot_descr: .word 6*8-1 .long sym_phys(trampoline_gdt) +.long 0 /* Needed for 64-bit lgdt */ + +cs32_switch_addr: +.long sym_phys(cs32_switch) +.long BOOT_CS32 .word ljmpl refers to an m32:16 not an m32:32 + +efi_st: +.quad 0 + +efi_ih: +.quad 0 .Lbad_cpu_msg: .asciz ERR: Not a 64-bit CPU! .Lbad_ldr_msg: .asciz ERR: Not a Multiboot bootloader! @@ -124,6 +152,133 @@ print_err: .Lhalt: hlt jmp .Lhalt +.code64 + +__efi64_start: +cld + +/* Bootloaders may set multiboot[12].mem_lower to a nonzero value */ +xor %edx,%edx + +/* Check for Multiboot2 bootloader */ +cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax +je efi_multiboot2_proto + +jmp not_multiboot not_multiboot is 32bit code. You must drop out of 64bit before using it, or make a 64bit variant. + +efi_multiboot2_proto: +/* Skip Multiboot2 information fixed part */ +lea MB2_fixed_sizeof(%ebx),%ecx + +0: +/* Get mem_lower from Multiboot2 information */ +cmpl$MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx) +jne 1f + +mov MB2_mem_lower(%ecx),%edx +jmp 4f + +1: +/* Get EFI SystemTable address from Multiboot2 information */ +cmpl$MULTIBOOT2_TAG_TYPE_EFI64,(%ecx) +jne 2f + +lea MB2_efi64_st(%ecx),%esi +lea efi_st(%rip),%edi +movsq This is complete overkill for copying a 64bit variable out of the tag and into a local variable. Just use a plain 64bit load and store. + +/* Do not go into real mode on EFI platform */ +movb$1,skip_realmode(%rip) + +jmp 4f + +2: +/* Get EFI ImageHandle address from Multiboot2 information */ +cmpl$MULTIBOOT2_TAG_TYPE_EFI64_IH,(%ecx) +jne 3f + +lea MB2_efi64_ih(%ecx),%esi +lea efi_ih(%rip),%edi +movsq And here. +jmp 4f + +3: +/* Is it the end of Multiboot2 information? */ +cmpl$MULTIBOOT2_TAG_TYPE_END,(%ecx) +je run_bs + +4: +/* Go to next Multiboot2 information tag */ +add MB2_tag_size(%ecx),%ecx +add $(MULTIBOOT2_TAG_ALIGN-1),%ecx +and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx +jmp 0b + +run_bs: +push%rax +push%rdx Does the EFI spec guarantee that we have a good stack to use at this point? + +/* Initialize BSS (no nasty surprises!) */ +lea __bss_start(%rip),%rdi +lea _end(%rip),%rcx +sub %rdi,%rcx +xor %rax,%rax xor %eax,%eax is shorter. +rep stosb It would be more efficient to make sure that the linker aligns __bss_start and _end on 8 byte boundaries, and use stosq instead. + +mov efi_ih(%rip),%rdi /* EFI ImageHandle */
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
On 30/01/2015 23:43, Daniel Kiper wrote: On Fri, Jan 30, 2015 at 07:06:53PM +, Andrew Cooper wrote: On 30/01/15 17:54, Daniel Kiper wrote: + +efi_multiboot2_proto: +/* Skip Multiboot2 information fixed part */ +lea MB2_fixed_sizeof(%ebx),%ecx + +0: +/* Get mem_lower from Multiboot2 information */ +cmpl$MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx) +jne 1f + +mov MB2_mem_lower(%ecx),%edx +jmp 4f + +1: +/* Get EFI SystemTable address from Multiboot2 information */ +cmpl$MULTIBOOT2_TAG_TYPE_EFI64,(%ecx) +jne 2f + +lea MB2_efi64_st(%ecx),%esi +lea efi_st(%rip),%edi +movsq This is complete overkill for copying a 64bit variable out of the tag and into a local variable. Just use a plain 64bit load and store. I am not sure what do you mean by 64bit load and store but I have just realized that we do not need these variables. They are remnants from early developments when I thought that we need ImageHandle and SystemTable here and later somewhere else. mov MB2_efi64_st(%rcx), %rdi mov %rdi, efi_st(%rip) But if they are not needed, drop the code completely. +jmp 4f + +3: +/* Is it the end of Multiboot2 information? */ +cmpl$MULTIBOOT2_TAG_TYPE_END,(%ecx) +je run_bs + +4: +/* Go to next Multiboot2 information tag */ +add MB2_tag_size(%ecx),%ecx +add $(MULTIBOOT2_TAG_ALIGN-1),%ecx +and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx +jmp 0b + +run_bs: +push%rax +push%rdx Does the EFI spec guarantee that we have a good stack to use at this point? Unified Extensible Firmware Interface Specification, Version 2.4 Errata B, section 2.3.4, x64 Platforms says: During boot services time the processor is in the following execution mode: ..., 128 KiB, or more, of available stack space. GRUB2 uses this stack too and do not move it to different memory region. So, I think that here we are on safe side. Sounds ok then. +/* Initialize BSS (no nasty surprises!) */ +lea __bss_start(%rip),%rdi +lea _end(%rip),%rcx +sub %rdi,%rcx +xor %rax,%rax xor %eax,%eax is shorter. +rep stosb It would be more efficient to make sure that the linker aligns __bss_start and _end on 8 byte boundaries, and use stosq instead. Right but just for this. Is it pays? We do this only once. The BSS in Xen is 300k. It is absolutely better to clear it 8 bytes at a time rather than 1. However, if you wish... +mov efi_ih(%rip),%rdi /* EFI ImageHandle */ +mov efi_st(%rip),%rsi /* EFI SystemTable */ +callefi_multiboot2 + +pop %rcx +pop %rax + +shl $10-4,%rcx /* Convert multiboot2.mem_lower to bytes/16 */ + +cli This looks suspiciously out of place. Surely the EFI spec doesn't permit entry with interrupts enabled? Unified Extensible Firmware Interface Specification, Version 2.4 Errata B, section 2.3.4, x64 Platforms says: During boot services time the processor is in the following execution mode: ..., Interrupts are enabled–though no interrupt services are supported other than the UEFI boot services timer functions (All loaded device drivers are serviced synchronously by “polling.”). So, I think that we should use BS with interrupts enabled and disable them after ExitBootServices(). Hmmm... Now I think that we should use cli immediately after efi_multiboot2() call. I presume then that the firmware has set up a valid idt somewhere and is actually serving any interrupts we get. diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index f8be3dd..c5725ca 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -75,6 +75,17 @@ static size_t wstrlen(const CHAR16 * s); static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2); +static void efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable); +static void efi_console_set_mode(void); +static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(void); +static UINTN efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, + UINTN cols, UINTN rows, UINTN depth); +static void efi_tables(void); +static void setup_efi_pci(void); +static void efi_variables(void); +static void efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop_mode); +static void efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable); + If any of these forward declarations are needed, they should be They are needed because efi-boot.h is included before above mentioned functions definitions. introduced in the appropriate create efi_$FOO patch. However, I can't I thought about that during development.
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
On Fri, Jan 30, 2015 at 07:06:53PM +, Andrew Cooper wrote: On 30/01/15 17:54, Daniel Kiper wrote: Signed-off-by: Daniel Kiper daniel.ki...@oracle.com --- xen/arch/x86/boot/head.S | 174 +++-- xen/arch/x86/efi/efi-boot.h | 29 +++ xen/arch/x86/setup.c | 23 ++--- xen/arch/x86/x86_64/asm-offsets.c |2 + xen/common/efi/boot.c | 11 +++ 5 files changed, 222 insertions(+), 17 deletions(-) diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 7861057..89f5aa7 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -8,6 +8,7 @@ #include asm/page.h #include asm/msr.h #include asm/cpufeature.h +#include asm/processor.h .text .code32 @@ -57,6 +58,9 @@ ENTRY(start) .long .Lmultiboot2_info_req_end - .Lmultiboot2_info_req .long MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO .long MULTIBOOT2_TAG_TYPE_MMAP +.long MULTIBOOT2_TAG_TYPE_EFI_BS +.long MULTIBOOT2_TAG_TYPE_EFI64 +.long MULTIBOOT2_TAG_TYPE_EFI64_IH .Lmultiboot2_info_req_end: .align MULTIBOOT2_TAG_ALIGN @@ -80,6 +84,19 @@ ENTRY(start) .long 0 /* Number of the lines - no preference. */ .long 0 /* Number of bits per pixel - no preference. */ +/* Do not disable EFI boot services. */ +.align MULTIBOOT2_TAG_ALIGN +.short MULTIBOOT2_HEADER_TAG_EFI_BS +.short MULTIBOOT2_HEADER_TAG_OPTIONAL +.long 8 /* Tag size. */ + +/* EFI64 entry point. */ +.align MULTIBOOT2_TAG_ALIGN +.short MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS_EFI64 +.short MULTIBOOT2_HEADER_TAG_OPTIONAL +.long 12 /* Tag size. */ +.long sym_phys(__efi64_start) + /* Multiboot2 header end tag. */ .align MULTIBOOT2_TAG_ALIGN .short MULTIBOOT2_HEADER_TAG_END @@ -94,6 +111,17 @@ ENTRY(start) gdt_boot_descr: .word 6*8-1 .long sym_phys(trampoline_gdt) +.long 0 /* Needed for 64-bit lgdt */ + +cs32_switch_addr: +.long sym_phys(cs32_switch) +.long BOOT_CS32 .word ljmpl refers to an m32:16 not an m32:32 + +efi_st: +.quad 0 + +efi_ih: +.quad 0 .Lbad_cpu_msg: .asciz ERR: Not a 64-bit CPU! .Lbad_ldr_msg: .asciz ERR: Not a Multiboot bootloader! @@ -124,6 +152,133 @@ print_err: .Lhalt: hlt jmp .Lhalt +.code64 + +__efi64_start: +cld + +/* Bootloaders may set multiboot[12].mem_lower to a nonzero value */ +xor %edx,%edx + +/* Check for Multiboot2 bootloader */ +cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax +je efi_multiboot2_proto + +jmp not_multiboot not_multiboot is 32bit code. You must drop out of 64bit before using it, or make a 64bit variant. + +efi_multiboot2_proto: +/* Skip Multiboot2 information fixed part */ +lea MB2_fixed_sizeof(%ebx),%ecx + +0: +/* Get mem_lower from Multiboot2 information */ +cmpl$MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx) +jne 1f + +mov MB2_mem_lower(%ecx),%edx +jmp 4f + +1: +/* Get EFI SystemTable address from Multiboot2 information */ +cmpl$MULTIBOOT2_TAG_TYPE_EFI64,(%ecx) +jne 2f + +lea MB2_efi64_st(%ecx),%esi +lea efi_st(%rip),%edi +movsq This is complete overkill for copying a 64bit variable out of the tag and into a local variable. Just use a plain 64bit load and store. I am not sure what do you mean by 64bit load and store but I have just realized that we do not need these variables. They are remnants from early developments when I thought that we need ImageHandle and SystemTable here and later somewhere else. +/* Do not go into real mode on EFI platform */ +movb$1,skip_realmode(%rip) + +jmp 4f + +2: +/* Get EFI ImageHandle address from Multiboot2 information */ +cmpl$MULTIBOOT2_TAG_TYPE_EFI64_IH,(%ecx) +jne 3f + +lea MB2_efi64_ih(%ecx),%esi +lea efi_ih(%rip),%edi +movsq And here. Ditto. +jmp 4f + +3: +/* Is it the end of Multiboot2 information? */ +cmpl$MULTIBOOT2_TAG_TYPE_END,(%ecx) +je run_bs + +4: +/* Go to next Multiboot2 information tag */ +add MB2_tag_size(%ecx),%ecx +add $(MULTIBOOT2_TAG_ALIGN-1),%ecx +and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx +jmp 0b + +run_bs: +push%rax +push%rdx Does the EFI spec guarantee that we have a good stack to
