Re: [PATCH 16/18] efi: create efi_exit_boot()
>>> On 30.01.15 at 18:54, wrote: > ..which gets memory map and calls ExitBootServices(). We need this > to support multiboot2 protocol on EFI platforms. Patches from 9 up to here all make sense on the basis that patch 18 does and assuming that you really need all this code moved out to separate functions. How much different is efi_multiboot2() introduced in #18 from what is left of efi_start() at this point? I.e. is splitting out all of this code really needed? If it is, please don't title all these patches "create ..." but "split out ..." or some such - you don't really create the code. Similarly the second sentence above is too imprecise for my taste - "we want to re-use this code to support ..." would seem more to the point. Jan ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 16/18] efi: create efi_exit_boot()
On Mon, Mar 02, 2015 at 04:45:47PM +, Jan Beulich wrote: > >>> On 30.01.15 at 18:54, wrote: > > ..which gets memory map and calls ExitBootServices(). We need this > > to support multiboot2 protocol on EFI platforms. > > Patches from 9 up to here all make sense on the basis that patch 18 > does and assuming that you really need all this code moved out to > separate functions. How much different is efi_multiboot2() introduced > in #18 from what is left of efi_start() at this point? I.e. is splitting out More or less efi_multiboot2() does not parse command line and do not load modules itself as efi_start() does. > all of this code really needed? I think that it is worth doing. First of all efi_start() is huge and its analysis is very difficult right now. So, splitting code into smaller chunks will improve readability a lot (I am still thinking about extracting command line parser and module loader from efi_start() even if both functions will be used only in efi_start(); this way we will have very simple functions doing one thing easy to understand). Additionally, we create pieces which are very easy to reuse in efi_multiboot2() which is very simple and again easy for analysis. Potentially we can reuse efi_start() in multiboot2 case. However, I prefer to have separate function because this way it is clear that multiboot2 case is different thing then native EFI loader stuff. Additionally, efi_start() is architecture independent and efi_multiboot2() is x86 only and it should live in x86 files. > If it is, please don't title all these patches "create ..." but "split out > ..." or some such - you don't really create the code. Similarly the > second sentence above is too imprecise for my taste - "we want to > re-use this code to support ..." would seem more to the point. OK. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 16/18] efi: create efi_exit_boot()
>>> On 27.03.15 at 13:00, wrote: > Additionally, efi_start() > is architecture independent and efi_multiboot2() is x86 only and it should > live in x86 files. Is that really the case? Looking at the grub2 sources I see support for other than x86... Jan ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 16/18] efi: create efi_exit_boot()
On Fri, Mar 27, 2015 at 12:10:56PM +, Jan Beulich wrote: > >>> On 27.03.15 at 13:00, wrote: > > Additionally, efi_start() > > is architecture independent and efi_multiboot2() is x86 only and it should > > live in x86 files. > > Is that really the case? Looking at the grub2 sources I see support > for other than x86... Well... In theory multiboot protocol was designed as arch independet. However, docs define more precisely stuff for i386 and MIPS (multiboot2 protocol only). As I know we do not care about MIPS. Additionally, so called muliboot protocol for ARM is completely different thing then multiboot protocols mentioned above (it is a mixture of EFI native loader with DT). So, it looks that from our point of view efi_multiboot2() is x86 only stuff (right now, I am not fortune teller, so, I am not able to tell what happens in the future ;-))) ). Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 16/18] efi: create efi_exit_boot()
On Fri, 2015-03-27 at 13:43 +0100, Daniel Kiper wrote: > On Fri, Mar 27, 2015 at 12:10:56PM +, Jan Beulich wrote: > > >>> On 27.03.15 at 13:00, wrote: > > > Additionally, efi_start() > > > is architecture independent and efi_multiboot2() is x86 only and it should > > > live in x86 files. > > > > Is that really the case? Looking at the grub2 sources I see support > > for other than x86... > > Well... In theory multiboot protocol was designed as arch independet. > However, docs define more precisely stuff for i386 and MIPS (multiboot2 > protocol only). As I know we do not care about MIPS. Additionally, so > called muliboot protocol for ARM is completely different thing then > multiboot protocols mentioned above The ARM multiboot DT thing fits into the multiboot1 "namespace", since it is unlikely there will ever be an ARM multiboot1. It's possible that someone might spec and implement multiboot2 for ARM as well, although whether than has any impact on the comments made in this threadlet I've no idea.. Ian. ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
