[v10,1/2] KVM: make io_bus interface more robust

Message ID	20090707210843.21953.65864.stgit@dev.haskins.net (mailing list archive)
State	New, archived
Headers	show Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n67LANsq024259 for <patchwork-kvm@patchwork.kernel.org>; Tue, 7 Jul 2009 21:10:23 GMT From: Gregory Haskins <ghaskins@novell.com> Subject: [KVM PATCH v10 1/2] KVM: make io_bus interface more robust To: kvm@vger.kernel.org Cc: linux-kernel@vger.kernel.org, mst@redhat.com, avi@redhat.com Date: Tue, 07 Jul 2009 17:08:44 -0400 Message-ID: <20090707210843.21953.65864.stgit@dev.haskins.net> In-Reply-To: <20090707210023.21953.22087.stgit@dev.haskins.net> References: <20090707210023.21953.22087.stgit@dev.haskins.net> User-Agent: StGIT/0.14.3 MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: kvm-owner@vger.kernel.org Precedence: bulk

Message ID

20090707210843.21953.65864.stgit@dev.haskins.net (mailing list archive)

State

New, archived

Headers

From: Gregory Haskins <ghaskins@novell.com>
Subject: [KVM PATCH v10 1/2] KVM: make io_bus interface more robust
To: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, mst@redhat.com, avi@redhat.com
Date: Tue, 07 Jul 2009 17:08:44 -0400
Message-ID: <20090707210843.21953.65864.stgit@dev.haskins.net>
In-Reply-To: <20090707210023.21953.22087.stgit@dev.haskins.net>
References: <20090707210023.21953.22087.stgit@dev.haskins.net>
User-Agent: StGIT/0.14.3
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Sender: kvm-owner@vger.kernel.org
Precedence: bulk

Commit Message

Gregory Haskins July 7, 2009, 9:08 p.m. UTC

Today kvm_io_bus_regsiter_dev() returns void and will internally BUG_ON
if it fails.  We want to create dynamic MMIO/PIO entries driven from
userspace later in the series, so we need to enhance the code to be more
robust with the following changes:

   1) Add a return value to the registration function
   2) Fix up all the callsites to check the return code, handle any
      failures, and percolate the error up to the caller.
   3) Add an unregister function that collapses holes in the array

Signed-off-by: Gregory Haskins <ghaskins@novell.com>
---

 arch/x86/kvm/i8254.c      |   20 ++++++++++++++++++--
 arch/x86/kvm/i8259.c      |    9 ++++++++-
 include/linux/kvm_host.h  |   10 +++++++---
 virt/kvm/coalesced_mmio.c |    8 ++++++--
 virt/kvm/ioapic.c         |    8 ++++++--
 virt/kvm/kvm_main.c       |   39 ++++++++++++++++++++++++++++++++++-----
 6 files changed, 79 insertions(+), 15 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Michael S. Tsirkin July 9, 2009, 6:22 p.m. UTC | #1

Not worth respinning for, but if you do generate v11 ...

