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

[oleksii.kurochko@xxxxxxxxx]

Hi Shawn,

On Thu, 2024-09-19 at 16:05 -0500, Shawn Anastasio wrote:
> 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)?
There is no specific reason, just decided it would be nice to abstract
the the full section declaration.

>  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).
I’m okay with the suggested approach. I’ll wait for a bit to see if
anyone has other comments. If there are no responses, I’ll resend the
patch series with the proposed changes.

Thanks!

~ Oleksii

> 
> > +
> >  #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.