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

[Roy Franz <roy.franz@xxxxxxxxxx>]

On Thu, Sep 11, 2014 at 7:03 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> 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?
The define is only used by the x86 specific code.  I can move it back
to the common code.


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

These are forward declarations of functions provided by boot.c but
used in efi-boot.h.  I was trying to keep the
#include of efi-boot.h near the top of the file.  I can move it to
below where these functions are defined and remove
the forward declarations.
I will remove the __init if they stay.

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

This is only used by the x86 code, and this is the x86 specific memory
management that uses
memory allocated by the linker script before start.  The ARM boot code
does not use this.

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

This does scanning of PCI ROMS, which is unlikely to do anything
useful on ARM platforms.  I also have
no way to test this on ARM.
I can try to pull this back in, but I may run into dependency issues
such as those with VGA, where I did not
want to pull in otherwise unused (and likely unusable on ARM)
drivers/subsystems in order to keep a little more
code in the common EFI boot path.

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

This is done later in boot - I am not familiar with the details.
Maybe Ian can add something here.

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

These are forward declarations.  I don't think that I can order
everything so that I don't need any forward declarations
anywhere.

>
>> +void __init efi_init_memory(void)
>
> Now that I look at it again, I think a good part of this is arch-
> independent too.
By "this" you mean the code in efi_init_memory?

>
> Jan
>

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