Introduce 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@gmail.com>
---
Changes in V2:
- drop SEC* at the end of ACPI AND DT device info
section mancros.
- refactor ADEV_INFO and DT_DEV_INFO macros.
---
xen/include/xen/xen.lds.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
index a17810bb28..b85c5e6576 100644
--- 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 = .;
+
#ifdef CONFIG_HYPFS
#define HYPFS_PARAM \
. = ALIGN(POINTER_ALIGN); \
--
2.46.1On 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?
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 could
also see a remote need for the ALIGN()'s value to be arch-controlled.
(Why is it uniformly 8 anyway on Arm?)
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).
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
Jan
On Wed, 2024-09-25 at 10:36 +0200, Jan Beulich wrote: > 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). If to go with what I suggested before then x86 will look like: diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index d48de67cfd..911585541e 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -3,6 +3,10 @@ #include <xen/cache.h> #include <xen/lib.h> + +#ifdef EFI +#define SIMPLE_DECL_SECTION +#endif #include <xen/xen.lds.h> #include <asm/page.h> #undef ENTRY @@ -12,9 +16,7 @@ #define FORMAT "pei-x86-64" #undef __XEN_VIRT_START -#undef DECL_SECTION #define __XEN_VIRT_START __image_base__ -#define DECL_SECTION(x) x : ENTRY(efi_start) diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h index a17810bb28..fb11ba7357 100644 --- a/xen/include/xen/xen.lds.h +++ b/xen/include/xen/xen.lds.h @@ -5,6 +5,8 @@ * Common macros to be used in architecture specific linker scripts. */ +#ifdef SIMPLE_DECL_SECTION + /* * Declare a section whose load address is based at PA 0 rather than * Xen's virtual base address. @@ -15,6 +17,10 @@ # define DECL_SECTION(x) x : AT(ADDR(x) - __XEN_VIRT_START) #endif +#else /* SIMPLE_DECL_SECION */ +# define DECL_SECTION(x) x : +#endif + /* * To avoid any confusion, please note that the EFI macro does not correspond * to EFI support and is used when linking a native EFI (i.e. PE/COFF) binary, Does it make sense? Or it would be better to follow way for each architecture: #undef DECL_SECTION #define DECL_SECTION(x) x : ~ Oleksii
On 25.09.2024 18:08, oleksii.kurochko@gmail.com wrote: > On Wed, 2024-09-25 at 10:36 +0200, Jan Beulich wrote: >> 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). > If to go with what I suggested before then x86 will look like: > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > index d48de67cfd..911585541e 100644 > --- a/xen/arch/x86/xen.lds.S > +++ b/xen/arch/x86/xen.lds.S > @@ -3,6 +3,10 @@ > > #include <xen/cache.h> > #include <xen/lib.h> > + > +#ifdef EFI > +#define SIMPLE_DECL_SECTION > +#endif > #include <xen/xen.lds.h> > #include <asm/page.h> > #undef ENTRY > @@ -12,9 +16,7 @@ > > #define FORMAT "pei-x86-64" > #undef __XEN_VIRT_START > -#undef DECL_SECTION > #define __XEN_VIRT_START __image_base__ > -#define DECL_SECTION(x) x : > > ENTRY(efi_start) > > diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h > index a17810bb28..fb11ba7357 100644 > --- a/xen/include/xen/xen.lds.h > +++ b/xen/include/xen/xen.lds.h > @@ -5,6 +5,8 @@ > * Common macros to be used in architecture specific linker scripts. > */ > > +#ifdef SIMPLE_DECL_SECTION #ifndef I guess? > @@ -15,6 +17,10 @@ > # define DECL_SECTION(x) x : AT(ADDR(x) - __XEN_VIRT_START) > #endif > > +#else /* SIMPLE_DECL_SECION */ > +# define DECL_SECTION(x) x : > +#endif > + > /* > * To avoid any confusion, please note that the EFI macro does not > correspond > * to EFI support and is used when linking a native EFI (i.e. PE/COFF) > binary, > > Does it make sense? Or it would be better to follow way for each > architecture: > #undef DECL_SECTION > #define DECL_SECTION(x) x : Hard to tell which one's better; I was asking myself that same question when writing an earlier reply. I'm slightly in favor of the form you have now. Jan
On Thu, 2024-09-26 at 08:23 +0200, Jan Beulich wrote: > On 25.09.2024 18:08, oleksii.kurochko@gmail.com wrote: > > On Wed, 2024-09-25 at 10:36 +0200, Jan Beulich wrote: > > > 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). > > If to go with what I suggested before then x86 will look like: > > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > > index d48de67cfd..911585541e 100644 > > --- a/xen/arch/x86/xen.lds.S > > +++ b/xen/arch/x86/xen.lds.S > > @@ -3,6 +3,10 @@ > > > > #include <xen/cache.h> > > #include <xen/lib.h> > > + > > +#ifdef EFI > > +#define SIMPLE_DECL_SECTION > > +#endif > > #include <xen/xen.lds.h> > > #include <asm/page.h> > > #undef ENTRY > > @@ -12,9 +16,7 @@ > > > > #define FORMAT "pei-x86-64" > > #undef __XEN_VIRT_START > > -#undef DECL_SECTION > > #define __XEN_VIRT_START __image_base__ > > -#define DECL_SECTION(x) x : > > > > ENTRY(efi_start) > > > > diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h > > index a17810bb28..fb11ba7357 100644 > > --- a/xen/include/xen/xen.lds.h > > +++ b/xen/include/xen/xen.lds.h > > @@ -5,6 +5,8 @@ > > * Common macros to be used in architecture specific linker > > scripts. > > */ > > > > +#ifdef SIMPLE_DECL_SECTION > > #ifndef I guess? > > > @@ -15,6 +17,10 @@ > > # define DECL_SECTION(x) x : AT(ADDR(x) - __XEN_VIRT_START) > > #endif > > > > +#else /* SIMPLE_DECL_SECION */ > > +# define DECL_SECTION(x) x : > > +#endif > > + > > /* > > * To avoid any confusion, please note that the EFI macro does not > > correspond > > * to EFI support and is used when linking a native EFI (i.e. > > PE/COFF) > > binary, > > > > Does it make sense? Or it would be better to follow way for each > > architecture: > > #undef DECL_SECTION > > #define DECL_SECTION(x) x : > > Hard to tell which one's better; I was asking myself that same > question > when writing an earlier reply. I'm slightly in favor of the form you > have > now. Do you mean moving only a content of section without secname and laddr ( in case of x86 and PPC ), and alignment to xen.lds.h ? ~ Oleksii
On 26.09.2024 11:15, oleksii.kurochko@gmail.com wrote: > On Thu, 2024-09-26 at 08:23 +0200, Jan Beulich wrote: >> On 25.09.2024 18:08, oleksii.kurochko@gmail.com wrote: >>> On Wed, 2024-09-25 at 10:36 +0200, Jan Beulich wrote: >>>> 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). >>> If to go with what I suggested before then x86 will look like: >>> >>> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S >>> index d48de67cfd..911585541e 100644 >>> --- a/xen/arch/x86/xen.lds.S >>> +++ b/xen/arch/x86/xen.lds.S >>> @@ -3,6 +3,10 @@ >>> >>> #include <xen/cache.h> >>> #include <xen/lib.h> >>> + >>> +#ifdef EFI >>> +#define SIMPLE_DECL_SECTION >>> +#endif >>> #include <xen/xen.lds.h> >>> #include <asm/page.h> >>> #undef ENTRY >>> @@ -12,9 +16,7 @@ >>> >>> #define FORMAT "pei-x86-64" >>> #undef __XEN_VIRT_START >>> -#undef DECL_SECTION >>> #define __XEN_VIRT_START __image_base__ >>> -#define DECL_SECTION(x) x : >>> >>> ENTRY(efi_start) >>> >>> diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h >>> index a17810bb28..fb11ba7357 100644 >>> --- a/xen/include/xen/xen.lds.h >>> +++ b/xen/include/xen/xen.lds.h >>> @@ -5,6 +5,8 @@ >>> * Common macros to be used in architecture specific linker >>> scripts. >>> */ >>> >>> +#ifdef SIMPLE_DECL_SECTION >> >> #ifndef I guess? >> >>> @@ -15,6 +17,10 @@ >>> # define DECL_SECTION(x) x : AT(ADDR(x) - __XEN_VIRT_START) >>> #endif >>> >>> +#else /* SIMPLE_DECL_SECION */ >>> +# define DECL_SECTION(x) x : >>> +#endif >>> + >>> /* >>> * To avoid any confusion, please note that the EFI macro does not >>> correspond >>> * to EFI support and is used when linking a native EFI (i.e. >>> PE/COFF) >>> binary, >>> >>> Does it make sense? Or it would be better to follow way for each >>> architecture: >>> #undef DECL_SECTION >>> #define DECL_SECTION(x) x : >> >> Hard to tell which one's better; I was asking myself that same >> question >> when writing an earlier reply. I'm slightly in favor of the form you >> have >> now. > Do you mean moving only a content of section without secname and laddr > ( in case of x86 and PPC ), and alignment to xen.lds.h ? No. I was solely referring to the DECL_SECTION() aspect. Jan
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
On 25.09.2024 13:02, oleksii.kurochko@gmail.com wrote:
> 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
#define DT_DEV_INFO_SEC(secname) \
DECL_SECTION(secname) { \
. = ALIGN(POINTER_ALIGN); \
_sdevice = .; \
*(secname) \
_edevice = .; \
} :text
would be my preferred form then.
Jan
> ( or if Arm isn't okay ):
> +#define DT_DEV_INFO_SEC(secname, align) \
> + . = ALIGN(align) \
> + DECL_SECTION(secname) { \
> + _sdevice = .; \
> + *(secname) \
> + _edevice = .; \
> + } :text
>
>
> ~ Oleksii
© 2016 - 2024 Red Hat, Inc.