A few EFI code questions

A few EFI code questions

[Daniel Kiper]

Hey,

1) Why is there in EFI code so many functions (e.g. efi_start(),
   efi_arch_edd(), ...) with local variables declared as a static?
   Though some of them have also regular local variables. I do not
   why it was decided that some of them must be the static and
   some of do not. It is a bit confusing. As I can see there is
   only one place which have to have local static (place_string()).
   Other seems to me as thing to save space on the stack but I do
   not think we need that. According to UEFI spec there will be
   "128 KiB or more of available stack space" when system runs in
   boot services mode. It is a lot of space. So, I think we can
   safely convert most of local static variables to normal local
   variables. Am I right?

2) I am going to add EDID support to EFI code. Should it be x86
   specific code or common one? As I can see EDID is defined as
   part of GOP so I think that EDID code should be placed in
   xen/common/efi/boot.c.

3) Should not we change xen/arch/*/efi/efi-boot.h to
   xen/arch/*/efi/efi-boot.c? efi-boot.h contains more
   code than definitions, declarations and short static
   functions. So, I think that it is more regular *.c file
   than header file.

Daniel

Re: A few EFI code questions

[Jan Beulich]

>>> On 03.12.14 at 22:02, <daniel.kiper@oracle.com> wrote:
> Hey,
> 
> 1) Why is there in EFI code so many functions (e.g. efi_start(),
>    efi_arch_edd(), ...) with local variables declared as a static?
>    Though some of them have also regular local variables. I do not
>    why it was decided that some of them must be the static and
>    some of do not. It is a bit confusing. As I can see there is
>    only one place which have to have local static (place_string()).
>    Other seems to me as thing to save space on the stack but I do
>    not think we need that. According to UEFI spec there will be
>    "128 KiB or more of available stack space" when system runs in
>    boot services mode. It is a lot of space. So, I think we can
>    safely convert most of local static variables to normal local
>    variables. Am I right?

No. Consider what code results when you e.g. make an EFI_GUID
instance non-static.

> 2) I am going to add EDID support to EFI code. Should it be x86
>    specific code or common one? As I can see EDID is defined as
>    part of GOP so I think that EDID code should be placed in
>    xen/common/efi/boot.c.

Yes.

> 3) Should not we change xen/arch/*/efi/efi-boot.h to
>    xen/arch/*/efi/efi-boot.c? efi-boot.h contains more
>    code than definitions, declarations and short static
>    functions. So, I think that it is more regular *.c file
>    than header file.

That's a matter of taste - I'd probably have made it .c too, but
didn't mind it being .h as done by Roy (presumably on the basis
that #include directives are preferred to have .h files as their
operands). The only thing I regret is that I didn't ask for the
pointless efi- prefix to be dropped.

Jan

Re: A few EFI code questions

[Roy Franz]

On Thu, Dec 4, 2014 at 1:35 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 03.12.14 at 22:02, <daniel.kiper@oracle.com> wrote:
>> Hey,
>>
>> 1) Why is there in EFI code so many functions (e.g. efi_start(),
>>    efi_arch_edd(), ...) with local variables declared as a static?
>>    Though some of them have also regular local variables. I do not
>>    why it was decided that some of them must be the static and
>>    some of do not. It is a bit confusing. As I can see there is
>>    only one place which have to have local static (place_string()).
>>    Other seems to me as thing to save space on the stack but I do
>>    not think we need that. According to UEFI spec there will be
>>    "128 KiB or more of available stack space" when system runs in
>>    boot services mode. It is a lot of space. So, I think we can
>>    safely convert most of local static variables to normal local
>>    variables. Am I right?
>
> No. Consider what code results when you e.g. make an EFI_GUID
> instance non-static.
>
>> 2) I am going to add EDID support to EFI code. Should it be x86
>>    specific code or common one? As I can see EDID is defined as
>>    part of GOP so I think that EDID code should be placed in
>>    xen/common/efi/boot.c.
>
> Yes.
>
>> 3) Should not we change xen/arch/*/efi/efi-boot.h to
>>    xen/arch/*/efi/efi-boot.c? efi-boot.h contains more
>>    code than definitions, declarations and short static
>>    functions. So, I think that it is more regular *.c file
>>    than header file.
>
> That's a matter of taste - I'd probably have made it .c too, but
> didn't mind it being .h as done by Roy (presumably on the basis
> that #include directives are preferred to have .h files as their
> operands). The only thing I regret is that I didn't ask for the
> pointless efi- prefix to be dropped.

I don't mind a change here, and I agree that it is more like a .c file
than a .h.  If a name change is done, is it worth dropping the "efi-" at
the same time?


>
> Jan
>

Re: A few EFI code questions

[Jan Beulich]

