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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.05.2022 04:36, Wei Chen wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> 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