Re: [Xen-devel] [PATCH] build: remove shim related targets

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH] build: remove shim related targets"):
> On Tue, Feb 20, 2018 at 03:09:18PM +, Ian Jackson wrote:
> > Roger Pau Monne writes ("[PATCH] build: remove shim related targets"):
> > > There's no need to have shim specific targets, so just use the regular
> > > xen makefile targets in order to build the shim binary.
> > 
> > I haven't been following this, mostly because I've been away, but:
> > 
> > Previously (in the Comet advice in the advisory, for example) we told
> > people to run this special Makefile target, and that special Makefile
> > target would rebuild the whole of the hypervisor in some special way.
> > 
> > Is that linkfarm and rebuild now no longer needed, then ?
> 
> That's still needed in order to build the shim with the right Kconfig
> (which should be different from the Kconfig used for the bare metal
> hypervisor build).
> 
> The main difference is that we can get rid of the shim related
> makefile targets inside of xen/ because the same makefile targets can
> be used to build a shim or a bare-metal version of Xen.

Ah I see.  Indeed if I look at the patch again the existing published
entrypoint shim target is being retained, and I am being needlessly
alarmed by the patch subject line.

Thanks for the explanation.

Ian.

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

Re: [Xen-devel] [PATCH] build: remove shim related targets

[Roger Pau Monné]

On Tue, Feb 20, 2018 at 03:09:18PM +, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH] build: remove shim related targets"):
> > There's no need to have shim specific targets, so just use the regular
> > xen makefile targets in order to build the shim binary.
> 
> I haven't been following this, mostly because I've been away, but:
> 
> Previously (in the Comet advice in the advisory, for example) we told
> people to run this special Makefile target, and that special Makefile
> target would rebuild the whole of the hypervisor in some special way.
> 
> Is that linkfarm and rebuild now no longer needed, then ?

That's still needed in order to build the shim with the right Kconfig
(which should be different from the Kconfig used for the bare metal
hypervisor build).

The main difference is that we can get rid of the shim related
makefile targets inside of xen/ because the same makefile targets can
be used to build a shim or a bare-metal version of Xen.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH] build: remove shim related targets

[Ian Jackson]

Roger Pau Monne writes ("[PATCH] build: remove shim related targets"):
> There's no need to have shim specific targets, so just use the regular
> xen makefile targets in order to build the shim binary.

I haven't been following this, mostly because I've been away, but:

Previously (in the Comet advice in the advisory, for example) we told
people to run this special Makefile target, and that special Makefile
target would rebuild the whole of the hypervisor in some special way.

Is that linkfarm and rebuild now no longer needed, then ?

Ian.

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

Re: [Xen-devel] [PATCH] build: remove shim related targets

[Wei Liu]

On Tue, Feb 20, 2018 at 11:29:58AM +, Wei Liu wrote:
> On Tue, Feb 20, 2018 at 11:26:44AM +, Roger Pau Monné wrote:
> > On Tue, Feb 20, 2018 at 11:17:47AM +, Wei Liu wrote:
> > > On Tue, Feb 20, 2018 at 08:23:51AM +, Roger Pau Monne wrote:
> > > > There's no need to have shim specific targets, so just use the regular
> > > > xen makefile targets in order to build the shim binary.
> > > > 
> > > > When the shim is build as part of the firmware directory use the
> > > > xen-syms as the shim binary.
> > > > 
> > > > Signed-off-by: Roger Pau Monné 
> > > > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> > > > index 389096139c..5563c813dd 100644
> > > > --- a/xen/arch/x86/Makefile
> > > > +++ b/xen/arch/x86/Makefile
> > > > @@ -78,12 +78,12 @@ 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
> > > > -notes_phdrs =
> > > > +ifeq ($(CONFIG_PVH_GUEST),y)
> > > > +notes_phdrs = --notes
> > > > +endif
> > > >  endif
> > > >  
> > > 
> > > The manual suggests following syntax is supported:
> > > 
> > > 
> > >   conditional-directive-one
> > >   text-if-one-is-true
> > >   else conditional-directive-two
> > >   text-if-two-is-true
> > >   else
> > >   text-if-one-and-two-are-false
> > >   endif
> > > 
> > > I slightly prefer
> > > 
> > >ifneq build_id_linker
> > >notes_phdrs = ...
> > >else if CONFIG_PVH_GUEST
> > >notes_phdrs = ...
> > >else
> > >notes_phdrs =
> > >endif
> > 
> > Is this supported by make 3.80?
> > 
> > Jan pointed out in another thread that using "else if" on a single
> > line is not supported by 3.80:
> > 
> > https://lists.xenproject.org/archives/html/xen-devel/2018-02/msg01659.html
> > 

Actually, see

https://www.cl.cam.ac.uk/teaching/1011/UnixTools/make.pdf


http://www.delorie.com/gnu/docs/make/make_77.html

Make 3.82 manual supports that syntax. But 3.80 doesn't.

Wei.

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

Re: [Xen-devel] [PATCH] build: remove shim related targets

[Wei Liu]

