Re: CVS commit: src/sys/arch/x86/x86

[Masanobu SAITOH <msaitoh%execsw.org@localhost>]

On 2019/05/16 13:20, Jason Thorpe wrote:
> 
> 
>> On May 15, 2019, at 9:19 PM, Maxime Villard <max%m00nbsd.net@localhost> wrote:
>>
>> Le 16/05/2019 à 04:36, SAITOH Masanobu a écrit :
>>> Module Name:	src
>>> Committed By:	msaitoh
>>> Date:		Thu May 16 02:36:30 UTC 2019
>>> Modified Files:
>>> 	src/sys/arch/x86/x86: procfs_machdep.c
>>> Log Message:
>>>  Use ci_feat_val[7] instead of directly getting cpuid 7 edx.
>>> To generate a diff of this commit:
>>> cvs rdiff -u -r1.28 -r1.29 src/sys/arch/x86/x86/procfs_machdep.c
>>> Please note that diffs are not public domain; they are subject to the
>>> copyright notices on the relevant files.
>>
>> The microcode updates CPUID7, if you read ci_feat_val you have the
>> initial value, not the updated value. So reading CPUID7 was the right
>> thing to do.
> 
> Should the microcode update procedure fix up the cached copy?

I think so, but our code doesn't do it.

 There are some code which copy a cpuid value to ci_feat_val[] and
modify some bits for workaround and use the copy. ci_feat_val[]
is better than trusting current cpuid values for consistency.

We should provide user an interface to know the current behavior
of the kernel. I think ci_feat_val[] is the data to explain the
behavior. If so, we should use ci_feat_val[] in x86/procfs_machdep.c.
But, sadly, ci_feat_val[] is not updated, so it might better to
use real cpuid value than ci_feat_val[]. The latest x86/procfs_machdep.c
(rev. 1.31) refers ci_feat_val[0..6] and not refer ci_feat_val[7].
It's inconsistent.

 If we update ci_feat_val, we should have callback function to
reflect the change. Callback is not required for many bits but
it's required for some others.

 I have a code locally to know which bit is changed after updating
microcode:

--------------
Index: x86/cpu_ucode_intel.c
===================================================================
RCS file: /cvsroot/src/sys/arch/x86/x86/cpu_ucode_intel.c,v
retrieving revision 1.17
diff -u -p -r1.17 cpu_ucode_intel.c
--- x86/cpu_ucode_intel.c	10 May 2019 18:21:01 -0000	1.17
+++ x86/cpu_ucode_intel.c	16 May 2019 04:28:13 -0000
@@ -178,9 +178,14 @@ cpu_ucode_intel_apply(struct cpu_ucode_s
 {
 	uint32_t ucodetarget, oucodeversion, nucodeversion;
 	struct intel1_ucode_header *uh;
+	struct cpu_info *ci;
+	struct cpu_info oldci;
+	int i;
 	int platformid, cpuid, error;
 	size_t newbufsize = 0;
 	void *uha;
+	uint64_t msr = 0;
+	u_int descs[4];
 
 	if (sc->loader_version != CPU_UCODE_LOADER_INTEL1 ||
 	    cpuno != CPU_UCODE_CURRENT_CPU)
@@ -221,15 +226,62 @@ cpu_ucode_intel_apply(struct cpu_ucode_s
 	intel_getcurrentucode(&nucodeversion, &platformid);
 	cpuid = curcpu()->ci_index;
 
-	kpreempt_enable();
-
 	if (nucodeversion != ucodetarget) {
+		kpreempt_enable();
 		error = EIO;
 		goto out;
 	}
 
-	printf("cpu %d: ucode 0x%x->0x%x\n", cpuid, oucodeversion,
-	    nucodeversion);
+	ci = curcpu();
+	oldci = *ci;
+#if 1
+	/*
+	 * Update cpu_info.
+	 *
+	 * XXX cpu_probe() is currently not intended to call multiple time
+	 * on a cpu.
+	 */
+	cpu_probe(curcpu());
+#endif
+	if (ci->ci_max_cpuid >= 7) {
+		x86_cpuid(7, descs);
+		if (descs[3] & CPUID_SEF_ARCH_CAP)
+			msr = rdmsr(MSR_IA32_ARCH_CAPABILITIES);
+	}
+	kpreempt_enable();
+
+	if ((ci->ci_max_cpuid >= 7) && (descs[3] & CPUID_SEF_ARCH_CAP))
+		printf("cpu%d: MSR_IA32_ARCH_CAPABILITIES=0x%lx\n", cpuid,
+		    msr);
+	printf("cpu%d: ucode 0x%x->0x%x\n", cpuid,
+	       oucodeversion, nucodeversion);
+	for (i = 0; i < __arraycount(ci->ci_feat_val); i++) {
+		uint32_t old, new, dif, set, unset;
+
+		old = oldci.ci_feat_val[i];
+		new = ci->ci_feat_val[i];
+		if (old != new) {
+			printf("cpu%d: ci_feat_val[%d] changed from %08x to "
+			    "%08x\n", cpuid, i, old, new);
+			dif = old ^ new;
+			set = new & dif;
+			unset = old & dif;
+
+			if (set != 0)
+				printf("cpu%d:   set: %08x\n", cpuid, set);
+			if (unset != 0)
+				printf("cpu%d: unset: %08x\n", cpuid, unset);
+
+			/*
+			 * Call hook if you want to do something.
+			 *
+			 * WARNING. If HyperThreading is enabled and the
+			 * microcode is updated to new one, another CPU's
+			 * feature bits also be changed and we cannot hook
+			 * for the CPU here.
+			 */
+		}
+	}
 
 out:
 	if (newbufsize != 0)
--------------
(The same diff is at http://www.netbsd.org/~msaitoh/ucode-20190516-0.dif)

This code has some problems:

	- It uses cpu_probe(). I think the function currently not intended to be
          called multiple times (though it might be safe now).
	- This is only for Intel.
	- Currently, cpu_ucode_intel_apply() is intended not
	  to run in parallel on all CPUs. It makes harder to call
	  callback function. For AMD, it does by xc_broadcast().

I hope someone solve all of those problems...

> -- thorpej
> 


-- 
-----------------------------------------------
                SAITOH Masanobu (msaitoh%execsw.org@localhost
                                 msaitoh%netbsd.org@localhost)