Re: [PATCH v2 16/49] KVM: x86: Don't update PV features caches when enabling enforcement capability

[Maxim Levitsky <mlevitsk@redhat.com>]

On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote:
> Revert the chunk of commit 01b4f510b9f4 ("kvm: x86: ensure pv_cpuid.features
> is initialized when enabling cap") that forced a PV features cache refresh
> during KVM_CAP_ENFORCE_PV_FEATURE_CPUID, as whatever ioctl() ordering
> issue it alleged to have fixed never existed upstream, and likely never
> existed in any kernel.
> 
> At the time of the commit, there was a tangentially related ioctl()
> ordering issue, as toggling KVM_X86_DISABLE_EXITS_HLT after KVM_SET_CPUID2
> would have resulted in KVM potentially leaving KVM_FEATURE_PV_UNHALT set.
> But (a) that bug affected the entire guest CPUID, not just the cache, (b)
> commit 01b4f510b9f4 didn't address that bug, it only refreshed the cache
> (with the bad CPUID), and (c) setting KVM_X86_DISABLE_EXITS_HLT after vCPU
> creation is completely broken as KVM configures HLT-exiting only during
> vCPU creation, which is why KVM_CAP_X86_DISABLE_EXITS is now disallowed if
> vCPUs have been created.
> 
> Another tangentially related bug was KVM's failure to clear the cache when
> handling KVM_SET_CPUID2, but again commit 01b4f510b9f4 did nothing to fix
> that bug.
> 
> The most plausible explanation for the what commit 01b4f510b9f4 was trying
> to fix is a bug that existed in Google's internal kernel that was the
> source of commit 01b4f510b9f4.  At the time, Google's internal kernel had
> not yet picked up commit 0d3b2ba16ba68 ("KVM: X86: Go on updating other
> CPUID leaves when leaf 1 is absent"), i.e. KVM would not initialize the
> PV features cache if KVM_SET_CPUID2 was called without a CPUID.0x1 entry.
> 
> Of course, no sane real world VMM would omit CPUID.0x1, including the KVM
> selftest added by commit ac4a4d6de22e ("selftests: kvm: test enforcement
> of paravirtual cpuid features").  And the test didn't actually try to
> verify multiple orderings, nor did the selftest enter the guest without
> doing KVM_SET_CPUID2, so who knows what motivated the change.
> 
> Regardless of why commit 01b4f510b9f4 ("kvm: x86: ensure pv_cpuid.features
> is initialized when enabling cap") was added, refreshing the cache during
> KVM_CAP_ENFORCE_PV_FEATURE_CPUID isn't necessary.
> 
> Cc: Oliver Upton <oliver.upton@linux.dev>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 2 +-
>  arch/x86/kvm/cpuid.h | 1 -
>  arch/x86/kvm/x86.c   | 3 ---
>  3 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index be1c8f43e090..a51e48663f53 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -242,7 +242,7 @@ static struct kvm_cpuid_entry2 *kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcp
>  					     vcpu->arch.cpuid_nent, base);
>  }
>  
> -void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)
> +static void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_cpuid_entry2 *best = kvm_find_kvm_cpuid_features(vcpu);
>  
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index 0a8b561b5434..7eb3d7318fc4 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -13,7 +13,6 @@ void kvm_set_cpu_caps(void);
>  
>  void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu);
>  void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu);
> -void kvm_update_pv_runtime(struct kvm_vcpu *vcpu);
>  struct kvm_cpuid_entry2 *kvm_find_cpuid_entry_index(struct kvm_vcpu *vcpu,
>  						    u32 function, u32 index);
>  struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c729227c6501..7160c5ab8e3e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5849,9 +5849,6 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
>  
>  	case KVM_CAP_ENFORCE_PV_FEATURE_CPUID:
>  		vcpu->arch.pv_cpuid.enforce = cap->args[0];
> -		if (vcpu->arch.pv_cpuid.enforce)
> -			kvm_update_pv_runtime(vcpu);
> -
>  		return 0;
>  	default:
>  		return -EINVAL;

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky