vmm(4): Partially unfuck multicore AMD support
AbandonedPublic
Actions

Authored by cem on Jan 5 2019, 2:51 AM.

Details

Reviewers

araujo
jhb
grehan
anish
avg
rgrimes

Group Reviewers

bhyve

Summary

CPUID emulation presented Intel topology information to the guest, but
disabled AMD topology information and in some cases passed through
garbage. (I.e., 8000_001[de] were passed through to the guest, but
guest threads can migrate between host cores and threads, so the
information presented was not consistent).

Slightly improve this situation by enabling AMD topology feature flag
and presenting at least the CPUID fields used by FreeBSD itself to probe
topology on more modern AMD64 hardware (Family 15h+). Older stuff is
probably less interesting.

Test Plan

Without this, guests on AMD Family 17h with more than a single core seems to
fail to launch APs. I have not yet tested the patch to confirm it fixes that
particular issue and may not be able to for some time (my AMD 17h is my
desktop); at a minimum it fixes the feature parity issue where '-c' flag
topology was not presented accurately to AMD guests.

Supporting documents for reference:

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 21839
Build 21099: arc lint + arc unit

Event Timeline

cem created this revision.Jan 5 2019, 2:51 AM

Herald added a reviewer: rgrimes. · View Herald TranscriptJan 5 2019, 2:51 AM

Harbormaster completed remote builds in B21839: Diff 52569.Jan 5 2019, 2:51 AM

rgrimes retitled this revision from vmm(4): Partially unfuck multicore AMD support to vmm(4): Partially fix multicore AMD support.Jan 5 2019, 5:15 AM

rgrimes added a reviewer: bhyve.

cem retitled this revision from vmm(4): Partially fix multicore AMD support to vmm(4): Partially unfuck multicore AMD support.Jan 5 2019, 5:35 AM

cem removed a reviewer: rgrimes.

Herald added a reviewer: rgrimes. · View Herald TranscriptJan 5 2019, 5:35 AM

cem edited the test plan for this revision. (Show Details)Jan 5 2019, 5:44 AM

cem removed a reviewer: rgrimes.

cem changed the visibility from "Public (No Login Required)" to "Custom Policy".

cem changed the edit policy from "All Users" to "Custom Policy".

cem changed the visibility from "Custom Policy" to "Public (No Login Required)".Jan 16 2019, 1:20 AM

Herald added a reviewer: rgrimes. · View Herald TranscriptJan 16 2019, 1:20 AM

cem abandoned this revision.Jan 16 2019, 1:33 AM

Hi @cem,

Thanks for the patch, it looks right for me, although I don't have any AMD machine right now to make more tests.
I have only two requests and I would like to check with you if you could follow them:

Give some more time for @jhb to also have a chance to take a look on this patch.
When you commit it, please don't use the word "unfuck", just to avoid open room for people to have something to create noise.

Overall, it looks good to me, thanks to work on that, really appreciate it.