| Message ID | 7521839bd265e0520fc448adf50361d18dfe53df.1727193766.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Move {acpi_}device_init() and device_get_class() to common code | expand |
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? 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: > 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
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
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); \
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(+)