[PATCH v2 0/2] x86: ucode and CPU Kconfig

[PATCH v2 0/2] x86: ucode and CPU Kconfig

[Andrew Cooper]

A fix to the recent Ucode changes which I ultimately didn't insist on owing to
the Xen 4.18 timeline, and enough of the start on the CPU Kconfig to allow
randconfig to check the boundary.

There are many more ucode fixes to come...

Andrew Cooper (2):
  x86/ucode: Move vendor specifics back out of early_microcode_init()
  x86/Kconfig: Introduce CONFIG_{AMD,INTEL} and conditionalise ucode

 xen/arch/x86/Kconfig                 |  2 ++
 xen/arch/x86/Kconfig.cpu             | 22 ++++++++++++++++++++++
 xen/arch/x86/cpu/microcode/Makefile  |  4 ++--
 xen/arch/x86/cpu/microcode/amd.c     | 10 +++++++++-
 xen/arch/x86/cpu/microcode/core.c    | 16 +++++-----------
 xen/arch/x86/cpu/microcode/intel.c   | 12 ++++++++++--
 xen/arch/x86/cpu/microcode/private.h | 23 ++++++++++++++++++-----
 7 files changed, 68 insertions(+), 21 deletions(-)
 create mode 100644 xen/arch/x86/Kconfig.cpu

-- 
2.30.2

[PATCH v2 1/2] x86/ucode: Move vendor specifics back out of early_microcode_init()

[Andrew Cooper]

I know it was me who dropped microcode_init_{intel,amd}() in c/s
dd5f07997f29 ("x86/ucode: Rationalise startup and family/model checks"), but
times have moved on.  We've gained new conditional support, and a wish to
compile-time specialise Xen to single platform.

(Re)introduce ucode_probe_{amd,intel}() and move the recent vendor specific
additions back out.  Encode the conditional support state in the NULL-ness of
hooks as it's already done on other paths.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
CC: Stefano Stabellini <stefano.stabellini@amd.com>
CC: Xenia Ragiadakou <xenia.ragiadakou@amd.com>

v2:
 * Undo unintentinal operand inversion in early_microcode_init()
---
 xen/arch/x86/cpu/microcode/amd.c     | 10 +++++++++-
 xen/arch/x86/cpu/microcode/core.c    | 16 +++++-----------
 xen/arch/x86/cpu/microcode/intel.c   | 12 ++++++++++--
 xen/arch/x86/cpu/microcode/private.h | 16 ++++++++++------
 4 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 75fc84e445ce..17e68697d5bf 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -434,9 +434,17 @@ static struct microcode_patch *cf_check cpu_request_microcode(
     return patch;
 }
 
