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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 30.03.2022 13:57, Jan Beulich wrote:
> On 30.03.2022 13:04, Michal Orzel wrote:
>> 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.
> 
> No, that's the case on Arm only. As Julien suggested, it is perhaps best
> to explain the difference between EFI and CONFIG_EFI, without going into
> arch specifics.
Could you please tell me what you are reffering to as there is no such 
identifier
in Xen (as opposed to Linux) like CONFIG_EFI ?

> 
> Jan
> 
>> Currently EFI macro is
>> only defined by x86 to link PE/COFF output, however it is not unique to this
>> architecture."
>>
>> Cheers,
>> Michal
>>
> 

Michal