| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <145980687a0fd85b783f8fd1412ee843e7c124e2.camel@redhat.com>
Date: Wed, 31 Aug 2022 12:36:18 +0300
From: Maxim Levitsky <mlevitsk@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>,
Paolo Bonzini <pbonzini@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
Li RongQing <lirongqing@...du.com>
Subject: Re: [PATCH 04/19] KVM: SVM: Replace "avic_mode" enum with
"x2avic_enabled" boolean
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@...gle.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@...hat.com>
Best regards,
Maxim Levitsky
Powered by blists - more mailing lists