Re: [Xen-devel] [PATCH 1/3] xen/x86: split boot trampoline into permanent and temporary part

[Juergen Gross <jgross@xxxxxxxx>]

On 22/03/17 12:33, Jan Beulich wrote:
>>>> On 21.03.17 at 14:10, <jgross@xxxxxxxx> wrote:
>> The hypervisor needs a trampoline in low memory for early boot and
>> later for bringing up cpus and during wakeup from suspend. Today this
>> trampoline is kept completely even if most of it isn't needed later.
>>
>> Split the trampoline into a permanent part and a temporary part needed
>> at early boot only. Introduce a new entry at the boundary.
> 
> s/entry/label/?

Yes.

> 
>> Reduce the stack for wakeup coding in order for the permanent
> 
> "coding"?

code

> 
>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -587,6 +587,8 @@ cmdline_parse_early:
>>  reloc:
>>  #include "reloc.S"
>>  
>> +        .align 16
> 
> Why?
> 
>>  ENTRY(trampoline_start)
> 
> ENTRY() already does this, with proper NOP padding.

Okay, will drop the .align

> 
>> --- a/xen/arch/x86/boot/trampoline.S
>> +++ b/xen/arch/x86/boot/trampoline.S
>> @@ -1,4 +1,20 @@
>> -        .code16
>> +/*
>> + * Trampoline code relocated to low memory.
>> + *
>> + * Care must taken when referencing symbols: they live in the relocated
>> + * trampoline and in the hypervisor binary. The hypervisor symbols can 
>> either
>> + * be accessed by their virtual address or by the physical address. When
>> + * using the physical address eventually the physical start address of the
>> + * hypervisor must be taken into account: after early boot the hypervisor
>> + * will copy itself to high memory and writes its physical start address to
>> + * trampoline_xen_phys_start in the low memory trampoline copy.
>> + *
>> + * Parts of the trampoline are needed for early boot only, while some other
>> + * parts are needed as long as the hypervisor is active (e.g. wakeup code
>> + * after suspend, bringup code for secondary cpus). The permanent parts 
>> should
>> + * not reference any temporary low memory trampoline parts as those parts 
>> are
>> + * not guaranteed to persist.
>> + */
> 
> It would of course be nice if we had a way to enforce this
> restriction, but I can't seem to be able to think of a workable
> one.
> 
>> @@ -131,6 +151,12 @@ start64:
>>          movabs  $__high_start,%rax
>>          jmpq    *%rax
>>  
>> +#include "wakeup.S"
>> +
>> +ENTRY(trampoline_temp_start)
> 
> s/temp/boot/ ?

I don't mind, so I'll rename it.

> 
>> --- a/xen/arch/x86/boot/wakeup.S
>> +++ b/xen/arch/x86/boot/wakeup.S
>> @@ -173,6 +173,8 @@ bogus_saved_magic:
>>          movw    $0x0e00 + 'S', 0xb8014
>>          jmp     bogus_saved_magic
>>  
>> +/* Stack for wakeup: rest of first trampoline page. */
>>          .align  16
>> -        .fill   PAGE_SIZE,1,0
>> +.Lwakeup_stack_start:
>> +        .fill   PAGE_SIZE - (.Lwakeup_stack_start - trampoline_start),1,0
>>  wakeup_stack:
> 
> Is this stack being used at boot time? If not, what's the point of
> putting a gap in here? Instead it could simply be calculated by its
> users as trampoline_start + PAGE_SIZE, overwriting whatever
> was left there. All that would then be desirable would be a
> BUILD_BUG_ON()-like construct to make sure the stack won't
> grow too small.

Okay, I'll do it this way.


Juergen

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