Re: [KVM PATCH v8 1/3] KVM: make io_bus interface more robust

[Gregory Haskins]

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@xxxxxxxxxx>
>   

Hi Avi,
  While the disposition on the iosignalfd patch w.r.t. current proposals
by Michael is up in the air, this patch has not had and comments in a
while.  Please at least consider this one for merging ASAP so we can
base some of the other io_bus cleanups on it.

Thanks,
-Greg

> ---
>
>  arch/x86/kvm/i8254.c      |   22 ++++++++++++++++++++--
>  arch/x86/kvm/i8259.c      |    9 ++++++++-
>  include/linux/kvm_host.h  |    6 ++++--
>  virt/kvm/coalesced_mmio.c |    8 ++++++--
>  virt/kvm/ioapic.c         |    9 +++++++--
>  virt/kvm/kvm_main.c       |   23 +++++++++++++++++++++--
>  6 files changed, 66 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index 331705f..1c41715 100644
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -586,6 +586,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)
> @@ -620,14 +621,31 @@ 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;
>  	}
>  
>  	return pit;
> +
> +fail:
> +	if (flags & KVM_PIT_SPEAKER_DUMMY)
> +		kvm_io_bus_unregister_dev(&kvm->pio_bus, &pit->speaker_dev);
> +
> +	kvm_io_bus_unregister_dev(&kvm->pio_bus, &pit->dev);
> +
> +	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 148c52a..66e37c2 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -532,6 +532,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;
> @@ -548,6 +550,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->pio_bus, &s->dev);
> +	ret = kvm_io_bus_register_dev(&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 e4e78db..eafa2b3 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -61,8 +61,10 @@ void kvm_io_bus_init(struct kvm_io_bus *bus);
>  void kvm_io_bus_destroy(struct kvm_io_bus *bus);
>  struct kvm_io_device *kvm_io_bus_find_dev(struct kvm_io_bus *bus,
>  					  gpa_t addr, int len, int is_write);
> -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);
> +void kvm_io_bus_unregister_dev(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 397f419..3e89db8 100644
> --- a/virt/kvm/coalesced_mmio.c
> +++ b/virt/kvm/coalesced_mmio.c
> @@ -94,6 +94,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)
> @@ -102,9 +103,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->mmio_bus, &dev->dev);
>  
> -	return 0;
> +	ret = kvm_io_bus_register_dev(&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 d8b2eca..28adb05 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -335,6 +335,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)
> @@ -343,7 +344,11 @@ 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->mmio_bus, &ioapic->dev);
> -	return 0;
> +
> +	ret = kvm_io_bus_register_dev(&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 9fab08e..6bd71f7 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2481,11 +2481,30 @@ struct kvm_io_device *kvm_io_bus_find_dev(struct kvm_io_bus *bus,
>  	return NULL;
>  }
>  
> -void kvm_io_bus_register_dev(struct kvm_io_bus *bus, struct kvm_io_device *dev)
> +/* assumes kvm->lock held */
> +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;
> +}
> +
> +/* assumes kvm->lock held */
> +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];
> +			return;
> +		}
> +	}
>  }
>  
>  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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Attachment: signature.asc
Description: OpenPGP digital signature