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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> +
>  

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.

> --- /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.

Jan

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