-const struct microcode_ops __initconst_cf_clobber amd_ucode_ops = {
+static const struct microcode_ops __initconst_cf_clobber amd_ucode_ops = {
     .cpu_request_microcode            = cpu_request_microcode,
     .collect_cpu_info                 = collect_cpu_info,
     .apply_microcode                  = apply_microcode,
     .compare_patch                    = compare_patch,
 };
+
+void __init ucode_probe_amd(struct microcode_ops *ops)
+{
+    if ( boot_cpu_data.x86 < 0x10 )
+        return;
+
+    *ops = amd_ucode_ops;
+}
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 65ebeb50deea..3fd1f516e042 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -847,25 +847,19 @@ int __init early_microcode_init(unsigned long *module_map,
 {
     const struct cpuinfo_x86 *c = &boot_cpu_data;
     int rc = 0;
-    bool can_load = false;
 
     switch ( c->x86_vendor )
     {
     case X86_VENDOR_AMD:
-        if ( c->x86 >= 0x10 )
-        {
-            ucode_ops = amd_ucode_ops;
-            can_load = true;
-        }
+        ucode_probe_amd(&ucode_ops);
         break;
 
     case X86_VENDOR_INTEL:
-        ucode_ops = intel_ucode_ops;
-        can_load = intel_can_load_microcode();
+        ucode_probe_intel(&ucode_ops);
         break;
     }
 
-    if ( !ucode_ops.apply_microcode )
+    if ( !ucode_ops.collect_cpu_info )
     {
         printk(XENLOG_INFO "Microcode loading not available\n");
         return -ENODEV;
@@ -882,10 +876,10 @@ int __init early_microcode_init(unsigned long *module_map,
      *
      * Take the hint in either case and ignore the microcode interface.
      */
-    if ( this_cpu(cpu_sig).rev == ~0 || !can_load )
+    if ( !ucode_ops.apply_microcode || this_cpu(cpu_sig).rev == ~0 )
     {
         printk(XENLOG_INFO "Microcode loading disabled due to: %s\n",
-               can_load ? "rev = ~0" : "HW toggle");
+               ucode_ops.apply_microcode ? "rev = ~0" : "HW toggle");
         ucode_ops.apply_microcode = NULL;
         return -ENODEV;
     }
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 060c529a6e5d..96f34b336b21 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -385,7 +385,7 @@ static struct microcode_patch *cf_check cpu_request_microcode(
     return patch;
 }
 
-bool __init intel_can_load_microcode(void)
+static bool __init can_load_microcode(void)
 {
     uint64_t mcu_ctrl;
 
@@ -398,9 +398,17 @@ bool __init intel_can_load_microcode(void)
     return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);
 }
 
-const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
+static const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
     .cpu_request_microcode            = cpu_request_microcode,
     .collect_cpu_info                 = collect_cpu_info,
     .apply_microcode                  = apply_microcode,
     .compare_patch                    = compare_patch,
 };
+
+void __init ucode_probe_intel(struct microcode_ops *ops)
+{
+    *ops = intel_ucode_ops;
+
+    if ( !can_load_microcode() )
+        ops->apply_microcode = NULL;
+}
diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
index d80787205a5e..b58611e908aa 100644
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -60,13 +60,17 @@ struct microcode_ops {
         const struct microcode_patch *new, const struct microcode_patch *old);
 };
 
-/**
- * Checks whether we can perform microcode updates on this Intel system
+/*
+ * Microcode loading falls into one of 3 states.
+ *   - No support at all
+ *   - Read-only (locked by firmware, or we're virtualised)
+ *   - Loading available
  *
- * @return True iff the microcode update facilities are enabled
+ * These are encoded by (not) filling in ops->collect_cpu_info (i.e. no
+ * support available) and (not) ops->apply_microcode (i.e. read only).
+ * Otherwise, all hooks must be filled in.
  */
-bool intel_can_load_microcode(void);
-
-extern const struct microcode_ops amd_ucode_ops, intel_ucode_ops;
+void ucode_probe_amd(struct microcode_ops *ops);
+void ucode_probe_intel(struct microcode_ops *ops);
 
 #endif /* ASM_X86_MICROCODE_PRIVATE_H */
-- 
2.30.2

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

[Andrew Cooper]

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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
CC: Stefano Stabellini <stefano.stabellini@amd.com>
CC: Xenia Ragiadakou <xenia.ragiadakou@amd.com>

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"
+
 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
+
+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.
+
+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) {}
+#endif
 
 #endif /* ASM_X86_MICROCODE_PRIVATE_H */
-- 
2.30.2

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

[Jan Beulich]

On 26.10.2023 22:55, 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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> CC: Stefano Stabellini <stefano.stabellini@amd.com>
> CC: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> 
> 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

What about the indentation issues mentioned in reply to v1?

As to using un-amended AMD and INTEL - Roger, what's your view here?
I'm not outright opposed, but to me it feels misleading to omit CPU
from those Kconfig symbols.

Jan

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

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

[Andrew Cooper]

