| Message ID | 1411080607-32365-3-git-send-email-roy.franz@linaro.org |
|---|---|
| State | New |
| Headers | show |
>>> On 19.09.14 at 00:49, <roy.franz@linaro.org> wrote: > @@ -687,82 +645,6 @@ static int __init set_color(u32 mask, int bpp, u8 *pos, u8 *sz) > return max(*pos + *sz, bpp); > } > > -extern const intpte_t __page_tables_start[], __page_tables_end[]; > -#define in_page_tables(v) ((intpte_t *)(v) >= __page_tables_start && \ > - (intpte_t *)(v) < __page_tables_end) > - > -#define PE_BASE_RELOC_ABS 0 > -#define PE_BASE_RELOC_HIGHLOW 3 > -#define PE_BASE_RELOC_DIR64 10 > - > -extern const struct pe_base_relocs { > - u32 rva; > - u32 size; > - u16 entries[]; > -} __base_relocs_start[], __base_relocs_end[]; > - > -static void __init relocate_image(unsigned long delta) > -{ > - const struct pe_base_relocs *base_relocs; > - > - for ( base_relocs = __base_relocs_start; base_relocs < __base_relocs_end; ) > - { > - unsigned int i, n; > - > - n = (base_relocs->size - sizeof(*base_relocs)) / > - sizeof(*base_relocs->entries); > - for ( i = 0; i < n; ++i ) > - { > - unsigned long addr = xen_phys_start + base_relocs->rva + > - (base_relocs->entries[i] & 0xfff); > - > - switch ( base_relocs->entries[i] >> 12 ) > - { > - case PE_BASE_RELOC_ABS: > - break; > - case PE_BASE_RELOC_HIGHLOW: > - if ( delta ) > - { > - *(u32 *)addr += delta; > - if ( in_page_tables(addr) ) > - *(u32 *)addr += xen_phys_start; > - } > - break; > - case PE_BASE_RELOC_DIR64: > - if ( delta ) > - { > - *(u64 *)addr += delta; > - if ( in_page_tables(addr) ) > - *(intpte_t *)addr += xen_phys_start; > - } > - break; > - default: > - blexit(L"Unsupported relocation type"); > - } > - } > - base_relocs = (const void *)(base_relocs->entries + i + (i & 1)); > - } > -} Hmm, so you're still moving the relocation processing code - why? I don't recall you having said you're sure you'll not need this on ARM. Jan
On Fri, 2014-09-19 at 09:37 +0100, Jan Beulich wrote: > >>> On 19.09.14 at 00:49, <roy.franz@linaro.org> wrote: > > @@ -687,82 +645,6 @@ static int __init set_color(u32 mask, int bpp, u8 *pos, u8 *sz) > > return max(*pos + *sz, bpp); > > } > > > > -extern const intpte_t __page_tables_start[], __page_tables_end[]; > > -#define in_page_tables(v) ((intpte_t *)(v) >= __page_tables_start && \ > > - (intpte_t *)(v) < __page_tables_end) > > - > > -#define PE_BASE_RELOC_ABS 0 > > -#define PE_BASE_RELOC_HIGHLOW 3 > > -#define PE_BASE_RELOC_DIR64 10 > > - > > -extern const struct pe_base_relocs { > > - u32 rva; > > - u32 size; > > - u16 entries[]; > > -} __base_relocs_start[], __base_relocs_end[]; > > - > > -static void __init relocate_image(unsigned long delta) > > -{ > > - const struct pe_base_relocs *base_relocs; > > - > > - for ( base_relocs = __base_relocs_start; base_relocs < __base_relocs_end; ) > > - { > > - unsigned int i, n; > > - > > - n = (base_relocs->size - sizeof(*base_relocs)) / > > - sizeof(*base_relocs->entries); > > - for ( i = 0; i < n; ++i ) > > - { > > - unsigned long addr = xen_phys_start + base_relocs->rva + > > - (base_relocs->entries[i] & 0xfff); > > - > > - switch ( base_relocs->entries[i] >> 12 ) > > - { > > - case PE_BASE_RELOC_ABS: > > - break; > > - case PE_BASE_RELOC_HIGHLOW: > > - if ( delta ) > > - { > > - *(u32 *)addr += delta; > > - if ( in_page_tables(addr) ) > > - *(u32 *)addr += xen_phys_start; > > - } > > - break; > > - case PE_BASE_RELOC_DIR64: > > - if ( delta ) > > - { > > - *(u64 *)addr += delta; > > - if ( in_page_tables(addr) ) > > - *(intpte_t *)addr += xen_phys_start; > > - } > > - break; > > - default: > > - blexit(L"Unsupported relocation type"); > > - } > > - } > > - base_relocs = (const void *)(base_relocs->entries + i + (i & 1)); > > - } > > -} > > Hmm, so you're still moving the relocation processing code - why? I > don't recall you having said you're sure you'll not need this on ARM. ARM relocates itself to the top of memory during bringup already. I don't think we need to do it here as well/instead. Ian.
>>> On 22.09.14 at 12:52, <Ian.Campbell@citrix.com> wrote: > On Fri, 2014-09-19 at 09:37 +0100, Jan Beulich wrote: >> >>> On 19.09.14 at 00:49, <roy.franz@linaro.org> wrote: >> > @@ -687,82 +645,6 @@ static int __init set_color(u32 mask, int bpp, u8 > *pos, u8 *sz) >> > return max(*pos + *sz, bpp); >> > } >> > >> > -extern const intpte_t __page_tables_start[], __page_tables_end[]; >> > -#define in_page_tables(v) ((intpte_t *)(v) >= __page_tables_start && \ >> > - (intpte_t *)(v) < __page_tables_end) >> > - >> > -#define PE_BASE_RELOC_ABS 0 >> > -#define PE_BASE_RELOC_HIGHLOW 3 >> > -#define PE_BASE_RELOC_DIR64 10 >> > - >> > -extern const struct pe_base_relocs { >> > - u32 rva; >> > - u32 size; >> > - u16 entries[]; >> > -} __base_relocs_start[], __base_relocs_end[]; >> > - >> > -static void __init relocate_image(unsigned long delta) >> > -{ >> > - const struct pe_base_relocs *base_relocs; >> > - >> > - for ( base_relocs = __base_relocs_start; base_relocs < > __base_relocs_end; ) >> > - { >> > - unsigned int i, n; >> > - >> > - n = (base_relocs->size - sizeof(*base_relocs)) / >> > - sizeof(*base_relocs->entries); >> > - for ( i = 0; i < n; ++i ) >> > - { >> > - unsigned long addr = xen_phys_start + base_relocs->rva + >> > - (base_relocs->entries[i] & 0xfff); >> > - >> > - switch ( base_relocs->entries[i] >> 12 ) >> > - { >> > - case PE_BASE_RELOC_ABS: >> > - break; >> > - case PE_BASE_RELOC_HIGHLOW: >> > - if ( delta ) >> > - { >> > - *(u32 *)addr += delta; >> > - if ( in_page_tables(addr) ) >> > - *(u32 *)addr += xen_phys_start; >> > - } >> > - break; >> > - case PE_BASE_RELOC_DIR64: >> > - if ( delta ) >> > - { >> > - *(u64 *)addr += delta; >> > - if ( in_page_tables(addr) ) >> > - *(intpte_t *)addr += xen_phys_start; >> > - } >> > - break; >> > - default: >> > - blexit(L"Unsupported relocation type"); >> > - } >> > - } >> > - base_relocs = (const void *)(base_relocs->entries + i + (i & 1)); >> > - } >> > -} >> >> Hmm, so you're still moving the relocation processing code - why? I >> don't recall you having said you're sure you'll not need this on ARM. > > ARM relocates itself to the top of memory during bringup already. I > don't think we need to do it here as well/instead. There are two relocations (on x86) actually: One (always used) is to move the hypervisor image to high physical memory. The other (needed under UEFI only) is to relocate the hypervisor from where the UEFI loader put it (running in physical mode, virtual and physical addresses are the same) to its linked virtual address. Are you saying that ARM isn't in need of this? Jan
On Mon, 2014-09-22 at 11:56 +0100, Jan Beulich wrote: > >>> On 22.09.14 at 12:52, <Ian.Campbell@citrix.com> wrote: > > On Fri, 2014-09-19 at 09:37 +0100, Jan Beulich wrote: > >> Hmm, so you're still moving the relocation processing code - why? I > >> don't recall you having said you're sure you'll not need this on ARM. > > > > ARM relocates itself to the top of memory during bringup already. I > > don't think we need to do it here as well/instead. > > There are two relocations (on x86) actually: One (always used) is > to move the hypervisor image to high physical memory. The other > (needed under UEFI only) is to relocate the hypervisor from where > the UEFI loader put it (running in physical mode, virtual and > physical addresses are the same) to its linked virtual address. Are > you saying that ARM isn't in need of this? Correct, the main ARM entry point (which the UEFI stub calls) takes care of enabling paging, including handling the move from (potentially differing) physical to virtual addresses. Ian.
>>> On 22.09.14 at 13:09, <Ian.Campbell@citrix.com> wrote: > On Mon, 2014-09-22 at 11:56 +0100, Jan Beulich wrote: >> >>> On 22.09.14 at 12:52, <Ian.Campbell@citrix.com> wrote: >> > On Fri, 2014-09-19 at 09:37 +0100, Jan Beulich wrote: >> >> Hmm, so you're still moving the relocation processing code - why? I >> >> don't recall you having said you're sure you'll not need this on ARM. >> > >> > ARM relocates itself to the top of memory during bringup already. I >> > don't think we need to do it here as well/instead. >> >> There are two relocations (on x86) actually: One (always used) is >> to move the hypervisor image to high physical memory. The other >> (needed under UEFI only) is to relocate the hypervisor from where >> the UEFI loader put it (running in physical mode, virtual and >> physical addresses are the same) to its linked virtual address. Are >> you saying that ARM isn't in need of this? > > Correct, the main ARM entry point (which the UEFI stub calls) takes care > of enabling paging, including handling the move from (potentially > differing) physical to virtual addresses. Ah, right - this really makes the need for doing the extra relocation step x86-64-specific. So Roy, I withdraw my objection then to moving these pieces into arch-specific code. Jan
>>> On 19.09.14 at 00:49, <roy.franz@linaro.org> wrote: > --- /dev/null > +++ b/xen/include/asm-x86/efi-boot.h > @@ -0,0 +1,135 @@ > +/* > + * Architecture specific implementation for EFI boot code. This file > + * is intended to be included by XXX _only_, and therefore can define > + * arch specific global variables. > + */ > +#include <asm/e820.h> > +#include <asm/edd.h> > +#define __ASSEMBLY__ /* avoid pulling in ACPI stuff (conflicts with EFI) */ > +#include <asm/fixmap.h> > +#undef __ASSEMBLY__ > +#include <asm/msr.h> > +#include <asm/processor.h> > + > +static struct file __initdata ucode; > +static multiboot_info_t __initdata mbi = { > + .flags = MBI_MODULES | MBI_LOADERNAME > +}; > +static module_t __initdata mb_modules[3]; > + > +extern char start[]; > +extern u32 cpuid_ext_features; I don't think these (and other extern-s) are valid to be put here. Jan
On Mon, Sep 22, 2014 at 5:54 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 19.09.14 at 00:49, <roy.franz@linaro.org> wrote: >> --- /dev/null >> +++ b/xen/include/asm-x86/efi-boot.h >> @@ -0,0 +1,135 @@ >> +/* >> + * Architecture specific implementation for EFI boot code. This file >> + * is intended to be included by XXX _only_, and therefore can define >> + * arch specific global variables. >> + */ >> +#include <asm/e820.h> >> +#include <asm/edd.h> >> +#define __ASSEMBLY__ /* avoid pulling in ACPI stuff (conflicts with EFI) */ >> +#include <asm/fixmap.h> >> +#undef __ASSEMBLY__ >> +#include <asm/msr.h> >> +#include <asm/processor.h> >> + >> +static struct file __initdata ucode; >> +static multiboot_info_t __initdata mbi = { >> + .flags = MBI_MODULES | MBI_LOADERNAME >> +}; >> +static module_t __initdata mb_modules[3]; >> + >> +extern char start[]; >> +extern u32 cpuid_ext_features; > > I don't think these (and other extern-s) are valid to be put here. > > Jan > Why not, and where should they go?? cpuid_ext_features is x86 architecture specific, and start[] is only used by the x86 code by the place_string() allocator. The extern declaration is in the file in which the variables are referenced. extern l4_pgentry_t *efi_l4_pgtable; maybe should be moved back to boot.c for now, but this is an x86 specific structure that is only referenced there due to the runtime code being #ifdef'ed out. Roy
>>> On 23.09.14 at 04:08, <roy.franz@linaro.org> wrote: > On Mon, Sep 22, 2014 at 5:54 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 19.09.14 at 00:49, <roy.franz@linaro.org> wrote: >>> --- /dev/null >>> +++ b/xen/include/asm-x86/efi-boot.h >>> @@ -0,0 +1,135 @@ >>> +/* >>> + * Architecture specific implementation for EFI boot code. This file >>> + * is intended to be included by XXX _only_, and therefore can define >>> + * arch specific global variables. >>> + */ >>> +#include <asm/e820.h> >>> +#include <asm/edd.h> >>> +#define __ASSEMBLY__ /* avoid pulling in ACPI stuff (conflicts with EFI) */ >>> +#include <asm/fixmap.h> >>> +#undef __ASSEMBLY__ >>> +#include <asm/msr.h> >>> +#include <asm/processor.h> >>> + >>> +static struct file __initdata ucode; >>> +static multiboot_info_t __initdata mbi = { >>> + .flags = MBI_MODULES | MBI_LOADERNAME >>> +}; >>> +static module_t __initdata mb_modules[3]; >>> + >>> +extern char start[]; >>> +extern u32 cpuid_ext_features; >> >> I don't think these (and other extern-s) are valid to be put here. > > Why not, and where should they go?? > > cpuid_ext_features is x86 architecture specific, and start[] is only > used by the x86 code by the place_string() allocator. The extern > declaration is in the file in which the variables are referenced. I'm sorry, I only now realized they're both currently being declared in boot.c, so moving them here is kind of okay. Nevertheless it would seem better to find them a more appropriate home: start[] could likely go alongside _start[] in xen/kernel.h, and cpuid_ext_features would seem to fit into either asm-x86/cpufeature.h or asm-x86/processor.h. > extern l4_pgentry_t *efi_l4_pgtable; > maybe should be moved back to boot.c for now, but this is an x86 > specific structure that is only referenced there due to the > runtime code being #ifdef'ed out. It's currently being declared in xen/arch/x86/efi/efi.h, and I can't really see why this isn't sufficient. Speaking of which - putting the new header under asm-x86/ instead of in x86/efi/ seems bogus with the changed symlinking model too. The header should be as private as possible. Perhaps what you put there could even go into said efi.h (possibly inside a suitable #if)? Jan
On Tue, Sep 23, 2014 at 5:24 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 23.09.14 at 04:08, <roy.franz@linaro.org> wrote: >> On Mon, Sep 22, 2014 at 5:54 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 19.09.14 at 00:49, <roy.franz@linaro.org> wrote: >>>> --- /dev/null >>>> +++ b/xen/include/asm-x86/efi-boot.h >>>> @@ -0,0 +1,135 @@ >>>> +/* >>>> + * Architecture specific implementation for EFI boot code. This file >>>> + * is intended to be included by XXX _only_, and therefore can define >>>> + * arch specific global variables. >>>> + */ >>>> +#include <asm/e820.h> >>>> +#include <asm/edd.h> >>>> +#define __ASSEMBLY__ /* avoid pulling in ACPI stuff (conflicts with EFI) */ >>>> +#include <asm/fixmap.h> >>>> +#undef __ASSEMBLY__ >>>> +#include <asm/msr.h> >>>> +#include <asm/processor.h> >>>> + >>>> +static struct file __initdata ucode; >>>> +static multiboot_info_t __initdata mbi = { >>>> + .flags = MBI_MODULES | MBI_LOADERNAME >>>> +}; >>>> +static module_t __initdata mb_modules[3]; >>>> + >>>> +extern char start[]; >>>> +extern u32 cpuid_ext_features; >>> >>> I don't think these (and other extern-s) are valid to be put here. >> >> Why not, and where should they go?? >> >> cpuid_ext_features is x86 architecture specific, and start[] is only >> used by the x86 code by the place_string() allocator. The extern >> declaration is in the file in which the variables are referenced. > > I'm sorry, I only now realized they're both currently being declared > in boot.c, so moving them here is kind of okay. Nevertheless it > would seem better to find them a more appropriate home: start[] > could likely go alongside _start[] in xen/kernel.h, and > cpuid_ext_features would seem to fit into either > asm-x86/cpufeature.h or asm-x86/processor.h. I moved start to xen/kernel.h, and cpuid_ext_features to asm-x86/processor.h alongside some other externs. cpufeature.h didn't have any externs, so putting it with other externs seemed a better fit. > >> extern l4_pgentry_t *efi_l4_pgtable; >> maybe should be moved back to boot.c for now, but this is an x86 >> specific structure that is only referenced there due to the >> runtime code being #ifdef'ed out. > > It's currently being declared in xen/arch/x86/efi/efi.h, and I > can't really see why this isn't sufficient. xen/arch/x86/efi/efi.h is a link to common/efi/efi.h, so it is a shared header. I could put it there with #ifdef, or leave it the the architecture specific header file. > > Speaking of which - putting the new header under asm-x86/ instead > of in x86/efi/ seems bogus with the changed symlinking model too. > The header should be as private as possible. Perhaps what you put > there could even go into said efi.h (possibly inside a suitable #if)? Yup, putting the efi-boot.h headers in arch/XX/efi is better, so I have moved them. Since the efi-boot.h is a 'special' include file - ie it defines global variables, I think having it directly included where it is intended is a good thing. This would also require a lot of forward declarations for functions and global variables referenced by this implementation header but defined in efi-boot.c. This is all avoided right now by including efi-boot.h a bit lower down. > > Jan >
>>> On 24.09.14 at 04:35, <roy.franz@linaro.org> wrote: > On Tue, Sep 23, 2014 at 5:24 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 23.09.14 at 04:08, <roy.franz@linaro.org> wrote: >>> extern l4_pgentry_t *efi_l4_pgtable; >>> maybe should be moved back to boot.c for now, but this is an x86 >>> specific structure that is only referenced there due to the >>> runtime code being #ifdef'ed out. >> >> It's currently being declared in xen/arch/x86/efi/efi.h, and I >> can't really see why this isn't sufficient. > > xen/arch/x86/efi/efi.h is a link to common/efi/efi.h, so it is a shared > header. I could put it there with #ifdef, or leave it the the architecture > specific header file. Following the as-private-as-possible model going the #ifdef route seems better to me even if resulting in slightly more ugly code. Jan
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 3bdc158..1a6cd7c 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -18,13 +18,6 @@ #include <xen/string.h> #include <xen/stringify.h> #include <xen/vga.h> -#include <asm/e820.h> -#include <asm/edd.h> -#define __ASSEMBLY__ /* avoid pulling in ACPI stuff (conflicts with EFI) */ -#include <asm/fixmap.h> -#undef __ASSEMBLY__ -#include <asm/msr.h> -#include <asm/processor.h> /* Using SetVirtualAddressMap() is incompatible with kexec: */ #undef USE_SET_VIRTUAL_ADDRESS_MAP @@ -41,9 +34,6 @@ typedef struct { EFI_SHIM_LOCK_VERIFY Verify; } EFI_SHIM_LOCK_PROTOCOL; -extern char start[]; -extern u32 cpuid_ext_features; - union string { CHAR16 *w; char *s; @@ -58,6 +48,13 @@ struct file { }; }; +static CHAR16 *FormatDec(UINT64 Val, CHAR16 *Buffer); +static CHAR16 *FormatHex(UINT64 Val, UINTN Width, CHAR16 *Buffer); +static void DisplayUint(UINT64 Val, INTN Width); +static CHAR16 *wstrcpy(CHAR16 *d, const CHAR16 *s); +static void noreturn blexit(const CHAR16 *str); +static void PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode); + static EFI_BOOT_SERVICES *__initdata efi_bs; static EFI_HANDLE __initdata efi_ih; @@ -69,19 +66,18 @@ static UINT32 __initdata mdesc_ver; static struct file __initdata cfg; static struct file __initdata kernel; static struct file __initdata ramdisk; -static struct file __initdata ucode; static struct file __initdata xsm; - -static multiboot_info_t __initdata mbi = { - .flags = MBI_MODULES | MBI_LOADERNAME -}; -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) +/* + * Include architecture specific implementation here, which references the + * static globals defined above. + */ +#include <asm/efi-boot.h> + static CHAR16 *__init FormatDec(UINT64 Val, CHAR16 *Buffer) { if ( Val >= 10 ) @@ -255,32 +251,6 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) blexit(mesg); } -static void __init place_string(u32 *addr, const char *s) -{ - static char *__initdata alloc = start; - - if ( s && *s ) - { - size_t len1 = strlen(s) + 1; - const char *old = (char *)(long)*addr; - size_t len2 = *addr ? strlen(old) + 1 : 0; - - alloc -= len1 + len2; - /* - * Insert new string before already existing one. This is needed - * for options passed on the command line to override options from - * the configuration file. - */ - memcpy(alloc, s, len1); - if ( *addr ) - { - alloc[len1 - 1] = ' '; - memcpy(alloc + len1, old, len2); - } - } - *addr = (long)alloc; -} - static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, CHAR16 *cmdline, UINTN cmdsize) { @@ -574,18 +544,6 @@ static void __init split_value(char *s) *s = 0; } -static void __init edd_put_string(u8 *dst, size_t n, const char *src) -{ - while ( n-- && *src ) - *dst++ = *src++; - if ( *src ) - PrintErrMesg(L"Internal error populating EDD info", - EFI_BUFFER_TOO_SMALL); - while ( n-- ) - *dst++ = ' '; -} -#define edd_put_string(d, s) edd_put_string(d, ARRAY_SIZE(d), s) - static void __init setup_efi_pci(void) { EFI_STATUS status; @@ -687,82 +645,6 @@ static int __init set_color(u32 mask, int bpp, u8 *pos, u8 *sz) return max(*pos + *sz, bpp); } -extern const intpte_t __page_tables_start[], __page_tables_end[]; -#define in_page_tables(v) ((intpte_t *)(v) >= __page_tables_start && \ - (intpte_t *)(v) < __page_tables_end) - -#define PE_BASE_RELOC_ABS 0 -#define PE_BASE_RELOC_HIGHLOW 3 -#define PE_BASE_RELOC_DIR64 10 - -extern const struct pe_base_relocs { - u32 rva; - u32 size; - u16 entries[]; -} __base_relocs_start[], __base_relocs_end[]; - -static void __init relocate_image(unsigned long delta) -{ - const struct pe_base_relocs *base_relocs; - - for ( base_relocs = __base_relocs_start; base_relocs < __base_relocs_end; ) - { - unsigned int i, n; - - n = (base_relocs->size - sizeof(*base_relocs)) / - sizeof(*base_relocs->entries); - for ( i = 0; i < n; ++i ) - { - unsigned long addr = xen_phys_start + base_relocs->rva + - (base_relocs->entries[i] & 0xfff); - - switch ( base_relocs->entries[i] >> 12 ) - { - case PE_BASE_RELOC_ABS: - break; - case PE_BASE_RELOC_HIGHLOW: - if ( delta ) - { - *(u32 *)addr += delta; - if ( in_page_tables(addr) ) - *(u32 *)addr += xen_phys_start; - } - break; - case PE_BASE_RELOC_DIR64: - if ( delta ) - { - *(u64 *)addr += delta; - if ( in_page_tables(addr) ) - *(intpte_t *)addr += xen_phys_start; - } - break; - default: - blexit(L"Unsupported relocation type"); - } - } - base_relocs = (const void *)(base_relocs->entries + i + (i & 1)); - } -} - -extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[]; -extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[]; - -static void __init relocate_trampoline(unsigned long phys) -{ - const s32 *trampoline_ptr; - - trampoline_phys = phys; - /* Apply relocations to trampoline. */ - for ( trampoline_ptr = __trampoline_rel_start; - trampoline_ptr < __trampoline_rel_stop; - ++trampoline_ptr ) - *(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += phys; - for ( trampoline_ptr = __trampoline_seg_start; - trampoline_ptr < __trampoline_seg_stop; - ++trampoline_ptr ) - *(u16 *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4; -} - void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) { @@ -879,7 +761,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) PrintStr(L"Xen " __stringify(XEN_VERSION) "." __stringify(XEN_SUBVERSION) XEN_EXTRAVERSION " (c/s " XEN_CHANGESET ") EFI loader\r\n"); - relocate_image(0); + efi_arch_relocate_image(0); if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, &cols, &rows) == EFI_SUCCESS ) @@ -1460,7 +1342,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) efi_memmap = (void *)efi_memmap + DIRECTMAP_VIRT_START; efi_fw_vendor = (void *)efi_fw_vendor + DIRECTMAP_VIRT_START; - relocate_image(__XEN_VIRT_START - xen_phys_start); + efi_arch_relocate_image(__XEN_VIRT_START - xen_phys_start); memcpy((void *)trampoline_phys, trampoline_start, cfg.size); /* Set system registers and transfer control. */ diff --git a/xen/include/asm-x86/efi-boot.h b/xen/include/asm-x86/efi-boot.h new file mode 100644 index 0000000..f250941 --- /dev/null +++ b/xen/include/asm-x86/efi-boot.h @@ -0,0 +1,135 @@ +/* + * Architecture specific implementation for EFI boot code. This file + * is intended to be included by XXX _only_, and therefore can define + * arch specific global variables. + */ +#include <asm/e820.h> +#include <asm/edd.h> +#define __ASSEMBLY__ /* avoid pulling in ACPI stuff (conflicts with EFI) */ +#include <asm/fixmap.h> +#undef __ASSEMBLY__ +#include <asm/msr.h> +#include <asm/processor.h> + +static struct file __initdata ucode; +static multiboot_info_t __initdata mbi = { + .flags = MBI_MODULES | MBI_LOADERNAME +}; +static module_t __initdata mb_modules[3]; + +extern char start[]; +extern u32 cpuid_ext_features; + +static void __init edd_put_string(u8 *dst, size_t n, const char *src) +{ + while ( n-- && *src ) + *dst++ = *src++; + if ( *src ) + PrintErrMesg(L"Internal error populating EDD info", + EFI_BUFFER_TOO_SMALL); + while ( n-- ) + *dst++ = ' '; +} +#define edd_put_string(d, s) edd_put_string(d, ARRAY_SIZE(d), s) + +extern const intpte_t __page_tables_start[], __page_tables_end[]; +#define in_page_tables(v) ((intpte_t *)(v) >= __page_tables_start && \ + (intpte_t *)(v) < __page_tables_end) + +#define PE_BASE_RELOC_ABS 0 +#define PE_BASE_RELOC_HIGHLOW 3 +#define PE_BASE_RELOC_DIR64 10 + +extern const struct pe_base_relocs { + u32 rva; + u32 size; + u16 entries[]; +} __base_relocs_start[], __base_relocs_end[]; + +static void __init efi_arch_relocate_image(unsigned long delta) +{ + const struct pe_base_relocs *base_relocs; + + for ( base_relocs = __base_relocs_start; base_relocs < __base_relocs_end; ) + { + unsigned int i, n; + + n = (base_relocs->size - sizeof(*base_relocs)) / + sizeof(*base_relocs->entries); + for ( i = 0; i < n; ++i ) + { + unsigned long addr = xen_phys_start + base_relocs->rva + + (base_relocs->entries[i] & 0xfff); + + switch ( base_relocs->entries[i] >> 12 ) + { + case PE_BASE_RELOC_ABS: + break; + case PE_BASE_RELOC_HIGHLOW: + if ( delta ) + { + *(u32 *)addr += delta; + if ( in_page_tables(addr) ) + *(u32 *)addr += xen_phys_start; + } + break; + case PE_BASE_RELOC_DIR64: + if ( delta ) + { + *(u64 *)addr += delta; + if ( in_page_tables(addr) ) + *(intpte_t *)addr += xen_phys_start; + } + break; + default: + blexit(L"Unsupported relocation type"); + } + } + base_relocs = (const void *)(base_relocs->entries + i + (i & 1)); + } +} + +extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[]; +extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[]; + +static void __init relocate_trampoline(unsigned long phys) +{ + const s32 *trampoline_ptr; + + trampoline_phys = phys; + /* Apply relocations to trampoline. */ + for ( trampoline_ptr = __trampoline_rel_start; + trampoline_ptr < __trampoline_rel_stop; + ++trampoline_ptr ) + *(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += phys; + for ( trampoline_ptr = __trampoline_seg_start; + trampoline_ptr < __trampoline_seg_stop; + ++trampoline_ptr ) + *(u16 *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4; +} + +static void __init place_string(u32 *addr, const char *s) +{ + static char *__initdata alloc = start; + + if ( s && *s ) + { + size_t len1 = strlen(s) + 1; + const char *old = (char *)(long)*addr; + size_t len2 = *addr ? strlen(old) + 1 : 0; + + alloc -= len1 + len2; + /* + * Insert new string before already existing one. This is needed + * for options passed on the command line to override options from + * the configuration file. + */ + memcpy(alloc, s, len1); + if ( *addr ) + { + alloc[len1 - 1] = ' '; + memcpy(alloc + len1, old, len2); + } + } + *addr = (long)alloc; +}
Move the global variables and functions that can be moved as-is from the common boot.c file to the x86 implementation header file. Signed-off-by: Roy Franz <roy.franz@linaro.org> --- xen/common/efi/boot.c | 148 +++++------------------------------------ xen/include/asm-x86/efi-boot.h | 135 +++++++++++++++++++++++++++++++++++++ 2 files changed, 150 insertions(+), 133 deletions(-) create mode 100644 xen/include/asm-x86/efi-boot.h