[PATCH-v2 0/3] KVM: Take missing slots_lock for kvm_io_bus_unregister_dev()

[PATCH-v2 0/3] KVM: Take missing slots_lock for kvm_io_bus_unregister_dev()

[Takuya Yoshikawa]

This patchset consists of three patches:

  [1/3] KVM: Take missing slots_lock for kvm_io_bus_unregister_dev()
  [2/3] KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP
  [3/3] KVM: Centralize irq_lock aquisition during KVM_CREATE_IRQCHIP

Though patch 1 is a fix, this is only for an error handling path.
Patches 2,3 are just for coding style consistency. So pick up those
whitch match your policy.


Changelog:
  moved slots_lock and irq_lock aquisition to their caller following
  Marcelo's advice.

Thanks,
  Takuya

[PATCH 1/3] KVM: Take missing slots_lock for kvm_io_bus_unregister_dev()

[Takuya Yoshikawa]

From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>

In KVM_CREATE_IRQCHIP, kvm_io_bus_unregister_dev() is called without taking
slots_lock in the error handling path.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/ia64/kvm/kvm-ia64.c |    2 ++
 arch/x86/kvm/x86.c       |    4 ++++
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 48a48bd..70d224d 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -951,7 +951,9 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto out;
 		r = kvm_setup_default_irq_routing(kvm);
 		if (r) {
+			mutex_lock(&kvm->slots_lock);
 			kvm_ioapic_destroy(kvm);
+			mutex_unlock(&kvm->slots_lock);
 			goto out;
 		}
 		break;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8d76150..3113aaf 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3308,8 +3308,10 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		if (vpic) {
 			r = kvm_ioapic_init(kvm);
 			if (r) {
+				mutex_lock(&kvm->slots_lock);
 				kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,
 							  &vpic->dev);
+				mutex_unlock(&kvm->slots_lock);
 				kfree(vpic);
 				goto create_irqchip_unlock;
 			}
@@ -3320,10 +3322,12 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		smp_wmb();
 		r = kvm_setup_default_irq_routing(kvm);
 		if (r) {
+			mutex_lock(&kvm->slots_lock);
 			mutex_lock(&kvm->irq_lock);
 			kvm_ioapic_destroy(kvm);
 			kvm_destroy_pic(kvm);
 			mutex_unlock(&kvm->irq_lock);
+			mutex_unlock(&kvm->slots_lock);
 		}
 	create_irqchip_unlock:
 		mutex_unlock(&kvm->lock);
-- 
1.7.1

Re: [PATCH 1/3] KVM: Take missing slots_lock for kvm_io_bus_unregister_dev()

[Avi Kivity]

On 12/15/2010 06:41 PM, Takuya Yoshikawa wrote:
> From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
>
> In KVM_CREATE_IRQCHIP, kvm_io_bus_unregister_dev() is called without taking
> slots_lock in the error handling path.

Thanks, applied.

-- 
error compiling committee.c: too many arguments to function

[PATCH 2/3] KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP

[Takuya Yoshikawa]

From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>

Move slots_lock aquisition from kvm_ioapic_init() and kvm_create_pic()
to their caller.

As a result, x86's KVM_CREATE_IRQCHIP is now covered by a unified slots_lock
section, including kvm_setup_default_irq_routing().

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/ia64/kvm/kvm-ia64.c |    2 ++
 arch/x86/kvm/i8259.c     |    2 --
 arch/x86/kvm/x86.c       |    6 ++----
 virt/kvm/ioapic.c        |    2 --
 4 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 70d224d..060c594 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -946,7 +946,9 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		}
 	case KVM_CREATE_IRQCHIP:
 		r = -EFAULT;
+		mutex_lock(&kvm->slots_lock);
 		r = kvm_ioapic_init(kvm);
+		mutex_unlock(&kvm->slots_lock);
 		if (r)
 			goto out;
 		r = kvm_setup_default_irq_routing(kvm);
diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index f628234..37d24bc 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -580,9 +580,7 @@ struct kvm_pic *kvm_create_pic(struct kvm *kvm)
 	 * Initialize PIO device
 	 */
 	kvm_iodevice_init(&s->dev, &picdev_ops);
-	mutex_lock(&kvm->slots_lock);
 	ret = kvm_io_bus_register_dev(kvm, KVM_PIO_BUS, &s->dev);
