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

[oleksii.kurochko@xxxxxxxxx]

On Wed, 2024-09-25 at 10:36 +0200, Jan Beulich wrote:
> On 24.09.2024 18:42, Oleksii Kurochko wrote:
> > --- a/xen/include/xen/xen.lds.h
> > +++ b/xen/include/xen/xen.lds.h
> > @@ -114,6 +114,11 @@
> >  
> >  /* List of constructs other than *_SECTIONS in alphabetical order.
> > */
> >  
> > +#define ADEV_INFO     \
> > +      _asdevice = .;  \
> > +      *(.adev.info)   \
> > +      _aedevice = .;
> > +
> >  #define BUGFRAMES                               \
> >      __start_bug_frames_0 = .;                   \
> >      *(.bug_frames.0)                            \
> > @@ -131,6 +136,11 @@
> >      *(.bug_frames.3)                            \
> >      __stop_bug_frames_3 = .;
> >  
> > +#define DT_DEV_INFO \
> > +      _sdevice = .; \
> > +      *(.dev.info)  \
> > +      _edevice = .;
> 
> I have a question more to the Arm maintainers than to you, Oleksii:
> Section
> names as well as the names of the symbols bounding the sections are
> overly
> unspecific. There's nothing indicating DT at all, and it's solely 'a'
> to
> indicate ACPI. I consider this a possible problem going forward, when
> this
> is now being generalized.
> 
> In turn I wonder about ADEV_INFO when comparing with DT_DEV_INFO. The
> latter makes clear it's DT. The former doesn't make clear it's ACPI;
> 'A'
> could stand for about anything, including "a device" (of any kind).
> 
> Finally, Oleksii, looking at Arm's present uses - why is the
> abstraction
> limited to the inner part of the section statements in the linker
> script?
I tried to abstract not only inner part of the section statements but
based on the comments to previous patch series, at least, I made an
abstraction not in the best way but I considered the comments as it
would be better to limit everything to the inner part.

> IOW why isn't it all (or at least quite a bit more) of
> 
>   . = ALIGN(8);
>   .dev.info : {
>       _sdevice = .;
>       *(.dev.info)
>       _edevice = .;
>   } :text
> 
> that moves into DT_DEV_INFO? I can see that the :text may want
> leaving
> to the architectures (yet then perhaps as a macro argument).
I prefer using a macro argument, but in this case (or a similar section
for ACPI), I think we could place the :text into common macros. If
someone needs to update the :text part in the future, it would be
better to introduce a macro argument when it becomes necessary as every
architecture uses :text for these sections.

>  I could
> also see a remote need for the ALIGN()'s value to be arch-controlled.
> (Why is it uniformly 8 anyway on Arm?)
I think it was done just to cover Arm32 and Arm64 alignment
requirements. Probably, POINTER_ALIGN hadn't been introduced before
this section was declared.
But it could align value could be make as macro argument.

> 
> PPC's desire to use DECL_SECTION() can certainly be covered by
> providing
> a (trivial) DECL_SECTION() also for Arm and RISC-V. Seeing that even
> x86
> overrides the default to the trivial form for building xen.efi, I'm
> inclined to suggest we should actually have a way for an arch to
> indicate
> to xen.lds.h that it wants just the trivial form (avoiding a later
> #undef).
In the first version I wanted to introduce the following:
   #ifndef USE_TRIVIAL_DECL_SECTION
   
   #ifdef CONFIG_LD_IS_GNU
   # define DECL_SECTION(x) x : AT(ADDR(#x) - __XEN_VIRT_START)
   #else
   # define DECL_SECTION(x) x : AT(ADDR(x) - __XEN_VIRT_START)
   #endif
   
   #else /* USE_TRIVIAL_DECL_SECTION
   # define DECL_SECTION(x) x :
   #endif
But I decided that it would be too much just to make ACPI_DEV_INFO and
DT_DEV_INFO more abstract and that was the reason why I had another
macro argument for DT_DEV_INFO() to abstract the usage of DECL_SECTION:
+#define USE_DECL_SECTION(x) DECL_SECTION(x)
+
+#define NOUSE_DECL_SECTION(x) x :
+
+#define ACPI_DEV_INFO_SEC(secname, DECL_SECTION_MACROS_NAME)\
+    DECL_SECTION_MACROS_NAME(secname) {                     \
+      _asdevice = .;                                        \
+      *(secname)                                            \
+      _aedevice = .;                                        \
+    } :text

> 
> When to be generalized I further wonder whether the ALIGN()s are
> actually
> well placed. I'd have expected
> 
>   .dev.info ALIGN(POINTER_ALIGN) : {
>       _sdevice = .;
>       *(.dev.info)
>       _edevice = .;
>   } :text
But in case of PPC which is using DECL_SECTION then shouldn't
DECL_SECTION macros have an align value as a macro argument?


Considering everything what was said above could we consider the
updated version of the initial abstraction for DT_DEV_INFO section (
and the similar for ACPI dev info )?
   +#define DT_DEV_INFO_SEC(secname)  \
   +    DECL_SECTION(secname) {       \
   +      _sdevice = .;               \
   +      *(secname)                  \
   +      _edevice = .;               \
   +    } :text

Or alignment could be also inside the macros:
 (if Arm is okay with using POINTER_ALIGN instead of 8 ):
   +#define DT_DEV_INFO_SEC(secname)  \
   +    . = ALIGN(POINTER_ALIGN)      \
   +    DECL_SECTION(secname) {       \
   +      _sdevice = .;               \
   +      *(secname)                  \
   +      _edevice = .;               \
   +    } :text
 ( or if Arm isn't okay ):
   +#define DT_DEV_INFO_SEC(secname, align)  \
   +    . = ALIGN(align)              \
   +    DECL_SECTION(secname) {       \
   +      _sdevice = .;               \
   +      *(secname)                  \
   +      _edevice = .;               \
   +    } :text


~ Oleksii