Re: [V3 PATCH 1/2] KVM: selftests: x86: Cache the cpu vendor type

[Sean Christopherson <seanjc@google.com>]

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
--