On Mon, Sep 08, 2014 at 02:37:52PM -0300, Henrique de Moraes Holschuh wrote:
> The protocol to safely read MSR 8BH, described in the Intel SDM vol 3A,
> section 9.11.7.1, explicitly determines that cpuid with EAX=1 must be
> used between the wrmsr(0x8B, 0); and the rdmsr(0x8B).
>
> The microcode driver was abusing sync_core() to do this, probably
> because it predates by nearly a decade the current "asm volatile
> (:::"memory")" implementation of native_cpuid(), which is required for
> the Intel MSR 8BH access protocol.
Huh, what? Have you taken a look at sync_core() first?
> sync_core() semanthics are that of being a speculative execution
> barrier, and not "run cpuid with EAX=1".
Again, what?
Hmm, let's see:
static inline void sync_core(void)
{
...
asm volatile("cpuid"
: "=a" (tmp)
: "0" (1)
: "ebx", "ecx", "edx", "memory");
What is the problem again?
I'm sorry but I don't understand what you're trying to fix here...
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Fri, 07 Nov 2014, Borislav Petkov wrote:
> On Mon, Sep 08, 2014 at 02:37:52PM -0300, Henrique de Moraes Holschuh wrote:
> > The protocol to safely read MSR 8BH, described in the Intel SDM vol 3A,
> > section 9.11.7.1, explicitly determines that cpuid with EAX=1 must be
> > used between the wrmsr(0x8B, 0); and the rdmsr(0x8B).
> >
> > The microcode driver was abusing sync_core() to do this, probably
> > because it predates by nearly a decade the current "asm volatile
> > (:::"memory")" implementation of native_cpuid(), which is required for
> > the Intel MSR 8BH access protocol.
>
> Huh, what? Have you taken a look at sync_core() first?
Yes, I did.
> > sync_core() semanthics are that of being a speculative execution
> > barrier, and not "run cpuid with EAX=1".
>
> Again, what?
sync_core() is a speculative execution barrier. That's what it is
documented to do. That's the reason _every_ caller other than the microcode
drivers call it.
In i486, sync_core() does a jmp.
In i586 and above, and x86-64, sync_core() does a cpuid(1).
sync_core() doesn't expect that its callers really want a cpuid(1). If we
ever get a reason to use some other way to insert a speculative execution
barrier, sync_core() is likely to switch to it.
> What is the problem again?
No real problem, other than the fact that the microcode drivers call
sync_core() for what might as well be considered an internal implementation
detail of sync_core().
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Fri, Nov 07, 2014 at 04:40:00PM -0200, Henrique de Moraes Holschuh wrote:
> No real problem, other than the fact that the microcode drivers call
> sync_core() for what might as well be considered an internal implementation
> detail of sync_core().
I think the comment there is enough.
But if you really want to be pedantic and make code more clear for
people who stare at it, this is what you should do:
---
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index c6826d1e8082..59b98a4417ed 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -159,7 +159,7 @@ static int apply_microcode_intel(int cpu)
wrmsr(MSR_IA32_UCODE_REV, 0, 0);
/* As documented in the SDM: Do a CPUID 1 here */
- sync_core();
+ cpuid_eax(1);
/* get the current revision from MSR 0x8B */
rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index b88343f7a3b3..6cb747113714 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -667,7 +667,7 @@ static int apply_microcode_early(struct mc_saved_data *mc_saved_data,
native_wrmsr(MSR_IA32_UCODE_REV, 0, 0);
/* As documented in the SDM: Do a CPUID 1 here */
- sync_core();
+ cpuid_eax(1);
/* get the current revision from MSR 0x8B */
native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
--
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--