[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 1/5] xen: define ACPI and DT device info sections macros
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |