Hi Jan, > -----Original Message----- > From: Jan Beulich <[email protected]> > Sent: 2022年5月18日 21:05 > To: Wei Chen <[email protected]> > Cc: nd <[email protected]>; Stefano Stabellini <[email protected]>; Julien > Grall <[email protected]>; Bertrand Marquis <[email protected]>; > Volodymyr Babchuk <[email protected]>; Andrew Cooper > <[email protected]>; Roger Pau Monné <[email protected]>; Wei > Liu <[email protected]>; Jiamei Xie <[email protected]>; xen- > [email protected] > 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
