Page MenuHomeFreeBSD

x86_emulate_cpuid() should clear upper 32 bits
AcceptedPublic

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

Details

Reviewers
rgrimes
jhb
Group Reviewers
bhyve
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
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pmooney_pfmooney.com requested review of this revision.May 6 2020, 2:46 AM
pmooney_pfmooney.com added reviewers: rgrimes, jhb.
rgrimes edited the summary of this revision. (Show Details)May 6 2020, 4:07 AM
rgrimes accepted this revision as: rgrimes.May 6 2020, 4:12 AM

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.

jhb added a comment.May 6 2020, 3:40 PM

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?

Updated to include feedback from @jhb

This revision now requires review to proceed.May 7 2020, 11:29 PM
rgrimes accepted this revision as: rgrimes.May 8 2020, 10:16 AM
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.

jhb accepted this revision.Jun 4 2020, 4:07 PM