>>> On 04.12.14 at 22:22, <roy.franz@linaro.org> wrote:
> On Thu, Dec 4, 2014 at 1:35 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 03.12.14 at 22:02, <daniel.kiper@oracle.com> wrote:
>>> 3) Should not we change xen/arch/*/efi/efi-boot.h to
>>>    xen/arch/*/efi/efi-boot.c? efi-boot.h contains more
>>>    code than definitions, declarations and short static
>>>    functions. So, I think that it is more regular *.c file
>>>    than header file.
>>
>> That's a matter of taste - I'd probably have made it .c too, but
>> didn't mind it being .h as done by Roy (presumably on the basis
>> that #include directives are preferred to have .h files as their
>> operands). The only thing I regret is that I didn't ask for the
>> pointless efi- prefix to be dropped.
> 
> I don't mind a change here, and I agree that it is more like a .c file
> than a .h.  If a name change is done, is it worth dropping the "efi-" at
> the same time?

If we indeed want to change the name (post 4.5), making both
adjustments at once would be kind of a requirement of mine.

Jan

Re: A few EFI code questions

[Ian Campbell]

On Fri, 2014-12-05 at 07:37 +0000, Jan Beulich wrote:
> >>> On 04.12.14 at 22:22, <roy.franz@linaro.org> wrote:
> > On Thu, Dec 4, 2014 at 1:35 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >>>>> On 03.12.14 at 22:02, <daniel.kiper@oracle.com> wrote:
> >>> 3) Should not we change xen/arch/*/efi/efi-boot.h to
> >>>    xen/arch/*/efi/efi-boot.c? efi-boot.h contains more
> >>>    code than definitions, declarations and short static
> >>>    functions. So, I think that it is more regular *.c file
> >>>    than header file.
> >>
> >> That's a matter of taste - I'd probably have made it .c too, but
> >> didn't mind it being .h as done by Roy (presumably on the basis
> >> that #include directives are preferred to have .h files as their
> >> operands). The only thing I regret is that I didn't ask for the
> >> pointless efi- prefix to be dropped.
> > 
> > I don't mind a change here, and I agree that it is more like a .c file
> > than a .h.  If a name change is done, is it worth dropping the "efi-" at
> > the same time?
> 
> If we indeed want to change the name (post 4.5), making both
> adjustments at once would be kind of a requirement of mine.

Random thought: *.inc for .c files which happen to be embedded into
another using #include?

Ian.

Re: A few EFI code questions

[Jan Beulich]

>>> On 05.12.14 at 10:33, <Ian.Campbell@citrix.com> wrote:
> On Fri, 2014-12-05 at 07:37 +0000, Jan Beulich wrote:
>> >>> On 04.12.14 at 22:22, <roy.franz@linaro.org> wrote:
>> > On Thu, Dec 4, 2014 at 1:35 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>>>> On 03.12.14 at 22:02, <daniel.kiper@oracle.com> wrote:
>> >>> 3) Should not we change xen/arch/*/efi/efi-boot.h to
>> >>>    xen/arch/*/efi/efi-boot.c? efi-boot.h contains more
>> >>>    code than definitions, declarations and short static
>> >>>    functions. So, I think that it is more regular *.c file
>> >>>    than header file.
>> >>
>> >> That's a matter of taste - I'd probably have made it .c too, but
>> >> didn't mind it being .h as done by Roy (presumably on the basis
>> >> that #include directives are preferred to have .h files as their
>> >> operands). The only thing I regret is that I didn't ask for the
>> >> pointless efi- prefix to be dropped.
>> > 
>> > I don't mind a change here, and I agree that it is more like a .c file
>> > than a .h.  If a name change is done, is it worth dropping the "efi-" at
>> > the same time?
>> 
>> If we indeed want to change the name (post 4.5), making both
>> adjustments at once would be kind of a requirement of mine.
> 
> Random thought: *.inc for .c files which happen to be embedded into
> another using #include?

That may conflict with certain editors' language detection, as .inc
may have other meanings (in the x86 Windows world I'd expect this
to be an assembler include file for example).

Jan

Re: A few EFI code questions

[Ian Campbell]

