From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roy Franz Subject: Re: [PATCH V2 01/12] Create efi-shared.[ch], and move string functions Date: Wed, 6 Aug 2014 16:42:29 -0700 Message-ID: References: <1405989815-25236-1-git-send-email-roy.franz@linaro.org> <1405989815-25236-2-git-send-email-roy.franz@linaro.org> <53CFFF74020000780002540C@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53CFFF74020000780002540C@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: keir , Ian Campbell , tim , xen-devel , Stefano Stabellini , linaro-uefi , Fu Wei List-Id: xen-devel@lists.xenproject.org On Wed, Jul 23, 2014 at 9:31 AM, Jan Beulich wrote: >>>> On 22.07.14 at 02:43, 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 >> --- >> 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 >> #include >> +#include >> #include >> #include >> #include >> @@ -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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include > > 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 >> +#include >> + >> + >> +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