(FACT-1715) Add data source for CPUID instruction #2
Conversation
|
Changes Unknown when pulling 9e1a2f7 on caseywilliams:cpuid into ** on puppetlabs:master**. |
| */ | ||
| struct cpuid_registers | ||
| { | ||
| /** |
There was a problem hiding this comment.
I'd say these comments are redundant, but I think travis will fail if we don't have 100% docs coverage :/
There was a problem hiding this comment.
That is correct! 😅
There was a problem hiding this comment.
If they have to be here, maybe we could make them a little more descriptive? Something like "Value from eax register", etc. Still redundant, but slightly less general (I'd argue any field could accurately be annotated with " value" :P).
| /** | ||
| * Most hypervisors store vendor information in this leaf | ||
| */ | ||
| constexpr static const unsigned int INFO_LEAF = 0x40000000; |
There was a problem hiding this comment.
Did something require this to be flagged as constexpr? Usually just being a static const should be enough to get the compiler to inline the value, unless it's involved in other constexpr functions
| dmi_source.product_name(), | ||
| re_virtualbox); | ||
| return cpuid_source.vendor() == "VBoxVBoxVBox" || | ||
| re_search(dmi_source.product_name(), re_virtualbox); |
There was a problem hiding this comment.
longer-term we're going to want to be able to track which data sources were successful, but this is good until we have a metadata object to work with 👍
| bool cpuid_base::has_hypervisor() const | ||
| { | ||
| auto regs = read_cpuid(1); | ||
| return static_cast<bool>(regs.ecx & (1 << 31)); |
There was a problem hiding this comment.
can we move this value to a constant named HYPERVISOR_PRESENT or something like that?
lib/src/sources/cpuid_source.cc
Outdated
| string cpuid_base::vendor() const | ||
| { | ||
| auto regs = read_cpuid(INFO_LEAF); | ||
| unsigned int result[13] = {regs.ebx, regs.ecx, regs.edx}; |
There was a problem hiding this comment.
this should be an array of size 4. It needs to be at least 13 /bytes/, not 13 ints
There was a problem hiding this comment.
You should also be able to initialize the trailing zero in the initializer list:
unsigned int result[4] = {regs.ebx, regs.ecx, regs.edx, 0};
lib/src/whereami.cc
Outdated
|
|
||
| return result; | ||
| } | ||
|
|
| */ | ||
| class cpuid_fixture_values : public sources::cpuid_base { | ||
| public: | ||
| cpuid_fixture_values(sources::cpuid_registers values); |
There was a problem hiding this comment.
should this instead take a map of leaf -> registers, so you can override multiple queries?
There was a problem hiding this comment.
Yeah, that will be much nicer
|
Changes Unknown when pulling 2cbfd20 on caseywilliams:cpuid into ** on puppetlabs:master**. |
Magisus
left a comment
Magisus
left a comment
There was a problem hiding this comment.
One suggestion, otherwise looks generally good. Didn't delve super deep into the processor logic, but I like the structure you have here so far. Sidenote: kinda cool to see some of the stuff from my Computer Architecture class showing up in production code :)
A question out of curiosity, why is this method preferred over other methods of virtualization detection? Is it more reliable, or faster, or what?
|
@Magisus Thanks! Calling CPUID (where available) is likely faster than reading DMI files in I'll update the comments on those register strings in a minute here. |
Adds a data source for the CPUID instruction that reports the following: 1. Whether CPUID output indicates that the current machine is a hypervisor guest, and 2. CPUID's reported vendor ID Updates the existing VirtualBox detector to check CPUID before DMI.
|
Changes Unknown when pulling 83870f8 on caseywilliams:cpuid into ** on puppetlabs:master**. |
4 participants
Adds a data source for the CPUID instruction that reports the following:
guest, and
Updates the existing VirtualBox detector to check CPUID before DMI.