On Tue, Feb 20, 2018 at 11:26:44AM +, Roger Pau Monné wrote:
> On Tue, Feb 20, 2018 at 11:17:47AM +, Wei Liu wrote:
> > On Tue, Feb 20, 2018 at 08:23:51AM +, Roger Pau Monne wrote:
> > > There's no need to have shim specific targets, so just use the regular
> > > xen makefile targets in order to build the shim binary.
> > > 
> > > When the shim is build as part of the firmware directory use the
> > > xen-syms as the shim binary.
> > > 
> > > Signed-off-by: Roger Pau Monné 
> > > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> > > index 389096139c..5563c813dd 100644
> > > --- a/xen/arch/x86/Makefile
> > > +++ b/xen/arch/x86/Makefile
> > > @@ -78,12 +78,12 @@ 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
> > > -notes_phdrs =
> > > +ifeq ($(CONFIG_PVH_GUEST),y)
> > > +notes_phdrs = --notes
> > > +endif
> > >  endif
> > >  
> > 
> > The manual suggests following syntax is supported:
> > 
> > 
> >   conditional-directive-one
> >   text-if-one-is-true
> >   else conditional-directive-two
> >   text-if-two-is-true
> >   else
> >   text-if-one-and-two-are-false
> >   endif
> > 
> > I slightly prefer
> > 
> >ifneq build_id_linker
> >notes_phdrs = ...
> >else if CONFIG_PVH_GUEST
> >notes_phdrs = ...
> >else
> >notes_phdrs =
> >endif
> 
> Is this supported by make 3.80?
> 
> Jan pointed out in another thread that using "else if" on a single
> line is not supported by 3.80:
> 
> https://lists.xenproject.org/archives/html/xen-devel/2018-02/msg01659.html
> 

Oh if there is a such reason then it is fine to keep the code as-is.
I don't have make 3.8 around to try it out.

Wei.

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

Re: [Xen-devel] [PATCH] build: remove shim related targets

[Roger Pau Monné]

On Tue, Feb 20, 2018 at 11:17:47AM +, Wei Liu wrote:
> On Tue, Feb 20, 2018 at 08:23:51AM +, Roger Pau Monne wrote:
> > There's no need to have shim specific targets, so just use the regular
> > xen makefile targets in order to build the shim binary.
> > 
> > When the shim is build as part of the firmware directory use the
> > xen-syms as the shim binary.
> > 
> > Signed-off-by: Roger Pau Monné 
> > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> > index 389096139c..5563c813dd 100644
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -78,12 +78,12 @@ 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
> > -notes_phdrs =
> > +ifeq ($(CONFIG_PVH_GUEST),y)
> > +notes_phdrs = --notes
> > +endif
> >  endif
> >  
> 
> The manual suggests following syntax is supported:
> 
> 
>   conditional-directive-one
>   text-if-one-is-true
>   else conditional-directive-two
>   text-if-two-is-true
>   else
>   text-if-one-and-two-are-false
>   endif
> 
> I slightly prefer
> 
>ifneq build_id_linker
>notes_phdrs = ...
>else if CONFIG_PVH_GUEST
>notes_phdrs = ...
>else
>notes_phdrs =
>endif

Is this supported by make 3.80?

Jan pointed out in another thread that using "else if" on a single
line is not supported by 3.80:

https://lists.xenproject.org/archives/html/xen-devel/2018-02/msg01659.html

Thanks, Roger.

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

Re: [Xen-devel] [PATCH] build: remove shim related targets

[Wei Liu]

On Tue, Feb 20, 2018 at 08:23:51AM +, Roger Pau Monne wrote:
> There's no need to have shim specific targets, so just use the regular
> xen makefile targets in order to build the shim binary.
> 
> When the shim is build as part of the firmware directory use the
> xen-syms as the shim binary.
> 
> Signed-off-by: Roger Pau Monné 
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 389096139c..5563c813dd 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -78,12 +78,12 @@ 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
> -notes_phdrs =
> +ifeq ($(CONFIG_PVH_GUEST),y)
> +notes_phdrs = --notes
> +endif
>  endif
>  

The manual suggests following syntax is supported:


  conditional-directive-one
  text-if-one-is-true
  else conditional-directive-two
  text-if-two-is-true
  else
  text-if-one-and-two-are-false
  endif

I slightly prefer

   ifneq build_id_linker
   notes_phdrs = ...
   else if CONFIG_PVH_GUEST
   notes_phdrs = ...
   else
   notes_phdrs =
   endif

Wei.

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

Re: [Xen-devel] [PATCH] build: remove shim related targets

[Jan Beulich]

