Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeul...@suse.com> > Sent: 2022年1月27日 17:00 > To: Wei Chen <wei.c...@arm.com> > Cc: Bertrand Marquis <bertrand.marq...@arm.com>; xen- > de...@lists.xenproject.org; sstabell...@kernel.org; jul...@xen.org > Subject: Re: [PATCH 20/37] xen: introduce CONFIG_EFI to stub API for non- > EFI architecture > > On 27.01.2022 09:51, Wei Chen wrote: > >> From: Xen-devel <xen-devel-boun...@lists.xenproject.org> On Behalf Of > Wei > >> Chen > >> Sent: 2022年1月27日 16:45 > >> > >>> From: Jan Beulich <jbeul...@suse.com> > >>> Sent: 2022年1月25日 18:35 > >>> > >>> On 23.09.2021 14:02, Wei Chen wrote: > >>>> --- a/xen/common/Kconfig > >>>> +++ b/xen/common/Kconfig > >>>> @@ -11,6 +11,16 @@ config COMPAT > >>>> config CORE_PARKING > >>>> bool > >>>> > >>>> +config EFI > >>>> + bool > >>>> + ---help--- > >>>> + This option provides support for runtime services provided > >>>> + by UEFI firmware (such as non-volatile variables, realtime > >>>> + clock, and platform reset). A UEFI stub is also provided to > >>>> + allow the kernel to be booted as an EFI application. This > >>>> + is only useful for kernels that may run on systems that have > >>>> + UEFI firmware. > >>> > >>> The way enabling of (full) EFI support works on x86, I consider it > >>> wrong / misleading to put the option in common code. At the very least > >>> the help text would need to call out the extra dependencies. Plus the > >>> help text of course then needs to be generic (i.e. applicable to both > >>> Arm and x86). That's notwithstanding the fact that without a prompt > >>> the help text won't ever be seen while configuring Xen. > >>> > >>> Also (nit): Indentation. And please don't use ---help--- anymore in > >>> new code. > >>> > >> > >> I have used CONFIG_ARM_EFI to replace this common EFI config in my > >> latest version. This Kconfig option has been removed. > >> And thanks, I will not use --help-- anymore. > >> > >>>> --- a/xen/include/xen/efi.h > >>>> +++ b/xen/include/xen/efi.h > >>>> @@ -25,6 +25,8 @@ extern struct efi efi; > >>>> > >>>> #ifndef __ASSEMBLY__ > >>>> > >>>> +#ifdef CONFIG_EFI > >>>> + > >>>> union xenpf_efi_info; > >>>> union compat_pf_efi_info; > >>>> > >>>> @@ -45,6 +47,8 @@ int efi_runtime_call(struct xenpf_efi_runtime_call > >> *); > >>>> int efi_compat_get_info(uint32_t idx, union compat_pf_efi_info *); > >>>> int efi_compat_runtime_call(struct compat_pf_efi_runtime_call *); > >>>> > >>>> +#endif /* CONFIG_EFI*/ > >>> > >>> I can see that in the later patch, when introducing inline stubs, > >>> you would need conditionals here, but I don't think you need them > >>> right here (or you may want to introduce the stubs right away). > >>> > >>> Also (nit): Missing blank in the comment. > > > > I am sorry, I had missed this comment. In my latest changes, > > I have introduced a stub file for non-EFI architectures. > > The reason why we don't use a macro to stub the helpers > > in efi.h is that, some architectures have implemented stub > > helpers in their stub.c. If we define stub helpers in > > efi.h, this will cause function redefinition error. We need > > to fix this error for all architectures. And some helpers > > is not easy to implement as a inline function in efi.h. > > So we use stub file instead of stubing in efi.h > > But you realize we already have such a stub file on x86? >
Yes, we found the redefinition errors that are caused by x86 stub file and new macros in stub.h. We had tries to add: ifeq ($(XEN_BUILD_EFI),y) CFLAGS-y += -DXEN_BUILD_EFI XEN_CFLAGS += -DXEN_BUILD_EFI endif x86/Makefile to gate these new macros, but it seems that we may need to change EFI build logic for x86. It will cause more risks for me. So I want to introduce a similar stub.c in arch/arm. > Jan
