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月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.
> 
> Jan