Re: [PATCH 04/19] KVM: SVM: Replace "avic_mode" enum with "x2avic_enabled" boolean

[Maxim Levitsky <mlevitsk@redhat.com>]

On Wed, 2022-08-31 at 00:34 +0000, Sean Christopherson wrote:
> Replace the "avic_mode" enum with a single bool to track whether or not
> x2AVIC is enabled.  KVM already has "apicv_enabled" that tracks if any
> flavor of AVIC is enabled, i.e. AVIC_MODE_NONE and AVIC_MODE_X1 are
> redundant and unnecessary noise.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/avic.c | 45 +++++++++++++++++++----------------------
>  arch/x86/kvm/svm/svm.c  |  2 +-
>  arch/x86/kvm/svm/svm.h  |  9 +--------
>  3 files changed, 23 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 1d516d658f9a..b59f8ee2671f 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -53,7 +53,7 @@ static DEFINE_HASHTABLE(svm_vm_data_hash, SVM_VM_DATA_HASH_BITS);
>  static u32 next_vm_id = 0;
>  static bool next_vm_id_wrapped = 0;
>  static DEFINE_SPINLOCK(svm_vm_data_hash_lock);
> -enum avic_modes avic_mode;
> +bool x2avic_enabled;
>  
>  /*
>   * This is a wrapper of struct amd_iommu_ir_data.
> @@ -231,8 +231,8 @@ static u64 *avic_get_physical_id_entry(struct kvm_vcpu *vcpu,
>  	u64 *avic_physical_id_table;
>  	struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
>  
> -	if ((avic_mode == AVIC_MODE_X1 && index > AVIC_MAX_PHYSICAL_ID) ||
> -	    (avic_mode == AVIC_MODE_X2 && index > X2AVIC_MAX_PHYSICAL_ID))
> +	if ((!x2avic_enabled && index > AVIC_MAX_PHYSICAL_ID) ||
> +	    (index > X2AVIC_MAX_PHYSICAL_ID))
>  		return NULL;
>  
>  	avic_physical_id_table = page_address(kvm_svm->avic_physical_id_table_page);
> @@ -279,8 +279,8 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
>  	int id = vcpu->vcpu_id;
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  
> -	if ((avic_mode == AVIC_MODE_X1 && id > AVIC_MAX_PHYSICAL_ID) ||
> -	    (avic_mode == AVIC_MODE_X2 && id > X2AVIC_MAX_PHYSICAL_ID))
> +	if ((!x2avic_enabled && id > AVIC_MAX_PHYSICAL_ID) ||
> +	    (id > X2AVIC_MAX_PHYSICAL_ID))
>  		return -EINVAL;
>  
>  	if (!vcpu->arch.apic->regs)
> @@ -532,7 +532,7 @@ unsigned long avic_vcpu_get_apicv_inhibit_reasons(struct kvm_vcpu *vcpu)
>  	 * AVIC must be disabled if x2AVIC isn't supported and the guest has
>  	 * x2APIC enabled.
>  	 */
> -	if (avic_mode != AVIC_MODE_X2 && apic_x2apic_mode(vcpu->arch.apic))
> +	if (!x2avic_enabled && apic_x2apic_mode(vcpu->arch.apic))
>  		return APICV_INHIBIT_REASON_X2APIC;
>  
>  	return 0;
> @@ -1086,10 +1086,7 @@ void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	struct vmcb *vmcb = svm->vmcb01.ptr;
>  
> -	if (!lapic_in_kernel(vcpu) || avic_mode == AVIC_MODE_NONE)
> -		return;
> -
> -	if (!enable_apicv)
> +	if (!lapic_in_kernel(vcpu) || !enable_apicv)
>  		return;
>  
>  	if (kvm_vcpu_apicv_active(vcpu)) {
> @@ -1165,32 +1162,32 @@ bool avic_hardware_setup(struct kvm_x86_ops *x86_ops)
>  	if (!npt_enabled)
>  		return false;
>  
> +	/* AVIC is a prerequisite for x2AVIC. */
> +	if (!boot_cpu_has(X86_FEATURE_AVIC) && !force_avic) {
> +		if (boot_cpu_has(X86_FEATURE_X2AVIC)) {
> +			pr_warn(FW_BUG "Cannot support x2AVIC due to AVIC is disabled");
> +			pr_warn(FW_BUG "Try enable AVIC using force_avic option");
> +		}
> +		return false;
> +	}
> +
>  	if (boot_cpu_has(X86_FEATURE_AVIC)) {
> -		avic_mode = AVIC_MODE_X1;
>  		pr_info("AVIC enabled\n");
>  	} else if (force_avic) {
>  		/*
>  		 * Some older systems does not advertise AVIC support.
>  		 * See Revision Guide for specific AMD processor for more detail.
>  		 */
> -		avic_mode = AVIC_MODE_X1;
>  		pr_warn("AVIC is not supported in CPUID but force enabled");
>  		pr_warn("Your system might crash and burn");
>  	}
>  
>  	/* AVIC is a prerequisite for x2AVIC. */
> -	if (boot_cpu_has(X86_FEATURE_X2AVIC)) {
> -		if (avic_mode == AVIC_MODE_X1) {
> -			avic_mode = AVIC_MODE_X2;
> -			pr_info("x2AVIC enabled\n");
> -		} else {
> -			pr_warn(FW_BUG "Cannot support x2AVIC due to AVIC is disabled");
> -			pr_warn(FW_BUG "Try enable AVIC using force_avic option");
> -		}
> -	}
> +	x2avic_enabled = boot_cpu_has(X86_FEATURE_X2AVIC);
> +	if (x2avic_enabled)
> +		pr_info("x2AVIC enabled\n");
>  
> -	if (avic_mode != AVIC_MODE_NONE)
> -		amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
> +	amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
>  
> -	return !!avic_mode;
> +	return true;
>  }
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index f3813dbacb9f..b25c89069128 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -821,7 +821,7 @@ void svm_set_x2apic_msr_interception(struct vcpu_svm *svm, bool intercept)
>  	if (intercept == svm->x2avic_msrs_intercepted)
>  		return;
>  
> -	if (avic_mode != AVIC_MODE_X2 ||
> +	if (!x2avic_enabled ||
>  	    !apic_x2apic_mode(svm->vcpu.arch.apic))
>  		return;
>  
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 6a7686bf6900..cee79ade400a 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -35,14 +35,7 @@ extern u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
>  extern bool npt_enabled;
>  extern int vgif;
>  extern bool intercept_smi;
> -
> -enum avic_modes {
> -	AVIC_MODE_NONE = 0,
> -	AVIC_MODE_X1,
> -	AVIC_MODE_X2,
> -};
> -
> -extern enum avic_modes avic_mode;
> +extern bool x2avic_enabled;
>  
>  /*
>   * Clean bits in VMCB.

Overall looks like an improvement, I would probably do it this way if I were to
implement this code, but the original code is alright as well.


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


Best regards,
	Maxim Levitsky