Re: [Xen-devel] [PATCH v4] x86: relocate pvh_info

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/01/18 16:13, Wei Liu wrote:
> Modify early boot code to relocate pvh info as well, so that we can be
> sure __va in __start_xen works.
>
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Cc: Doug Goldstein <cardoe@xxxxxxxxxx>
>
> v4: inlcude autoconf.h directly. The code itself is unchanged.
> ---
>  xen/arch/x86/boot/Makefile |  4 +++
>  xen/arch/x86/boot/defs.h   |  3 +++
>  xen/arch/x86/boot/head.S   | 25 ++++++++++--------
>  xen/arch/x86/boot/reloc.c  | 64 
> +++++++++++++++++++++++++++++++++++++++++-----
>  4 files changed, 78 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> index c6246c85d2..1b3f121a2f 100644
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -7,6 +7,10 @@ CMDLINE_DEPS = $(DEFS_H_DEPS) video.h
>  RELOC_DEPS = $(DEFS_H_DEPS) $(BASEDIR)/include/xen/multiboot.h \
>            $(BASEDIR)/include/xen/multiboot2.h

+ autoconf.h

However, it would much better to take xen/kconfig.h ...

>  
> +ifeq ($(CONFIG_PVH_GUEST),y)
> +RELOC_DEPS += $(BASEDIR)/include/public/arch-x86/hvm/start_info.h
> +endif

and this unconditionally, and ...

> +
>  head.o: cmdline.S reloc.S
>  
>  cmdline.S: cmdline.c $(CMDLINE_DEPS)
> diff --git a/xen/arch/x86/boot/defs.h b/xen/arch/x86/boot/defs.h
> index 6abdc15446..05921a64a3 100644
> --- a/xen/arch/x86/boot/defs.h
> +++ b/xen/arch/x86/boot/defs.h
> @@ -51,6 +51,9 @@ typedef unsigned short u16;
>  typedef unsigned int u32;
>  typedef unsigned long long u64;
>  typedef unsigned int size_t;
> +typedef u8 uint8_t;
> +typedef u32 uint32_t;
> +typedef u64 uint64_t;
>  
>  #define U16_MAX              ((u16)(~0U))
>  #define UINT_MAX     (~0U)
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 0f652cea11..aa2e2a93c8 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -414,6 +414,7 @@ __pvh_start:
>  
>          /* Set trampoline_phys to use mfn 1 to avoid having a mapping at VA 
> 0 */
>          movw    $0x1000, sym_esi(trampoline_phys)
> +        movl    (%ebx), %eax /* mov $XEN_HVM_START_MAGIC_VALUE, %eax */
>          jmp     trampoline_setup
>  
>  #endif /* CONFIG_PVH_GUEST */
> @@ -578,18 +579,20 @@ trampoline_setup:
>          /* Get bottom-most low-memory stack address. */
>          add     $TRAMPOLINE_SPACE,%ecx
>  
> -#ifdef CONFIG_PVH_GUEST
> -        cmpb    $0, sym_fs(pvh_boot)
> -        jne     1f
> -#endif
> -
> -        /* Save the Multiboot info struct (after relocation) for later use. 
> */
> +        /* Save Multiboot / PVH info struct (after relocation) for later 
> use. */
>          push    %ecx                /* Bottom-most low-memory stack address. 
> */
> -        push    %ebx                /* Multiboot information address. */
> -        push    %eax                /* Multiboot magic. */
> +        push    %ebx                /* Multiboot / PVH information address. 
> */
> +        push    %eax                /* Magic number. */
>          call    reloc
> -        mov     %eax,sym_fs(multiboot_ptr)
> +#ifdef CONFIG_PVH_GUEST
> +        cmp     $0,sym_fs(pvh_boot)
> +        je      1f
> +        mov     %eax,sym_fs(pvh_start_info_pa)
> +        jmp     2f
> +#endif
>  1:
> +        mov     %eax,sym_fs(multiboot_ptr)
> +2:

For new code, please have spaces after commas for readibility.

