Use this_cpu_* helpers to query the cpu vendor.
Suggested-by: Sean Christopherson <[email protected]>
Signed-off-by: Vishal Annapurve <[email protected]>
---
.../selftests/kvm/include/x86_64/processor.h | 22 +++++++++++++++++++
.../selftests/kvm/lib/x86_64/processor.c | 16 ++------------
2 files changed, 24 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 2a5f47d51388..84edac133d8f 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -555,6 +555,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 acfa1d01e7df..a799af572f3f 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -1006,26 +1006,14 @@ 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();
}
-/*
- * Exclude early K5 samples with a vendor string of "AMDisbetter!"
- */
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)
--
2.39.0.314.g84b9a713c41-goog
On Wed, Dec 28, 2022, Vishal Annapurve wrote:
> Use this_cpu_* helpers to query the cpu vendor.
Neither the changelog nor the shortlog captures what this patch actually does,
or rather what I inteded it to do. Specifically, what I suggested (or intended
to suggest) was:
KVM: selftests: Rename vendor string helpers to use "this_cpu" prefix
> Suggested-by: Sean Christopherson <[email protected]>
> Signed-off-by: Vishal Annapurve <[email protected]>
> ---
...
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index acfa1d01e7df..a799af572f3f 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -1006,26 +1006,14 @@ 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)
Drop the is_intel_cpu() and is_amd_cpu() wrappers. The whole point of the rename
was so that it's obvious at the call site that the function is checking the "current"
CPU context.
That obviously means dropping the is_host_cpu_amd() and is_host_cpu_intel() wrappers
too. IMO, the extra layer to jump through (more from a code reading perspective then
a code generation perspective) isn't worth protecting the booleans.
It's slightly more churn (in between patches, not overall), but the benefit is that
it allows squasing patches 2 and 3 into a single patch, e.g. "KVM: selftests: Cache
host CPU vendor (AMD vs. Intel)"
---
tools/testing/selftests/kvm/include/x86_64/processor.h | 3 +++
tools/testing/selftests/kvm/lib/x86_64/processor.c | 8 ++++----
tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c | 4 ++--
tools/testing/selftests/kvm/x86_64/mmio_warning_test.c | 2 +-
.../testing/selftests/kvm/x86_64/pmu_event_filter_test.c | 4 ++--
.../kvm/x86_64/vmx_exception_with_invalid_guest_state.c | 2 +-
6 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index fdb1af5ca611..c7885f72132a 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -19,6 +19,9 @@
#include "../kvm_util.h"
+extern bool host_cpu_is_intel;
+extern bool host_cpu_is_amd;
+
#define NMI_VECTOR 0x02
#define X86_EFLAGS_FIXED (1u << 1)
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index 0b8de34aa10e..84915bc7d689 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -19,8 +19,8 @@
#define MAX_NR_CPUID_ENTRIES 100
vm_vaddr_t exception_handlers;
-static bool host_cpu_is_amd;
-static bool host_cpu_is_intel;
+bool host_cpu_is_amd;
+bool host_cpu_is_intel;
static void regs_dump(FILE *stream, struct kvm_regs *regs, uint8_t indent)
{
@@ -115,7 +115,7 @@ static void sregs_dump(FILE *stream, struct kvm_sregs *sregs, uint8_t indent)
bool kvm_is_tdp_enabled(void)
{
- if (this_cpu_is_intel())
+ if (host_cpu_is_intel)
return get_kvm_intel_param_bool("ept");
else
return get_kvm_amd_param_bool("npt");
@@ -1218,7 +1218,7 @@ unsigned long vm_compute_max_gfn(struct kvm_vm *vm)
max_gfn = (1ULL << (vm->pa_bits - vm->page_shift)) - 1;
/* Avoid reserved HyperTransport region on AMD processors. */
- if (!this_cpu_is_amd())
+ if (!host_cpu_is_amd)
return max_gfn;
/* On parts with <40 physical address bits, the area is fully hidden */
diff --git a/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c b/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
index 5489c9836ec8..0f728f05ea82 100644
--- a/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
+++ b/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
@@ -48,10 +48,10 @@ static void guest_main(void)
const uint8_t *other_hypercall_insn;
uint64_t ret;
- if (this_cpu_is_intel()) {
+ if (host_cpu_is_intel) {
native_hypercall_insn = vmx_vmcall;
other_hypercall_insn = svm_vmmcall;
- } else if (this_cpu_is_amd()) {
+ } else if (host_cpu_is_amd) {
native_hypercall_insn = svm_vmmcall;
other_hypercall_insn = vmx_vmcall;
} else {
diff --git a/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c b/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c
index b0a2a0bae0f3..ce1ccc4c1503 100644
--- a/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c
+++ b/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c
@@ -93,7 +93,7 @@ int main(void)
{
int warnings_before, warnings_after;
- TEST_REQUIRE(this_cpu_is_intel());
+ TEST_REQUIRE(host_cpu_is_intel);
TEST_REQUIRE(!vm_is_unrestricted_guest(NULL));
diff --git a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
index c728822461b2..4dbb454e1760 100644
--- a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
+++ b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
@@ -363,7 +363,7 @@ static void test_pmu_config_disable(void (*guest_code)(void))
*/
static bool use_intel_pmu(void)
{
- return this_cpu_is_intel() &&
+ return host_cpu_is_intel &&
kvm_cpu_property(X86_PROPERTY_PMU_VERSION) &&
kvm_cpu_property(X86_PROPERTY_PMU_NR_GP_COUNTERS) &&
kvm_pmu_has(X86_PMU_FEATURE_BRANCH_INSNS_RETIRED);
@@ -397,7 +397,7 @@ static bool use_amd_pmu(void)
uint32_t family = kvm_cpu_family();
uint32_t model = kvm_cpu_model();
- return this_cpu_is_amd() &&
+ return host_cpu_is_amd &&
(is_zen1(family, model) ||
is_zen2(family, model) ||
is_zen3(family, model));
diff --git a/tools/testing/selftests/kvm/x86_64/vmx_exception_with_invalid_guest_state.c b/tools/testing/selftests/kvm/x86_64/vmx_exception_with_invalid_guest_state.c
index 53e1ef2fc774..ccdfa5dc1a4d 100644
--- a/tools/testing/selftests/kvm/x86_64/vmx_exception_with_invalid_guest_state.c
+++ b/tools/testing/selftests/kvm/x86_64/vmx_exception_with_invalid_guest_state.c
@@ -111,7 +111,7 @@ int main(int argc, char *argv[])
struct kvm_vcpu *vcpu;
struct kvm_vm *vm;
- TEST_REQUIRE(this_cpu_is_intel());
+ TEST_REQUIRE(host_cpu_is_intel);
TEST_REQUIRE(!vm_is_unrestricted_guest(NULL));
vm = vm_create_with_one_vcpu(&vcpu, guest_code);
base-commit: 04b420511919f7b78f17f5fa6dc92975a8b2d7c4
--
On Mon, Jan 9, 2023 at 10:07 AM Sean Christopherson <[email protected]> wrote:
>
> On Wed, Dec 28, 2022, Vishal Annapurve wrote:
> > Use this_cpu_* helpers to query the cpu vendor.
>
> Neither the changelog nor the shortlog captures what this patch actually does,
> or rather what I inteded it to do. Specifically, what I suggested (or intended
> to suggest) was:
>
> KVM: selftests: Rename vendor string helpers to use "this_cpu" prefix
>
> > Suggested-by: Sean Christopherson <[email protected]>
> > Signed-off-by: Vishal Annapurve <[email protected]>
> > ---
>
> ...
>
> > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > index acfa1d01e7df..a799af572f3f 100644
> > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > @@ -1006,26 +1006,14 @@ 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)
>
> Drop the is_intel_cpu() and is_amd_cpu() wrappers. The whole point of the rename
> was so that it's obvious at the call site that the function is checking the "current"
> CPU context.
>
> That obviously means dropping the is_host_cpu_amd() and is_host_cpu_intel() wrappers
> too. IMO, the extra layer to jump through (more from a code reading perspective then
> a code generation perspective) isn't worth protecting the booleans.
>
> It's slightly more churn (in between patches, not overall), but the benefit is that
> it allows squasing patches 2 and 3 into a single patch, e.g. "KVM: selftests: Cache
> host CPU vendor (AMD vs. Intel)"
>
> ---
> tools/testing/selftests/kvm/include/x86_64/processor.h | 3 +++
> tools/testing/selftests/kvm/lib/x86_64/processor.c | 8 ++++----
> tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c | 4 ++--
> tools/testing/selftests/kvm/x86_64/mmio_warning_test.c | 2 +-
> .../testing/selftests/kvm/x86_64/pmu_event_filter_test.c | 4 ++--
> .../kvm/x86_64/vmx_exception_with_invalid_guest_state.c | 2 +-
> 6 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index fdb1af5ca611..c7885f72132a 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -19,6 +19,9 @@
>
> #include "../kvm_util.h"
>
> +extern bool host_cpu_is_intel;
> +extern bool host_cpu_is_amd;
> +
> #define NMI_VECTOR 0x02
>
> #define X86_EFLAGS_FIXED (1u << 1)
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 0b8de34aa10e..84915bc7d689 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -19,8 +19,8 @@
> #define MAX_NR_CPUID_ENTRIES 100
>
> vm_vaddr_t exception_handlers;
> -static bool host_cpu_is_amd;
> -static bool host_cpu_is_intel;
> +bool host_cpu_is_amd;
> +bool host_cpu_is_intel;
>
> static void regs_dump(FILE *stream, struct kvm_regs *regs, uint8_t indent)
> {
> @@ -115,7 +115,7 @@ static void sregs_dump(FILE *stream, struct kvm_sregs *sregs, uint8_t indent)
>
> bool kvm_is_tdp_enabled(void)
> {
> - if (this_cpu_is_intel())
> + if (host_cpu_is_intel)
> return get_kvm_intel_param_bool("ept");
> else
> return get_kvm_amd_param_bool("npt");
> @@ -1218,7 +1218,7 @@ unsigned long vm_compute_max_gfn(struct kvm_vm *vm)
> max_gfn = (1ULL << (vm->pa_bits - vm->page_shift)) - 1;
>
> /* Avoid reserved HyperTransport region on AMD processors. */
> - if (!this_cpu_is_amd())
> + if (!host_cpu_is_amd)
> return max_gfn;
>
> /* On parts with <40 physical address bits, the area is fully hidden */
> diff --git a/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c b/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
> index 5489c9836ec8..0f728f05ea82 100644
> --- a/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
> @@ -48,10 +48,10 @@ static void guest_main(void)
> const uint8_t *other_hypercall_insn;
> uint64_t ret;
>
> - if (this_cpu_is_intel()) {
> + if (host_cpu_is_intel) {
> native_hypercall_insn = vmx_vmcall;
> other_hypercall_insn = svm_vmmcall;
> - } else if (this_cpu_is_amd()) {
> + } else if (host_cpu_is_amd) {
> native_hypercall_insn = svm_vmmcall;
> other_hypercall_insn = vmx_vmcall;
> } else {
> diff --git a/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c b/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c
> index b0a2a0bae0f3..ce1ccc4c1503 100644
> --- a/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c
> @@ -93,7 +93,7 @@ int main(void)
> {
> int warnings_before, warnings_after;
>
> - TEST_REQUIRE(this_cpu_is_intel());
> + TEST_REQUIRE(host_cpu_is_intel);
>
> TEST_REQUIRE(!vm_is_unrestricted_guest(NULL));
>
> diff --git a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
> index c728822461b2..4dbb454e1760 100644
> --- a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
> @@ -363,7 +363,7 @@ static void test_pmu_config_disable(void (*guest_code)(void))
> */
> static bool use_intel_pmu(void)
> {
> - return this_cpu_is_intel() &&
> + return host_cpu_is_intel &&
> kvm_cpu_property(X86_PROPERTY_PMU_VERSION) &&
> kvm_cpu_property(X86_PROPERTY_PMU_NR_GP_COUNTERS) &&
> kvm_pmu_has(X86_PMU_FEATURE_BRANCH_INSNS_RETIRED);
> @@ -397,7 +397,7 @@ static bool use_amd_pmu(void)
> uint32_t family = kvm_cpu_family();
> uint32_t model = kvm_cpu_model();
>
> - return this_cpu_is_amd() &&
> + return host_cpu_is_amd &&
> (is_zen1(family, model) ||
> is_zen2(family, model) ||
> is_zen3(family, model));
> diff --git a/tools/testing/selftests/kvm/x86_64/vmx_exception_with_invalid_guest_state.c b/tools/testing/selftests/kvm/x86_64/vmx_exception_with_invalid_guest_state.c
> index 53e1ef2fc774..ccdfa5dc1a4d 100644
> --- a/tools/testing/selftests/kvm/x86_64/vmx_exception_with_invalid_guest_state.c
> +++ b/tools/testing/selftests/kvm/x86_64/vmx_exception_with_invalid_guest_state.c
> @@ -111,7 +111,7 @@ int main(int argc, char *argv[])
> struct kvm_vcpu *vcpu;
> struct kvm_vm *vm;
>
> - TEST_REQUIRE(this_cpu_is_intel());
> + TEST_REQUIRE(host_cpu_is_intel);
> TEST_REQUIRE(!vm_is_unrestricted_guest(NULL));
>
> vm = vm_create_with_one_vcpu(&vcpu, guest_code);
>
> base-commit: 04b420511919f7b78f17f5fa6dc92975a8b2d7c4
> --
>
Ack, that makes sense. Will include this suggestion in the next version.