On 2019/05/16 13:20, Jason Thorpe wrote:
>
>
>> On May 15, 2019, at 9:19 PM, Maxime Villard <m...@m00nbsd.net> 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 (msai...@execsw.org
msai...@netbsd.org)
