| Message ID | 20220511014639.197825-3-wei.chen@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Device tree based NUMA support for Arm - Part#1 | expand |
On Wed, 11 May 2022, Wei Chen wrote: > x86 is using compiler feature testing to decide EFI build > enable or not. When EFI build is disabled, x86 will use an > efi/stub.c file to replace efi/runtime.c for build objects. > Following this idea, we introduce a stub file for Arm, but > use CONFIG_ARM_EFI to decide EFI build enable or not. > > And the most functions in x86 EFI stub.c can be reused for > other architectures, like Arm. So we move them to common > and keep the x86 specific function in x86/efi/stub.c. > > To avoid the symbol link conflict error when linking common > stub files to x86/efi. We add a regular file check in efi > stub files' link script. Depends on this check we can bypass > the link behaviors for existed stub files in x86/efi. > > As there is no Arm specific EFI stub function for Arm in > current stage, Arm still can use the existed symbol link > method for EFI stub files. > > Signed-off-by: Wei Chen <wei.chen@arm.com> > Tested-by: Jiamei Xie <jiamei.xie@arm.com> The ARM side looks OK to me, I'll let Jan comment on the x86 side. > --- > v2 -> v3: > 1. Add existed file check, if a regular stub files, > the common/stub files' link will be ignored. > 2. Keep stub.c in x86/efi to include common/efi/stub.c > 3. Restore efi_compat_xxx stub functions to x86/efi.c. > Other architectures will not use efi_compat_xxx. > 4. Remove ARM_EFI dependency from ARM_64. > 5. Add comment for adding stub.o to EFIOBJ-y. > 6. Merge patch#2 and patch#3 to one patch. > v1 -> v2: > 1. Drop the copy of stub.c from Arm EFI. > 2. Share common codes of x86 EFI stub for other architectures. > 3. Use CONFIG_ARM_EFI to replace CONFIG_EFI > 4. Remove help text and make CONFIG_ARM_EFI invisible. > 5. Merge one following patch: > xen/arm: introduce a stub file for non-EFI architectures > 6. Use the common stub.c instead of creating new one. > --- > xen/arch/arm/Kconfig | 4 ++++ > xen/arch/arm/Makefile | 2 +- > xen/arch/arm/efi/Makefile | 8 ++++++++ > xen/arch/x86/efi/stub.c | 32 +------------------------------- > xen/common/efi/efi-common.mk | 3 ++- > xen/common/efi/stub.c | 32 ++++++++++++++++++++++++++++++++ > 6 files changed, 48 insertions(+), 33 deletions(-) > create mode 100644 xen/common/efi/stub.c > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig > index ecfa6822e4..8a16d43bd5 100644 > --- a/xen/arch/arm/Kconfig > +++ b/xen/arch/arm/Kconfig > @@ -6,6 +6,7 @@ config ARM_64 > def_bool y > depends on !ARM_32 > select 64BIT > + select ARM_EFI > select HAS_FAST_MULTIPLY > > config ARM > @@ -33,6 +34,9 @@ config ACPI > Advanced Configuration and Power Interface (ACPI) support for Xen is > an alternative to device tree on ARM64. > > +config ARM_EFI > + bool > + > config GICV3 > bool "GICv3 driver" > depends on ARM_64 && !NEW_VGIC > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > index 1d862351d1..bb7a6151c1 100644 > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -1,6 +1,5 @@ > obj-$(CONFIG_ARM_32) += arm32/ > obj-$(CONFIG_ARM_64) += arm64/ > -obj-$(CONFIG_ARM_64) += efi/ > obj-$(CONFIG_ACPI) += acpi/ > obj-$(CONFIG_HAS_PCI) += pci/ > ifneq ($(CONFIG_NO_PLAT),y) > @@ -20,6 +19,7 @@ obj-y += domain.o > obj-y += domain_build.init.o > obj-y += domctl.o > obj-$(CONFIG_EARLY_PRINTK) += early_printk.o > +obj-y += efi/ > obj-y += gic.o > obj-y += gic-v2.o > obj-$(CONFIG_GICV3) += gic-v3.o > diff --git a/xen/arch/arm/efi/Makefile b/xen/arch/arm/efi/Makefile > index 4313c39066..dffe72e589 100644 > --- a/xen/arch/arm/efi/Makefile > +++ b/xen/arch/arm/efi/Makefile > @@ -1,4 +1,12 @@ > include $(srctree)/common/efi/efi-common.mk > > +ifeq ($(CONFIG_ARM_EFI),y) > obj-y += $(EFIOBJ-y) > obj-$(CONFIG_ACPI) += efi-dom0.init.o > +else > +# Add stub.o to EFIOBJ-y to re-use the clean-files in > +# efi-common.mk. Otherwise the link of stub.c in arm/efi > +# will not be cleaned in "make clean". > +EFIOBJ-y += stub.o > +obj-y += stub.o > +endif > diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c > index 9984932626..f2365bc041 100644 > --- a/xen/arch/x86/efi/stub.c > +++ b/xen/arch/x86/efi/stub.c > @@ -1,7 +1,5 @@ > #include <xen/efi.h> > -#include <xen/errno.h> > #include <xen/init.h> > -#include <xen/lib.h> > #include <asm/asm_defns.h> > #include <asm/efibind.h> > #include <asm/page.h> > @@ -10,6 +8,7 @@ > #include <efi/eficon.h> > #include <efi/efidevp.h> > #include <efi/efiapi.h> > +#include "../../../common/efi/stub.c" > > /* > * Here we are in EFI stub. EFI calls are not supported due to lack > @@ -45,11 +44,6 @@ void __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, > unreachable(); > } > > -bool efi_enabled(unsigned int feature) > -{ > - return false; > -} > - > void __init efi_init_memory(void) { } > > bool efi_boot_mem_unused(unsigned long *start, unsigned long *end) > @@ -62,32 +56,8 @@ bool efi_boot_mem_unused(unsigned long *start, unsigned long *end) > > void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e) { } > > -bool efi_rs_using_pgtables(void) > -{ > - return false; > -} > - > -unsigned long efi_get_time(void) > -{ > - BUG(); > - return 0; > -} > - > -void efi_halt_system(void) { } > -void efi_reset_system(bool warm) { } > - > -int efi_get_info(uint32_t idx, union xenpf_efi_info *info) > -{ > - return -ENOSYS; > -} > - > int efi_compat_get_info(uint32_t idx, union compat_pf_efi_info *) > __attribute__((__alias__("efi_get_info"))); > > -int efi_runtime_call(struct xenpf_efi_runtime_call *op) > -{ > - return -ENOSYS; > -} > - > int efi_compat_runtime_call(struct compat_pf_efi_runtime_call *) > __attribute__((__alias__("efi_runtime_call"))); > diff --git a/xen/common/efi/efi-common.mk b/xen/common/efi/efi-common.mk > index 4298ceaee7..3a5b9958b3 100644 > --- a/xen/common/efi/efi-common.mk > +++ b/xen/common/efi/efi-common.mk > @@ -9,7 +9,8 @@ CFLAGS-y += -iquote $(srcdir) > # e.g.: It transforms "dir/foo/bar" into successively > # "dir foo bar", ".. .. ..", "../../.." > $(obj)/%.c: $(srctree)/common/efi/%.c FORCE > - $(Q)ln -nfs $(subst $(space),/,$(patsubst %,..,$(subst /, ,$(obj))))/source/common/efi/$(<F) $@ > + $(Q)test -f $@ || \ > + ln -nfs $(subst $(space),/,$(patsubst %,..,$(subst /, ,$(obj))))/source/common/efi/$(<F) $@ > > clean-files += $(patsubst %.o, %.c, $(EFIOBJ-y:.init.o=.o) $(EFIOBJ-)) > > diff --git a/xen/common/efi/stub.c b/xen/common/efi/stub.c > new file mode 100644 > index 0000000000..15694632c2 > --- /dev/null > +++ b/xen/common/efi/stub.c > @@ -0,0 +1,32 @@ > +#include <xen/efi.h> > +#include <xen/errno.h> > +#include <xen/lib.h> > + > +bool efi_enabled(unsigned int feature) > +{ > + return false; > +} > + > +bool efi_rs_using_pgtables(void) > +{ > + return false; > +} > + > +unsigned long efi_get_time(void) > +{ > + BUG(); > + return 0; > +} > + > +void efi_halt_system(void) { } > +void efi_reset_system(bool warm) { } > + > +int efi_get_info(uint32_t idx, union xenpf_efi_info *info) > +{ > + return -ENOSYS; > +} > + > +int efi_runtime_call(struct xenpf_efi_runtime_call *op) > +{ > + return -ENOSYS; > +} > -- > 2.25.1 >
On 11.05.2022 03:46, Wei Chen wrote: > x86 is using compiler feature testing to decide EFI build > enable or not. When EFI build is disabled, x86 will use an > efi/stub.c file to replace efi/runtime.c for build objects. > Following this idea, we introduce a stub file for Arm, but > use CONFIG_ARM_EFI to decide EFI build enable or not. > > And the most functions in x86 EFI stub.c can be reused for > other architectures, like Arm. So we move them to common > and keep the x86 specific function in x86/efi/stub.c. > > To avoid the symbol link conflict error when linking common > stub files to x86/efi. We add a regular file check in efi > stub files' link script. Depends on this check we can bypass > the link behaviors for existed stub files in x86/efi. > > As there is no Arm specific EFI stub function for Arm in > current stage, Arm still can use the existed symbol link > method for EFI stub files. Wouldn't it be better to mandate that every arch has its stub.c, and in the Arm one all you'd do (for now) is #include the common one? (But see also below.) > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -1,6 +1,5 @@ > obj-$(CONFIG_ARM_32) += arm32/ > obj-$(CONFIG_ARM_64) += arm64/ > -obj-$(CONFIG_ARM_64) += efi/ > obj-$(CONFIG_ACPI) += acpi/ > obj-$(CONFIG_HAS_PCI) += pci/ > ifneq ($(CONFIG_NO_PLAT),y) > @@ -20,6 +19,7 @@ obj-y += domain.o > obj-y += domain_build.init.o > obj-y += domctl.o > obj-$(CONFIG_EARLY_PRINTK) += early_printk.o > +obj-y += efi/ > obj-y += gic.o > obj-y += gic-v2.o > obj-$(CONFIG_GICV3) += gic-v3.o > diff --git a/xen/arch/arm/efi/Makefile b/xen/arch/arm/efi/Makefile > index 4313c39066..dffe72e589 100644 > --- a/xen/arch/arm/efi/Makefile > +++ b/xen/arch/arm/efi/Makefile > @@ -1,4 +1,12 @@ > include $(srctree)/common/efi/efi-common.mk > > +ifeq ($(CONFIG_ARM_EFI),y) > obj-y += $(EFIOBJ-y) > obj-$(CONFIG_ACPI) += efi-dom0.init.o > +else > +# Add stub.o to EFIOBJ-y to re-use the clean-files in > +# efi-common.mk. Otherwise the link of stub.c in arm/efi > +# will not be cleaned in "make clean". > +EFIOBJ-y += stub.o > +obj-y += stub.o > +endif I realize Stefano indicated he's happy with the Arm side, but I still wonder: What use is the stub on Arm32? Even further - once you have a config option (rather than x86'es build-time check plus x86'es dual- purposing of all object files), why do you need a stub in the first place? You ought to be able to deal with things via inline functions and macros, I would think. > --- a/xen/common/efi/efi-common.mk > +++ b/xen/common/efi/efi-common.mk > @@ -9,7 +9,8 @@ CFLAGS-y += -iquote $(srcdir) > # e.g.: It transforms "dir/foo/bar" into successively > # "dir foo bar", ".. .. ..", "../../.." > $(obj)/%.c: $(srctree)/common/efi/%.c FORCE > - $(Q)ln -nfs $(subst $(space),/,$(patsubst %,..,$(subst /, ,$(obj))))/source/common/efi/$(<F) $@ > + $(Q)test -f $@ || \ > + ln -nfs $(subst $(space),/,$(patsubst %,..,$(subst /, ,$(obj))))/source/common/efi/$(<F) $@ Please can you indent the "ln" to match "test", such that it's easily visible (without paying attention to line continuation characters) that these two lines are a single command? Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2022年5月18日 21:05 > To: Wei Chen <Wei.Chen@arm.com> > Cc: nd <nd@arm.com>; Stefano Stabellini <sstabellini@kernel.org>; Julien > Grall <julien@xen.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper > <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>; Wei > Liu <wl@xen.org>; Jiamei Xie <Jiamei.Xie@arm.com>; xen- > devel@lists.xenproject.org > Subject: Re: [PATCH v3 2/9] xen: reuse x86 EFI stub functions for Arm > > On 11.05.2022 03:46, Wei Chen wrote: > > x86 is using compiler feature testing to decide EFI build > > enable or not. When EFI build is disabled, x86 will use an > > efi/stub.c file to replace efi/runtime.c for build objects. > > Following this idea, we introduce a stub file for Arm, but > > use CONFIG_ARM_EFI to decide EFI build enable or not. > > > > And the most functions in x86 EFI stub.c can be reused for > > other architectures, like Arm. So we move them to common > > and keep the x86 specific function in x86/efi/stub.c. > > > > To avoid the symbol link conflict error when linking common > > stub files to x86/efi. We add a regular file check in efi > > stub files' link script. Depends on this check we can bypass > > the link behaviors for existed stub files in x86/efi. > > > > As there is no Arm specific EFI stub function for Arm in > > current stage, Arm still can use the existed symbol link > > method for EFI stub files. > > Wouldn't it be better to mandate that every arch has its stub.c, > and in the Arm one all you'd do (for now) is #include the common > one? (But see also below.) > Personally, I don't like to include a C file into another C file. But I am OK as long as the Arm maintainers agree. @Stefano Stabellini @Bertrand Marquis @Julien Grall > > --- a/xen/arch/arm/Makefile > > +++ b/xen/arch/arm/Makefile > > @@ -1,6 +1,5 @@ > > obj-$(CONFIG_ARM_32) += arm32/ > > obj-$(CONFIG_ARM_64) += arm64/ > > -obj-$(CONFIG_ARM_64) += efi/ > > obj-$(CONFIG_ACPI) += acpi/ > > obj-$(CONFIG_HAS_PCI) += pci/ > > ifneq ($(CONFIG_NO_PLAT),y) > > @@ -20,6 +19,7 @@ obj-y += domain.o > > obj-y += domain_build.init.o > > obj-y += domctl.o > > obj-$(CONFIG_EARLY_PRINTK) += early_printk.o > > +obj-y += efi/ > > obj-y += gic.o > > obj-y += gic-v2.o > > obj-$(CONFIG_GICV3) += gic-v3.o > > diff --git a/xen/arch/arm/efi/Makefile b/xen/arch/arm/efi/Makefile > > index 4313c39066..dffe72e589 100644 > > --- a/xen/arch/arm/efi/Makefile > > +++ b/xen/arch/arm/efi/Makefile > > @@ -1,4 +1,12 @@ > > include $(srctree)/common/efi/efi-common.mk > > > > +ifeq ($(CONFIG_ARM_EFI),y) > > obj-y += $(EFIOBJ-y) > > obj-$(CONFIG_ACPI) += efi-dom0.init.o > > +else > > +# Add stub.o to EFIOBJ-y to re-use the clean-files in > > +# efi-common.mk. Otherwise the link of stub.c in arm/efi > > +# will not be cleaned in "make clean". > > +EFIOBJ-y += stub.o > > +obj-y += stub.o > > +endif > > I realize Stefano indicated he's happy with the Arm side, but I still > wonder: What use is the stub on Arm32? Even further - once you have a > config option (rather than x86'es build-time check plus x86'es dual- > purposing of all object files), why do you need a stub in the first > place? You ought to be able to deal with things via inline functions > and macros, I would think. > We will use efi_enabled() on some common codes of Arm. In the last version, I had used static inline function, but that will need an CONFIG_EFI in xen/efi.h to gate the definitions of EFI functions, otherwise we just can implement the efi_enabled in non-static-inline way. Or use another name to wrapper efi_enabled. (patch#20, 21) But as x86 has its own way to decide EFI build or not, the CONFIG_EFI has been rejected. In this case, we use CONFIG_ARM_EFI for Arm itself. For CONFIG_ARM_EFI, it's impossible to be used in xen/efi.h to gate definitions. So if I want to use macros or static-inline functions, I need to use #ifdef CONFIG_ARM_EFI in everywhere to gate xen/efi.h. Or use another header file to warpper xen/efi.h. > > --- a/xen/common/efi/efi-common.mk > > +++ b/xen/common/efi/efi-common.mk > > @@ -9,7 +9,8 @@ CFLAGS-y += -iquote $(srcdir) > > # e.g.: It transforms "dir/foo/bar" into successively > > # "dir foo bar", ".. .. ..", "../../.." > > $(obj)/%.c: $(srctree)/common/efi/%.c FORCE > > - $(Q)ln -nfs $(subst $(space),/,$(patsubst %,..,$(subst > /, ,$(obj))))/source/common/efi/$(<F) $@ > > + $(Q)test -f $@ || \ > > + ln -nfs $(subst $(space),/,$(patsubst %,..,$(subst > /, ,$(obj))))/source/common/efi/$(<F) $@ > > Please can you indent the "ln" to match "test", such that it's easily > visible (without paying attention to line continuation characters) > that these two lines are a single command? > Yeah, of course, I will do it. > Jan
On 19.05.2022 04:36, Wei Chen wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 2022年5月18日 21:05 >> >> On 11.05.2022 03:46, Wei Chen wrote: >>> x86 is using compiler feature testing to decide EFI build >>> enable or not. When EFI build is disabled, x86 will use an >>> efi/stub.c file to replace efi/runtime.c for build objects. >>> Following this idea, we introduce a stub file for Arm, but >>> use CONFIG_ARM_EFI to decide EFI build enable or not. >>> >>> And the most functions in x86 EFI stub.c can be reused for >>> other architectures, like Arm. So we move them to common >>> and keep the x86 specific function in x86/efi/stub.c. >>> >>> To avoid the symbol link conflict error when linking common >>> stub files to x86/efi. We add a regular file check in efi >>> stub files' link script. Depends on this check we can bypass >>> the link behaviors for existed stub files in x86/efi. >>> >>> As there is no Arm specific EFI stub function for Arm in >>> current stage, Arm still can use the existed symbol link >>> method for EFI stub files. >> >> Wouldn't it be better to mandate that every arch has its stub.c, >> and in the Arm one all you'd do (for now) is #include the common >> one? (But see also below.) >> > > Personally, I don't like to include a C file into another C file. > But I am OK as long as the Arm maintainers agree. > @Stefano Stabellini @Bertrand Marquis @Julien Grall Well - an alternative is to follow the boot.c model: Have a per-arch stub.h which the common stub.c includes. >>> --- a/xen/arch/arm/Makefile >>> +++ b/xen/arch/arm/Makefile >>> @@ -1,6 +1,5 @@ >>> obj-$(CONFIG_ARM_32) += arm32/ >>> obj-$(CONFIG_ARM_64) += arm64/ >>> -obj-$(CONFIG_ARM_64) += efi/ >>> obj-$(CONFIG_ACPI) += acpi/ >>> obj-$(CONFIG_HAS_PCI) += pci/ >>> ifneq ($(CONFIG_NO_PLAT),y) >>> @@ -20,6 +19,7 @@ obj-y += domain.o >>> obj-y += domain_build.init.o >>> obj-y += domctl.o >>> obj-$(CONFIG_EARLY_PRINTK) += early_printk.o >>> +obj-y += efi/ >>> obj-y += gic.o >>> obj-y += gic-v2.o >>> obj-$(CONFIG_GICV3) += gic-v3.o >>> diff --git a/xen/arch/arm/efi/Makefile b/xen/arch/arm/efi/Makefile >>> index 4313c39066..dffe72e589 100644 >>> --- a/xen/arch/arm/efi/Makefile >>> +++ b/xen/arch/arm/efi/Makefile >>> @@ -1,4 +1,12 @@ >>> include $(srctree)/common/efi/efi-common.mk >>> >>> +ifeq ($(CONFIG_ARM_EFI),y) >>> obj-y += $(EFIOBJ-y) >>> obj-$(CONFIG_ACPI) += efi-dom0.init.o >>> +else >>> +# Add stub.o to EFIOBJ-y to re-use the clean-files in >>> +# efi-common.mk. Otherwise the link of stub.c in arm/efi >>> +# will not be cleaned in "make clean". >>> +EFIOBJ-y += stub.o >>> +obj-y += stub.o >>> +endif >> >> I realize Stefano indicated he's happy with the Arm side, but I still >> wonder: What use is the stub on Arm32? Even further - once you have a >> config option (rather than x86'es build-time check plus x86'es dual- >> purposing of all object files), why do you need a stub in the first >> place? You ought to be able to deal with things via inline functions >> and macros, I would think. >> > > We will use efi_enabled() on some common codes of Arm. In the last > version, I had used static inline function, but that will need an > CONFIG_EFI in xen/efi.h to gate the definitions of EFI functions, > otherwise we just can implement the efi_enabled in non-static-inline > way. Or use another name to wrapper efi_enabled. (patch#20, 21) > But as x86 has its own way to decide EFI build or not, the CONFIG_EFI > has been rejected. In this case, we use CONFIG_ARM_EFI for Arm itself. > > For CONFIG_ARM_EFI, it's impossible to be used in xen/efi.h to gate > definitions. So if I want to use macros or static-inline functions, > I need to use #ifdef CONFIG_ARM_EFI in everywhere to gate xen/efi.h. > Or use another header file to warpper xen/efi.h. Actually I wouldn't mind CONFIG_EFI - eventually we may want to make this a use-visible option even on x86. Sooner or later (I hope) we're going to bump the tool chain base line requirements, at which point the build time check on x86 can at least partly go away. Plus there we support EFI in two different ways, and CONFIG_EFI would hence have a meaning even when the build time check fails. The one thing that I wouldn't view as reasonable is a prompt-less EFI option, which x86 would (not entirely correctly) select unconditionally. Jan
Hi, > On 19 May 2022, at 03:36, Wei Chen <Wei.Chen@arm.com> wrote: > > Hi Jan, > >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 2022年5月18日 21:05 >> To: Wei Chen <Wei.Chen@arm.com> >> Cc: nd <nd@arm.com>; Stefano Stabellini <sstabellini@kernel.org>; Julien >> Grall <julien@xen.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; >> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper >> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>; Wei >> Liu <wl@xen.org>; Jiamei Xie <Jiamei.Xie@arm.com>; xen- >> devel@lists.xenproject.org >> Subject: Re: [PATCH v3 2/9] xen: reuse x86 EFI stub functions for Arm >> >> On 11.05.2022 03:46, Wei Chen wrote: >>> x86 is using compiler feature testing to decide EFI build >>> enable or not. When EFI build is disabled, x86 will use an >>> efi/stub.c file to replace efi/runtime.c for build objects. >>> Following this idea, we introduce a stub file for Arm, but >>> use CONFIG_ARM_EFI to decide EFI build enable or not. >>> >>> And the most functions in x86 EFI stub.c can be reused for >>> other architectures, like Arm. So we move them to common >>> and keep the x86 specific function in x86/efi/stub.c. >>> >>> To avoid the symbol link conflict error when linking common >>> stub files to x86/efi. We add a regular file check in efi >>> stub files' link script. Depends on this check we can bypass >>> the link behaviors for existed stub files in x86/efi. >>> >>> As there is no Arm specific EFI stub function for Arm in >>> current stage, Arm still can use the existed symbol link >>> method for EFI stub files. >> >> Wouldn't it be better to mandate that every arch has its stub.c, >> and in the Arm one all you'd do (for now) is #include the common >> one? (But see also below.) >> > > Personally, I don't like to include a C file into another C file. > But I am OK as long as the Arm maintainers agree. > @Stefano Stabellini @Bertrand Marquis @Julien Grall I agree with Wei here and if we are realistic the current way the EFI code works needs a redesign anyway and asking him to change this in this serie is not right. So I am OK with Wei solution. Cheers Bertrand
Hi Wei, On 19/05/2022 03:36, Wei Chen wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 2022年5月18日 21:05 >> To: Wei Chen <Wei.Chen@arm.com> >> Cc: nd <nd@arm.com>; Stefano Stabellini <sstabellini@kernel.org>; Julien >> Grall <julien@xen.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; >> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper >> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>; Wei >> Liu <wl@xen.org>; Jiamei Xie <Jiamei.Xie@arm.com>; xen- >> devel@lists.xenproject.org >> Subject: Re: [PATCH v3 2/9] xen: reuse x86 EFI stub functions for Arm >> >> On 11.05.2022 03:46, Wei Chen wrote: >>> x86 is using compiler feature testing to decide EFI build >>> enable or not. When EFI build is disabled, x86 will use an >>> efi/stub.c file to replace efi/runtime.c for build objects. >>> Following this idea, we introduce a stub file for Arm, but >>> use CONFIG_ARM_EFI to decide EFI build enable or not. >>> >>> And the most functions in x86 EFI stub.c can be reused for >>> other architectures, like Arm. So we move them to common >>> and keep the x86 specific function in x86/efi/stub.c. >>> >>> To avoid the symbol link conflict error when linking common >>> stub files to x86/efi. We add a regular file check in efi >>> stub files' link script. Depends on this check we can bypass >>> the link behaviors for existed stub files in x86/efi. >>> >>> As there is no Arm specific EFI stub function for Arm in >>> current stage, Arm still can use the existed symbol link >>> method for EFI stub files. >> >> Wouldn't it be better to mandate that every arch has its stub.c, >> and in the Arm one all you'd do (for now) is #include the common >> one? (But see also below.) >> > > Personally, I don't like to include a C file into another C file. Same here. I know we already use it in EFI but I am not in favor to add more of them. Cheers,
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index ecfa6822e4..8a16d43bd5 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -6,6 +6,7 @@ config ARM_64 def_bool y depends on !ARM_32 select 64BIT + select ARM_EFI select HAS_FAST_MULTIPLY config ARM @@ -33,6 +34,9 @@ config ACPI Advanced Configuration and Power Interface (ACPI) support for Xen is an alternative to device tree on ARM64. +config ARM_EFI + bool + config GICV3 bool "GICv3 driver" depends on ARM_64 && !NEW_VGIC diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 1d862351d1..bb7a6151c1 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -1,6 +1,5 @@ obj-$(CONFIG_ARM_32) += arm32/ obj-$(CONFIG_ARM_64) += arm64/ -obj-$(CONFIG_ARM_64) += efi/ obj-$(CONFIG_ACPI) += acpi/ obj-$(CONFIG_HAS_PCI) += pci/ ifneq ($(CONFIG_NO_PLAT),y) @@ -20,6 +19,7 @@ obj-y += domain.o obj-y += domain_build.init.o obj-y += domctl.o obj-$(CONFIG_EARLY_PRINTK) += early_printk.o +obj-y += efi/ obj-y += gic.o obj-y += gic-v2.o obj-$(CONFIG_GICV3) += gic-v3.o diff --git a/xen/arch/arm/efi/Makefile b/xen/arch/arm/efi/Makefile index 4313c39066..dffe72e589 100644 --- a/xen/arch/arm/efi/Makefile +++ b/xen/arch/arm/efi/Makefile @@ -1,4 +1,12 @@ include $(srctree)/common/efi/efi-common.mk +ifeq ($(CONFIG_ARM_EFI),y) obj-y += $(EFIOBJ-y) obj-$(CONFIG_ACPI) += efi-dom0.init.o +else +# Add stub.o to EFIOBJ-y to re-use the clean-files in +# efi-common.mk. Otherwise the link of stub.c in arm/efi +# will not be cleaned in "make clean". +EFIOBJ-y += stub.o +obj-y += stub.o +endif diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c index 9984932626..f2365bc041 100644 --- a/xen/arch/x86/efi/stub.c +++ b/xen/arch/x86/efi/stub.c @@ -1,7 +1,5 @@ #include <xen/efi.h> -#include <xen/errno.h> #include <xen/init.h> -#include <xen/lib.h> #include <asm/asm_defns.h> #include <asm/efibind.h> #include <asm/page.h> @@ -10,6 +8,7 @@ #include <efi/eficon.h> #include <efi/efidevp.h> #include <efi/efiapi.h> +#include "../../../common/efi/stub.c" /* * Here we are in EFI stub. EFI calls are not supported due to lack @@ -45,11 +44,6 @@ void __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, unreachable(); } -bool efi_enabled(unsigned int feature) -{ - return false; -} - void __init efi_init_memory(void) { } bool efi_boot_mem_unused(unsigned long *start, unsigned long *end) @@ -62,32 +56,8 @@ bool efi_boot_mem_unused(unsigned long *start, unsigned long *end) void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e) { } -bool efi_rs_using_pgtables(void) -{ - return false; -} - -unsigned long efi_get_time(void) -{ - BUG(); - return 0; -} - -void efi_halt_system(void) { } -void efi_reset_system(bool warm) { } - -int efi_get_info(uint32_t idx, union xenpf_efi_info *info) -{ - return -ENOSYS; -} - int efi_compat_get_info(uint32_t idx, union compat_pf_efi_info *) __attribute__((__alias__("efi_get_info"))); -int efi_runtime_call(struct xenpf_efi_runtime_call *op) -{ - return -ENOSYS; -} - int efi_compat_runtime_call(struct compat_pf_efi_runtime_call *) __attribute__((__alias__("efi_runtime_call"))); diff --git a/xen/common/efi/efi-common.mk b/xen/common/efi/efi-common.mk index 4298ceaee7..3a5b9958b3 100644 --- a/xen/common/efi/efi-common.mk +++ b/xen/common/efi/efi-common.mk @@ -9,7 +9,8 @@ CFLAGS-y += -iquote $(srcdir) # e.g.: It transforms "dir/foo/bar" into successively # "dir foo bar", ".. .. ..", "../../.." $(obj)/%.c: $(srctree)/common/efi/%.c FORCE - $(Q)ln -nfs $(subst $(space),/,$(patsubst %,..,$(subst /, ,$(obj))))/source/common/efi/$(<F) $@ + $(Q)test -f $@ || \ + ln -nfs $(subst $(space),/,$(patsubst %,..,$(subst /, ,$(obj))))/source/common/efi/$(<F) $@ clean-files += $(patsubst %.o, %.c, $(EFIOBJ-y:.init.o=.o) $(EFIOBJ-)) diff --git a/xen/common/efi/stub.c b/xen/common/efi/stub.c new file mode 100644 index 0000000000..15694632c2 --- /dev/null +++ b/xen/common/efi/stub.c @@ -0,0 +1,32 @@ +#include <xen/efi.h> +#include <xen/errno.h> +#include <xen/lib.h> + +bool efi_enabled(unsigned int feature) +{ + return false; +} + +bool efi_rs_using_pgtables(void) +{ + return false; +} + +unsigned long efi_get_time(void) +{ + BUG(); + return 0; +} + +void efi_halt_system(void) { } +void efi_reset_system(bool warm) { } + +int efi_get_info(uint32_t idx, union xenpf_efi_info *info) +{ + return -ENOSYS; +} + +int efi_runtime_call(struct xenpf_efi_runtime_call *op) +{ + return -ENOSYS; +}