| 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: <d3bdaa2e-c268-4828-8f85-75fd0f859887@redhat.com>
Date: Tue, 8 Apr 2025 19:51:24 +0200
From: Paolo Bonzini <pbonzini@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>, Joerg Roedel <joro@...tes.org>,
David Woodhouse <dwmw2@...radead.org>, Lu Baolu <baolu.lu@...ux.intel.com>
Cc: kvm@...r.kernel.org, iommu@...ts.linux.dev, linux-kernel@...r.kernel.org,
Maxim Levitsky <mlevitsk@...hat.com>,
Joao Martins <joao.m.martins@...cle.com>, David Matlack <dmatlack@...gle.com>
Subject: Re: [PATCH 62/67] KVM: SVM: Don't check vCPU's blocking status when
toggling AVIC on/off
On 4/4/25 21:39, Sean Christopherson wrote:
> Don't query a vCPU's blocking status when toggling AVIC on/off; barring
> KVM bugs, the vCPU can't be blocking when refrecing AVIC controls. And if
refrecing -> refreshing
Paolo
> there are KVM bugs, ensuring the vCPU and its associated IRTEs are in the
> correct state is desirable, i.e. well worth any overhead in a buggy
> scenario.
>
> Isolating the "real" load/put flows will allow moving the IOMMU IRTE
> (de)activation logic from avic_refresh_apicv_exec_ctrl() to
> avic_update_iommu_vcpu_affinity(), i.e. will allow updating the vCPU's
> physical ID entry and its IRTEs in a common path, under a single critical
> section of ir_list_lock.
>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
> arch/x86/kvm/svm/avic.c | 65 +++++++++++++++++++++++------------------
> 1 file changed, 37 insertions(+), 28 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 0425cc374a79..d5fa915d0827 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -838,7 +838,7 @@ static void avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int cpu)
> WARN_ON_ONCE(amd_iommu_update_ga(cpu, ir->data));
> }
>
> -void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> +static void __avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> {
> struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
> int h_physical_id = kvm_cpu_get_apicid(cpu);
> @@ -854,16 +854,6 @@ void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> if (WARN_ON_ONCE(vcpu->vcpu_id * sizeof(entry) >= PAGE_SIZE))
> return;
>
> - /*
> - * No need to update anything if the vCPU is blocking, i.e. if the vCPU
> - * is being scheduled in after being preempted. The CPU entries in the
> - * Physical APIC table and IRTE are consumed iff IsRun{ning} is '1'.
> - * If the vCPU was migrated, its new CPU value will be stuffed when the
> - * vCPU unblocks.
> - */
> - if (kvm_vcpu_is_blocking(vcpu))
> - return;
> -
> /*
> * Grab the per-vCPU interrupt remapping lock even if the VM doesn't
> * _currently_ have assigned devices, as that can change. Holding
> @@ -898,31 +888,33 @@ void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> spin_unlock_irqrestore(&svm->ir_list_lock, flags);
> }
>
> -void avic_vcpu_put(struct kvm_vcpu *vcpu)
> +void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> +{
> + /*
> + * No need to update anything if the vCPU is blocking, i.e. if the vCPU
> + * is being scheduled in after being preempted. The CPU entries in the
> + * Physical APIC table and IRTE are consumed iff IsRun{ning} is '1'.
> + * If the vCPU was migrated, its new CPU value will be stuffed when the
> + * vCPU unblocks.
> + */
> + if (kvm_vcpu_is_blocking(vcpu))
> + return;
> +
> + __avic_vcpu_load(vcpu, cpu);
> +}
> +
> +static void __avic_vcpu_put(struct kvm_vcpu *vcpu)
> {
> struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
> struct vcpu_svm *svm = to_svm(vcpu);
> unsigned long flags;
> - u64 entry;
> + u64 entry = svm->avic_physical_id_entry;
>
> lockdep_assert_preemption_disabled();
>
> if (WARN_ON_ONCE(vcpu->vcpu_id * sizeof(entry) >= PAGE_SIZE))
> return;
>
> - /*
> - * Note, reading the Physical ID entry outside of ir_list_lock is safe
> - * as only the pCPU that has loaded (or is loading) the vCPU is allowed
> - * to modify the entry, and preemption is disabled. I.e. the vCPU
> - * can't be scheduled out and thus avic_vcpu_{put,load}() can't run
> - * recursively.
> - */
> - entry = svm->avic_physical_id_entry;
> -
> - /* Nothing to do if IsRunning == '0' due to vCPU blocking. */
> - if (!(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK))
> - return;
> -
> /*
> * Take and hold the per-vCPU interrupt remapping lock while updating
> * the Physical ID entry even though the lock doesn't protect against
> @@ -942,7 +934,24 @@ void avic_vcpu_put(struct kvm_vcpu *vcpu)
> WRITE_ONCE(kvm_svm->avic_physical_id_table[vcpu->vcpu_id], entry);
>
> spin_unlock_irqrestore(&svm->ir_list_lock, flags);
> +}
>
> +void avic_vcpu_put(struct kvm_vcpu *vcpu)
> +{
> + /*
> + * Note, reading the Physical ID entry outside of ir_list_lock is safe
> + * as only the pCPU that has loaded (or is loading) the vCPU is allowed
> + * to modify the entry, and preemption is disabled. I.e. the vCPU
> + * can't be scheduled out and thus avic_vcpu_{put,load}() can't run
> + * recursively.
> + */
> + u64 entry = to_svm(vcpu)->avic_physical_id_entry;
> +
> + /* Nothing to do if IsRunning == '0' due to vCPU blocking. */
> + if (!(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK))
> + return;
> +
> + __avic_vcpu_put(vcpu);
> }
>
> void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu)
> @@ -983,9 +992,9 @@ void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
> avic_refresh_virtual_apic_mode(vcpu);
>
> if (activated)
> - avic_vcpu_load(vcpu, vcpu->cpu);
> + __avic_vcpu_load(vcpu, vcpu->cpu);
> else
> - avic_vcpu_put(vcpu);
> + __avic_vcpu_put(vcpu);
>
> /*
> * Here, we go through the per-vcpu ir_list to update all existing
Powered by blists - more mailing lists