All of lore.kernel.org
 help / color / mirror / Atom feed
* [V3 PATCH 0/2] Execute hypercalls from guests according to cpu
@ 2022-12-22 23:04 Vishal Annapurve
  2022-12-22 23:04 ` [V3 PATCH 1/2] KVM: selftests: x86: Cache the cpu vendor type Vishal Annapurve
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Vishal Annapurve @ 2022-12-22 23:04 UTC (permalink / raw)
  To: x86, kvm, linux-kernel, linux-kselftest
  Cc: pbonzini, shuah, bgardon, seanjc, oupton, peterx, vkuznets,
	dmatlack, pgonda, andrew.jones, Vishal Annapurve

Confidential VMs(CVMs) need to execute hypercall instruction as per the CPU
type. Normally KVM emulates the vmcall/vmmcall instruction by patching
the guest code at runtime. Such a guest memory manipulation by KVM is
not allowed with CVMs.

This series adds support of executing hypercall as per the native cpu
type queried using cpuid instruction. CPU vendor type is stored after
one time execution of cpuid instruction to be reused later.

Changes in v3:
1) Guest logic is modified to not rely on host cpu type and instead query
cpu vendor using cpuid instruction.
2) Existing callers of vmmcall/vmcall are not updated to avoid enforcing
native hypercall instruction across all users which are mostly
non-confidential usecases.

v2:
https://lore.kernel.org/all/20220915000448.1674802-1-vannapurve@google.com/

More discussion around this change:
https://lore.kernel.org/lkml/Y1Hhw40H58EmZ6lK@google.com/

Vishal Annapurve (2):
  KVM: selftests: x86: Cache the cpu vendor type
  KVM: selftests: x86: Add native hypercall support

 .../selftests/kvm/include/x86_64/processor.h  |  3 ++
 .../selftests/kvm/lib/x86_64/processor.c      | 51 +++++++++++++++++--
 2 files changed, 49 insertions(+), 5 deletions(-)

-- 
2.39.0.314.g84b9a713c41-goog


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [V3 PATCH 1/2] KVM: selftests: x86: Cache the cpu vendor type
  2022-12-22 23:04 [V3 PATCH 0/2] Execute hypercalls from guests according to cpu Vishal Annapurve
@ 2022-12-22 23:04 ` Vishal Annapurve
  2022-12-23  0:24   ` Sean Christopherson
  2022-12-22 23:04 ` [V3 PATCH 2/2] KVM: selftests: x86: Add native hypercall support Vishal Annapurve
  2022-12-23 17:37 ` [V3 PATCH 0/2] Execute hypercalls from guests according to cpu Paolo Bonzini
  2 siblings, 1 reply; 8+ messages in thread
From: Vishal Annapurve @ 2022-12-22 23:04 UTC (permalink / raw)
  To: x86, kvm, linux-kernel, linux-kselftest
  Cc: pbonzini, shuah, bgardon, seanjc, oupton, peterx, vkuznets,
	dmatlack, pgonda, andrew.jones, Vishal Annapurve

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)
 {
-	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])
+		is_cpu_vendor_intel = true;
+	else {
+		if (ebx == amd_id_chunks[0] && edx == amd_id_chunks[1] &&
+			ecx == amd_id_chunks[2])
+			is_cpu_vendor_amd = true;
+	}
+	cpu_vendor_checked = true;
 }
 
 bool is_intel_cpu(void)
 {
-	return cpu_vendor_string_is("GenuineIntel");
+	check_cpu_vendor();
+
+	return is_cpu_vendor_intel;
 }
 
 /*
@@ -1036,7 +1057,9 @@ bool is_intel_cpu(void)
  */
 bool is_amd_cpu(void)
 {
-	return cpu_vendor_string_is("AuthenticAMD");
+	check_cpu_vendor();
+
+	return is_cpu_vendor_amd;
 }
 
 void kvm_get_cpu_address_width(unsigned int *pa_bits, unsigned int *va_bits)
-- 
2.39.0.314.g84b9a713c41-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [V3 PATCH 2/2] KVM: selftests: x86: Add native hypercall support
  2022-12-22 23:04 [V3 PATCH 0/2] Execute hypercalls from guests according to cpu Vishal Annapurve
  2022-12-22 23:04 ` [V3 PATCH 1/2] KVM: selftests: x86: Cache the cpu vendor type Vishal Annapurve
