Re: [PATCH] EFI: strip xen.efi when putting it on the EFI partition

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Jun 09, 2022 at 05:52:45PM +0200, Jan Beulich wrote:
> With debug info retained, xen.efi can be quite large. Unlike for xen.gz
> there's no intermediate step (mkelf32 there) involved which would strip
> debug info kind of as a side effect. While the installing of xen.efi on
> the EFI partition is an optional step (intended to be a courtesy to the
> developer), adjust it also for the purpose of documenting what distros
> would be expected to do during boot loader configuration (which is what
> would normally put xen.efi into the EFI partition).
> 
> Model the control over stripping after Linux'es module installation,
> except that the stripped executable is constructed in the build area
> instead of in the destination location. This is to conserve on space
> used there - EFI partitions tend to be only a few hundred Mb in size.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> GNU strip 2.38 appears to have issues when acting on a PE binary:
> - file name symbols are also stripped; while there is a separate
>   --keep-file-symbols option (which I would have thought to be on by
>   default anyway), its use so far makes no difference,
> - the string table grows in size, when one would expect it to retain its
>   size (or shrink),
> - linker version is changed in and timestamp zapped from the header.
> Older GNU strip (observed with 2.35.1) doesn't work at all ("Data
> Directory size (1c) exceeds space left in section (8)").
> 
> Future GNU strip is going to honor --keep-file-symbols (and will also
> have the other issues fixed). Question is whether we should use that
> option (for the symbol table as a whole to make sense), or whether
> instead we should (by default) strip the symbol table as well.

I guess trying to keep those symbol might be useful, if it's not too
big, to help debugging system in production.

> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -465,6 +465,22 @@ endif
>  .PHONY: _build
>  _build: $(TARGET)$(CONFIG_XEN_INSTALL_SUFFIX)
>  
> +# Strip
> +#
> +# INSTALL_EFI_STRIP, if defined, will cause xen.efi to be stripped before it
> +# is installed. If INSTALL_EFI_STRIP is '1', then the default option
> +# --strip-debug will be used. Otherwise, INSTALL_EFI_STRIP value will be used
> +# as the option(s) to the strip command.

It would be useful to also document INSTALL_EFI_STRIP in ./INSTALL or in
./docs/misc/efi.pandoc (efi.pandoc is where EFI_VENDOR is documented for
example, so probably a better place for the doc of the new option).

> +ifdef INSTALL_EFI_STRIP
> +
> +ifeq ($(INSTALL_EFI_STRIP),1)
> +efi-strip-opt := --strip-debug
> +else
> +efi-strip-opt := $(INSTALL_EFI_STRIP)
> +endif
> +
> +endif
> +
>  .PHONY: _install
>  _install: D=$(DESTDIR)
>  _install: T=$(notdir $(TARGET))
> @@ -489,6 +505,9 @@ _install: $(TARGET)$(CONFIG_XEN_INSTALL_
>               ln -sf $(T)-$(XEN_FULLVERSION).efi 
> $(D)$(EFI_DIR)/$(T)-$(XEN_VERSION).efi; \
>               ln -sf $(T)-$(XEN_FULLVERSION).efi $(D)$(EFI_DIR)/$(T).efi; \
>               if [ -n '$(EFI_MOUNTPOINT)' -a -n '$(EFI_VENDOR)' ]; then \
> +                     $(if $(efi-strip-opt), \
> +                          $(STRIP) $(efi-strip-opt) -p -o 
> $(TARGET).efi.stripped $(TARGET).efi && \
> +                          $(INSTALL_DATA) $(TARGET).efi.stripped 
> $(D)$(EFI_MOUNTPOINT)/efi/$(EFI_VENDOR)/$(T)-$(XEN_FULLVERSION).efi ||) \

This optional part of the script that ends with "||" means that if
`strip` or `install` fails, we would install the non stripped binary. Is
this something that's acceptable? (Plus of doing that is avoiding
changing the next line, and avoiding a longer $(if ,).

>                       $(INSTALL_DATA) $(TARGET).efi 
> $(D)$(EFI_MOUNTPOINT)/efi/$(EFI_VENDOR)/$(T)-$(XEN_FULLVERSION).efi; \
>               elif [ "$(D)" = "$(patsubst $(shell cd $(XEN_ROOT) && 
> pwd)/%,%,$(D))" ]; then \
>                       echo 'EFI installation only partially done (EFI_VENDOR 
> not set)' >&2; \

An other thing, the "*.efi.stripped" file is I think going to be left
over and not removed during `make clean`.

Cheers,

-- 
Anthony PERARD