On Thu, Dec 22, 2022, Vishal Annapurve wrote:
Query cpuid once per guest selftest and store the cpu vendor type so that subsequent queries can reuse the cached cpu vendor type.
Signed-off-by: Vishal Annapurve vannapurve@google.com
.../selftests/kvm/lib/x86_64/processor.c | 33 ++++++++++++++++--- 1 file changed, 28 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c index 097cef430774..1e93bb3cb058 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c @@ -20,6 +20,9 @@ vm_vaddr_t exception_handlers; +static bool is_cpu_vendor_intel; +static bool is_cpu_vendor_amd;
static void regs_dump(FILE *stream, struct kvm_regs *regs, uint8_t indent) { fprintf(stream, "%*srax: 0x%.16llx rbx: 0x%.16llx " @@ -1017,18 +1020,36 @@ void kvm_x86_state_cleanup(struct kvm_x86_state *state) free(state); } -static bool cpu_vendor_string_is(const char *vendor) +static void check_cpu_vendor(void)
I don't love the name, "check" makes me think the purpose of the helper is to assert something. Maybe cache_cpu_vendor()? Though this might be a moot point (more at the end).
{
- const uint32_t *chunk = (const uint32_t *)vendor;
- const char *intel_id = "GenuineIntel";
- const uint32_t *intel_id_chunks = (const uint32_t *)intel_id;
- const char *amd_id = "AuthenticAMD";
- const uint32_t *amd_id_chunks = (const uint32_t *)amd_id;
- static bool cpu_vendor_checked; uint32_t eax, ebx, ecx, edx;
- if (cpu_vendor_checked)
return;- cpuid(0, &eax, &ebx, &ecx, &edx);
- return (ebx == chunk[0] && edx == chunk[1] && ecx == chunk[2]);
- if (ebx == intel_id_chunks[0] && edx == intel_id_chunks[1] &&
ecx == intel_id_chunks[2])
Align indentation, should be:
if (ebx == intel_id_chunks[0] && edx == intel_id_chunks[1] && ecx == intel_id_chunks[2])
is_cpu_vendor_intel = true;- else {
if (ebx == amd_id_chunks[0] && edx == amd_id_chunks[1] &&ecx == amd_id_chunks[2])
Same here.
is_cpu_vendor_amd = true;- }
Though that's likely a moot point since manually checking the chunks (multiple times!) is rather heinous. Just store the CPUID output into an array and then use strncmp() to check the vendor. Performance isn't a priority for this code.
static void cache_cpu_vendor(void) { uint32_t ign, vendor[3]; static bool cached;
if (cached) return;
cpuid(0, &ign, &vendor[0], &vendor[2], &vendor[1]);
is_cpu_vendor_intel = !strncmp((char *)vendor, "GenuineIntel", 12); is_cpu_vendor_amd = !strncmp((char *)vendor, "AuthenticAMD", 12);
cached = true; }
That said, I like the v2 approach a lot more, we just need to better name the host variables to make it very clear that the info being cached is the _host_ vendor, and then provide separate helpers that bypass caching (though I don't think there would be any users?), again with better names.
The confidential VM use case, and really kvm_hypercall() in general, cares about the host vendor, i.e. which hypercall instruction won't fault. Actually, even fix_hypercall_test is in the same boat.
I don't like auto-caching the guest info because unlike the host (assuming no shenanigans in a nested setup), the guest vendor string can be changed. I don't think it's likely that we'll have a test that wants to muck with the vendor string on the fly, but it's not impossible, e.g. fix_hypercall_test dances pretty close to that.
The independent guest vs. host caching in this version is also very subtle, though that can be solved with comments.
E.g. first make is_intel_cpu() and is_amd_cpu() wrappers to non-caching helpers that use "this_cpu_..." naming to match other selftests code. Then rename is_intel_cpu() and is_amd_cpu() to is_{intel,amd}_host(), with a blurb in the changelog calling out that fix_hypercall_test wants the host vendor and also passes through the host CPUID vendor. And then do the precomputation stuff like in v2.
E.g. for step #1
--- .../selftests/kvm/include/x86_64/processor.h | 22 +++++++++++++++++++ .../selftests/kvm/lib/x86_64/processor.c | 13 ++--------- 2 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h index b1a31de7108a..34523a876336 100644 --- a/tools/testing/selftests/kvm/include/x86_64/processor.h +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h @@ -554,6 +554,28 @@ static inline uint32_t this_cpu_model(void) return x86_model(this_cpu_fms()); }
+static inline bool this_cpu_vendor_string_is(const char *vendor) +{ + const uint32_t *chunk = (const uint32_t *)vendor; + uint32_t eax, ebx, ecx, edx; + + cpuid(0, &eax, &ebx, &ecx, &edx); + return (ebx == chunk[0] && edx == chunk[1] && ecx == chunk[2]); +} + +static inline bool this_cpu_is_intel(void) +{ + return this_cpu_vendor_string_is("GenuineIntel"); +} + +/* + * Exclude early K5 samples with a vendor string of "AMDisbetter!" + */ +static inline bool this_cpu_is_amd(void) +{ + return this_cpu_vendor_string_is("AuthenticAMD"); +} + static inline uint32_t __this_cpu_has(uint32_t function, uint32_t index, uint8_t reg, uint8_t lo, uint8_t hi) { diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c index c4d368d56cfe..fae1a8a81652 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c @@ -1006,18 +1006,9 @@ void kvm_x86_state_cleanup(struct kvm_x86_state *state) free(state); }
-static bool cpu_vendor_string_is(const char *vendor) -{ - const uint32_t *chunk = (const uint32_t *)vendor; - uint32_t eax, ebx, ecx, edx; - - cpuid(0, &eax, &ebx, &ecx, &edx); - return (ebx == chunk[0] && edx == chunk[1] && ecx == chunk[2]); -} - bool is_intel_cpu(void) { - return cpu_vendor_string_is("GenuineIntel"); + return this_cpu_is_intel(); }
/* @@ -1025,7 +1016,7 @@ bool is_intel_cpu(void) */ bool is_amd_cpu(void) { - return cpu_vendor_string_is("AuthenticAMD"); + return this_cpu_is_amd(); }
void kvm_get_cpu_address_width(unsigned int *pa_bits, unsigned int *va_bits)
base-commit: d70e740240a298d0ff54d26f48f2fb034e3cb172