Re: [PATCH v4 1/6] xen: introduce SIMPLE_DECL_SECTION

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.09.2024 12:42, oleksii.kurochko@xxxxxxxxx wrote:
> On Fri, 2024-09-27 at 11:41 +0200, Roger Pau Monné wrote:
>> On Fri, Sep 27, 2024 at 11:07:58AM +0200,
>> oleksii.kurochko@xxxxxxxxx wrote:
>>> On Fri, 2024-09-27 at 09:58 +0200, Roger Pau Monné wrote:
>>>> On Thu, Sep 26, 2024 at 06:54:20PM +0200, Oleksii Kurochko wrote:
>>>>> Introduce SIMPLE_DECL_SECTION to cover the case when
>>>>> an architecture wants to declare a section without specifying
>>>>> of load address for the section.
>>>>>
>>>>> Update x86/xen.lds.S to use SIMPLE_DECL_SECTION.
>>>>
>>>> No strong opinion, but I feel SIMPLE is not very descriptive.  It
>>>> might be better to do it the other way around: introduce a define
>>>> for
>>>> when the DECL_SECTION macro should specify a load address:
>>>> DECL_SECTION_WITH_LADDR for example.
>>> In the next patch, two sections are introduced: dt_dev_info and
>>> acpi_dev_info. The definition of these sections has been made
>>> common
>>> and moved to xen.lds.h, and it looks like this:
>>>    +#define DT_DEV_INFO(secname)    \
>>>    +  . = ALIGN(POINTER_ALIGN);     \
>>>    +  DECL_SECTION(secname) {       \
>>>    +       _sdevice = .;           \
>>>    +       *(secname)              \
>>>    +       _edevice = .;           \
>>>    +  } :text
>>> (A similar approach is used for ACPI, please refer to the next
>>> patch in
>>> this series.)
>>>
>>> For PPC, DECL_SECTION should specify a load address, whereas for
>>> Arm
>>> and RISC-V, it should not.
>>>
>>> With this generalization, the name of DECL_SECTION should have the
>>> same
>>> name in both cases, whether a load address needs to be specified or
>>> not
>>
>> Oh, sorry, I think you misunderstood my suggestion.
>>
>> I'm not suggesting to introduce a new macro named
>> DECL_SECTION_WITH_LADDR(), but rather to use DECL_SECTION_WITH_LADDR
>> instead of SIMPLE_DECL_SECTION in order to signal whether
>> DECL_SECTION() should specify a load address or not, iow:
>>
>> #ifdef DECL_SECTION_WITH_LADDR
>> # define DECL_SECTION(x) x : AT(ADDR(x) - __XEN_VIRT_START)
>> #else
>> # define DECL_SECTION(x) x :
>> #endif
> Thanks for the clarification, I really misunderstood your initial
> suggestion.
> 
> I'm okay with the renaming; perhaps it will indeed make things a bit
> clearer.
> 
> If Jan doesn’t mind (since he gave the Ack), I'll rename the define in
> the next patch version.
> Jan, do you mind if I proceed with the renaming?

I'm not overly fussed, so fee free to go ahead and retain my ack.

Jan