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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 10/04/2024 4:14 pm, Roger Pau Monné wrote:
> On Thu, Oct 26, 2023 at 09:55:39PM +0100, 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>
>> ---
>> 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
>> ---
>>  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"
> Since we are not targeting at the CPU only, would this be better named
> as Kconfig.vendor?  Or Kconfig.platform?  (I'm OK if you prefer to
> leave as .cpu, just suggesting more neutral names.

Well - its based on ...

>
>> +
>>  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..3c5d88fdfd16
>> --- /dev/null
>> +++ b/xen/arch/x86/Kconfig.cpu
>> @@ -0,0 +1,22 @@
>> +menu "Supported CPU vendors"
>> +    visible if EXPERT

... this, and I think "cpu" is the thing that is going to be most
meaningful to people.  (Also because this is what Linux calls the file.)

Technically platform would be the right term, but "CPU vendors" is what
you call Intel / AMD / etc in the x86 world.

>> +
>> +config AMD
>> +    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
>> +    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.
> There seems to be a weird mix between hard tabs and spaces above.
> Naming is OK for me.

Yeah.  I already fixed those locally.

>
>> +
>> +endmenu
>> diff --git a/xen/arch/x86/cpu/microcode/Makefile 
>> b/xen/arch/x86/cpu/microcode/Makefile
>> index aae235245b06..30d600544f45 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) += amd.o
>>  obj-y += core.o
>> -obj-y += intel.o
>> +obj-$(CONFIG_INTEL) += intel.o
>> diff --git a/xen/arch/x86/cpu/microcode/private.h 
>> b/xen/arch/x86/cpu/microcode/private.h
>> index b58611e908aa..da556fe5060a 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
>>  void ucode_probe_amd(struct microcode_ops *ops);
>> +#else
>> +static inline void ucode_probe_amd(struct microcode_ops *ops) {}
>> +#endif
>> +
>> +#ifdef CONFIG_INTEL
>>  void ucode_probe_intel(struct microcode_ops *ops);
>> +#else
>> +static inline void ucode_probe_intel(struct microcode_ops *ops) {}
> This is stale now, and will need some updating to match what's in
> private.h.

There's nothing state I can see.

Patch 1 does significantly edit this vs what's currently in staging.

~Andrew