-	mutex_unlock(&kvm->slots_lock);
 	if (ret < 0) {
 		kfree(s);
 		return NULL;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3113aaf..736ab93 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3300,6 +3300,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		struct kvm_pic *vpic;
 
 		mutex_lock(&kvm->lock);
+		mutex_lock(&kvm->slots_lock);
 		r = -EEXIST;
 		if (kvm->arch.vpic)
 			goto create_irqchip_unlock;
@@ -3308,10 +3309,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		if (vpic) {
 			r = kvm_ioapic_init(kvm);
 			if (r) {
-				mutex_lock(&kvm->slots_lock);
 				kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,
 							  &vpic->dev);
-				mutex_unlock(&kvm->slots_lock);
 				kfree(vpic);
 				goto create_irqchip_unlock;
 			}
@@ -3322,14 +3321,13 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		smp_wmb();
 		r = kvm_setup_default_irq_routing(kvm);
 		if (r) {
-			mutex_lock(&kvm->slots_lock);
 			mutex_lock(&kvm->irq_lock);
 			kvm_ioapic_destroy(kvm);
 			kvm_destroy_pic(kvm);
 			mutex_unlock(&kvm->irq_lock);
-			mutex_unlock(&kvm->slots_lock);
 		}
 	create_irqchip_unlock:
+		mutex_unlock(&kvm->slots_lock);
 		mutex_unlock(&kvm->lock);
 		break;
 	}
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 0b9df83..532bffc 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -393,9 +393,7 @@ int kvm_ioapic_init(struct kvm *kvm)
 	kvm_ioapic_reset(ioapic);
 	kvm_iodevice_init(&ioapic->dev, &ioapic_mmio_ops);
 	ioapic->kvm = kvm;
-	mutex_lock(&kvm->slots_lock);
 	ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, &ioapic->dev);
