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

[Roy Franz <roy.franz@linaro.org>]

[-- Attachment #1.1: Type: text/plain, Size: 5066 bytes --]

On Fri, Sep 12, 2014 at 12:04 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 11.09.14 at 19:33, <roy.franz@linaro.org> wrote:
> > On Thu, Sep 11, 2014 at 7:03 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >>>>> On 10.09.14 at 02:51, <roy.franz@linaro.org> 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.
>
> Yes please, as the conflict with kexec is an arch-independent one.
>
> >>> -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.
>
> I.e. the only arch-specific thing here is the initializer of "alloc". Or
> are you saying that you don't need a place_string() function in
> ARM at all? Is that because to stuff respective information into DT?
>
ARM doesn't use place_string() at all.  All the information is placed into
the DT that has EFI allocated memory for it.


>
> >>> -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.
>
> Why not? Obviously if the ROMs contain x86 code, they're useless,
> but in order to, say, do remote boot I suppose you need option
> ROMs (with ARM code) on ARM too.
>

Yes, something like that may exist someday.  This can get moved back
with the #ifdef'ing of the runtime.c implementation.

>
> > 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.
>
> The base line should be too keep everything here that is potentially
> usable on more than one arch (without limiting our thinking to ARM
> and x86). EFI's protocol based approach abstracts this quite nicely
> - if something isn't available, you simply won't find th respective
> protocol.
>

I'll take a look at just factoring out the VGA specific stuff - I agree that
is all that needs to be in the x86 specific file.



>
> >>> --- /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.
>
> I realize you may need forward declarations. But you declare arch-
> independent functions in an arch header 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.
> > By "this" you mean the code in efi_init_memory?
>
> Yes, at least everything up to and including the
> SetVirtualAddressMap() call.
>

This is only called from x86/mm.c, so it is currently unused on ARM.
If it causes compile problems on ARM I can #ifdef it, since this will need
to be dealt with as part
of adding runtime service support.

>
> Jan
>

[-- Attachment #1.2: Type: text/html, Size: 7521 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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