Page MenuHomeFreeBSD

x86_emulate_cpuid() should clear upper 32 bits
ClosedPublic

Authored by pmooney_pfmooney.com on May 6 2020, 2:46 AM.

Details

Summary

Per the Intel manuals, CPUID is supposed to unconditionally zero the upper 32 bits of the involved (rax/rbx/rcx/rdx) registers. Previously, the emulation would cast pointers to the 64-bit register values down to uint32_t, which while properly manipulating the lower bits, would leave any garbage in the upper bits uncleared. While no existing guest OSes seem to stumble over this in practice, the bhyve emulation should match x86 expectations.

This was discovered through alignment warnings emitted by gcc9, while testing it against SmartOS/bhyve.

SmartOS bug: https://smartos.org/bugview/OS-8168

Test Plan

Without this patch, a guest running under bhyve could issue CPUID instructions with garbage in the upper 32-bits of the effected registers (say %rax), and it would survive in the result value. With the patch applied, that garbage is no longer visible after the CPUID.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

I just fixed a couple typos in the summary, looks ok.

This revision is now accepted and ready to land.May 6 2020, 4:12 AM

I do not have a machine available to test on FreeBSD at the moment, but the equivalent patch for SmartOS bhyve does appear to have the desired effect.

Hmm, it's a bit odd to have the variables still called 'eax', etc while being 64-bit. I think I might prefer that we instead fix the callers to use local uint32_t variables that are passed to x86_emulate_cpuid and then assigned back to the destination registers after the function returns.

In D24727#544327, @jhb wrote:

Hmm, it's a bit odd to have the variables still called 'eax', etc while being 64-bit. I think I might prefer that we instead fix the callers to use local uint32_t variables that are passed to x86_emulate_cpuid and then assigned back to the destination registers after the function returns.

I thought about doing it that way, and while the 64->32->64bit logic is small and simple, it would have been replicated for both VMX and SVM. Would you be OK with the argument names updated to their r-prefixed counterparts to show that x86_emulate_cpuid() is acting on the full 64-bit registers?

This revision now requires review to proceed.May 7 2020, 11:29 PM
This revision is now accepted and ready to land.May 8 2020, 10:16 AM

If that seems reasonable, it's now I intend to commit the change to illumos bhyve.

This was integrated as 12746 into illumos bhyve.