On 27/10/2023 2:47 pm, Roger Pau Monné wrote:
> On Fri, Oct 27, 2023 at 09:12:40AM +0200, Jan Beulich wrote:
>> On 26.10.2023 22:55, 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@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Wei Liu <wl@xen.org>
>>> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>>> CC: Stefano Stabellini <stefano.stabellini@amd.com>
>>> CC: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>>
>>> 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
>> What about the indentation issues mentioned in reply to v1?
>>
>> As to using un-amended AMD and INTEL - Roger, what's your view here?
> I think it would be good to add a suffix, like we do for
> {AMD,INTEL}_IOMMU options, and reserve the plain AMD and INTEL options
> as platform/system level options that enable both VENDOR_{CPU,IOMMU}
> sub options.
>
> So yes, {INTEL,AMD}_CPU seems a good option.

Really?  You do realise that, unlike the IOMMU names, this is going to
be plastered all over the Makefiles and header files?

And it breaks the careful attempt not to use the ambigous term when
describing what the symbol means.

I'll send out a v2.5 so you can see it in context, but I'm going to say
straight up - I think this is a mistake.

~Andrew

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

[Jan Beulich]

On 27.10.2023 21:18, Andrew Cooper wrote:
> On 27/10/2023 2:47 pm, Roger Pau Monné wrote:
>> On Fri, Oct 27, 2023 at 09:12:40AM +0200, Jan Beulich wrote:
>>> On 26.10.2023 22:55, 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@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>> CC: Wei Liu <wl@xen.org>
>>>> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>>>> CC: Stefano Stabellini <stefano.stabellini@amd.com>
>>>> CC: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>>>
>>>> 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
>>> What about the indentation issues mentioned in reply to v1?
>>>
>>> As to using un-amended AMD and INTEL - Roger, what's your view here?
>> I think it would be good to add a suffix, like we do for
>> {AMD,INTEL}_IOMMU options, and reserve the plain AMD and INTEL options
>> as platform/system level options that enable both VENDOR_{CPU,IOMMU}
>> sub options.
>>
>> So yes, {INTEL,AMD}_CPU seems a good option.
> 
> Really?  You do realise that, unlike the IOMMU names, this is going to
> be plastered all over the Makefiles and header files?
> 
> And it breaks the careful attempt not to use the ambigous term when
> describing what the symbol means.

I wonder what you mean here: Describing what the symbol means is all
done in plain text, i.e. independent of the symbol name.

> I'll send out a v2.5 so you can see it in context, but I'm going to say
> straight up - I think this is a mistake.

So in the longer run perhaps we want CONFIG_{AMD,INTEL} _and_
CONFIG_{AMD,INTEL}_CPU? The former mainly to control the defaults of
CONFIG_{AMD,INTEL}_{CPU,IOMMU} (could also be viewed as kind of a
shorthand)?

Jan

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

[Roger Pau Monné]

On Fri, Oct 27, 2023 at 08:18:18PM +0100, Andrew Cooper wrote:
> On 27/10/2023 2:47 pm, Roger Pau Monné wrote:
> > On Fri, Oct 27, 2023 at 09:12:40AM +0200, Jan Beulich wrote:
> >> On 26.10.2023 22:55, 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@citrix.com>
> >>> ---
> >>> CC: Jan Beulich <JBeulich@suse.com>
> >>> CC: Roger Pau Monné <roger.pau@citrix.com>
> >>> CC: Wei Liu <wl@xen.org>
> >>> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> >>> CC: Stefano Stabellini <stefano.stabellini@amd.com>
> >>> CC: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> >>>
> >>> 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
> >> What about the indentation issues mentioned in reply to v1?
> >>
> >> As to using un-amended AMD and INTEL - Roger, what's your view here?
> > I think it would be good to add a suffix, like we do for
> > {AMD,INTEL}_IOMMU options, and reserve the plain AMD and INTEL options
> > as platform/system level options that enable both VENDOR_{CPU,IOMMU}
> > sub options.
> >
> > So yes, {INTEL,AMD}_CPU seems a good option.
> 
> Really?  You do realise that, unlike the IOMMU names, this is going to
> be plastered all over the Makefiles and header files?

What's it different from using INTEL_CPU than just plain INTEL?  It's
still going to be plastered all over the Makefiles and header files.

> And it breaks the careful attempt not to use the ambigous term when
> describing what the symbol means.
> 
> I'll send out a v2.5 so you can see it in context, but I'm going to say
> straight up - I think this is a mistake.

