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

Commit Message

Paolo Bonzini March 28, 2017, 8:28 a.m. UTC

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:


but with each part properly squashed into the right patch.

Paolo

> Signed-off-by: David Hildenbrand <[e-mail]>
> ---
>  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;
>  		}
>

Comments

David Hildenbrand April 3, 2017, 7:27 a.m. UTC | #1

On 28.03.2017 10:28, Paolo Bonzini wrote:
> 
> 
> 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.
> 

My assumption was that if vpic/vioapic checks worked until now without
memory barriers, the same should be true when checking against
irqchip_mode. But as we are dropping some implicit barriers and
irqchip_mode is updated more frequently, you may be right.

Also, using rmb/wmb in a uniform fashion with irqchip_mode looks
cleaner, as we are removing all implicit "cannot happen because of XXX"
special cases.

> 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.

Sure, thanks!

> 
> Paolo

Paolo Bonzini April 3, 2017, 8:58 a.m. UTC | #2

On 03/04/2017 09:27, 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.
>>
> My assumption was that if vpic/vioapic checks worked until now without
> memory barriers, the same should be true when checking against
> irqchip_mode.

All that was missing previously was smp_read_barrier_depends().  While a
compiler _can_ do things that are prohibited by
smp_read_barrier_depends(), it's much harder to trigger than for
smp_rmb().  For smp_rmb(), the scheduler's whims may cause it to break
the intended semantics.

Paolo

> But as we are dropping some implicit barriers and
> irqchip_mode is updated more frequently, you may be right.
> 
> Also, using rmb/wmb in a uniform fashion with irqchip_mode looks
> cleaner, as we are removing all implicit "cannot happen because of XXX"
> special cases.

Patch

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;