>>> On 20.02.18 at 11:06,  wrote:
> On Tue, Feb 20, 2018 at 02:04:24AM -0700, Jan Beulich wrote:
>> >>> On 20.02.18 at 09:23,  wrote:
>> > --- a/tools/firmware/xen-dir/Makefile
>> > +++ b/tools/firmware/xen-dir/Makefile
>> > @@ -48,10 +48,10 @@ shim-%config: $(D) FORCE
>> >KCONFIG_CONFIG=$(CURDIR)/shim.config
>> >  
>> >  xen-shim: $(D) shim-olddefconfig
>> > -  $(MAKE) -C $(D)/xen install-shim \
>> > +  $(MAKE) -C $(D)/xen build \
>> 
>> install-xen ?
> 
> If the install target is used it's more complicated to fetch the
> symbols file afterwards, because it's going to contain the Xen version
> in the name. The symbols file when using the install target is at:
> 
> $(DESTDIR)$(DEBUG_DIR)/xen-syms-$(XEN_FULLVERSION)
> 
> And obtaining XEN_FULLVERSION from the xen-shim build makefile is not
> trivial.

Fair enough.

Jan


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

Re: [Xen-devel] [PATCH] build: remove shim related targets

[Roger Pau Monné]

On Tue, Feb 20, 2018 at 02:04:24AM -0700, Jan Beulich wrote:
> >>> On 20.02.18 at 09:23,  wrote:
> > There's no need to have shim specific targets, so just use the regular
> > xen makefile targets in order to build the shim binary.
> > 
> > When the shim is build as part of the firmware directory use the
> > xen-syms as the shim binary.
> 
> Why the (much larger) xen-syms instead of xen?

IIRC Andrew argued that he would like the installed shim to contain
symbols. I've used xen-syms in order to replicate current
functionality, which installs a xen-shim binary with the symbols.

In the new version of this patch I plan to use xen, and install
xen-syms in the DEBUG_DIR.

> > --- a/tools/firmware/xen-dir/Makefile
> > +++ b/tools/firmware/xen-dir/Makefile
> > @@ -48,10 +48,10 @@ shim-%config: $(D) FORCE
> > KCONFIG_CONFIG=$(CURDIR)/shim.config
> >  
> >  xen-shim: $(D) shim-olddefconfig
> > -   $(MAKE) -C $(D)/xen install-shim \
> > +   $(MAKE) -C $(D)/xen build \
> 
> install-xen ?

If the install target is used it's more complicated to fetch the
symbols file afterwards, because it's going to contain the Xen version
in the name. The symbols file when using the install target is at:

$(DESTDIR)$(DEBUG_DIR)/xen-syms-$(XEN_FULLVERSION)

And obtaining XEN_FULLVERSION from the xen-shim build makefile is not
trivial.

> > XEN_CONFIG_EXPERT=y \
> > -   KCONFIG_CONFIG=$(CURDIR)/shim.config \
> > -   DESTDIR=$(CURDIR)
> > +   KCONFIG_CONFIG=$(CURDIR)/shim.config
> > +   cp $(D)/xen/xen-syms xen-shim
> 
> ln? Or do here what was done ...

I can certainly use ln.

> > @@ -149,11 +149,6 @@ $(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)
> > -$(TARGET)-shim: $(TARGET)-syms
> > -   $(OBJCOPY) -O $(FORMAT) $< $@
> 
> ... here so far? The removal of this step would otherwsie require
> more explanation - I recall having inquired about this before: If
> there was a reason for doing this, if shouldn't be dropped without
> saying why the other binary is now suddenly fine, but wasn't back
> then. Or alternatively it would need to be clarified that this step
> was indeed pointless.

I will add the following to the commit message:

"The objcopy step of the shim build is also removed in this patch:
since the shim is booted in PVH mode there's no need for the resulting
binary to be in elf32 format. Xen can load PVH kernels with either a
32 or 64bit elf header."

Thanks, Roger.

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

Re: [Xen-devel] [PATCH] build: remove shim related targets

[Jan Beulich]

>>> On 20.02.18 at 09:23,  wrote:
> There's no need to have shim specific targets, so just use the regular
> xen makefile targets in order to build the shim binary.
> 
> When the shim is build as part of the firmware directory use the
> xen-syms as the shim binary.

Why the (much larger) xen-syms instead of xen?

> --- a/tools/firmware/xen-dir/Makefile
> +++ b/tools/firmware/xen-dir/Makefile
> @@ -48,10 +48,10 @@ shim-%config: $(D) FORCE
>   KCONFIG_CONFIG=$(CURDIR)/shim.config
>  
>  xen-shim: $(D) shim-olddefconfig
> - $(MAKE) -C $(D)/xen install-shim \
> + $(MAKE) -C $(D)/xen build \

install-xen ?

>   XEN_CONFIG_EXPERT=y \
> - KCONFIG_CONFIG=$(CURDIR)/shim.config \
> - DESTDIR=$(CURDIR)
> + KCONFIG_CONFIG=$(CURDIR)/shim.config
> + cp $(D)/xen/xen-syms xen-shim

ln? Or do here what was done ...

> @@ -149,11 +149,6 @@ $(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)
> -$(TARGET)-shim: $(TARGET)-syms
> - $(OBJCOPY) -O $(FORMAT) $< $@

... here so far? The removal of this step would otherwsie require
more explanation - I recall having inquired about this before: If
there was a reason for doing this, if shouldn't be dropped without
saying why the other binary is now suddenly fine, but wasn't back
then. Or alternatively it would need to be clarified that this step
was indeed pointless.

Jan


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