diff mbox series

[v2,2/7] x86: don't build with EFI support in shim-exclusive mode

Message ID 1a501ca8-8cf0-6fd0-547e-30b709fec6fc@suse.com
State Superseded
Headers show
Series x86: build adjustments | expand

Commit Message

Jan Beulich Aug. 7, 2020, 11:32 a.m. UTC
There's no need for xen.efi at all, and there's also no need for EFI
support in xen.gz since the shim runs in PVH mode, i.e. without any
firmware (and hence by implication also without EFI one).

The slightly odd looking use of $(space) is to ensure the new ifneq()
evaluates consistently between "build" and "install" invocations of
make.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
There are further anomalies associated with the need to use $(space)
here:
- xen.efi rebuilding gets suppressed when installing (typically as
  root) from a non-root-owned tree. I think we should similarly suppress
  re-building of xen.gz as well in this case, as tool chains available
  may vary (and hence a partial or full re-build may mistakenly occur).
- xen.lds (re-)generation has a dependency issue: The value of
  XEN_BUILD_EFI changing between builds (like would happen on a pre-
  built tree with a shim-exclusive config, on which then this patch
  would be applied) does not cause it to be re-built. Anthony's
  switching to Linux'es build system will address this afaict, so I
  didn't see a need to supply a separate patch.

Comments

Roger Pau Monne Aug. 18, 2020, 1 p.m. UTC | #1
On Fri, Aug 07, 2020 at 01:32:38PM +0200, Jan Beulich wrote:
> There's no need for xen.efi at all, and there's also no need for EFI
> support in xen.gz since the shim runs in PVH mode, i.e. without any
> firmware (and hence by implication also without EFI one).
> 
> The slightly odd looking use of $(space) is to ensure the new ifneq()
> evaluates consistently between "build" and "install" invocations of
> make.

I would likely add a comment to the code itself, as it's not obvious
without a hint IMO.

> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I wonder however if there would be a less tricky way to arrange all
this. Maybe the Kconfig work will remove some of this hacks?

Thanks, Roger.
Jan Beulich Aug. 19, 2020, 7:33 a.m. UTC | #2
On 18.08.2020 15:00, Roger Pau Monné wrote:
> On Fri, Aug 07, 2020 at 01:32:38PM +0200, Jan Beulich wrote:
>> There's no need for xen.efi at all, and there's also no need for EFI
>> support in xen.gz since the shim runs in PVH mode, i.e. without any
>> firmware (and hence by implication also without EFI one).
>>
>> The slightly odd looking use of $(space) is to ensure the new ifneq()
>> evaluates consistently between "build" and "install" invocations of
>> make.
> 
> I would likely add a comment to the code itself, as it's not obvious
> without a hint IMO.

I did indeed consider this, as I agree in principle. The problem is
where to put such a comment - ahead of the entire macro is not a
good place imo, and I can't see any other good place either. As a
result I thought that the use being odd looking will either make
readers think of why it's there by itself, or direct them towards
the commit introducing it.

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> I wonder however if there would be a less tricky way to arrange all
> this. Maybe the Kconfig work will remove some of this hacks?

Not sure.

Jan
Roger Pau Monne Aug. 19, 2020, 7:59 a.m. UTC | #3
On Wed, Aug 19, 2020 at 09:33:47AM +0200, Jan Beulich wrote:
> On 18.08.2020 15:00, Roger Pau Monné wrote:
> > On Fri, Aug 07, 2020 at 01:32:38PM +0200, Jan Beulich wrote:
> >> There's no need for xen.efi at all, and there's also no need for EFI
> >> support in xen.gz since the shim runs in PVH mode, i.e. without any
> >> firmware (and hence by implication also without EFI one).
> >>
> >> The slightly odd looking use of $(space) is to ensure the new ifneq()
> >> evaluates consistently between "build" and "install" invocations of
> >> make.
> > 
> > I would likely add a comment to the code itself, as it's not obvious
> > without a hint IMO.
> 
> I did indeed consider this, as I agree in principle. The problem is
> where to put such a comment - ahead of the entire macro is not a
> good place imo, and I can't see any other good place either. As a
> result I thought that the use being odd looking will either make
> readers think of why it's there by itself, or direct them towards
> the commit introducing it.

Ahead of the efi-y assignation would be fine for me, but I guess
people would use git blame and find the right commit seeing the kind
of strange construction that we use there.

Thanks, Roger.
diff mbox series

Patch

--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -80,7 +80,9 @@  x86_emulate.o: x86_emulate/x86_emulate.c
 
 efi-y := $(shell if [ ! -r $(BASEDIR)/include/xen/compile.h -o \
                       -O $(BASEDIR)/include/xen/compile.h ]; then \
-                         echo '$(TARGET).efi'; fi)
+                         echo '$(TARGET).efi'; fi) \
+         $(space)
+efi-$(CONFIG_PV_SHIM_EXCLUSIVE) :=
 
 ifneq ($(build_id_linker),)
 notes_phdrs = --notes
@@ -113,11 +115,13 @@  $(TARGET): $(TARGET)-syms $(efi-y) boot/
 		{ echo "No Multiboot2 header found" >&2; false; }
 	mv $(TMP) $(TARGET)
 
+ifneq ($(efi-y),)
 # Check if the compiler supports the MS ABI.
 export XEN_BUILD_EFI := $(shell $(CC) $(XEN_CFLAGS) -c efi/check.c -o efi/check.o 2>/dev/null && echo y)
 # Check if the linker supports PE.
 XEN_BUILD_PE := $(if $(XEN_BUILD_EFI),$(shell $(LD) -mi386pep --subsystem=10 -o efi/check.efi efi/check.o 2>/dev/null && echo y))
 CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI
+endif
 
 ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS)
 EFI_OBJS-$(XEN_BUILD_EFI) := efi/relocs-dummy.o