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: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Wei
> Chen
> Sent: 2022年1月27日 16:45
> To: Jan Beulich <jbeulich@xxxxxxxx>
> 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
> 
> Hi Jan,
> 
> > -----Original Message-----
> > From: Jan Beulich <jbeulich@xxxxxxxx>
> > Sent: 2022年1月25日 18:35
> > 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 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

> > Jan