From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roy Franz Subject: Re: [PATCH V4 02/15] Move x86 specific funtions/variables to arch header Date: Fri, 12 Sep 2014 09:52:31 -0700 Message-ID: References: <1410310325-4509-1-git-send-email-roy.franz@linaro.org> <1410310325-4509-3-git-send-email-roy.franz@linaro.org> <5411C7B60200007800033F12@mail.emea.novell.com> <5412B70C02000078000344D2@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6643405119303087612==" Return-path: In-Reply-To: <5412B70C02000078000344D2@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: keir , Ian Campbell , tim , xen-devel , Stefano Stabellini , Fu Wei List-Id: xen-devel@lists.xenproject.org --===============6643405119303087612== Content-Type: multipart/alternative; boundary=001a11c1c3e06df73a0502e11c56 --001a11c1c3e06df73a0502e11c56 Content-Type: text/plain; charset=UTF-8 On Fri, Sep 12, 2014 at 12:04 AM, Jan Beulich wrote: > >>> On 11.09.14 at 19:33, wrote: > > On Thu, Sep 11, 2014 at 7:03 AM, Jan Beulich wrote: > >>>>> On 10.09.14 at 02:51, 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 > >>> +#include > >>> +#define __ASSEMBLY__ /* avoid pulling in ACPI stuff (conflicts with > EFI) */ > >>> +#include > >>> +#undef __ASSEMBLY__ > >>> +#include > >>> +#include > >>> + > >>> +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 > --001a11c1c3e06df73a0502e11c56 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
On F= ri, Sep 12, 2014 at 12:04 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>> On= 11.09.14 at 19:33, <roy.franz@l= inaro.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.=C2=A0 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)
>>> -{
>>> -=C2=A0 =C2=A0 static char *__initdata alloc =3D start;
>>> -
>>> -=C2=A0 =C2=A0 if ( s && *s )
>>> -=C2=A0 =C2=A0 {
>>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 size_t len1 =3D strlen(s) + 1; >>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 const char *old =3D (char *)(long= )*addr;
>>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 size_t len2 =3D *addr ? strlen(ol= d) + 1 : 0;
>>> -
>>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 alloc -=3D len1 + len2;
>>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 /*
>>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* Insert new string before = already existing one. This is needed
>>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* for options passed on the= command line to override options from
>>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* the configuration file. >>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
>>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 memcpy(alloc, s, len1);
>>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( *addr )
>>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 {
>>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 alloc[len1 - 1] =3D= ' ';
>>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 memcpy(alloc + len1= , old, len2);
>>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>>> -=C2=A0 =C2=A0 }
>>> -=C2=A0 =C2=A0 *addr =3D (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.=C2=A0 The ARM boot= code
> does not use this.

I.e. the only arch-specific thing here is the initializer of &q= uot;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. =C2=A0All the inf= ormation is placed into
the DT that has EFI allocated memory for = it.
=C2=A0

>>> -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, s= omething like that may exist someday. =C2=A0This 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 n= eeds to be in the x86 specific file.

=C2=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px= #ccc solid;padding-left:1ex">

>>> --- /dev/null
>>> +++ b/xen/include/asm-x86/efi-boot.h
>>> @@ -0,0 +1,451 @@
>>> +/*
>>> + * Architecture specific implementation for EFI boot code.=C2= =A0 This file
>>> + * is intended to be included by XXX _only_, and therefore ca= n define
>>> + * arch specific global variables.
>>> + */
>>> +#include <asm/e820.h>
>>> +#include <asm/edd.h>
>>> +#define __ASSEMBLY__ /* avoid pulling in ACPI stuff (conflict= s 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 =3D {
>>> +=C2=A0 =C2=A0 .flags =3D 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 ErrCo= de);
>>
>> What are these two doing here?
>
> These are forward declarations.=C2=A0 I don't think that I can ord= er
> everything so that I don't need any forward declarations
> anywhere.

I realize you may need forward declarations. But you declare ar= ch-
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-<= br> >> 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 on= ly 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.=C2=A0
=

Jan

--001a11c1c3e06df73a0502e11c56-- --===============6643405119303087612== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============6643405119303087612==--