On Fri, 2014-12-05 at 09:47 +0000, Jan Beulich wrote:
> >>> On 05.12.14 at 10:33, <Ian.Campbell@citrix.com> wrote:
> > On Fri, 2014-12-05 at 07:37 +0000, Jan Beulich wrote:
> >> >>> On 04.12.14 at 22:22, <roy.franz@linaro.org> wrote:
> >> > On Thu, Dec 4, 2014 at 1:35 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>>>> On 03.12.14 at 22:02, <daniel.kiper@oracle.com> wrote:
> >> >>> 3) Should not we change xen/arch/*/efi/efi-boot.h to
> >> >>>    xen/arch/*/efi/efi-boot.c? efi-boot.h contains more
> >> >>>    code than definitions, declarations and short static
> >> >>>    functions. So, I think that it is more regular *.c file
> >> >>>    than header file.
> >> >>
> >> >> That's a matter of taste - I'd probably have made it .c too, but
> >> >> didn't mind it being .h as done by Roy (presumably on the basis
> >> >> that #include directives are preferred to have .h files as their
> >> >> operands). The only thing I regret is that I didn't ask for the
> >> >> pointless efi- prefix to be dropped.
> >> > 
> >> > I don't mind a change here, and I agree that it is more like a .c file
> >> > than a .h.  If a name change is done, is it worth dropping the "efi-" at
> >> > the same time?
> >> 
> >> If we indeed want to change the name (post 4.5), making both
> >> adjustments at once would be kind of a requirement of mine.
> > 
> > Random thought: *.inc for .c files which happen to be embedded into
> > another using #include?
> 
> That may conflict with certain editors' language detection, as .inc
> may have other meanings (in the x86 Windows world I'd expect this
> to be an assembler include file for example).

Oh, so does my emacs apparently (a leftover .emacs snippet from a
previous life...). Nevermind that suggestion then.

The existing comment at the top of the included files is probably
sufficient.

Ian.

Re: A few EFI code questions

[Daniel Kiper]

On Thu, Dec 04, 2014 at 09:35:01AM +0000, Jan Beulich wrote:
> >>> On 03.12.14 at 22:02, <daniel.kiper@oracle.com> wrote:
> > Hey,
> >
> > 1) Why is there in EFI code so many functions (e.g. efi_start(),
> >    efi_arch_edd(), ...) with local variables declared as a static?
> >    Though some of them have also regular local variables. I do not
> >    why it was decided that some of them must be the static and
> >    some of do not. It is a bit confusing. As I can see there is
> >    only one place which have to have local static (place_string()).
> >    Other seems to me as thing to save space on the stack but I do
> >    not think we need that. According to UEFI spec there will be
> >    "128 KiB or more of available stack space" when system runs in
> >    boot services mode. It is a lot of space. So, I think we can
> >    safely convert most of local static variables to normal local
> >    variables. Am I right?
>
> No. Consider what code results when you e.g. make an EFI_GUID
> instance non-static.

It could be quite big...

> > 2) I am going to add EDID support to EFI code. Should it be x86
> >    specific code or common one? As I can see EDID is defined as
> >    part of GOP so I think that EDID code should be placed in
> >    xen/common/efi/boot.c.
>
> Yes.

Granted!

> > 3) Should not we change xen/arch/*/efi/efi-boot.h to
> >    xen/arch/*/efi/efi-boot.c? efi-boot.h contains more
> >    code than definitions, declarations and short static
> >    functions. So, I think that it is more regular *.c file
> >    than header file.
>
> That's a matter of taste - I'd probably have made it .c too, but
> didn't mind it being .h as done by Roy (presumably on the basis
> that #include directives are preferred to have .h files as their
> operands). The only thing I regret is that I didn't ask for the
> pointless efi- prefix to be dropped.

As I can see a few people people agree to some extent with my suggestion.
Great! Sadly if we wish .c file than simple boot.c (as Jan suggested we can
drop efi- prefix) conflicts with exiting boot.c link. Is efi-boot.c OK?
Or maybe boot-arch.c? boot.h is OK for sure. Which one do you prefer?
Do you have better ideas?

Daniel

Re: A few EFI code questions

[Jan Beulich]

>>> On 05.12.14 at 15:51, <daniel.kiper@oracle.com> wrote:
> On Thu, Dec 04, 2014 at 09:35:01AM +0000, Jan Beulich wrote:
>> >>> On 03.12.14 at 22:02, <daniel.kiper@oracle.com> wrote:
>> > 3) Should not we change xen/arch/*/efi/efi-boot.h to
>> >    xen/arch/*/efi/efi-boot.c? efi-boot.h contains more
>> >    code than definitions, declarations and short static
>> >    functions. So, I think that it is more regular *.c file
>> >    than header file.
>>
>> That's a matter of taste - I'd probably have made it .c too, but
>> didn't mind it being .h as done by Roy (presumably on the basis
>> that #include directives are preferred to have .h files as their
>> operands). The only thing I regret is that I didn't ask for the
>> pointless efi- prefix to be dropped.
> 
> As I can see a few people people agree to some extent with my suggestion.
> Great! Sadly if we wish .c file than simple boot.c (as Jan suggested we can
> drop efi- prefix) conflicts with exiting boot.c link. Is efi-boot.c OK?
> Or maybe boot-arch.c? boot.h is OK for sure. Which one do you prefer?
> Do you have better ideas?

