Re: [PATCH v2 11/24] KVM: x86: don't take kvm->irq_lock when creating IRQCHIP

[Paolo Bonzini <pbonzini@xxxxxxxxxx>]

On 23/03/2017 15:13, David Hildenbrand wrote:
> I don't see any reason any more for this lock, seemed to be used to protect
> removal of kvm->arch.vpic / kvm->arch.vioapic when already partially
> inititalized, now access is properly protected using kvm->arch.irqchip_mode
> and this shouldn't be necessary anymore.

Do more readers of irqchip_mode need memory barriers now?  Definitely we need an smp_wmb() after setting INIT_IN_PROGRESS.

Something like this:

diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
index 3eb140ba57b6..b1df4d53fad1 100644
--- a/arch/x86/kvm/ioapic.h
+++ b/arch/x86/kvm/ioapic.h
@@ -107,7 +107,9 @@ struct kvm_ioapic {
 
 static inline int ioapic_in_kernel(struct kvm *kvm)
 {
-	return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
+	int mode = kvm->arch.irqchip_mode; 
+	smp_rmb();
+	return mode == KVM_IRQCHIP_KERNEL;
 }
 
 void kvm_rtc_eoi_tracking_restore_one(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index 2455f16ee98a..6f7b3fb92dd4 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -80,26 +80,30 @@ struct kvm_pic {
 
 static inline int pic_in_kernel(struct kvm *kvm)
 {
-	return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
+	int mode = kvm->arch.irqchip_mode; 
+	smp_rmb();
+	return mode == KVM_IRQCHIP_KERNEL;
 }
 
 static inline int irqchip_split(struct kvm *kvm)
 {
-	return kvm->arch.irqchip_mode == KVM_IRQCHIP_SPLIT;
+	int mode = kvm->arch.irqchip_mode; 
+	smp_rmb();
+	return mode == KVM_IRQCHIP_SPLIT;
 }
 
 static inline int irqchip_kernel(struct kvm *kvm)
 {
-	return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
+	int mode = kvm->arch.irqchip_mode; 
+	smp_rmb();
+	return mode == KVM_IRQCHIP_KERNEL;
 }
 
 static inline int irqchip_in_kernel(struct kvm *kvm)
 {
-	bool ret = kvm->arch.irqchip_mode > KVM_IRQCHIP_INIT_IN_PROGRESS;
-
-	/* Matches with wmb after initializing kvm->irq_routing. */
+	int mode = kvm->arch.irqchip_mode; 
 	smp_rmb();
-	return ret;
+	return mode > KVM_IRQCHIP_INIT_IN_PROGRESS;
 }
 
 void kvm_pic_reset(struct kvm_kpic_state *s);
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 45ab43e5fbc5..e8306eeb5613 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -286,6 +286,7 @@ int kvm_set_routing_entry(struct kvm *kvm,
 	if (kvm->arch.irqchip_mode == KVM_IRQCHIP_NONE)
 		goto out;
 
+	smp_rmb();
 	switch (ue->type) {
 	case KVM_IRQ_ROUTING_IRQCHIP:
 		if (irqchip_split(kvm))
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5dca5aed9c93..ed9143f305f9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3933,6 +3933,8 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 		if (kvm->created_vcpus)
 			goto split_irqchip_unlock;
 		kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS;
+		/* Pairs with pic_in_kernel/ioapic_in_kernel/etc. */
+		smp_wmb();
 		r = kvm_setup_empty_irq_routing(kvm);
 		if (r) {
 			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
@@ -4026,6 +4028,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		}
 
 		kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS;
+		/* Pairs with pic_in_kernel/ioapic_in_kernel/etc. */
+		smp_wmb();
 		r = kvm_setup_default_irq_routing(kvm);
 		if (r) {
 			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;

but with each part properly squashed into the right patch.

Paolo

> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
> ---
>  arch/x86/kvm/x86.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1636da0..db1a5ff 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4030,10 +4030,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  		if (r) {
>  			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
>  			mutex_lock(&kvm->slots_lock);
> -			mutex_lock(&kvm->irq_lock);
>  			kvm_ioapic_destroy(kvm);
>  			kvm_pic_destroy(kvm);
> -			mutex_unlock(&kvm->irq_lock);
>  			mutex_unlock(&kvm->slots_lock);
>  			goto create_irqchip_unlock;
>  		}
>