Re: [PATCH v2.5 2/2] x86/Kconfig: Introduce CONFIG_{AMD,INTEL} and conditionalise ucode

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 27 Oct 2023, Andrew Cooper wrote:
> We eventually want to be able to build a stripped down Xen for a single
> platform.  Make a start with CONFIG_{AMD,INTEL} (hidden behind EXPERT, but
> available to randconfig), and adjust the microcode logic.
> 
> No practical change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

I read the discussion here:
https://marc.info/?l=xen-devel&m=169865507432363
https://lore.kernel.org/xen-devel/ZT9yNrdoCKZs3_uY@macbook/

I think we want two top-level simple CONFIG_AMD and CONFIG_INTEL
options. Do we also want finer granular sub-options such as
CONFIG_AMD_CPU and CONFIG_INTEL_CPU, which would be controlled by the
top-level options CONFIG_AMD and CONFIG_INTEL? I think we could go
either way. CONFIG_{AMD,INTEL} could be sufficient, but also having
them control CONFIG_{AMD,INTEL}_CPU and CONFIG_{AMD,INTEL}_IOMMU is
fine too.

We already have CONFIG_{AMD,INTEL}_IOMMU. At the time I wondered if we
could have just used CONFIG_{AMD,INTEL} for evertything. But given we
have CONFIG_{AMD,INTEL}_IOMMU, I can see why the reviewers suggested to
use CONFIG_{AMD,INTEL}_CPU.

I would have introduced CONFIG_{AMD,INTEL} only, given that it is not
possible to have an AMD CPU with an INTEL IOMMU and vice versa. 

Anyway, I don't really have a strong opinion either way but we need this
patch to move forward one way or another so:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

(I would also ck different versions of this patch.)


> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> CC: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> CC: Xenia Ragiadakou <xenia.ragiadakou@xxxxxxx>
> 
> I've intentionally ignored the other vendors for now.  They can be put into
> Kconfig by whomever figures out the actual dependencies between their init
> routines.
> 
> v2:
>  * Tweak text
> v2.5:
>  * Fix indentation
>  * Rename with _CPU suffixes as requested
> ---
>  xen/arch/x86/Kconfig                 |  2 ++
>  xen/arch/x86/Kconfig.cpu             | 22 ++++++++++++++++++++++
>  xen/arch/x86/cpu/microcode/Makefile  |  4 ++--
>  xen/arch/x86/cpu/microcode/private.h |  9 +++++++++
>  4 files changed, 35 insertions(+), 2 deletions(-)
>  create mode 100644 xen/arch/x86/Kconfig.cpu
> 
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index eac77573bd75..d9eacdd7e0fa 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -49,6 +49,8 @@ config HAS_CC_CET_IBT
>  
>  menu "Architecture Features"
>  
> +source "arch/x86/Kconfig.cpu"
> +
>  source "arch/Kconfig"
>  
>  config PV
> diff --git a/xen/arch/x86/Kconfig.cpu b/xen/arch/x86/Kconfig.cpu
> new file mode 100644
> index 000000000000..b68c41977a3b
> --- /dev/null
> +++ b/xen/arch/x86/Kconfig.cpu
> @@ -0,0 +1,22 @@
> +menu "Supported CPU vendors"
> +     visible if EXPERT
> +
> +config AMD_CPU
> +     bool "AMD"
> +     default y
> +     help
> +       Detection, tunings and quirks for AMD platforms.
> +
> +       May be turned off in builds targetting other vendors.  Otherwise,
> +       must be enabled for Xen to work suitably on AMD platforms.
> +
> +config INTEL_CPU
> +     bool "Intel"
> +     default y
> +     help
> +       Detection, tunings and quirks for Intel platforms.
> +
> +       May be turned off in builds targetting other vendors.  Otherwise,
> +       must be enabled for Xen to work suitably on Intel platforms.
> +
> +endmenu
> diff --git a/xen/arch/x86/cpu/microcode/Makefile 
> b/xen/arch/x86/cpu/microcode/Makefile
> index aae235245b06..194ddc239ee3 100644
> --- a/xen/arch/x86/cpu/microcode/Makefile
> +++ b/xen/arch/x86/cpu/microcode/Makefile
> @@ -1,3 +1,3 @@
> -obj-y += amd.o
> +obj-$(CONFIG_AMD_CPU) += amd.o
>  obj-y += core.o
> -obj-y += intel.o
> +obj-$(CONFIG_INTEL_CPU) += intel.o
> diff --git a/xen/arch/x86/cpu/microcode/private.h 
> b/xen/arch/x86/cpu/microcode/private.h
> index b58611e908aa..bb329933ac89 100644
> --- a/xen/arch/x86/cpu/microcode/private.h
> +++ b/xen/arch/x86/cpu/microcode/private.h
> @@ -70,7 +70,16 @@ struct microcode_ops {
>   * support available) and (not) ops->apply_microcode (i.e. read only).
>   * Otherwise, all hooks must be filled in.
>   */
> +#ifdef CONFIG_AMD_CPU
>  void ucode_probe_amd(struct microcode_ops *ops);
> +#else
> +static inline void ucode_probe_amd(struct microcode_ops *ops) {}
> +#endif
> +
> +#ifdef CONFIG_INTEL_CPU
>  void ucode_probe_intel(struct microcode_ops *ops);
> +#else
> +static inline void ucode_probe_intel(struct microcode_ops *ops) {}
> +#endif
>  
>  #endif /* ASM_X86_MICROCODE_PRIVATE_H */
> -- 
> 2.30.2
> 
>