Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros

From: Michal Orzel <michal.orzel@arm.com>
To: Jan Beulich <jbeulich@suse.com>, Julien Grall <julien@xen.org>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Bertrand Marquis" <bertrand.marquis@arm.com>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 2/2] xen: Populate xen.lds.h and make use of its macros
Date: Wed, 30 Mar 2022 13:04:15 +0200	[thread overview]
Message-ID: <ffa0e581-6a32-5c3c-7e63-acd5086c6822@arm.com> (raw)
In-Reply-To: <1013a14b-a45b-f37f-b2e0-e63b186a2956@suse.com>

Hello,

On 30.03.2022 12:42, Jan Beulich wrote:
> On 30.03.2022 12:32, Julien Grall wrote:
>> On 29/03/2022 12:42, Jan Beulich wrote:
>>> On 29.03.2022 12:54, Julien Grall wrote:
>>>> On 29/03/2022 11:12, Michal Orzel wrote:
>>>>> On 29.03.2022 11:54, Julien Grall wrote:
>>>>>> On 22/03/2022 08:02, Michal Orzel wrote:
>>>>>>> --- a/xen/include/xen/xen.lds.h
>>>>>>> +++ b/xen/include/xen/xen.lds.h
>>>>>>> @@ -5,4 +5,104 @@
>>>>>>>      * Common macros to be used in architecture specific linker scripts.
>>>>>>>      */
>>>>>>>     +/* Macros to declare debug sections. */
>>>>>>> +#ifdef EFI
>>>>>>
>>>>>> AFAIK, we don't define EFI on Arm (just CONFIG_EFI). Yet we do support EFI on arm64.
>>>>>>
>>>>>> As this #ifdef is now in generic code, can you explain how this is meant to be used?
>>>>>>
>>>>> As we do not define EFI on arm, all the stuff protected by #ifdef EFI is x86 specific.
>>>>
>>>> I find the name "EFI" too generic to figure out that this code can only
>>>> be used by x86.
>>>>
>>>> But, from my understanding, this header is meant to contain generic
>>>> code. It feels a bit odd that we are moving arch specific code.
>>>>
>>>> To be honest, I don't quite understand why we need to make the
>>>> diffferentiation on x86. So I guess the first question is how this is
>>>> meant to be used on x86?
>>>
>>> We produce two linker scripts from the single source file: One (with EFI
>>> undefined) to link the ELF binary, and another (with EFI defined) to link
>>> the PE/COFF output. If "EFI" is too imprecise as a name for the identifier,
>>> I wouldn't mind renaming it (to PE_COFF?), but at the same time I'm not
>>> convinced this is really necessary.
>>
>> Thank for the explanation (and the other ones in this thread). You are 
>> right the confusion arised from "generating" vs "linking".
>>
>> Renaming to PE_COFF may help to avoid the confusion with CONFIG_EFI. 
>> That said, it would possibly make more difficult to associate the flag 
>> with "linking an EFI binary".
> 
> Indeed. And EFI_PE_COFF is getting a little unwieldy for my taste.
> 
>> I think some documentaion about the define EFI would be help so there 
>> are no more confusion between CONFIG_EFI/EFI. But I am not sure where to 
>> put it. Maybe at the top of the header?
> 
> That's perhaps the best place, yes.
> 
In this case how about the following comment at the top of xen.lds.h:

"To avoid any confusion about EFI macro used in this header vs EFI support,
the former is used when linking a native EFI (i.e. PE/COFF) binary, whereas
the latter means support for generating EFI binary. Currently EFI macro is
only defined by x86 to link PE/COFF output, however it is not unique to this
architecture."

> Jan
> 

Cheers,
Michal