Re: [V3 PATCH 1/2] KVM: selftests: x86: Cache the cpu vendor type
From: Sean Christopherson
Date: Thu Dec 22 2022 - 19:24:56 EST
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@xxxxxxxxxx>
> ---
> .../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
--