On Thu, Mar 07, 2024, Sandipan Das wrote:
> On 3/5/2024 2:19 AM, Sean Christopherson wrote:
> The following are excerpts from some changes that I have been working on. Instead
> of a boolean flag, this saves the compatible vendor in kvm_vcpu_arch and tries
> to match it against X86_VENDOR_* values. The goal is to replace
> guest_cpuid_is_{intel,amd_or_hygon}() with
> guest_vendor_is_compatible(vcpu, X86_VENDOR_{INTEL,AMD}). Is this viable?
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index d271ba20a0b2..c4ada5b12fc3 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1042,6 +1042,7 @@ struct kvm_vcpu_arch {
> #if IS_ENABLED(CONFIG_HYPERV)
> hpa_t hv_root_tdp;
> #endif
> + u8 compat_vendor;
Ooh, clever, much better than my idea of using multiple booleans.
One potential flaw though: the vCPU structure is zero-allocated, and so this will
get a false positive on X86_VENDOR_INTEL if userspace never sets guest CPUID.
That might actually be desirable though? E.g. it's closer to KVM's current
behavior. Maybe. I dunno :-)
Anyways, KVM *does* need to be consistent, i.e. the default needs to yield the
same behavior as guest CPUID without entry 0x0 so that setting *other* CPUID
entries doesn't change from INTEL=>UNKNOWN. More below.
> };
>
> struct kvm_lpage_info {
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index adba49afb5fe..00170762e72a 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -376,6 +376,13 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> kvm_hv_set_cpuid(vcpu, kvm_cpuid_has_hyperv(vcpu->arch.cpuid_entries,
> vcpu->arch.cpuid_nent));
>
> + if (guest_cpuid_is_intel_compatible(vcpu))
> + vcpu->arch.compat_vendor = X86_VENDOR_INTEL;
> + else if (guest_cpuid_is_amd_or_hygon(vcpu))
> + vcpu->arch.compat_vendor = X86_VENDOR_AMD;
> + else
> + vcpu->arch.compat_vendor = X86_VENDOR_UNKNOWN;
I would prefer to provide a helper for just getting the compat vendor. E.g.
static u8 kvm_vcpu_get_compat_vendor(struct kvm_vcpu *vcpu)
{
struct kvm_cpuid_entry2 *best;
best = kvm_find_cpuid_entry(vcpu, 0);
if (!best)
return ???;
if (is_guest_vendor_intel(best->ebx, best->ecx, best->edx) ||
is_guest_vendor_centaur(best->ebx, best->ecx, best->edx) ||
is_guest_vendor_zhaoxin(best->ebx, best->ecx, best->edx))
return X86_VENDOR_INTEL;
if (is_guest_vendor_amd(best->ebx, best->ecx, best->edx) ||
is_guest_vendor_hygon(best->ebx, best->ecx, best->edx))
return X86_VENDOR_AMD;
return X86_VENDOR_UNKNOWN;
}
and then a follow-up patch can remove guest_cpuid_is_amd_or_hygon() once all
users are converted to guest_vendor_is_compatible().
