Re: [PATCH v2 1/5] xen: define ACPI and DT device info sections macros

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.09.2024 23:05, Shawn Anastasio wrote:
> On 9/17/24 11:15 AM, Oleksii Kurochko wrote:
>> --- a/xen/include/xen/xen.lds.h
>> +++ b/xen/include/xen/xen.lds.h
>> @@ -114,6 +114,21 @@
>>  
>>  /* List of constructs other than *_SECTIONS in alphabetical order. */
>>  
>> +#define USE_DECL_SECTION(x) DECL_SECTION(x)
>> +
>> +#define NOUSE_DECL_SECTION(x) x :
>> +
>> +#ifdef CONFIG_ACPI
>> +#define ACPI_DEV_INFO_SEC(secname, DECL_SECTION_MACROS_NAME)  \
>> +    DECL_SECTION_MACROS_NAME(secname) {                     \
>> +      _asdevice = .;                                        \
>> +      *(secname)                                            \
>> +      _aedevice = .;                                        \
>> +    } :text
>> +#else
>> +#define ACPI_DEV_INFO_SEC(secname, DECL_SECTION_MACROS_NAME)
>> +#endif /* CONFIG_ACPI */
> 
> This works, but is there a reason you didn't just define the actual
> entries themselves in the macro, like is done for most of the other
> macros in this file (including BUFRAMES right below this)? Something
> like:
> 
> #define ADEV_INFO     \
>     _asdevice = .;    \
>     *(secname)        \
>     _aedevice = .;    \
> 
> Parameterizing the section name seems pointless since there are other
> parts of the code that rely on them, and parameterizing the macro for
> declaring a section adds unnecessary boilerplate for non-ppc/x86
> architectures (the NOUSE_DECL_SECTION no-op).
> 
>> +
>>  #define BUGFRAMES                               \
>>      __start_bug_frames_0 = .;                   \
>>      *(.bug_frames.0)                            \
>> @@ -131,6 +146,17 @@
>>      *(.bug_frames.3)                            \
>>      __stop_bug_frames_3 = .;
>>  
>> +#ifdef CONFIG_HAS_DEVICE_TREE
>> +#define DT_DEV_INFO_SEC(secname, DECL_SECTION_MACROS_NAME)  \
>> +    DECL_SECTION_MACROS_NAME(secname) {                     \
>> +      _sdevice = .;                                         \
>> +      *(secname)                                            \
>> +      _edevice = .;                                         \
>> +    } :text
>> +#else
>> +#define DECL_DT_DEV_INFO_SEC(secname, DECL_SECTION_MACROS_NAME)
>> +#endif /* CONFIG_HAS_DEVICE_TREE */
> 
> Ditto. Also, if you go with the approach I mentioned, then you wouldn't
> need to guard these on CONFIG_HAS_DEVICE_TREE/CONFIG_ACPI since that
> would be done in the linker scripts themselves.

+1

Jan