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

[Roy Franz <roy.franz@xxxxxxxxxx>]

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

>>> On 11.09.14 at 19:33, <roy.franz@xxxxxxxxxx> wrote:
> 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.

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

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