| Message ID | 20221228192438.2835203-2-vannapurve@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Execute hypercalls according to host cpu | expand |
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 <seanjc@google.com> > Signed-off-by: Vishal Annapurve <vannapurve@google.com> > --- ... > 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 <seanjc@google.com> 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 <seanjc@google.com> > > Signed-off-by: Vishal Annapurve <vannapurve@google.com> > > --- > > ... > > > 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.
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)
Use this_cpu_* helpers to query the cpu vendor. Suggested-by: Sean Christopherson <seanjc@google.com> Signed-off-by: Vishal Annapurve <vannapurve@google.com> --- .../selftests/kvm/include/x86_64/processor.h | 22 +++++++++++++++++++ .../selftests/kvm/lib/x86_64/processor.c | 16 ++------------ 2 files changed, 24 insertions(+), 14 deletions(-)