Re: [Xen-devel] [PATCH 16/18] efi: create efi_exit_boot()

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Mon, Mar 02, 2015 at 04:45:47PM +0000, Jan Beulich wrote:
> >>> On 30.01.15 at 18:54, <daniel.kiper@xxxxxxxxxx> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel