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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Jan 05, 2018 at 04:39:33AM -0700, Jan Beulich wrote:
> >>> 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.
> 

There is a later patch in this series to link xen under tools/firmware/
to build the shim there, which would need build system patch like this.

The can be cleaned up somehow. At the time I wasn't sure how best to
proceed (and certainly didn't take part in the discussion between Andrew
and you).

Suggestions welcome.

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

Not sure, Andrew made this change. I will leave this to him.

> > @@ -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)?
> 

We can use a new source file.

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

My goal was to include the note section when building the shim. Your
comment looks correct to me. I will clean this up in the next version.

Wei.

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