Re: [Xen-devel] [PATCH RFC v1 20/74] x86: produce a binary that can be booted as PVH

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.18 at 14:05, <wei.liu2@xxxxxxxxxx> wrote:
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Again I assume a description is still being intended to be written

> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -75,6 +75,8 @@ efi-y := $(shell if [ ! -r 
> $(BASEDIR)/include/xen/compile.h -o \
>                        -O $(BASEDIR)/include/xen/compile.h ]; then \
>                           echo '$(TARGET).efi'; fi)
>  
> +shim-$(CONFIG_PVH_GUEST) := $(TARGET)-shim
> +
>  ifneq ($(build_id_linker),)
>  notes_phdrs = --notes
>  else
> @@ -93,7 +95,7 @@ endif
>  syms-warn-dup-y := --warn-dup
>  syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
>  
> -$(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
> +$(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 $(shim-y)
>       ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) 
> $(XEN_IMG_OFFSET) \
>                      `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . 
> __2M_rwdata_end$$/0x\1/p'`

Hmm, so you mean to build shim and "normal" Xen at the same time,
with all the same objects? That's rather unexpected following the
earlier exchange Andrew and I had. I would expect the shim to not
require quite a few bits and pieces, and hence wanting to be built
independently.

> @@ -144,6 +146,11 @@ $(TARGET)-syms: prelink.o xen.lds 
> $(BASEDIR)/common/symbols-dummy.o
>               >$(@D)/$(@F).map
>       rm -f $(@D)/.$(@F).[0-9]*
>  
> +# Use elf32-x86-64 if toolchain support exists, elf32-i386 otherwise.
> +$(TARGET)-shim: FORMAT = $(firstword $(filter elf32-x86-64,$(shell 
> $(OBJCOPY) --help)) elf32-i386)

What are the implications of using one vs the other? If elf32-i386
works, why not use it all the time?

> @@ -374,6 +375,15 @@ cs32_switch:
>          /* Jump to earlier loaded address. */
>          jmp     *%edi
>  
> +
> +#ifdef CONFIG_PVH_GUEST

No double blank lines please.

> +ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_ENTRY, .long sym_offs(__pvh_start))
> +
> +__pvh_start:
> +        ud2a
> +
> +#endif /* CONFIG_PVH_GUEST */
> +
>  __start:

Does the new code strictly need to live here? Can't is be kept both
out of the resulting binary sequence currently resulting here and
out of this source file altogether (by introducing a new pvh.S or
shim.S)?

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -34,7 +34,7 @@ OUTPUT_ARCH(i386:x86-64)
>  PHDRS
>  {
>    text PT_LOAD ;
> -#if defined(BUILD_ID) && !defined(EFI)
> +#if (defined(BUILD_ID) && !defined(EFI)) || defined (CONFIG_PVH_GUEST)

Did you mean

#if (defined(BUILD_ID) || defined(CONFIG_PVH_GUEST)) && !defined(EFI)

? Of course this would be moot if main and shim binary were to
be built independently.

Also - stray blank.

> @@ -128,6 +128,12 @@ SECTIONS
>         __param_end = .;
>    } :text
>  
> +#if defined(CONFIG_PVH_GUEST) && !defined(EFI)

The EFI part here then also wouldn't be necessary, afaict.

Jan


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