-	mutex_unlock(&kvm->slots_lock);
 	if (ret < 0) {
 		kvm->arch.vioapic = NULL;
 		kfree(ioapic);
-- 
1.7.1

Re: [PATCH 2/3] KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP

[Avi Kivity]

On 12/15/2010 06:43 PM, Takuya Yoshikawa wrote:
> From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
>
> Move slots_lock aquisition from kvm_ioapic_init() and kvm_create_pic()
> to their caller.
>
> As a result, x86's KVM_CREATE_IRQCHIP is now covered by a unified slots_lock
> section, including kvm_setup_default_irq_routing().
>
>

I'm not sure about this...

> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3300,6 +3300,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
>   		struct kvm_pic *vpic;
>
>   		mutex_lock(&kvm->lock);
> +		mutex_lock(&kvm->slots_lock);

here, the reader might wonder why we take slots_lock, since we aren't 
manipulating anything covered by the lock directly.

>   		r = -EEXIST;
>   		if (kvm->arch.vpic)
>   			goto create_irqchip_unlock;
> @@ -3308,10 +3309,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
>   		if (vpic) {
>   			r = kvm_ioapic_init(kvm);
>   			if (r) {
> -				mutex_lock(&kvm->slots_lock);

and here, the reader might wonder why we don't take slots_lock, which 
protects io_bus.

Maybe we ought to move slots_lock acquisition to kvm_io_bus_register() 
and friends.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/3] KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP

[Takuya Yoshikawa]

(2010/12/16 18:11), Avi Kivity wrote:
> On 12/15/2010 06:43 PM, Takuya Yoshikawa wrote:
>> From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
>>
>> Move slots_lock aquisition from kvm_ioapic_init() and kvm_create_pic()
>> to their caller.
>>
>> As a result, x86's KVM_CREATE_IRQCHIP is now covered by a unified slots_lock
>> section, including kvm_setup_default_irq_routing().
>>
>>
>
> I'm not sure about this...
>
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -3300,6 +3300,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
>> struct kvm_pic *vpic;
>>
>> mutex_lock(&kvm->lock);
>> + mutex_lock(&kvm->slots_lock);
>
> here, the reader might wonder why we take slots_lock, since we aren't manipulating anything covered by the lock directly.
>
>> r = -EEXIST;
>> if (kvm->arch.vpic)
>> goto create_irqchip_unlock;
>> @@ -3308,10 +3309,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
>> if (vpic) {
>> r = kvm_ioapic_init(kvm);
>> if (r) {
>> - mutex_lock(&kvm->slots_lock);
>
> and here, the reader might wonder why we don't take slots_lock, which protects io_bus.
>
> Maybe we ought to move slots_lock acquisition to kvm_io_bus_register() and friends.
>

So it will move the lock acquisition to the opposite ( callee ) side than mine.
   At first, I tried to do that, but there are so many ...

Anyway, your suggestion seems to be the best way if possible.


One question: how about kvm_io_bus_[read|write] ?

These are called from the emulator but I could not find where slots_lock
are held though I can see the comments

   "kvm_io_bus_[read|write] - called under kvm->slots_lock"


Takuya

Re: [PATCH 2/3] KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP

[Avi Kivity]

On 12/16/2010 11:29 AM, Takuya Yoshikawa wrote:
>> and here, the reader might wonder why we don't take slots_lock, which 
>> protects io_bus.
>>
>> Maybe we ought to move slots_lock acquisition to 
>> kvm_io_bus_register() and friends.
>>
>
>
> So it will move the lock acquisition to the opposite ( callee ) side 
> than mine.
>   At first, I tried to do that, but there are so many ...
>
> Anyway, your suggestion seems to be the best way if possible.
>
>
> One question: how about kvm_io_bus_[read|write] ?
>
> These are called from the emulator but I could not find where slots_lock
> are held though I can see the comments
>
>   "kvm_io_bus_[read|write] - called under kvm->slots_lock"
>

They're under srcu now, the comments are outdated.

We used to have slots_lock be a rwsem, taken for read or write as 
necessary.  Now we use srcu for read, and the slots_lock mutex + 
synchronize_srcu for write.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/3] KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP

[Takuya Yoshikawa]

(2010/12/16 18:34), Avi Kivity wrote:
>>> Maybe we ought to move slots_lock acquisition to kvm_io_bus_register() and friends.
>>>
>>
>>
>> So it will move the lock acquisition to the opposite ( callee ) side than mine.
>> At first, I tried to do that, but there are so many ...
>>
>> Anyway, your suggestion seems to be the best way if possible.
>>
>>
>> One question: how about kvm_io_bus_[read|write] ?
>>
>> These are called from the emulator but I could not find where slots_lock
>> are held though I can see the comments
>>
>> "kvm_io_bus_[read|write] - called under kvm->slots_lock"
>>
>
> They're under srcu now, the comments are outdated.
>
> We used to have slots_lock be a rwsem, taken for read or write as necessary. Now we use srcu for read, and the slots_lock mutex + synchronize_srcu for write.
>

Ah, would you mind updating the comments?
  - or just removing outdated ones?


I will reread these locking based on your answer!


Thanks,
   Takuya

Re: [PATCH 2/3] KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP

[Avi Kivity]

On 12/16/2010 11:47 AM, Takuya Yoshikawa wrote:
>> They're under srcu now, the comments are outdated.
>>
>> We used to have slots_lock be a rwsem, taken for read or write as 
>> necessary. Now we use srcu for read, and the slots_lock mutex + 
>> synchronize_srcu for write.
>>
>
>
> Ah, would you mind updating the comments?
>  - or just removing outdated ones?
>
>

Fixing the comments would be good.

> I will reread these locking based on your answer!
>

One day I'll write Documentation/kvm/locking.txt, so it could get 
outdated as well.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/3] KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP

[Takuya Yoshikawa]

Avi Kivity <avi@redhat.com> wrote:
> 
> One day I'll write Documentation/kvm/locking.txt, so it could get 
> outdated as well.
> 

I seemed to be in EOY(End Of Year) mode.

I remember why I could not move slots_lock acquisition to
kvm_io_bus_[un]register().

The locking order constraint forced me to take slots_lock
outside of irq_lock.

So there won't be much things to be done anymore except your
documentation!

Thanks,
  Takuya

[PATCH 3/3] KVM: Centralize irq_lock aquisition during KVM_CREATE_IRQCHIP

[Takuya Yoshikawa]

From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>

Move irq_lock aquisition from kvm_setup_default_irq_routing(), inside of
kvm_set_irq_routing(), to its caller.

This makes the lock management clearer, with a bit longer irq_lock section.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/ia64/kvm/kvm-ia64.c |    2 ++
 arch/x86/kvm/x86.c       |    4 ++--
 virt/kvm/irq_comm.c      |    2 --
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 060c594..34a1699 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -951,7 +951,9 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		mutex_unlock(&kvm->slots_lock);
 		if (r)
 			goto out;
+		mutex_lock(&kvm->irq_lock);
 		r = kvm_setup_default_irq_routing(kvm);
+		mutex_unlock(&kvm->irq_lock);
 		if (r) {
 			mutex_lock(&kvm->slots_lock);
 			kvm_ioapic_destroy(kvm);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 736ab93..5c70869 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3319,13 +3319,13 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		smp_wmb();
 		kvm->arch.vpic = vpic;
 		smp_wmb();
+		mutex_lock(&kvm->irq_lock);
 		r = kvm_setup_default_irq_routing(kvm);
 		if (r) {
-			mutex_lock(&kvm->irq_lock);
 			kvm_ioapic_destroy(kvm);
 			kvm_destroy_pic(kvm);
-			mutex_unlock(&kvm->irq_lock);
 		}
+		mutex_unlock(&kvm->irq_lock);
 	create_irqchip_unlock:
 		mutex_unlock(&kvm->slots_lock);
 		mutex_unlock(&kvm->lock);
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index 9f614b4..265fd2f 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -407,10 +407,8 @@ int kvm_set_irq_routing(struct kvm *kvm,
 		++ue;
 	}
 
-	mutex_lock(&kvm->irq_lock);
 	old = kvm->irq_routing;
 	kvm_irq_routing_update(kvm, new);
-	mutex_unlock(&kvm->irq_lock);
 
 	synchronize_rcu();
 
-- 
1.7.1