| Message ID | 413dfb16280c3ec541df8775d31902a4b12a64fe.1727365854.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 26.09.2024 18:54, Oleksii Kurochko wrote: > Introduce SIMPLE_DECL_SECTION to cover the case when > an architecture wants to declare a section without specifying > of load address for the section. > > Update x86/xen.lds.S to use SIMPLE_DECL_SECTION. > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> Acked-by: Jan Beulich <jbeulich@suse.com>
On Thu, Sep 26, 2024 at 06:54:20PM +0200, Oleksii Kurochko wrote: > Introduce SIMPLE_DECL_SECTION to cover the case when > an architecture wants to declare a section without specifying > of load address for the section. > > Update x86/xen.lds.S to use SIMPLE_DECL_SECTION. No strong opinion, but I feel SIMPLE is not very descriptive. It might be better to do it the other way around: introduce a define for when the DECL_SECTION macro should specify a load address: DECL_SECTION_WITH_LADDR for example. > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > Changes in V4: > - new patch > --- > xen/arch/x86/xen.lds.S | 6 ++++-- > xen/include/xen/xen.lds.h | 6 ++++++ > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > index b60d2f0d82..9275a566e1 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 A nit, but we have been trying to add some indentation to make the ifdef blocks easier to read, so this would become: #ifdef EFI # define SIMPLE_DECL_SECTION #endif If it's not too much fuzz to adjust here and below. Thanks, Roger.
On Fri, 2024-09-27 at 09:58 +0200, Roger Pau Monné wrote: > On Thu, Sep 26, 2024 at 06:54:20PM +0200, Oleksii Kurochko wrote: > > Introduce SIMPLE_DECL_SECTION to cover the case when > > an architecture wants to declare a section without specifying > > of load address for the section. > > > > Update x86/xen.lds.S to use SIMPLE_DECL_SECTION. > > No strong opinion, but I feel SIMPLE is not very descriptive. It > might be better to do it the other way around: introduce a define for > when the DECL_SECTION macro should specify a load address: > DECL_SECTION_WITH_LADDR for example. In the next patch, two sections are introduced: dt_dev_info and acpi_dev_info. The definition of these sections has been made common and moved to xen.lds.h, and it looks like this: +#define DT_DEV_INFO(secname) \ + . = ALIGN(POINTER_ALIGN); \ + DECL_SECTION(secname) { \ + _sdevice = .; \ + *(secname) \ + _edevice = .; \ + } :text (A similar approach is used for ACPI, please refer to the next patch in this series.) For PPC, DECL_SECTION should specify a load address, whereas for Arm and RISC-V, it should not. With this generalization, the name of DECL_SECTION should have the same name in both cases, whether a load address needs to be specified or not ~ Oleksii
On Fri, Sep 27, 2024 at 11:07:58AM +0200, oleksii.kurochko@gmail.com wrote: > On Fri, 2024-09-27 at 09:58 +0200, Roger Pau Monné wrote: > > On Thu, Sep 26, 2024 at 06:54:20PM +0200, Oleksii Kurochko wrote: > > > Introduce SIMPLE_DECL_SECTION to cover the case when > > > an architecture wants to declare a section without specifying > > > of load address for the section. > > > > > > Update x86/xen.lds.S to use SIMPLE_DECL_SECTION. > > > > No strong opinion, but I feel SIMPLE is not very descriptive. It > > might be better to do it the other way around: introduce a define for > > when the DECL_SECTION macro should specify a load address: > > DECL_SECTION_WITH_LADDR for example. > In the next patch, two sections are introduced: dt_dev_info and > acpi_dev_info. The definition of these sections has been made common > and moved to xen.lds.h, and it looks like this: > +#define DT_DEV_INFO(secname) \ > + . = ALIGN(POINTER_ALIGN); \ > + DECL_SECTION(secname) { \ > + _sdevice = .; \ > + *(secname) \ > + _edevice = .; \ > + } :text > (A similar approach is used for ACPI, please refer to the next patch in > this series.) > > For PPC, DECL_SECTION should specify a load address, whereas for Arm > and RISC-V, it should not. > > With this generalization, the name of DECL_SECTION should have the same > name in both cases, whether a load address needs to be specified or not Oh, sorry, I think you misunderstood my suggestion. I'm not suggesting to introduce a new macro named DECL_SECTION_WITH_LADDR(), but rather to use DECL_SECTION_WITH_LADDR instead of SIMPLE_DECL_SECTION in order to signal whether DECL_SECTION() should specify a load address or not, iow: #ifdef DECL_SECTION_WITH_LADDR # define DECL_SECTION(x) x : AT(ADDR(x) - __XEN_VIRT_START) #else # define DECL_SECTION(x) x : #endif Thanks, Roger.
On Fri, 2024-09-27 at 11:41 +0200, Roger Pau Monné wrote: > On Fri, Sep 27, 2024 at 11:07:58AM +0200, > oleksii.kurochko@gmail.com wrote: > > On Fri, 2024-09-27 at 09:58 +0200, Roger Pau Monné wrote: > > > On Thu, Sep 26, 2024 at 06:54:20PM +0200, Oleksii Kurochko wrote: > > > > Introduce SIMPLE_DECL_SECTION to cover the case when > > > > an architecture wants to declare a section without specifying > > > > of load address for the section. > > > > > > > > Update x86/xen.lds.S to use SIMPLE_DECL_SECTION. > > > > > > No strong opinion, but I feel SIMPLE is not very descriptive. It > > > might be better to do it the other way around: introduce a define > > > for > > > when the DECL_SECTION macro should specify a load address: > > > DECL_SECTION_WITH_LADDR for example. > > In the next patch, two sections are introduced: dt_dev_info and > > acpi_dev_info. The definition of these sections has been made > > common > > and moved to xen.lds.h, and it looks like this: > > +#define DT_DEV_INFO(secname) \ > > + . = ALIGN(POINTER_ALIGN); \ > > + DECL_SECTION(secname) { \ > > + _sdevice = .; \ > > + *(secname) \ > > + _edevice = .; \ > > + } :text > > (A similar approach is used for ACPI, please refer to the next > > patch in > > this series.) > > > > For PPC, DECL_SECTION should specify a load address, whereas for > > Arm > > and RISC-V, it should not. > > > > With this generalization, the name of DECL_SECTION should have the > > same > > name in both cases, whether a load address needs to be specified or > > not > > Oh, sorry, I think you misunderstood my suggestion. > > I'm not suggesting to introduce a new macro named > DECL_SECTION_WITH_LADDR(), but rather to use DECL_SECTION_WITH_LADDR > instead of SIMPLE_DECL_SECTION in order to signal whether > DECL_SECTION() should specify a load address or not, iow: > > #ifdef DECL_SECTION_WITH_LADDR > # define DECL_SECTION(x) x : AT(ADDR(x) - __XEN_VIRT_START) > #else > # define DECL_SECTION(x) x : > #endif Thanks for the clarification, I really misunderstood your initial suggestion. I'm okay with the renaming; perhaps it will indeed make things a bit clearer. If Jan doesn’t mind (since he gave the Ack), I'll rename the define in the next patch version. Jan, do you mind if I proceed with the renaming? ~ Oleksii
On 27.09.2024 12:42, oleksii.kurochko@gmail.com wrote: > On Fri, 2024-09-27 at 11:41 +0200, Roger Pau Monné wrote: >> On Fri, Sep 27, 2024 at 11:07:58AM +0200, >> oleksii.kurochko@gmail.com wrote: >>> On Fri, 2024-09-27 at 09:58 +0200, Roger Pau Monné wrote: >>>> On Thu, Sep 26, 2024 at 06:54:20PM +0200, Oleksii Kurochko wrote: >>>>> Introduce SIMPLE_DECL_SECTION to cover the case when >>>>> an architecture wants to declare a section without specifying >>>>> of load address for the section. >>>>> >>>>> Update x86/xen.lds.S to use SIMPLE_DECL_SECTION. >>>> >>>> No strong opinion, but I feel SIMPLE is not very descriptive. It >>>> might be better to do it the other way around: introduce a define >>>> for >>>> when the DECL_SECTION macro should specify a load address: >>>> DECL_SECTION_WITH_LADDR for example. >>> In the next patch, two sections are introduced: dt_dev_info and >>> acpi_dev_info. The definition of these sections has been made >>> common >>> and moved to xen.lds.h, and it looks like this: >>> +#define DT_DEV_INFO(secname) \ >>> + . = ALIGN(POINTER_ALIGN); \ >>> + DECL_SECTION(secname) { \ >>> + _sdevice = .; \ >>> + *(secname) \ >>> + _edevice = .; \ >>> + } :text >>> (A similar approach is used for ACPI, please refer to the next >>> patch in >>> this series.) >>> >>> For PPC, DECL_SECTION should specify a load address, whereas for >>> Arm >>> and RISC-V, it should not. >>> >>> With this generalization, the name of DECL_SECTION should have the >>> same >>> name in both cases, whether a load address needs to be specified or >>> not >> >> Oh, sorry, I think you misunderstood my suggestion. >> >> I'm not suggesting to introduce a new macro named >> DECL_SECTION_WITH_LADDR(), but rather to use DECL_SECTION_WITH_LADDR >> instead of SIMPLE_DECL_SECTION in order to signal whether >> DECL_SECTION() should specify a load address or not, iow: >> >> #ifdef DECL_SECTION_WITH_LADDR >> # define DECL_SECTION(x) x : AT(ADDR(x) - __XEN_VIRT_START) >> #else >> # define DECL_SECTION(x) x : >> #endif > Thanks for the clarification, I really misunderstood your initial > suggestion. > > I'm okay with the renaming; perhaps it will indeed make things a bit > clearer. > > If Jan doesn’t mind (since he gave the Ack), I'll rename the define in > the next patch version. > Jan, do you mind if I proceed with the renaming? I'm not overly fussed, so fee free to go ahead and retain my ack. Jan
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index b60d2f0d82..9275a566e1 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 24b8900ffe..8135732756 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. */ +#ifndef 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_SECTION */ +#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,
Introduce SIMPLE_DECL_SECTION to cover the case when an architecture wants to declare a section without specifying of load address for the section. Update x86/xen.lds.S to use SIMPLE_DECL_SECTION. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V4: - new patch --- xen/arch/x86/xen.lds.S | 6 ++++-- xen/include/xen/xen.lds.h | 6 ++++++ 2 files changed, 10 insertions(+), 2 deletions(-)