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

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

Hi Oleksii,

On 9/17/24 11:15 AM, Oleksii Kurochko wrote:
> Introduce conditional macros to define device
> information sections based on the configuration of ACPI
> or device tree support. These sections are required for
> common code of device initialization and getting an information
> about a device.
> 
> These macros are expected to be used across different
> architectures (Arm, PPC, RISC-V), so they are moved to
> the common xen/xen.lds.h, based on their original definition
> in Arm.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in v2:
>  - New patch.
> ---
>  xen/include/xen/xen.lds.h | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
> index a17810bb28..aa7301139d 100644
> --- 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.

Thanks,
Shawn