Re: [RFC 01/10] x86/microcode/intel: expose collect_cpu_info_early() for IFS

From: Borislav Petkov <bp@alien8.de>
To: Jithu Joseph <jithu.joseph@intel.com>
Cc: hdegoede@redhat.com, markgross@kernel.org, tglx@linutronix.de,
	mingo@redhat.com, dave.hansen@linux.intel.com, x86@kernel.org,
	hpa@zytor.com, corbet@lwn.net, gregkh@linuxfoundation.org,
	andriy.shevchenko@linux.intel.com, ashok.raj@intel.com,
	tony.luck@intel.com, rostedt@goodmis.org,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	platform-driver-x86@vger.kernel.org, patches@lists.linux.dev,
	ravi.v.shankar@intel.com
Subject: Re: [RFC 01/10] x86/microcode/intel: expose collect_cpu_info_early() for IFS
Date: Wed, 2 Mar 2022 11:30:12 +0100	[thread overview]
Message-ID: <Yh9HKhESkcUIYzSr@nazgul.tnic> (raw)
In-Reply-To: <20220301195457.21152-2-jithu.joseph@intel.com>

On Tue, Mar 01, 2022 at 11:54:48AM -0800, Jithu Joseph wrote:
> @@ -58,6 +58,7 @@ static inline bool cpu_signatures_match(unsigned int s1, unsigned int p1,
>  	/* ... or they intersect. */
>  	return p1 & p2;
>  }
> +EXPORT_SYMBOL_GPL(cpu_signatures_match);
>  
>  /*
>   * Returns 1 if update has been found, 0 otherwise.
> @@ -342,7 +343,7 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save)
>  	return patch;
>  }
>  
> -static int collect_cpu_info_early(struct ucode_cpu_info *uci)
> +int collect_cpu_info_early(struct ucode_cpu_info *uci)
>  {
>  	unsigned int val[2];
>  	unsigned int family, model;
> @@ -372,6 +373,7 @@ static int collect_cpu_info_early(struct ucode_cpu_info *uci)
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(collect_cpu_info_early);

This is not how this is done - simply exporting a couple of random
functions just because you need them:

drivers/platform/x86/intel/ifs/load.c: In function ‘find_ifs_matching_signature’:
drivers/platform/x86/intel/ifs/load.c:213:6: error: invalid use of void expression
  213 |  if (!cpu_signatures_match(uci->cpu_sig.sig, uci->cpu_sig.pf, shdr->sig, shdr->pf)) {
      |      ^
make[5]: *** [scripts/Makefile.build:288: drivers/platform/x86/intel/ifs/load.o] Error 1
make[4]: *** [scripts/Makefile.build:550: drivers/platform/x86/intel/ifs] Error 2
make[3]: *** [scripts/Makefile.build:550: drivers/platform/x86/intel] Error 2
make[2]: *** [scripts/Makefile.build:550: drivers/platform/x86] Error 2
make[1]: *** [scripts/Makefile.build:550: drivers/platform] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1831: drivers] Error 2

Hell, even your function signatures don't match.

And then, with that fixed it would build but then one would wonder a
long time why that ifs thing doesn't work. Well:

# CONFIG_MICROCODE is not set

So the proper thing to do is to extract the generic functionality for
comparing microcode patch signatures and for gathering the information
collect_cpu_info_early() collects, into intel-generic functions, like
I've done with intel_get_microcode_revision(), move that functionality
to arch/x86/kernel/cpu/intel.c and then use it *both* in the microcode
loader *and* your driver.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette