Re: [PATCH v2 5/7] x86: relocate pvh_info

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Jan 19, 2018 at 03:34:56PM +0000, Wei Liu wrote:
> diff --git a/xen/arch/x86/boot/build32.mk b/xen/arch/x86/boot/build32.mk
> index 48c7407c00..028ac19b96 100644
> --- a/xen/arch/x86/boot/build32.mk
> +++ b/xen/arch/x86/boot/build32.mk
> @@ -36,5 +36,8 @@ CFLAGS := $(filter-out -flto,$(CFLAGS))
>  cmdline.o: cmdline.c $(CMDLINE_DEPS)
>  
>  reloc.o: reloc.c $(RELOC_DEPS)
> +ifeq ($(CONFIG_PVH_GUEST),y)
> +reloc.o: CFLAGS += -DCONFIG_PVH_GUEST
> +endif

I would maybe do this above, where the rest of the CFLAGS are set.
Certainly setting -DCONFIG_PVH_GUEST shouldn't cause issues elsewhere.

CFLAGS-$(CONFIG_PVH_GUEST) += -DCONFIG_PVH_GUEST
CFLAGS += $(CFLAGS-y)

>  .PRECIOUS: %.bin %.lnk
> 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;

This this seems to be always expanding, maybe better to simply replace
the stdbool.h include above with types.h?

>  #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..614e53081e 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    $0x336ec578, %eax /* mov $XEN_HVM_START_MAGIC_VALUE, %eax */

Hm, if XEN_HVM_START_MAGIC_VALUE cannot be used I would rather prefer
to use (%ebx).

> -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);

Newline between non-fallthrough cases.

Thanks, Roger.

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