On Tue, Jul 07, 2009 at 05:08:44PM -0400, Gregory Haskins wrote:
> Today kvm_io_bus_regsiter_dev() returns void and will internally BUG_ON
> if it fails.  We want to create dynamic MMIO/PIO entries driven from
> userspace later in the series, so we need to enhance the code to be more
> robust with the following changes:
> 
>    1) Add a return value to the registration function
>    2) Fix up all the callsites to check the return code, handle any
>       failures, and percolate the error up to the caller.
>    3) Add an unregister function that collapses holes in the array
> 
> Signed-off-by: Gregory Haskins <ghaskins@novell.com>
> ---
> 
>  arch/x86/kvm/i8254.c      |   20 ++++++++++++++++++--
>  arch/x86/kvm/i8259.c      |    9 ++++++++-
>  include/linux/kvm_host.h  |   10 +++++++---
>  virt/kvm/coalesced_mmio.c |    8 ++++++--
>  virt/kvm/ioapic.c         |    8 ++++++--
>  virt/kvm/kvm_main.c       |   39 ++++++++++++++++++++++++++++++++++-----
>  6 files changed, 79 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index 8c3ac30..4441355 100644
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -591,6 +591,7 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
>  {
>  	struct kvm_pit *pit;
>  	struct kvm_kpit_state *pit_state;
> +	int ret;
>  
>  	pit = kzalloc(sizeof(struct kvm_pit), GFP_KERNEL);
>  	if (!pit)
> @@ -625,14 +626,29 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
>  	kvm_register_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
>  
>  	kvm_iodevice_init(&pit->dev, &pit_dev_ops);
> -	__kvm_io_bus_register_dev(&kvm->pio_bus, &pit->dev);
> +	ret = __kvm_io_bus_register_dev(&kvm->pio_bus, &pit->dev);
> +	if (ret < 0)
> +		goto fail;
>  
>  	if (flags & KVM_PIT_SPEAKER_DUMMY) {
>  		kvm_iodevice_init(&pit->speaker_dev, &speaker_dev_ops);
> -		__kvm_io_bus_register_dev(&kvm->pio_bus, &pit->speaker_dev);
> +		ret = __kvm_io_bus_register_dev(&kvm->pio_bus,
> +						&pit->speaker_dev);
> +		if (ret < 0)
> +			goto fail_unregister;
>  	}
>  
>  	return pit;
> +
> +fail_unregister:
> +	__kvm_io_bus_unregister_dev(&kvm->pio_bus, &pit->dev);
> +
> +fail:
> +	if (pit->irq_source_id >= 0)
> +		kvm_free_irq_source_id(kvm, pit->irq_source_id);
> +
> +	kfree(pit);
> +	return NULL;
>  }
>  
>  void kvm_free_pit(struct kvm *kvm)
> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index 1d1bb75..670e426 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -536,6 +536,8 @@ static const struct kvm_io_device_ops picdev_ops = {
>  struct kvm_pic *kvm_create_pic(struct kvm *kvm)
>  {
>  	struct kvm_pic *s;
> +	int ret;
> +
>  	s = kzalloc(sizeof(struct kvm_pic), GFP_KERNEL);
>  	if (!s)
>  		return NULL;
> @@ -552,6 +554,11 @@ struct kvm_pic *kvm_create_pic(struct kvm *kvm)
>  	 * Initialize PIO device
>  	 */
>  	kvm_iodevice_init(&s->dev, &picdev_ops);
> -	kvm_io_bus_register_dev(kvm, &kvm->pio_bus, &s->dev);
> +	ret = kvm_io_bus_register_dev(kvm, &kvm->pio_bus, &s->dev);
> +	if (ret < 0) {
> +		kfree(s);
> +		return NULL;
> +	}
> +
>  	return s;
>  }
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 8e04a34..306bc67 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -64,10 +64,14 @@ int kvm_io_bus_write(struct kvm_io_bus *bus, gpa_t addr, int len,
>  		     const void *val);
>  int kvm_io_bus_read(struct kvm_io_bus *bus, gpa_t addr, int len,
>  		    void *val);
> -void __kvm_io_bus_register_dev(struct kvm_io_bus *bus,
> +int __kvm_io_bus_register_dev(struct kvm_io_bus *bus,
> +			       struct kvm_io_device *dev);
> +int kvm_io_bus_register_dev(struct kvm *kvm, struct kvm_io_bus *bus,
> +			    struct kvm_io_device *dev);
> +void __kvm_io_bus_unregister_dev(struct kvm_io_bus *bus,
> +				 struct kvm_io_device *dev);
> +void kvm_io_bus_unregister_dev(struct kvm *kvm, struct kvm_io_bus *bus,
>  			       struct kvm_io_device *dev);
> -void kvm_io_bus_register_dev(struct kvm *kvm, struct kvm_io_bus *bus,
> -			     struct kvm_io_device *dev);
>  
>  struct kvm_vcpu {
>  	struct kvm *kvm;
> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
> index 0352f81..04d69cd 100644
> --- a/virt/kvm/coalesced_mmio.c
> +++ b/virt/kvm/coalesced_mmio.c
> @@ -92,6 +92,7 @@ static const struct kvm_io_device_ops coalesced_mmio_ops = {
>  int kvm_coalesced_mmio_init(struct kvm *kvm)
>  {
>  	struct kvm_coalesced_mmio_dev *dev;
> +	int ret;
>  
>  	dev = kzalloc(sizeof(struct kvm_coalesced_mmio_dev), GFP_KERNEL);
>  	if (!dev)
> @@ -100,9 +101,12 @@ int kvm_coalesced_mmio_init(struct kvm *kvm)
>  	kvm_iodevice_init(&dev->dev, &coalesced_mmio_ops);
>  	dev->kvm = kvm;
>  	kvm->coalesced_mmio_dev = dev;
> -	kvm_io_bus_register_dev(kvm, &kvm->mmio_bus, &dev->dev);
>  
> -	return 0;
> +	ret = kvm_io_bus_register_dev(kvm, &kvm->mmio_bus, &dev->dev);
> +	if (ret < 0)
> +		kfree(dev);
> +
> +	return ret;
>  }
>  
>  int kvm_vm_ioctl_register_coalesced_mmio(struct kvm *kvm,
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 92496ff..048836d 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -340,6 +340,7 @@ static const struct kvm_io_device_ops ioapic_mmio_ops = {
>  int kvm_ioapic_init(struct kvm *kvm)
>  {
>  	struct kvm_ioapic *ioapic;
> +	int ret;
>  
>  	ioapic = kzalloc(sizeof(struct kvm_ioapic), GFP_KERNEL);
>  	if (!ioapic)
> @@ -348,7 +349,10 @@ int kvm_ioapic_init(struct kvm *kvm)
>  	kvm_ioapic_reset(ioapic);
>  	kvm_iodevice_init(&ioapic->dev, &ioapic_mmio_ops);
>  	ioapic->kvm = kvm;
> -	kvm_io_bus_register_dev(kvm, &kvm->mmio_bus, &ioapic->dev);
> -	return 0;
> +	ret = kvm_io_bus_register_dev(kvm, &kvm->mmio_bus, &ioapic->dev);
> +	if (ret < 0)
> +		kfree(ioapic);
> +
> +	return ret;
>  }
>  
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 05b6bc7..dd92b44 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2533,21 +2533,50 @@ int kvm_io_bus_read(struct kvm_io_bus *bus, gpa_t addr, int len, void *val)
>  	return -EOPNOTSUPP;
>  }
>  
> -void kvm_io_bus_register_dev(struct kvm *kvm, struct kvm_io_bus *bus,
> +int kvm_io_bus_register_dev(struct kvm *kvm, struct kvm_io_bus *bus,
>  			     struct kvm_io_device *dev)
>  {
> +	int ret;
> +
>  	down_write(&kvm->slots_lock);
> -	__kvm_io_bus_register_dev(bus, dev);
> +	ret = __kvm_io_bus_register_dev(bus, dev);
>  	up_write(&kvm->slots_lock);
> +
> +	return ret;
>  }
>  
>  /* An unlocked version. Caller must have write lock on slots_lock. */
> -void __kvm_io_bus_register_dev(struct kvm_io_bus *bus,
> -			     struct kvm_io_device *dev)
> +int __kvm_io_bus_register_dev(struct kvm_io_bus *bus,
> +			      struct kvm_io_device *dev)
>  {
> -	BUG_ON(bus->dev_count > (NR_IOBUS_DEVS-1));
> +	if (bus->dev_count > NR_IOBUS_DEVS-1)

Make this NR_IOBUS_DEVS - 1 (spaces around -)

> +		return -ENOSPC;
>  
>  	bus->devs[bus->dev_count++] = dev;
> +
> +	return 0;
> +}
> +
> +void kvm_io_bus_unregister_dev(struct kvm *kvm,
> +			       struct kvm_io_bus *bus,
> +			       struct kvm_io_device *dev)
> +{
> +	down_write(&kvm->slots_lock);
> +	__kvm_io_bus_unregister_dev(bus, dev);
> +	up_write(&kvm->slots_lock);
> +}
> +
> +/* An unlocked version. Caller must have write lock on slots_lock. */
> +void __kvm_io_bus_unregister_dev(struct kvm_io_bus *bus,
> +				 struct kvm_io_device *dev)
> +{
> +	int i;
> +
> +	for (i = 0; i < bus->dev_count; i++)
> +		if (bus->devs[i] == dev) {
> +			bus->devs[i] = bus->devs[--bus->dev_count];
> +			break;
> +		}
>  }
>  
>  static struct notifier_block kvm_cpu_notifier = {
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index 8c3ac30..4441355 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -591,6 +591,7 @@  struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
 {
 	struct kvm_pit *pit;
 	struct kvm_kpit_state *pit_state;
+	int ret;
 
 	pit = kzalloc(sizeof(struct kvm_pit), GFP_KERNEL);
 	if (!pit)
@@ -625,14 +626,29 @@  struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
 	kvm_register_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
 
 	kvm_iodevice_init(&pit->dev, &pit_dev_ops);
-	__kvm_io_bus_register_dev(&kvm->pio_bus, &pit->dev);
+	ret = __kvm_io_bus_register_dev(&kvm->pio_bus, &pit->dev);
+	if (ret < 0)
+		goto fail;
 
 	if (flags & KVM_PIT_SPEAKER_DUMMY) {
 		kvm_iodevice_init(&pit->speaker_dev, &speaker_dev_ops);
-		__kvm_io_bus_register_dev(&kvm->pio_bus, &pit->speaker_dev);
+		ret = __kvm_io_bus_register_dev(&kvm->pio_bus,
+						&pit->speaker_dev);
+		if (ret < 0)
+			goto fail_unregister;
 	}
 
 	return pit;
+
+fail_unregister:
+	__kvm_io_bus_unregister_dev(&kvm->pio_bus, &pit->dev);
+
+fail:
+	if (pit->irq_source_id >= 0)
+		kvm_free_irq_source_id(kvm, pit->irq_source_id);
+
+	kfree(pit);
+	return NULL;
 }
 
 void kvm_free_pit(struct kvm *kvm)
diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 1d1bb75..670e426 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -536,6 +536,8 @@  static const struct kvm_io_device_ops picdev_ops = {
 struct kvm_pic *kvm_create_pic(struct kvm *kvm)
 {
 	struct kvm_pic *s;
+	int ret;
+
 	s = kzalloc(sizeof(struct kvm_pic), GFP_KERNEL);
 	if (!s)
 		return NULL;
@@ -552,6 +554,11 @@  struct kvm_pic *kvm_create_pic(struct kvm *kvm)
 	 * Initialize PIO device
 	 */
 	kvm_iodevice_init(&s->dev, &picdev_ops);
-	kvm_io_bus_register_dev(kvm, &kvm->pio_bus, &s->dev);
+	ret = kvm_io_bus_register_dev(kvm, &kvm->pio_bus, &s->dev);
+	if (ret < 0) {
+		kfree(s);
+		return NULL;
+	}
+
 	return s;
 }
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 8e04a34..306bc67 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -64,10 +64,14 @@  int kvm_io_bus_write(struct kvm_io_bus *bus, gpa_t addr, int len,
 		     const void *val);
 int kvm_io_bus_read(struct kvm_io_bus *bus, gpa_t addr, int len,
 		    void *val);
-void __kvm_io_bus_register_dev(struct kvm_io_bus *bus,
+int __kvm_io_bus_register_dev(struct kvm_io_bus *bus,
+			       struct kvm_io_device *dev);
+int kvm_io_bus_register_dev(struct kvm *kvm, struct kvm_io_bus *bus,
+			    struct kvm_io_device *dev);
+void __kvm_io_bus_unregister_dev(struct kvm_io_bus *bus,
+				 struct kvm_io_device *dev);
+void kvm_io_bus_unregister_dev(struct kvm *kvm, struct kvm_io_bus *bus,
 			       struct kvm_io_device *dev);
-void kvm_io_bus_register_dev(struct kvm *kvm, struct kvm_io_bus *bus,
-			     struct kvm_io_device *dev);
 
 struct kvm_vcpu {
 	struct kvm *kvm;
diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
index 0352f81..04d69cd 100644
--- a/virt/kvm/coalesced_mmio.c
+++ b/virt/kvm/coalesced_mmio.c
@@ -92,6 +92,7 @@  static const struct kvm_io_device_ops coalesced_mmio_ops = {
 int kvm_coalesced_mmio_init(struct kvm *kvm)
 {
 	struct kvm_coalesced_mmio_dev *dev;
+	int ret;
 
 	dev = kzalloc(sizeof(struct kvm_coalesced_mmio_dev), GFP_KERNEL);
 	if (!dev)
@@ -100,9 +101,12 @@  int kvm_coalesced_mmio_init(struct kvm *kvm)
 	kvm_iodevice_init(&dev->dev, &coalesced_mmio_ops);
 	dev->kvm = kvm;
 	kvm->coalesced_mmio_dev = dev;
-	kvm_io_bus_register_dev(kvm, &kvm->mmio_bus, &dev->dev);
 
-	return 0;
+	ret = kvm_io_bus_register_dev(kvm, &kvm->mmio_bus, &dev->dev);
+	if (ret < 0)
+		kfree(dev);
+
+	return ret;
 }
 
 int kvm_vm_ioctl_register_coalesced_mmio(struct kvm *kvm,
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 92496ff..048836d 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -340,6 +340,7 @@  static const struct kvm_io_device_ops ioapic_mmio_ops = {
 int kvm_ioapic_init(struct kvm *kvm)
 {
 	struct kvm_ioapic *ioapic;
+	int ret;
 
 	ioapic = kzalloc(sizeof(struct kvm_ioapic), GFP_KERNEL);
 	if (!ioapic)
@@ -348,7 +349,10 @@  int kvm_ioapic_init(struct kvm *kvm)
 	kvm_ioapic_reset(ioapic);
 	kvm_iodevice_init(&ioapic->dev, &ioapic_mmio_ops);
 	ioapic->kvm = kvm;
-	kvm_io_bus_register_dev(kvm, &kvm->mmio_bus, &ioapic->dev);
-	return 0;
+	ret = kvm_io_bus_register_dev(kvm, &kvm->mmio_bus, &ioapic->dev);
+	if (ret < 0)
+		kfree(ioapic);
+
+	return ret;
 }
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 05b6bc7..dd92b44 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2533,21 +2533,50 @@  int kvm_io_bus_read(struct kvm_io_bus *bus, gpa_t addr, int len, void *val)
 	return -EOPNOTSUPP;
 }
 
-void kvm_io_bus_register_dev(struct kvm *kvm, struct kvm_io_bus *bus,
+int kvm_io_bus_register_dev(struct kvm *kvm, struct kvm_io_bus *bus,
 			     struct kvm_io_device *dev)
 {
+	int ret;
+
 	down_write(&kvm->slots_lock);
-	__kvm_io_bus_register_dev(bus, dev);
+	ret = __kvm_io_bus_register_dev(bus, dev);
 	up_write(&kvm->slots_lock);
+
+	return ret;
 }
 
 /* An unlocked version. Caller must have write lock on slots_lock. */
-void __kvm_io_bus_register_dev(struct kvm_io_bus *bus,
-			     struct kvm_io_device *dev)
+int __kvm_io_bus_register_dev(struct kvm_io_bus *bus,
+			      struct kvm_io_device *dev)
 {
-	BUG_ON(bus->dev_count > (NR_IOBUS_DEVS-1));
+	if (bus->dev_count > NR_IOBUS_DEVS-1)
+		return -ENOSPC;
 
 	bus->devs[bus->dev_count++] = dev;
+
+	return 0;
+}
+
+void kvm_io_bus_unregister_dev(struct kvm *kvm,
+			       struct kvm_io_bus *bus,
+			       struct kvm_io_device *dev)
+{
+	down_write(&kvm->slots_lock);
+	__kvm_io_bus_unregister_dev(bus, dev);
+	up_write(&kvm->slots_lock);
+}
+
+/* An unlocked version. Caller must have write lock on slots_lock. */
+void __kvm_io_bus_unregister_dev(struct kvm_io_bus *bus,
+				 struct kvm_io_device *dev)
+{
+	int i;
+
+	for (i = 0; i < bus->dev_count; i++)
+		if (bus->devs[i] == dev) {
+			bus->devs[i] = bus->devs[--bus->dev_count];
+			break;
+		}
 }
 
 static struct notifier_block kvm_cpu_notifier = {

[v10,1/2] KVM: make io_bus interface more robust

Commit Message

Comments

Patch