Re: [PATCH V2 01/12] Create efi-shared.[ch], and move string functions

[Roy Franz <roy.franz@linaro.org>]

On Wed, Aug 6, 2014 at 11:17 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 07.08.14 at 01:55, <roy.franz@linaro.org> wrote:
>> On Mon, Jul 28, 2014 at 9:10 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 28.07.14 at 18:04, <Ian.Campbell@citrix.com> wrote:
>>>> Rather than arch .c files including common .c (or .inc) files how about
>>>> making xen/arch/*/efi/Makefile link xen/commmon/efi/built-in.o into it's
>>>> own built-in.o instead of having xen/common/Makefile do it like would
>>>> normally happen?
>>>
>>> That's an option. But I agree the inclusion of .c in another .c isn't
>>> really nice; I would therefore anyway favor a (set of) arch header
>>> file(s) providing everything the common code can't do on its own.
>>
>> I think I have pretty good handle addressing all of the feedback with
>> the exception of this build issue. Jan - I don't understand your above
>> suggestion.
>
> The process would be
> - move all files containing pieces you want to re-use from arch/x86/efi/
>   to common/efi/, with no other changes than to make them build again
> - possibly in a step by step manner, replace any code portions not
>   suitable for non-x86 by inline functions placed in said header
> - add ARM variants of anything moved to the x86-specific helper
>   header (and possible empty x86 variants of stuff ARM needs that
>   x86 doesn't)
>

OK, I was missing the inline function bit of it.  I took a quick look at
more of what the x86 boot.c code does and I'm still not a fan of this.
I think the differences of device tree versus not, e820 maps, etc will lead
to either conditional code, or awkward helper functions.

>From a more practical view, this would be a major refactor of the x86 code,
which I don't think I could complete in the XEN 4.5 timeline.


>> Another way to handle this would be for the x86 EFI code to copy
>> (or link) the efi-shared.c file locally for building, where it's use would
>> be controlled by the PE/COFF toolchain autodetection.  It seems that
>> some kind of games will need to be played to have the autodetection
>> cooperate with building shared code.
>
> Yes, that would be an option too, with the linking done in a prerequisite
> build step. While I generally dislike this model, I do see its simplicity
> being a possible advantage here.

I'm not a huge fan of it either, but I think it's nicer that
#including a C file,
and it keeps all of the clever makefile bits in the x86/efi directory.  It's the
x86 EFI autoconfig that is complicating the build, so this keeps all
the trickery
together.  The Linux self-decompressor uses this to get various bits of libfdt.

I think the makefile copying the shared c file is better than what I have now,
as it avoids the dead code in the non-efi x86 case (which was a
consequence I had
overlooked at the time.)
I'm going to implement this as part of my next version of the patch
series, which I should
have out early next week.

Roy
>
> Jan
>