@ 2022-12-22 23:04 ` Vishal Annapurve
  2022-12-23  0:37   ` Sean Christopherson
  2022-12-23 17:37 ` [V3 PATCH 0/2] Execute hypercalls from guests according to cpu Paolo Bonzini
  2 siblings, 1 reply; 8+ messages in thread
From: Vishal Annapurve @ 2022-12-22 23:04 UTC (permalink / raw)
  To: x86, kvm, linux-kernel, linux-kselftest
  Cc: pbonzini, shuah, bgardon, seanjc, oupton, peterx, vkuznets,
	dmatlack, pgonda, andrew.jones, Vishal Annapurve

Add an API to execute hypercall as per the cpu type by checking the
underlying CPU. KVM emulates vmcall/vmmcall instruction by modifying
guest memory contents with hypercall instruction as per the cpu type.

Confidential VMs need to execute hypercall instruction without it being
emulated by KVM as KVM can not modify guest memory contents.

Signed-off-by: Vishal Annapurve <vannapurve@google.com>
---
 .../selftests/kvm/include/x86_64/processor.h   |  3 +++
 .../selftests/kvm/lib/x86_64/processor.c       | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 4d5dd9a467e1..3617f83bb2e5 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -1039,6 +1039,9 @@ uint64_t *vm_get_page_table_entry(struct kvm_vm *vm, uint64_t vaddr);
 uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
 		       uint64_t a3);
 
+uint64_t kvm_native_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
+		       uint64_t a3);
+
 void __vm_xsave_require_permission(int bit, const char *name);
 
 #define vm_xsave_require_permission(perm)	\
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index 1e93bb3cb058..429e55f2609f 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -1202,6 +1202,24 @@ uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
 	return r;
 }
 
+uint64_t kvm_native_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
+		       uint64_t a3)
+{
+	uint64_t r;
+
+	if (is_amd_cpu()) {
+		asm volatile("vmmcall"
+		     : "=a"(r)
+		     : "a"(nr), "b"(a0), "c"(a1), "d"(a2), "S"(a3));
+	} else {
+		asm volatile("vmcall"
+		     : "=a"(r)
+		     : "a"(nr), "b"(a0), "c"(a1), "d"(a2), "S"(a3));
+	}
+
+	return r;
+}
+
 const struct kvm_cpuid2 *kvm_get_supported_hv_cpuid(void)
 {
 	static struct kvm_cpuid2 *cpuid;
-- 
2.39.0.314.g84b9a713c41-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [V3 PATCH 1/2] KVM: selftests: x86: Cache the cpu vendor type
  2022-12-22 23:04 ` [V3 PATCH 1/2] KVM: selftests: x86: Cache the cpu vendor type Vishal Annapurve
@ 2022-12-23  0:24   ` Sean Christopherson
  2022-12-23  2:45     ` Vishal Annapurve
  0 siblings, 1 reply; 8+ messages in thread
From: Sean Christopherson @ 2022-12-23  0:24 UTC (permalink / raw)
  To: Vishal Annapurve
  Cc: x86, kvm, linux-kernel, linux-kselftest, pbonzini, shuah,
	bgardon, oupton, peterx, vkuznets, dmatlack, pgonda,
	andrew.jones

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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [V3 PATCH 2/2] KVM: selftests: x86: Add native hypercall support
  2022-12-22 23:04 ` [V3 PATCH 2/2] KVM: selftests: x86: Add native hypercall support Vishal Annapurve
@ 2022-12-23  0:37   ` Sean Christopherson
  0 siblings, 0 replies; 8+ messages in thread
From: Sean Christopherson @ 2022-12-23  0:37 UTC (permalink / raw)
  To: Vishal Annapurve
  Cc: x86, kvm, linux-kernel, linux-kselftest, pbonzini, shuah,
	bgardon, oupton, peterx, vkuznets, dmatlack, pgonda,
	andrew.jones

On Thu, Dec 22, 2022, Vishal Annapurve wrote:
> Add an API to execute hypercall as per the cpu type by checking the
> underlying CPU. KVM emulates vmcall/vmmcall instruction by modifying
> guest memory contents with hypercall instruction as per the cpu type.
> 
> Confidential VMs need to execute hypercall instruction without it being
> emulated by KVM as KVM can not modify guest memory contents.
> 
> Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> ---
>  .../selftests/kvm/include/x86_64/processor.h   |  3 +++
>  .../selftests/kvm/lib/x86_64/processor.c       | 18 ++++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index 4d5dd9a467e1..3617f83bb2e5 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -1039,6 +1039,9 @@ uint64_t *vm_get_page_table_entry(struct kvm_vm *vm, uint64_t vaddr);
>  uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
>  		       uint64_t a3);
>  
> +uint64_t kvm_native_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
> +		       uint64_t a3);
> +
>  void __vm_xsave_require_permission(int bit, const char *name);
>  
>  #define vm_xsave_require_permission(perm)	\
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 1e93bb3cb058..429e55f2609f 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -1202,6 +1202,24 @@ uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
>  	return r;
>  }
>  
> +uint64_t kvm_native_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,

Just do this in kvm_hypercall().  David didn't say "don't do that", he said "don't
do that in a single patch".  Except for fix_hypercall_test, selftests should always
use the native hypercall instruction and not rely on KVM's patching, e.g. patching
will go sideways if someone gets clever and makes guest code not-writable.

> +		       uint64_t a3)

Align parameters.

