Re: [XEN PATCH v6 21/31] build: set XEN_BUILD_EFI earlier

From: Jan Beulich <jbeulich@suse.com>
To: Anthony PERARD <anthony.perard@citrix.com>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>,
	"Julien Grall" <julien@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [XEN PATCH v6 21/31] build: set XEN_BUILD_EFI earlier
Date: Tue, 10 Aug 2021 09:44:16 +0200	[thread overview]
Message-ID: <82429ede-6bbe-ee51-861e-cd8d0c627311@suse.com> (raw)
In-Reply-To: <YRFQ5ufigapRnXgr@perard>

On 09.08.2021 17:59, Anthony PERARD wrote:
> On Thu, Aug 05, 2021 at 09:27:18AM +0200, Jan Beulich wrote:
>> On 01.07.2021 16:10, Anthony PERARD wrote:
>>> We are going to need the variable XEN_BUILD_EFI earlier.
>>>
>>> This early check is using "try-run" to allow to have a temporary
>>> output file in case it is needed for $(CC) to build the *.c file.
>>>
>>> The "efi/check.o" file is still needed in "arch/x86/Makefile" so the
>>> check is currently duplicated.
>>
>> Why is this? Can't you ...
>>
>>> --- a/xen/arch/x86/Makefile
>>> +++ b/xen/arch/x86/Makefile
>>> @@ -126,7 +126,7 @@ $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
>>>  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)
>>> +XEN_BUILD_EFI := $(shell $(CC) $(XEN_CFLAGS) -c efi/check.c -o efi/check.o 2>/dev/null && echo y)
>>>  CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI
>>
>> ... use here what you ...
>>
>>> --- a/xen/arch/x86/arch.mk
>>> +++ b/xen/arch/x86/arch.mk
>>> @@ -60,5 +60,10 @@ ifeq ($(CONFIG_UBSAN),y)
>>>  $(call cc-option-add,CFLAGS_UBSAN,CC,-fno-sanitize=alignment)
>>>  endif
>>>  
>>> +ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y)
>>> +# Check if the compiler supports the MS ABI.
>>> +export XEN_BUILD_EFI := $(call try-run,$(CC) $(CFLAGS) -c arch/x86/efi/check.c -o "$$TMPO",y)
>>> +endif
>>
>> ... export here?
> 
> The problem with the check for EFI support is that there several step,
> with a step depending on the binary produced by the previous one.
> 
> XEN_BUILD_EFI
>     In addition to check "__ms_abi__" attribute is supported by $CC, the
>     file "efi/check.o" is produced.
> XEN_BUILD_PE
>     It is using "efi/check.o" to check for PE support and produce
>     "efi/check.efi".
> "efi/check.efi" is also used by the Makefile for additional checks
> (mkreloc).
> 
> 
> So, if I let the duplicated check for $(XEN_BUILD_EFI) is that it felt
> wrong to produce "efi/check.o" in "arch/x86/arch.mk" and then later use
> it in "arch/x86/Makefile". I could maybe move the command that create
> efi/check.o in the $(XEN_BUILD_PE) check, or I could try to move most of
> the checks done for EFI into x86/arch.mk. Or maybe just creating the
> "efi/check.o" file in x86/arch.mk and use it in x86/Makefile, with a
> comment.
> 
> What do you think?

The last option looks to promise the least code churn while still
eliminating the duplication. So that's one option I'd be fine with, the
other being to do all of this together in a single place.

Jan