boot.h would be my preference given how things look like right now,
but I don't think this possibility of renaming warrants a much longer
discussion. Please also remember that renaming always implies more
cumbersome backporting, even if only slightly more.

Jan

Re: A few EFI code questions

[Daniel Kiper]

On Fri, Dec 05, 2014 at 03:00:14PM +0000, Jan Beulich wrote:
> >>> On 05.12.14 at 15:51, <daniel.kiper@oracle.com> wrote:
> > On Thu, Dec 04, 2014 at 09:35:01AM +0000, Jan Beulich wrote:
> >> >>> On 03.12.14 at 22:02, <daniel.kiper@oracle.com> wrote:
> >> > 3) Should not we change xen/arch/*/efi/efi-boot.h to
> >> >    xen/arch/*/efi/efi-boot.c? efi-boot.h contains more
> >> >    code than definitions, declarations and short static
> >> >    functions. So, I think that it is more regular *.c file
> >> >    than header file.
> >>
> >> That's a matter of taste - I'd probably have made it .c too, but
> >> didn't mind it being .h as done by Roy (presumably on the basis
> >> that #include directives are preferred to have .h files as their
> >> operands). The only thing I regret is that I didn't ask for the
> >> pointless efi- prefix to be dropped.
> >
> > As I can see a few people people agree to some extent with my suggestion.
> > Great! Sadly if we wish .c file than simple boot.c (as Jan suggested we can
> > drop efi- prefix) conflicts with exiting boot.c link. Is efi-boot.c OK?
> > Or maybe boot-arch.c? boot.h is OK for sure. Which one do you prefer?
> > Do you have better ideas?
>
> boot.h would be my preference given how things look like right now,

Granted!

> but I don't think this possibility of renaming warrants a much longer
> discussion. Please also remember that renaming always implies more
> cumbersome backporting, even if only slightly more.

I suppose that you are thinking about backporting my EFI + multiboot2
patches somewhere. If you wish I can rename this file after my patch
series or even later to take some fixes for bugs in my code not
discovered earlier. Is it OK for you?

Daniel

Re: A few EFI code questions

[Jan Beulich]

>>> On 05.12.14 at 17:40, <daniel.kiper@oracle.com> wrote:
> On Fri, Dec 05, 2014 at 03:00:14PM +0000, Jan Beulich wrote:
>> but I don't think this possibility of renaming warrants a much longer
>> discussion. Please also remember that renaming always implies more
>> cumbersome backporting, even if only slightly more.
> 
> I suppose that you are thinking about backporting my EFI + multiboot2
> patches somewhere.

Not really, I was just thinking about bug fixes in general.

Jan

Re: A few EFI code questions

[Daniel Kiper]

On Fri, Dec 05, 2014 at 05:00:52PM +0000, Jan Beulich wrote:
> >>> On 05.12.14 at 17:40, <daniel.kiper@oracle.com> wrote:
> > On Fri, Dec 05, 2014 at 03:00:14PM +0000, Jan Beulich wrote:
> >> but I don't think this possibility of renaming warrants a much longer
> >> discussion. Please also remember that renaming always implies more
> >> cumbersome backporting, even if only slightly more.
> >
> > I suppose that you are thinking about backporting my EFI + multiboot2
> > patches somewhere.
>
> Not really, I was just thinking about bug fixes in general.

OK. So, go or no go for efi-boot.h name change to boot.h (of course
after 4.5 release)? If yes then when? After or before my EFI + multiboot2
patches? I would like to know that in advance because I am going to
release first version next week.

Daniel

Re: A few EFI code questions

[Jan Beulich]

>>> On 05.12.14 at 18:59, <daniel.kiper@oracle.com> wrote:
> On Fri, Dec 05, 2014 at 05:00:52PM +0000, Jan Beulich wrote:
>> >>> On 05.12.14 at 17:40, <daniel.kiper@oracle.com> wrote:
>> > On Fri, Dec 05, 2014 at 03:00:14PM +0000, Jan Beulich wrote:
>> >> but I don't think this possibility of renaming warrants a much longer
>> >> discussion. Please also remember that renaming always implies more
>> >> cumbersome backporting, even if only slightly more.
>> >
>> > I suppose that you are thinking about backporting my EFI + multiboot2
>> > patches somewhere.
>>
>> Not really, I was just thinking about bug fixes in general.
> 
> OK. So, go or no go for efi-boot.h name change to boot.h (of course
> after 4.5 release)? If yes then when? After or before my EFI + multiboot2
> patches? I would like to know that in advance because I am going to
> release first version next week.

I'd say a name change (and only that) should be done before
finishing 4.5 or only when (later) there's a real need to change it.

Jan