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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> --- 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