>  
>          /*
>           * Now trampoline_phys points to the following structure (lowest 
> address
> @@ -598,12 +601,12 @@ trampoline_setup:
>           * +------------------------+
>           * | TRAMPOLINE_STACK_SPACE |
>           * +------------------------+
> -         * |        mbi data        |
> +         * |     Data (MBI / PVH)   |
>           * +- - - - - - - - - - - - +
>           * |    TRAMPOLINE_SPACE    |
>           * +------------------------+
>           *
> -         * mbi data grows downwards from the highest address of 
> TRAMPOLINE_SPACE
> +         * Data grows downwards from the highest address of TRAMPOLINE_SPACE
>           * region to the end of the trampoline. The rest of TRAMPOLINE_SPACE 
> is
>           * reserved for trampoline code and data.
>           */
> diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
> index b992678b5e..1fe19294ad 100644
> --- a/xen/arch/x86/boot/reloc.c
> +++ b/xen/arch/x86/boot/reloc.c
> @@ -14,8 +14,8 @@
>  
>  /*
>   * This entry point is entered from xen/arch/x86/boot/head.S with:
> - *   - 0x4(%esp) = MULTIBOOT_MAGIC,
> - *   - 0x8(%esp) = MULTIBOOT_INFORMATION_ADDRESS,
> + *   - 0x4(%esp) = MAGIC,
> + *   - 0x8(%esp) = INFORMATION_ADDRESS,
>   *   - 0xc(%esp) = TOPMOST_LOW_MEMORY_STACK_ADDRESS.
>   */
>  asm (
> @@ -29,6 +29,8 @@ asm (
>  #include "../../../include/xen/multiboot.h"
>  #include "../../../include/xen/multiboot2.h"
>  
> +#include "../../../include/generated/autoconf.h"
> +
>  #define get_mb2_data(tag, type, member)   (((multiboot2_tag_##type##_t 
> *)(tag))->member)
>  #define get_mb2_string(tag, type, member) ((u32)get_mb2_data(tag, type, 
> member))
>  
> @@ -71,6 +73,41 @@ static u32 copy_string(u32 src)
>      return copy_mem(src, p - src + 1);
>  }
>  
> +#ifdef CONFIG_PVH_GUEST

... drop this ifdef and ...

> +
> +#include <public/arch-x86/hvm/start_info.h>
> +
> +static struct hvm_start_info *pvh_info_reloc(u32 in)
> +{
> +    struct hvm_start_info *out;
> +
> +    out = _p(copy_mem(in, sizeof(*out)));
> +
> +    if ( out->cmdline_paddr )
> +        out->cmdline_paddr = copy_string(out->cmdline_paddr);
> +
> +    if ( out->nr_modules )
> +    {
> +        unsigned int i;
> +        struct hvm_modlist_entry *mods;
> +
> +        out->modlist_paddr =
> +            copy_mem(out->modlist_paddr,
> +                     out->nr_modules * sizeof(struct hvm_modlist_entry));
> +
> +        mods = _p(out->modlist_paddr);
> +
> +        for ( i = 0; i < out->nr_modules; i++ )
> +        {
> +            if ( mods[i].cmdline_paddr )
> +                mods[i].cmdline_paddr = copy_string(mods[i].cmdline_paddr);
> +        }
> +    }
> +
> +    return out;
> +}
> +#endif
> +
>  static multiboot_info_t *mbi_reloc(u32 mbi_in)
>  {
>      int i;
> @@ -226,14 +263,27 @@ static multiboot_info_t *mbi2_reloc(u32 mbi_in)
>      return mbi_out;
>  }
>  
> -multiboot_info_t __stdcall *reloc(u32 mb_magic, u32 mbi_in, u32 trampoline)
> +void __stdcall *reloc(u32 magic, u32 in, u32 trampoline)
>  {
>      alloc = trampoline;
>  
> -    if ( mb_magic == MULTIBOOT2_BOOTLOADER_MAGIC )
> -        return mbi2_reloc(mbi_in);
> -    else
> -        return mbi_reloc(mbi_in);
> +    switch ( magic )
> +    {
> +    case MULTIBOOT_BOOTLOADER_MAGIC:
> +        return mbi_reloc(in);
> +
> +    case MULTIBOOT2_BOOTLOADER_MAGIC:
> +        return mbi2_reloc(in);
> +
> +#ifdef CONFIG_PVH_GUEST
> +    case XEN_HVM_START_MAGIC_VALUE:
> +        return pvh_info_reloc(in);
> +#endif

Rearrange this as:

case XEN_HVM_START_MAGIC_VALUE:
    if ( IS_ENABLED(CONFIG_PVH_GUEST) )
        return pvh_info_reloc(in);
    /* Fallthrough */

Which drops all of the ifdefary.

~Andrew

> +
> +    default:
> +        /* Nothing we can do */
> +        return NULL;
> +    }
>  }
>  
>  /*


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel