Re: [Xen-devel] [PATCH V4 02/15] Move x86 specific funtions/variables to arch header

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.09.14 at 02:51, <roy.franz@xxxxxxxxxx> wrote:
> -/* Using SetVirtualAddressMap() is incompatible with kexec: */
> -#undef USE_SET_VIRTUAL_ADDRESS_MAP

In which way is this arch-specific?

> @@ -41,8 +31,10 @@ typedef struct {
>      EFI_SHIM_LOCK_VERIFY Verify;
>  } EFI_SHIM_LOCK_PROTOCOL;
>  
> -extern char start[];
> -extern u32 cpuid_ext_features;
> +static CHAR16 *__init FormatDec(UINT64 Val, CHAR16 *Buffer);
> +static CHAR16 *__init FormatHex(UINT64 Val, UINTN Width, CHAR16 *Buffer);
> +static void __init DisplayUint(UINT64 Val, INTN Width);
> +static CHAR16 *__init wstrcpy(CHAR16 *d, const CHAR16 *s);

Why do you need these declarations? And if you need them, no
__init annotations here please (they only belong on the
definitions).

> -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;
> -}

How much of this is really arch-specific?

> -static void __init setup_efi_pci(void)

And this doesn't seem arch-specific either (it only depends on
HAS_PCI or some such).

> -static void __init relocate_image(unsigned long delta)

I can see that some of this may need an arch abstraction, but why
would you not want to do this on ARM (or elsewhere)? In fact - how
do you get away without?

> --- /dev/null
> +++ b/xen/include/asm-x86/efi-boot.h
> @@ -0,0 +1,451 @@
> +/*
> + * 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];
> +
> +static void noreturn blexit(const CHAR16 *str);
> +static void PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode);

What are these two doing here?

> +void __init efi_init_memory(void)

Now that I look at it again, I think a good part of this is arch-
independent too.

Jan


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