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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

"coding"?

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

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

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

Jan


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