My point is that we might want to reserve the top level names (iow:
CONFIG_INTEL, CONFIG_AMD, CONFIG_{VENDOR}) for system wide options
that enable both the CPU and the IOMMU code needed for the selected
vendor.

I'm happy to use a name different than {INTEL,AMD}_CPU for the vendor
CPU/platform code.

Alternatively, I would be fine to use CONFIG_{INTEL,AMD} as long as
the existing CONFIG_{AMD,INTEL}_IOMMU Kconfig options are made
dependent on the newly introduced CONFIG_{INTEL,AMD} options, so when
disabling CONFIG_AMD CONFIG_AMD_IOMMU also gets disabled.

Maybe that's already the intention of the suggested CONFIG_{AMD,INTEL}
and I'm missing the point.

Thanks, Roger.

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

[Andrew Cooper]

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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
CC: Stefano Stabellini <stefano.stabellini@amd.com>
CC: Xenia Ragiadakou <xenia.ragiadakou@amd.com>

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

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4639 bytes --]

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@citrix.com>

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@kernel.org>

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


> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> CC: Stefano Stabellini <stefano.stabellini@amd.com>
> CC: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> 
> 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
> 
> 

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

[Andrew Cooper]

On 09/04/2024 10:42 pm, Stefano Stabellini wrote:
> 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@citrix.com>
> 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@kernel.org>

FWIW, I have a very strong preference for the v2 of this patch (which is
just simply CONFIG_{AMD,INTEL}), rather than this instance of it.

Subdividing it is a mistake IMO, as it draws boundaries that doesn't
exist in reality.

There's nothing CONFIG_{AMD,INTEL} can reasonably be used for which
isn't this purpose.

Having IOMMU separate makes sense at least in principle.  There are
(well - were) some systems without an IOMMU, and you could be targetting
one of those.

However, there's nothing you can reasonably to to pick and choose
between microcode loading vs the other large number of
$foo/{intel,amd}.c splits we have through the codebase.

~Andrew

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

[Roger Pau Monné]

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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> CC: Stefano Stabellini <stefano.stabellini@amd.com>
> CC: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> 
> 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.

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

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

Thanks, Roger.

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

[Andrew Cooper]

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@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>> CC: Stefano Stabellini <stefano.stabellini@amd.com>
>> CC: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>
>> 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

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

[Roger Pau Monné]

On Wed, Apr 10, 2024 at 05:21:37PM +0100, Andrew Cooper wrote:
> On 10/04/2024 4:14 pm, Roger Pau Monné wrote:
> > On Thu, Oct 26, 2023 at 09:55:39PM +0100, Andrew Cooper wrote:
> >> +
> >> +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.

With that fixed:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

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

Oh, sorry, I'm missed patch 1 then.

Thanks, Roger.

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

[Andrew Cooper]

On 10/04/2024 5:34 pm, Roger Pau Monné wrote:
> On Wed, Apr 10, 2024 at 05:21:37PM +0100, Andrew Cooper wrote:
>> On 10/04/2024 4:14 pm, Roger Pau Monné wrote:
>>> On Thu, Oct 26, 2023 at 09:55:39PM +0100, Andrew Cooper wrote:
>>>> +
>>>> +config AMD
>>>> +	bool "AMD"

After double checking what {menu,old}config looks like, I've extended
these prompts "Support $X CPU" so they make more sense in the context
they're asked in.


>>>> +        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.
> With that fixed:
>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

We can always tweak later if necessary.

~Andrew

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]

On Wed, 10 Apr 2024, Roger Pau Monné wrote:
> On Wed, Apr 10, 2024 at 05:21:37PM +0100, Andrew Cooper wrote:
> > On 10/04/2024 4:14 pm, Roger Pau Monné wrote:
> > > On Thu, Oct 26, 2023 at 09:55:39PM +0100, Andrew Cooper wrote:
> > >> +
> > >> +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.
> 
> With that fixed:
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

This is fine for me too

Acked-by: Stefano Stabellini <sstabellini@kernel.org>