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

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

CC Stefano/Xenia as I know you want to go down this line, but I don't recall
patches in this area yet.
---
 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..09575b19d6dc 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 ? "HW toggle" : "rev = ~0");
         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 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.

CC Stefano/Xenia as I know you want to go down this line, but I don't recall
patches to this effect yet.
---
 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..0ce09b292045
--- /dev/null
+++ b/xen/arch/x86/Kconfig.cpu
@@ -0,0 +1,22 @@
+menu "Supported processor vendors"
+	visible if EXPERT
+
+config AMD
+	bool "AMD"
+        default y
+        help
+          Detection, tunings and quirks for AMD processors.
+
+	  May be turned off in builds targetting other vendors.  Otherwise,
+	  must be enabled for Xen to work suitably on AMD processors.
+
+config INTEL
+	bool "Intel"
+        default y
+        help
+          Detection, tunings and quirks for Intel processors.
+
+	  May be turned off in builds targetting other vendors.  Otherwise,
+	  must be enabled for Xen to work suitably on Intel processors.
+
+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 1/2] x86/ucode: Move vendor specifics back out of early_microcode_init()

[Stefano Stabellini]

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

On Wed, 25 Oct 2023, Andrew Cooper wrote:
> 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>
> ---
> 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>
> 
> CC Stefano/Xenia as I know you want to go down this line, but I don't recall
> patches in this area yet.

Yep, anything we can do to specialized the Xen build just to the
components needed (e.g. AMD drivers yes, Intel drivers no) it is an
improvement

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

[Jan Beulich]

On 25.10.2023 20:06, Andrew Cooper wrote:
> --- 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 )

Here ucode_ops.apply_microcode simply replaces can_load, as expected.

>      {
>          printk(XENLOG_INFO "Microcode loading disabled due to: %s\n",
> -               can_load ? "rev = ~0" : "HW toggle");
> +               ucode_ops.apply_microcode ? "HW toggle" : "rev = ~0");

Here, otoh, you invert sense, which I don't think is right. With 2nd
and 3rd operands swapped back
Reviewed-by: Jan Beulich <jbeulich@suse.com>

> @@ -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;
> +}

I was wondering why you didn't use the return value to supply the pointer
to the caller, but this override explains it.

Jan

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

[Jan Beulich]

On 25.10.2023 20:06, 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.

Linux uses different names for the Kconfig symbols. While I'm unconvinced
of the SUP part, I wonder whether we wouldn't better use CPU in the names.
One immediate question here is how the IOMMU interaction is intended to
end up: Do we want to permit either vendor's CPUs with the other vendor's
IOMMUs to be usable?

> --- /dev/null
> +++ b/xen/arch/x86/Kconfig.cpu
> @@ -0,0 +1,22 @@
> +menu "Supported processor vendors"
> +	visible if EXPERT
> +
> +config AMD
> +	bool "AMD"
> +        default y
> +        help
> +          Detection, tunings and quirks for AMD processors.
> +
> +	  May be turned off in builds targetting other vendors.  Otherwise,
> +	  must be enabled for Xen to work suitably on AMD processors.
> +
> +config INTEL
> +	bool "Intel"
> +        default y
> +        help
> +          Detection, tunings and quirks for Intel processors.
> +
> +	  May be turned off in builds targetting other vendors.  Otherwise,
> +	  must be enabled for Xen to work suitably on Intel processors.
> +
> +endmenu

Nit: Throughout this hunk there's an inconsistency with indentation
(hard tabs not used in some places where they ought to be).

Also, depending on the verdict on the aspect mentioned at the top,
"processors" may want replacing by "systems" or "platforms" or some
such if we mean these to cover more than just the CPUs.

Jan

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

[Andrew Cooper]

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

On 26/10/2023 8:55 am, Jan Beulich wrote:
> On 25.10.2023 20:06, 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.
> Linux uses different names for the Kconfig symbols. While I'm unconvinced
> of the SUP part, I wonder whether we wouldn't better use CPU in the names.

I don't see what that gets us, other than a longer name.

> One immediate question here is how the IOMMU interaction is intended to
> end up: Do we want to permit either vendor's CPUs with the other vendor's
> IOMMUs to be usable?

From a randconfig point of view, yes.  These options are only targetting
a specific platform, and we can absolutely make that the end user's
responsibility to describe their platform correctly.


The more interesting question is perhaps VT-x and SVM, given that VIA
have shipped VT-x and Hygon have shipped SVM and AMD-Vi.

I do specifically want to to integrate the HVM setup better with CPU
init - KVM dropped an enormous amount of complexity by doing this - but
I expect we'll end up with VTX and SVM options rather than using
INTEL/AMD for this.

There is a bit of linkage between EPT/VT-d and NPT/AMD-Vi (in principle
at least) in the form of HAP/IOMMU pagetable sharing, but as it's "just"
an exchange of superpage sizes, iommu-pt pointer and height, I think we
can make an abstraction which doesn't force a vendor match.
>> --- /dev/null
>> +++ b/xen/arch/x86/Kconfig.cpu
>> @@ -0,0 +1,22 @@
>> +menu "Supported processor vendors"
>> +	visible if EXPERT
>> +
>> +config AMD
>> +	bool "AMD"
>> +        default y
>> +        help
>> +          Detection, tunings and quirks for AMD processors.
>> +
>> +	  May be turned off in builds targetting other vendors.  Otherwise,
>> +	  must be enabled for Xen to work suitably on AMD processors.
>> +
>> +config INTEL
>> +	bool "Intel"
>> +        default y
>> +        help
>> +          Detection, tunings and quirks for Intel processors.
>> +
>> +	  May be turned off in builds targetting other vendors.  Otherwise,
>> +	  must be enabled for Xen to work suitably on Intel processors.
>> +
>> +endmenu
> Nit: Throughout this hunk there's an inconsistency with indentation
> (hard tabs not used in some places where they ought to be).

