| Message ID | 3049dd691f79c688751abaae63c0ccdc36680fbb.1726579819.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 |
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@gmail.com> > --- > 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
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@gmail.com> > > --- > > 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.
On 19.09.2024 23:05, Shawn Anastasio wrote: > On 9/17/24 11:15 AM, Oleksii Kurochko wrote: >> --- 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. +1 Jan
On 17.09.2024 18:15, Oleksii Kurochko wrote: > --- 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 */ > + > #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 */ Any specific reason for the _SEC suffixes in the names? We don't have such elsewhere, as can be seen (by example) ... > #ifdef CONFIG_HYPFS > #define HYPFS_PARAM \ > . = ALIGN(POINTER_ALIGN); \ ... even in context here. Jan
On Mon, 2024-09-23 at 12:08 +0200, Jan Beulich wrote: > On 17.09.2024 18:15, Oleksii Kurochko wrote: > > --- 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 */ > > + > > #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 */ > > Any specific reason for the _SEC suffixes in the names? We don't have > such elsewhere, as can be seen (by example) ... > > > #ifdef CONFIG_HYPFS > > #define HYPFS_PARAM \ > > . = ALIGN(POINTER_ALIGN); \ > > ... even in context here. The _SEC suffixes can be removed; they were only used to highlight that it was a section declaration. I'll drop it in the next patch version. ~ Oleksii
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 */ + #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 */ + #ifdef CONFIG_HYPFS #define HYPFS_PARAM \ . = ALIGN(POINTER_ALIGN); \
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@gmail.com> --- Changes in v2: - New patch. --- xen/include/xen/xen.lds.h | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)