RE: [PATCH 20/37] xen: introduce CONFIG_EFI to stub API for non-EFI architecture

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2022年1月27日 17:00
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; sstabellini@xxxxxxxxxx; julien@xxxxxxx
> 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-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of
> Wei
> >> Chen
> >> Sent: 2022年1月27日 16:45
> >>
> >>> From: Jan Beulich <jbeulich@xxxxxxxx>
> >>> 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