Oh yes, that's unintended.

> Also, depending on the verdict on the aspect mentioned at the top,
> "processors" may want replacing by "systems" or "platforms" or some
> such if we mean these to cover more than just the CPUs.

I really don't want to use CPU because that term is overloaded enough
already.  Maybe it's ok in the overall menu text, but "plaform/system
vendor" would be the OEMs rather than the processor vendors.

We do have various platform quirks in Xen, but they're almost all DMI or
PCI based, rather than vendor based.

I could be persuaded to use CPU in the menu, and s/processors/platforms/
elsewhere.

~Andrew

[-- Attachment #2: Type: text/html, Size: 4270 bytes --]

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

[Andrew Cooper]

On 26/10/2023 8:45 am, Jan Beulich wrote:
> On 25.10.2023 20:06, Andrew Cooper wrote:
>> --- 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 )
> Here ucode_ops.apply_microcode simply replaces can_load, as expected.
>
>>      {
>>          printk(XENLOG_INFO "Microcode loading disabled due to: %s\n",
>> -               can_load ? "rev = ~0" : "HW toggle");
>> +               ucode_ops.apply_microcode ? "HW toggle" : "rev = ~0");
> Here, otoh, you invert sense, which I don't think is right. With 2nd
> and 3rd operands swapped back
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Oh, right.  This did get adjusted several times.  I'll fix.

It's a weird construct anyway, and it gets cleaned up differently by
some of my later work.

>
>> @@ -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;
>> +}
> I was wondering why you didn't use the return value to supply the pointer
> to the caller, but this override explains it.

Yes.  The other option was to export (in private.h at least) the root
ucode_ops, which I decided against in the end.

I also toyed with the idea of having the probe functions return int, so
we could get EOPNOTSUPP or so in the compiled-out case, but that's
almost completely redundant with clobbering the hook, and it's added
complexity for somethign that only randconfig is going to care about.

The only thing I'm not entirely happy about is the volume of the
ifdefary for these ucode_probe() functions in the following patch, but
it's the least invasive option overall IMO.

~Andrew

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

[Jan Beulich]

On 26.10.2023 13:10, Andrew Cooper wrote:
> On 26/10/2023 8:55 am, Jan Beulich wrote:
>> On 25.10.2023 20:06, 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.
>> Linux uses different names for the Kconfig symbols. While I'm unconvinced
>> of the SUP part, I wonder whether we wouldn't better use CPU in the names.
> 
> I don't see what that gets us, other than a longer name.

Just to mention the (I think) obvious - on the IOMMU side we already have
AMD_IOMMU and INTEL_IOMMU. It would be odd to have just AMD and INTEL here,
yet then ...

>> One immediate question here is how the IOMMU interaction is intended to
>> end up: Do we want to permit either vendor's CPUs with the other vendor's
>> IOMMUs to be usable?
> 
> From a randconfig point of view, yes.  These options are only targetting
> a specific platform, and we can absolutely make that the end user's
> responsibility to describe their platform correctly.

... <vendor>_IOMMU not depending on <vendor>. Whereas the lack of a
dependency on <vendor>_CPU would be quite natural, imo.

> The more interesting question is perhaps VT-x and SVM, given that VIA
> have shipped VT-x and Hygon have shipped SVM and AMD-Vi.
> 
> I do specifically want to to integrate the HVM setup better with CPU
> init - KVM dropped an enormous amount of complexity by doing this - but
> I expect we'll end up with VTX and SVM options rather than using
> INTEL/AMD for this.

I'd certainly prefer us using VTX/SVM (and those then having dependencies
on the main || niche vendors), with the caveat that SVM also has had a
meaning for Intel for quite some time, iirc.

Jan

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

[Andrew Cooper]

On 26/10/2023 12:35 pm, Jan Beulich wrote:
> On 26.10.2023 13:10, Andrew Cooper wrote:
>> On 26/10/2023 8:55 am, Jan Beulich wrote:
>>> On 25.10.2023 20:06, 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.
>>> Linux uses different names for the Kconfig symbols. While I'm unconvinced
>>> of the SUP part, I wonder whether we wouldn't better use CPU in the names.
>> I don't see what that gets us, other than a longer name.
> Just to mention the (I think) obvious - on the IOMMU side we already have
> AMD_IOMMU and INTEL_IOMMU. It would be odd to have just AMD and INTEL here,
> yet then ...
>
>>> One immediate question here is how the IOMMU interaction is intended to
>>> end up: Do we want to permit either vendor's CPUs with the other vendor's
>>> IOMMUs to be usable?
>> From a randconfig point of view, yes.  These options are only targetting
>> a specific platform, and we can absolutely make that the end user's
>> responsibility to describe their platform correctly.
> ... <vendor>_IOMMU not depending on <vendor>.

Odd possibly, but not something to worry about.

It's mostly because of asymmetric marketing because while VTD is fine
and recognisable, AMD-Vi has AMD's name in it even for the non-AMD vendors.

Anyone liable to even notice in the first place will probably know
enough to understand why it's like that.

Furthermore, ...

>  Whereas the lack of a
> dependency on <vendor>_CPU would be quite natural, imo.

... this doesn't really work either as IOMMUs are non-really-optional
uncore components these days.


Names are just that - names.  They can be changed if needs be, and it's
the help text which matters to clarify the intent.

~Andrew