| Message ID | 1405989815-25236-2-git-send-email-roy.franz@linaro.org |
|---|---|
| State | New |
| Headers | show |
>>> On 22.07.14 at 02:43, <roy.franz@linaro.org> 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@linaro.org> > --- > 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
On Wed, 2014-07-23 at 17:31 +0100, Jan Beulich wrote: > > -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? That alternative seems pretty gross to me, what is the problem with a global EfiStdOut or something like that? > 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). In the case where the toolchain doesn't EFI won't the unnecessary code built in xen/common/efi simply get discarded by the linker? Even if not it looks like ~20K of mostly __init stuff, which doesn't seem like the end of the world, especially given that more and more toolstacks do support EFI with time. Ian.
>>> On 28.07.14 at 17:41, <Ian.Campbell@citrix.com> wrote: > On Wed, 2014-07-23 at 17:31 +0100, Jan Beulich wrote: >> > -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? > > That alternative seems pretty gross to me, what is the problem with a > global EfiStdOut or something like that? It's not a big problem, but I still prefer to avoid making symbols global whenever possible. >> 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). > > In the case where the toolchain doesn't EFI won't the unnecessary code > built in xen/common/efi simply get discarded by the linker? Not that I'm aware of - afaik no code or data inside a .o would ever get thrown away by the linker without it being specifically asked to do so. > Even if not it looks like ~20K of mostly __init stuff, which doesn't > seem like the end of the world, especially given that more and more > toolstacks do support EFI with time. Right now - with the runtime code not moved over yet - it's mostly __init. Plus (with the linker not being able to discard that code) it carries the risk of having references to symbols that don't exist in the non-EFI build. Jan
On Mon, 2014-07-28 at 16:52 +0100, Jan Beulich wrote: > >> 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). > > > > In the case where the toolchain doesn't EFI won't the unnecessary code > > built in xen/common/efi simply get discarded by the linker? > > Not that I'm aware of - afaik no code or data inside a .o would > ever get thrown away by the linker without it being specifically > asked to do so. Hrm perhaps I'm thinking of one of the whole programmer optimisation things then. > > > Even if not it looks like ~20K of mostly __init stuff, which doesn't > > seem like the end of the world, especially given that more and more > > toolstacks do support EFI with time. > > Right now - with the runtime code not moved over yet - it's > mostly __init. Plus (with the linker not being able to discard that > code) it carries the risk of having references to symbols that > don't exist in the non-EFI build. Perhaps we can put the relevant code into efi specific sections and DTRT in xen.lds.S? Ian.
>>> On 28.07.14 at 17:56, <Ian.Campbell@citrix.com> wrote: > On Mon, 2014-07-28 at 16:52 +0100, Jan Beulich wrote: >> > Even if not it looks like ~20K of mostly __init stuff, which doesn't >> > seem like the end of the world, especially given that more and more >> > toolstacks do support EFI with time. >> >> Right now - with the runtime code not moved over yet - it's >> mostly __init. Plus (with the linker not being able to discard that >> code) it carries the risk of having references to symbols that >> don't exist in the non-EFI build. > > Perhaps we can put the relevant code into efi specific sections and DTRT > in xen.lds.S? Maybe it could be made work, but I'd be wary of linker version issues then. Jan
On Mon, 2014-07-28 at 17:00 +0100, Jan Beulich wrote: > >>> On 28.07.14 at 17:56, <Ian.Campbell@citrix.com> wrote: > > On Mon, 2014-07-28 at 16:52 +0100, Jan Beulich wrote: > >> > Even if not it looks like ~20K of mostly __init stuff, which doesn't > >> > seem like the end of the world, especially given that more and more > >> > toolstacks do support EFI with time. > >> > >> Right now - with the runtime code not moved over yet - it's > >> mostly __init. Plus (with the linker not being able to discard that > >> code) it carries the risk of having references to symbols that > >> don't exist in the non-EFI build. > > > > Perhaps we can put the relevant code into efi specific sections and DTRT > > in xen.lds.S? > > Maybe it could be made work, but I'd be wary of linker version issues > then. 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? Ian.
>>> On 28.07.14 at 18:04, <Ian.Campbell@citrix.com> wrote: > On Mon, 2014-07-28 at 17:00 +0100, Jan Beulich wrote: >> >>> On 28.07.14 at 17:56, <Ian.Campbell@citrix.com> wrote: >> > On Mon, 2014-07-28 at 16:52 +0100, Jan Beulich wrote: >> >> > Even if not it looks like ~20K of mostly __init stuff, which doesn't >> >> > seem like the end of the world, especially given that more and more >> >> > toolstacks do support EFI with time. >> >> >> >> Right now - with the runtime code not moved over yet - it's >> >> mostly __init. Plus (with the linker not being able to discard that >> >> code) it carries the risk of having references to symbols that >> >> don't exist in the non-EFI build. >> > >> > Perhaps we can put the relevant code into efi specific sections and DTRT >> > in xen.lds.S? >> >> Maybe it could be made work, but I'd be wary of linker version issues >> then. > > 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. Jan
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk index 576985e..48f79a5 100644 --- a/xen/arch/x86/Rules.mk +++ b/xen/arch/x86/Rules.mk @@ -13,6 +13,7 @@ HAS_EHCI := y HAS_KEXEC := y HAS_GDBSX := y xenoprof := y +EFI := y # # If you change any of these configuration options then you must diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c index 2b515f2..2b6bea3 100644 --- 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; + -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; static UINT32 __initdata mdesc_ver; @@ -77,111 +69,6 @@ static multiboot_info_t __initdata mbi = { }; static module_t __initdata mb_modules[3]; -static CHAR16 __initdata newline[] = L"\r\n"; - -#define PrintStr(s) StdOut->OutputString(StdOut, s) -#define PrintErr(s) StdErr->OutputString(StdErr, s) - -static CHAR16 *__init FormatDec(UINT64 Val, CHAR16 *Buffer) -{ - if ( Val >= 10 ) - Buffer = FormatDec(Val / 10, Buffer); - *Buffer = (CHAR16)(L'0' + Val % 10); - return Buffer + 1; -} - -static CHAR16 *__init FormatHex(UINT64 Val, UINTN Width, CHAR16 *Buffer) -{ - if ( Width > 1 || Val >= 0x10 ) - Buffer = FormatHex(Val >> 4, Width ? Width - 1 : 0, Buffer); - *Buffer = (CHAR16)((Val &= 0xf) < 10 ? L'0' + Val : L'a' + Val - 10); - return Buffer + 1; -} - -static void __init DisplayUint(UINT64 Val, INTN Width) -{ - CHAR16 PrintString[32], *end; - - if (Width < 0) - end = FormatDec(Val, PrintString); - else - { - PrintStr(L"0x"); - end = FormatHex(Val, Width, PrintString); - } - *end = 0; - PrintStr(PrintString); -} - -static CHAR16 *__init wstrcpy(CHAR16 *d, const CHAR16 *s) -{ - CHAR16 *r = d; - - while ( (*d++ = *s++) != 0 ) - ; - return r; -} - -static int __init wstrcmp(const CHAR16 *s1, const CHAR16 *s2) -{ - while ( *s1 && *s1 == *s2 ) - { - ++s1; - ++s2; - } - return *s1 - *s2; -} - -static int __init wstrncmp(const CHAR16 *s1, const CHAR16 *s2, UINTN n) -{ - while ( n && *s1 && *s1 == *s2 ) - { - --n; - ++s1; - ++s2; - } - return n ? *s1 - *s2 : 0; -} - -static CHAR16 *__init s2w(union string *str) -{ - const char *s = str->s; - CHAR16 *w; - void *ptr; - - if ( efi_bs->AllocatePool(EfiLoaderData, (strlen(s) + 1) * sizeof(*w), - &ptr) != EFI_SUCCESS ) - return NULL; - - w = str->w = ptr; - do { - *w = *s++; - } while ( *w++ ); - - return str->w; -} - -static char *__init w2s(const union string *str) -{ - const CHAR16 *w = str->w; - char *s = str->s; - - do { - if ( *w > 0x007f ) - return NULL; - *s = *w++; - } while ( *s++ ); - - return str->s; -} - -static bool_t __init match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2) -{ - return guid1->Data1 == guid2->Data1 && - guid1->Data2 == guid2->Data2 && - guid1->Data3 == guid2->Data3 && - !memcmp(guid1->Data4, guid2->Data4, sizeof(guid1->Data4)); -} static void __init noreturn blexit(const CHAR16 *str) { diff --git a/xen/common/Makefile b/xen/common/Makefile index 3683ae3..42db42f 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -66,5 +66,7 @@ subdir-$(x86_64) += hvm subdir-$(coverage) += gcov +subdir-$(EFI) += efi + subdir-y += libelf subdir-$(HAS_DEVICE_TREE) += libfdt diff --git a/xen/common/efi/Makefile b/xen/common/efi/Makefile new file mode 100644 index 0000000..c724ac2 --- /dev/null +++ b/xen/common/efi/Makefile @@ -0,0 +1,3 @@ +obj-y += efi-shared.o + +CFLAGS += -fshort-wchar diff --git a/xen/common/efi/efi-shared.c b/xen/common/efi/efi-shared.c new file mode 100644 index 0000000..b990292 --- /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> + + +SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut; +SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdErr; +EFI_BOOT_SERVICES *__initdata efi_bs; + + +CHAR16 __initdata newline[] = L"\r\n"; + +CHAR16 *__init FormatDec(UINT64 Val, CHAR16 *Buffer) +{ + if ( Val >= 10 ) + Buffer = FormatDec(Val / 10, Buffer); + *Buffer = (CHAR16)(L'0' + Val % 10); + return Buffer + 1; +} + +CHAR16 *__init FormatHex(UINT64 Val, UINTN Width, CHAR16 *Buffer) +{ + if ( Width > 1 || Val >= 0x10 ) + Buffer = FormatHex(Val >> 4, Width ? Width - 1 : 0, Buffer); + *Buffer = (CHAR16)((Val &= 0xf) < 10 ? L'0' + Val : L'a' + Val - 10); + return Buffer + 1; +} + + +void __init DisplayUint(UINT64 Val, INTN Width) +{ + CHAR16 PrintString[32], *end; + + if ( Width < 0 ) + end = FormatDec(Val, PrintString); + else + { + PrintStr(L"0x"); + end = FormatHex(Val, Width, PrintString); + } + *end = 0; + PrintStr(PrintString); +} + +CHAR16 *__init wstrcpy(CHAR16 *d, const CHAR16 *s) +{ + CHAR16 *r = d; + + while ( (*d++ = *s++) != 0 ) + ; + return r; +} + +int __init wstrcmp(const CHAR16 *s1, const CHAR16 *s2) +{ + while ( *s1 && *s1 == *s2 ) + { + ++s1; + ++s2; + } + return *s1 - *s2; +} + +int __init wstrncmp(const CHAR16 *s1, const CHAR16 *s2, UINTN n) +{ + while ( n && *s1 && *s1 == *s2 ) + { + --n; + ++s1; + ++s2; + } + return n ? *s1 - *s2 : 0; +} + +CHAR16 *__init s2w(union string *str) +{ + const char *s = str->s; + CHAR16 *w; + void *ptr; + + if ( efi_bs->AllocatePool(EfiLoaderData, (strlen(s) + 1) * sizeof(*w), + &ptr) != EFI_SUCCESS ) + return NULL; + + w = str->w = ptr; + do { + *w = *s++; + } while ( *w++ ); + + return str->w; +} + +char *__init w2s(const union string *str) +{ + const CHAR16 *w = str->w; + char *s = str->s; + + do { + if ( *w > 0x007f ) + return NULL; + *s = *w++; + } while ( *s++ ); + + return str->s; +} + +bool_t __init match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2) +{ + return guid1->Data1 == guid2->Data1 && + guid1->Data2 == guid2->Data2 && + guid1->Data3 == guid2->Data3 && + !memcmp(guid1->Data4, guid2->Data4, sizeof(guid1->Data4)); +} + + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/include/efi/efi-shared.h b/xen/include/efi/efi-shared.h new file mode 100644 index 0000000..38f8f39 --- /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); + +#endif + + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */
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@linaro.org> --- 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 +++++++++++++++++++++++++++++++++++++++++++ xen/include/efi/efi-shared.h | 50 +++++++++++++++ 6 files changed, 205 insertions(+), 120 deletions(-) create mode 100644 xen/common/efi/Makefile create mode 100644 xen/common/efi/efi-shared.c create mode 100644 xen/include/efi/efi-shared.h