Am 23.01.2017 um 15:52 schrieb Dmitry Vyukov: > On Mon, Jan 23, 2017 at 3:25 PM, David Hildenbrand <david@xxxxxxxxxx> wrote: >> Am 22.01.2017 um 10:04 schrieb Dmitry Vyukov: >>> Currently svm_vm_data_hash_lock is left uninitialized. >>> This causes lockdep warnings. Properly initialize it. >>> >>> Signed-off-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx> >>> Cc: Joerg Roedel <joro@xxxxxxxxxx> >>> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx> >>> Cc: "Radim Krčmář" <rkrcmar@xxxxxxxxxx> >>> Cc: kvm@xxxxxxxxxxxxxxx >>> Cc: syzkaller@xxxxxxxxxxxxxxxx >>> --- >>> arch/x86/kvm/svm.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >>> index 08a4d3ab3455..b928a9c34987 100644 >>> --- a/arch/x86/kvm/svm.c >>> +++ b/arch/x86/kvm/svm.c >>> @@ -972,7 +972,7 @@ static void svm_disable_lbrv(struct vcpu_svm *svm) >>> */ >>> #define SVM_VM_DATA_HASH_BITS 8 >>> DECLARE_HASHTABLE(svm_vm_data_hash, SVM_VM_DATA_HASH_BITS); >>> -static spinlock_t svm_vm_data_hash_lock; >>> +static DEFINE_SPINLOCK(svm_vm_data_hash_lock); >>> >>> /* Note: >>> * This function is called from IOMMU driver to notify >>> >> >> We have >> >> spin_lock_init(&svm_vm_data_hash_lock); >> >> in svm_hardware_setup(). >> >> If this isn't called, wouldn't the right fix be to find out why? > > > spin_lock_init is called conditionally if avic. > Then avic_vm_init returns 0, if !avic. > But then avic_vm_destroy does not check avic and unconditionally uses > the spinlock. > Perhaps the right fix then will be: > > @@ -1382,6 +1383,9 @@ static void avic_vm_destroy(struct kvm *kvm) > unsigned long flags; > struct kvm_arch *vm_data = &kvm->arch; > > + if (!avic) > + return 0; > + > avic_free_vm_id(vm_data->avic_vm_id); > > if (vm_data->avic_logical_id_table_page) > > > Unfortunately I don't remember how I managed to trigger this warning > because I don't have any SVM-capable hardware... > But I remember that I did not do anything special besides just > enabling spinlock checks and then doing something trivial. > Could somebody try to use SVM with the spinlock checks enabled? I > don't feel comfortable sending a non-trivial patch without testing > it... > Or stick to your initial patch and simply remove the previous initialization (sounds clean to me). But we'd better double check if that additional check in avic_vm_destroy() is also reasonable (maybe something else is touched that shouldn't be). -- David
