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

[Roy Franz <roy.franz@xxxxxxxxxx>]

On Wed, Jul 23, 2014 at 9:31 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 22.07.14 at 02:43, <roy.franz@xxxxxxxxxx> wrote:
>> Create the files for EFI code that will be shared with the ARM stub,
>> and move string functions and some global variables.
>>
>> Signed-off-by: Roy Franz <roy.franz@xxxxxxxxxx>
>> ---
>>  xen/arch/x86/Rules.mk        |   1 +
>>  xen/arch/x86/efi/boot.c      | 127 +++-----------------------------------
>>  xen/common/Makefile          |   2 +
>>  xen/common/efi/Makefile      |   3 +
>>  xen/common/efi/efi-shared.c  | 142 
>> +++++++++++++++++++++++++++++++++++++++++++
>
> This clearly should be just efi.c or, provided you keep the separation
> between boot and runtime code, boot.c.
>
>>  xen/include/efi/efi-shared.h |  50 +++++++++++++++
>
> This one should at least get the efi- prefix dropped as being redundant
> with the efi/ one, or even be named internal.h.
>
>> --- a/xen/arch/x86/efi/boot.c
>> +++ b/xen/arch/x86/efi/boot.c
>> @@ -1,6 +1,7 @@
>>  #include "efi.h"
>>  #include <efi/efiprot.h>
>>  #include <efi/efipciio.h>
>> +#include <efi/efi-shared.h>
>>  #include <public/xen.h>
>>  #include <xen/compile.h>
>>  #include <xen/ctype.h>
>> @@ -44,25 +45,16 @@ typedef struct {
>>  extern char start[];
>>  extern u32 cpuid_ext_features;
>>
>> -union string {
>> -    CHAR16 *w;
>> -    char *s;
>> -    const char *cs;
>> -};
>>
>> -struct file {
>> -    UINTN size;
>> -    union {
>> -        EFI_PHYSICAL_ADDRESS addr;
>> -        void *ptr;
>> -    };
>> -};
>> +/* Variables supplied/used by shared EFI code. */
>> +extern CHAR16 __initdata newline[];
>> +extern EFI_BOOT_SERVICES *__initdata efi_bs;
>> +extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
>> +extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdErr;
>
> Why are these declarations not being moved into the shared
> header? Plus, with them being just declarations, they should not
> have the __initdata tags. And, with them being external now, they
> should be properly prefixed with efi_ or Efi.

moved/fixed
>
>> +
>>
>
> Please avoid introducing double blank lines (not just here).
>
>> -static EFI_BOOT_SERVICES *__initdata efi_bs;
>>  static EFI_HANDLE __initdata efi_ih;
>>
>> -static SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
>> -static SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdErr;
>
> In the end I'm not really happy anyway with them becoming non-
> static - rather than building separate .o-s in the common efi/
> directory, would it not be possible to just have the .c files there,
> but include them from the arch ones? This would also address the
> tool chain detection issue you described in the cover letter (without
> the addressing of which I can't see how things will ultimately work).
>
>> --- /dev/null
>> +++ b/xen/common/efi/efi-shared.c
>> @@ -0,0 +1,142 @@
>> +/* EFI code shared between architectures. */
>> +
>> +#include <asm/efibind.h>
>> +#include <efi/efidef.h>
>> +#include <efi/efierr.h>
>> +#include <efi/eficon.h>
>> +#include <efi/efidevp.h>
>> +#include <efi/eficapsule.h>
>> +#include <efi/efiapi.h>
>> +#include <xen/efi.h>
>> +#include <xen/spinlock.h>
>> +#include <asm/page.h>
>> +#include <efi/efiprot.h>
>> +#include <efi/efipciio.h>
>> +#include <efi/efi-shared.h>
>> +#include <public/xen.h>
>> +#include <efi/efi-shared.h>
>> +#include <xen/compile.h>
>> +#include <xen/ctype.h>
>> +#include <xen/init.h>
>> +#include <asm/processor.h>
>
> Looks like you blindly copied all includes - I'd prefer only those to be
> added that are actually needed.

fixed
>
>> --- /dev/null
>> +++ b/xen/include/efi/efi-shared.h
>> @@ -0,0 +1,50 @@
>> +#ifndef __EFI_SHARED_H__
>> +#define __EFI_SHARED_H__
>> +
>> +#include <efi/efidef.h>
>> +#include <xen/init.h>
>> +
>> +
>> +union string {
>> +    CHAR16 *w;
>> +    char *s;
>> +    const char *cs;
>> +};
>> +
>> +struct file {
>> +    UINTN size;
>> +    union {
>> +        EFI_PHYSICAL_ADDRESS addr;
>> +        void *ptr;
>> +    };
>> +};
>> +
>> +
>> +#define PrintStr(s) StdOut->OutputString(StdOut, s)
>> +#define PrintErr(s) StdErr->OutputString(StdErr, s)
>> +
>> +
>> +
>> +CHAR16 * FormatDec(UINT64 Val, CHAR16 *Buffer);
>> +CHAR16 * FormatHex(UINT64 Val, UINTN Width, CHAR16 *Buffer);
>> +
>> +void __init DisplayUint(UINT64 Val, INTN Width);
>> +CHAR16 *__init wstrcpy(CHAR16 *d, const CHAR16 *s);
>> +int __init wstrcmp(const CHAR16 *s1, const CHAR16 *s2);
>> +int __init wstrncmp(const CHAR16 *s1, const CHAR16 *s2, UINTN n);
>> +CHAR16 *__init s2w(union string *str);
>> +char *__init w2s(const union string *str);
>> +bool_t __init match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
>
> Again no __init on declarations please. And hence no need to include
> xen/init.h here.

fixed.
>
> Jan

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