RE: [PATCH v3 2/9] xen: reuse x86 EFI stub functions for Arm

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2022年5月18日 21:05
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: nd <nd@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Julien
> Grall <julien@xxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; Wei
> Liu <wl@xxxxxxx>; Jiamei Xie <Jiamei.Xie@xxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> 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