| Message ID | 20221013121319.994170-5-vannapurve@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Minor improvements to the selftest setup logic | expand |
On Thu, Oct 13, 2022, Vishal Annapurve wrote: > Cache the vendor CPU type in a global variable so that multiple calls > to is_amd/intel_cpu() do not need to re-execute CPUID. > > Sync the global variable is_cpu_amd into the guest so the guest can also > avoid executing CPUID instruction. > > Suggested-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Vishal Annapurve <vannapurve@google.com> > --- > tools/testing/selftests/kvm/lib/x86_64/processor.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c > index fa65e8142c16..f508e58346e9 100644 > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c > @@ -19,6 +19,7 @@ > #define MAX_NR_CPUID_ENTRIES 100 > > vm_vaddr_t exception_handlers; > +static bool is_cpu_amd; This should probably have a "host" qualifier, e.g. is_host_cpu_amd. More below. > > static void regs_dump(FILE *stream, struct kvm_regs *regs, uint8_t indent) > { > @@ -1046,7 +1047,7 @@ static bool cpu_vendor_string_is(const char *vendor) > > bool is_intel_cpu(void) It'll be more churn, but I think we should drop the wrappers in this patch so that we can visually audit all users. There is technically a subtle functional change here, as previously executing is_intel_cpu() and is_amd_cpu() in the guest will consume the _guest_ CPUID, whereas with this change, the guest will now consume the _host_ CPUID. It just so happens that the existing user and the new user both want to query Intel vs. AMD for VMCALL vs. VMMCALL, i.e. care about the host even when checking from the guest. It's extreme paranoia since I don't think there are any parallel series that are adding is_intel_cpu()/is_amd_cpu() users, not to mention that I don't think any selftests does cross-vendor virtualization, but on the other hand the paranoia doesn't cost much. > { > - return cpu_vendor_string_is("GenuineIntel"); > + return !is_cpu_amd; Please keep the explicit "GenuineIntel" check, i.e. add is_host_cpu_intel. KVM technically supports other vendors, e.g. Centaur and Zhaoxin for VMX, and Hygon for AMD, so it's not impossible that someone could run on Centuar or Zhaoxin and get a false positive. Again, extreme paranoia, but doesn't cost much. > } > > /* > @@ -1054,7 +1055,7 @@ bool is_intel_cpu(void) > */ > bool is_amd_cpu(void) > { > - return cpu_vendor_string_is("AuthenticAMD"); > + return is_cpu_amd; > } > > void kvm_get_cpu_address_width(unsigned int *pa_bits, unsigned int *va_bits) > @@ -1328,8 +1329,13 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm) > return get_kvm_intel_param_bool("unrestricted_guest"); > } > > +void kvm_selftest_arch_init(void) > +{ > + is_cpu_amd = cpu_vendor_string_is("AuthenticAMD"); > +} > > void kvm_arch_vm_post_create(struct kvm_vm *vm) > { > vm_create_irqchip(vm); > + sync_global_to_guest(vm, is_cpu_amd); > } > -- > 2.38.0.rc1.362.ged0d419d3c-goog >
On Fri, Oct 21, 2022 at 5:33 AM Sean Christopherson <seanjc@google.com> wrote: > > On Thu, Oct 13, 2022, Vishal Annapurve wrote: > > Cache the vendor CPU type in a global variable so that multiple calls > > to is_amd/intel_cpu() do not need to re-execute CPUID. > > > > Sync the global variable is_cpu_amd into the guest so the guest can also > > avoid executing CPUID instruction. > > > > Suggested-by: Sean Christopherson <seanjc@google.com> > > Signed-off-by: Vishal Annapurve <vannapurve@google.com> > > --- > > tools/testing/selftests/kvm/lib/x86_64/processor.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c > > index fa65e8142c16..f508e58346e9 100644 > > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c > > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c > > @@ -19,6 +19,7 @@ > > #define MAX_NR_CPUID_ENTRIES 100 > > > > vm_vaddr_t exception_handlers; > > +static bool is_cpu_amd; > > This should probably have a "host" qualifier, e.g. is_host_cpu_amd. More below. > Ack. > > > > static void regs_dump(FILE *stream, struct kvm_regs *regs, uint8_t indent) > > { > > @@ -1046,7 +1047,7 @@ static bool cpu_vendor_string_is(const char *vendor) > > > > bool is_intel_cpu(void) > > It'll be more churn, but I think we should drop the wrappers in this patch so > that we can visually audit all users. There is technically a subtle functional > change here, as previously executing is_intel_cpu() and is_amd_cpu() in the guest > will consume the _guest_ CPUID, whereas with this change, the guest will now > consume the _host_ CPUID. > > It just so happens that the existing user and the new user both want to query > Intel vs. AMD for VMCALL vs. VMMCALL, i.e. care about the host even when checking > from the guest. It's extreme paranoia since I don't think there are any parallel > series that are adding is_intel_cpu()/is_amd_cpu() users, not to mention that I > don't think any selftests does cross-vendor virtualization, but on the other hand > the paranoia doesn't cost much. > Ack. I think this patch should also be the part of a different series which deals with executing hypercall according to the cpu type, there is no immediate need for this change in this series. Will incorporate your feedback in the next version of this patch. > > { > > - return cpu_vendor_string_is("GenuineIntel"); > > + return !is_cpu_amd; > > Please keep the explicit "GenuineIntel" check, i.e. add is_host_cpu_intel. KVM > technically supports other vendors, e.g. Centaur and Zhaoxin for VMX, and Hygon > for AMD, so it's not impossible that someone could run on Centuar or Zhaoxin and > get a false positive. Again, extreme paranoia, but doesn't cost much. > Ack, makes sense. > > } > > > > /* > > @@ -1054,7 +1055,7 @@ bool is_intel_cpu(void) > > */ > > bool is_amd_cpu(void) > > { > > - return cpu_vendor_string_is("AuthenticAMD"); > > + return is_cpu_amd; > > } > > > > void kvm_get_cpu_address_width(unsigned int *pa_bits, unsigned int *va_bits) > > @@ -1328,8 +1329,13 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm) > > return get_kvm_intel_param_bool("unrestricted_guest"); > > } > > > > +void kvm_selftest_arch_init(void) > > +{ > > + is_cpu_amd = cpu_vendor_string_is("AuthenticAMD"); > > +} > > > > void kvm_arch_vm_post_create(struct kvm_vm *vm) > > { > > vm_create_irqchip(vm); > > + sync_global_to_guest(vm, is_cpu_amd); > > } > > -- > > 2.38.0.rc1.362.ged0d419d3c-goog > >
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c index fa65e8142c16..f508e58346e9 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c @@ -19,6 +19,7 @@ #define MAX_NR_CPUID_ENTRIES 100 vm_vaddr_t exception_handlers; +static bool is_cpu_amd; static void regs_dump(FILE *stream, struct kvm_regs *regs, uint8_t indent) { @@ -1046,7 +1047,7 @@ static bool cpu_vendor_string_is(const char *vendor) bool is_intel_cpu(void) { - return cpu_vendor_string_is("GenuineIntel"); + return !is_cpu_amd; } /* @@ -1054,7 +1055,7 @@ bool is_intel_cpu(void) */ bool is_amd_cpu(void) { - return cpu_vendor_string_is("AuthenticAMD"); + return is_cpu_amd; } void kvm_get_cpu_address_width(unsigned int *pa_bits, unsigned int *va_bits) @@ -1328,8 +1329,13 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm) return get_kvm_intel_param_bool("unrestricted_guest"); } +void kvm_selftest_arch_init(void) +{ + is_cpu_amd = cpu_vendor_string_is("AuthenticAMD"); +} void kvm_arch_vm_post_create(struct kvm_vm *vm) { vm_create_irqchip(vm); + sync_global_to_guest(vm, is_cpu_amd); }
Cache the vendor CPU type in a global variable so that multiple calls to is_amd/intel_cpu() do not need to re-execute CPUID. Sync the global variable is_cpu_amd into the guest so the guest can also avoid executing CPUID instruction. Suggested-by: Sean Christopherson <seanjc@google.com> Signed-off-by: Vishal Annapurve <vannapurve@google.com> --- tools/testing/selftests/kvm/lib/x86_64/processor.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)