> +{
> +	uint64_t r;
> +
> +	if (is_amd_cpu()) {

Curly brace is unnecessary.  Though I think I'd prefer to not duplicate the asm
blob (too many darn inputs).  It's a little ugly, but I prefer it over duplicating
the entire blob.  The approach could also likely be macrofied for other hypercall
usage (future problem).

	asm volatile("test %[use_vmmcall], %[use_vmmcall]\n\t"
		     "jnz 1f\n\t"
		     "vmcall\n\t"
		     "jmp 2f\n\t"
		     "1: vmmcall\n\t"
		     "2:"
		     : "=a"(r)
		     : "a"(nr), "b"(a0), "c"(a1), "d"(a2), "S"(a3),
		       [use_vmmcall] "r" (is_amd_cpu()));
	return r;

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [V3 PATCH 1/2] KVM: selftests: x86: Cache the cpu vendor type
  2022-12-23  0:24   ` Sean Christopherson
@ 2022-12-23  2:45     ` Vishal Annapurve
  0 siblings, 0 replies; 8+ messages in thread
From: Vishal Annapurve @ 2022-12-23  2:45 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: x86, kvm, linux-kernel, linux-kselftest, pbonzini, shuah,
	bgardon, oupton, peterx, vkuznets, dmatlack, pgonda,
	andrew.jones

On Thu, Dec 22, 2022 at 4:24 PM Sean Christopherson <seanjc@google.com> wrote:
>
> ...
> 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;
>   }

You probably mean to suggest using memcmp here since this section will
get executed from the guest context (which may not have access to
strncmp). Makes sense to structure the implementation this way as I
get more comfortable using string_override.c.

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

Ok, I misunderstood the earlier discussion [1] about using host cpuid
to check the vendor type instead of the guest cpuid being problematic.
If all the current and foreseeable users would be ok with native
hypercall using host cpuid then I will fall back to approach in V2.

[1] https://lore.kernel.org/lkml/Y1Hhw40H58EmZ6lK@google.com/

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

Ack.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [V3 PATCH 0/2] Execute hypercalls from guests according to cpu
  2022-12-22 23:04 [V3 PATCH 0/2] Execute hypercalls from guests according to cpu Vishal Annapurve
  2022-12-22 23:04 ` [V3 PATCH 1/2] KVM: selftests: x86: Cache the cpu vendor type Vishal Annapurve
  2022-12-22 23:04 ` [V3 PATCH 2/2] KVM: selftests: x86: Add native hypercall support Vishal Annapurve
@ 2022-12-23 17:37 ` Paolo Bonzini
  2022-12-23 22:36   ` Vishal Annapurve
  2 siblings, 1 reply; 8+ messages in thread
From: Paolo Bonzini @ 2022-12-23 17:37 UTC (permalink / raw)
  To: Vishal Annapurve
  Cc: x86, kvm, linux-kernel, linux-kselftest, pbonzini, shuah,
	bgardon, seanjc, oupton, peterx, vkuznets, dmatlack, pgonda,
	andrew.jones

> This series adds support of executing hypercall as per the native cpu
> type queried using cpuid instruction. CPU vendor type is stored after
> one time execution of cpuid instruction to be reused later.

Makes sense, are you going to add more patches that use the new function?

Paolo



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [V3 PATCH 0/2] Execute hypercalls from guests according to cpu
  2022-12-23 17:37 ` [V3 PATCH 0/2] Execute hypercalls from guests according to cpu Paolo Bonzini
@ 2022-12-23 22:36   ` Vishal Annapurve
  0 siblings, 0 replies; 8+ messages in thread
From: Vishal Annapurve @ 2022-12-23 22:36 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: x86, kvm, linux-kernel, linux-kselftest, shuah, bgardon, seanjc,
	oupton, peterx, vkuznets, dmatlack, pgonda, andrew.jones

On Fri, Dec 23, 2022 at 9:38 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> > This series adds support of executing hypercall as per the native cpu
> > type queried using cpuid instruction. CPU vendor type is stored after
> > one time execution of cpuid instruction to be reused later.
>
> Makes sense, are you going to add more patches that use the new function?
>
> Paolo
>
>

Yeah, another series [1] uploaded recently, uses this newly added function.

[1] https://lore.kernel.org/lkml/20221223001352.3873203-1-vannapurve@google.com/

- Vishal

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-12-23 22:36 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-22 23:04 [V3 PATCH 0/2] Execute hypercalls from guests according to cpu Vishal Annapurve
2022-12-22 23:04 ` [V3 PATCH 1/2] KVM: selftests: x86: Cache the cpu vendor type Vishal Annapurve
2022-12-23  0:24   ` Sean Christopherson
2022-12-23  2:45     ` Vishal Annapurve
2022-12-22 23:04 ` [V3 PATCH 2/2] KVM: selftests: x86: Add native hypercall support Vishal Annapurve
2022-12-23  0:37   ` Sean Christopherson
2022-12-23 17:37 ` [V3 PATCH 0/2] Execute hypercalls from guests according to cpu Paolo Bonzini
2022-12-23 22:36   ` Vishal Annapurve

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.