| Message ID | 20090706203315.14222.25490.stgit@dev.haskins.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Looks good to me. One thing that's kind of ugly is the cleanup in i8254, see below. And a couple of other style comments. On Mon, Jul 06, 2009 at 04:33:15PM -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 | 22 ++++++++++++++++++++-- > 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 | 41 ++++++++++++++++++++++++++++++++++++----- > 6 files changed, 83 insertions(+), 15 deletions(-) > > diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c > index 8c3ac30..298312d 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,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); The above works because we scan the whole array; so it's safe to call unregister on a device that we didn't register, and even on a device we didn't init. But IMO it's cleaner not to assume this and do cleanup properly. No? > + > + 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) { I thought the function returns 0 on success? If so can we just if (ret) all over? > + kfree(s); > + return NULL; > + } > + kill empty line > 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); > + kill empty line > + 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); kill empty line > + > + return ret; > } > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 05b6bc7..11595c7 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -2533,21 +2533,52 @@ 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) Let's document return value? > { > + 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); kill empty line? this one is kind of iffy > + > + 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)) as long as we are touching this: (NR_IOBUS_DEVS-1) -> NR_IOBUS_DEVS - 1? > + return -ENOSPC; > > bus->devs[bus->dev_count++] = dev; kill empty line > + > + 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++) { > + kill empty line > + if (bus->devs[i] == dev) { > + bus->devs[i] = bus->devs[--bus->dev_count]; > + break; > + } > + } no {} around single statement > } > > 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
Michael S. Tsirkin wrote: > Looks good to me. One thing that's kind of ugly is the cleanup in i8254, > see below. And a couple of other style comments. > > On Mon, Jul 06, 2009 at 04:33:15PM -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 | 22 ++++++++++++++++++++-- >> 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 | 41 ++++++++++++++++++++++++++++++++++++----- >> 6 files changed, 83 insertions(+), 15 deletions(-) >> >> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c >> index 8c3ac30..298312d 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,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); >> > > The above works because we scan the whole array; so it's safe to call > unregister on a device that we didn't register, and even on a device we > didn't init. But IMO it's cleaner not to assume this and do > cleanup properly. No? > Ack (this was a left-over from when the code was structured differently) > >> + >> + 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; >> + >> [A] >> 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) { >> > > I thought the function returns 0 on success? > If so can we just if (ret) all over? > > I guess, but what does that churn buy us? >> + kfree(s); >> + return NULL; >> + } >> + >> > > kill empty line > Are they docking your pay for every whitespace that goes into KVM or something ;) These patches pass checkpatch.pl and I happen to like the extra whitespace for readability. I agree that a random isolated whitespace hunk, or double whitespace in a row are probably inadvertent and should be pointed out. But these little one liners in the middle of code I generally do on purpose (for instance, [A]). I suppose its personal preference either way, so I guess unless Avi objects lets just each have our own style in that regard. > >> 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; >> [B] >> + ret = kvm_io_bus_register_dev(kvm, &kvm->mmio_bus, &dev->dev); >> + if (ret < 0) >> + kfree(dev); >> + >> > > kill empty line > Why do you object here especially when there is precedence with something like the space before the return with [B]? I think big mono-blocks of code are ugly and harder to read, personally. > >> + 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); >> > > kill empty line > Ditto for all of these. > >> + >> + return ret; >> } >> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >> index 05b6bc7..11595c7 100644 >> --- a/virt/kvm/kvm_main.c >> +++ b/virt/kvm/kvm_main.c >> @@ -2533,21 +2533,52 @@ 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) >> > > Let's document return value? > Ok > >> { >> + 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); >> > > kill empty line? this one is kind of iffy > > >> + >> + 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)) >> > > as long as we are touching this: (NR_IOBUS_DEVS-1) -> NR_IOBUS_DEVS - 1? > > >> + return -ENOSPC; >> >> bus->devs[bus->dev_count++] = dev; >> > > kill empty line > > >> + >> + 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++) { >> + >> > > kill empty line > > >> + if (bus->devs[i] == dev) { >> + bus->devs[i] = bus->devs[--bus->dev_count]; >> + break; >> + } >> + } >> > > no {} around single statement > > > >> } >> >> static struct notifier_block kvm_cpu_notifier = { >>
On Tue, Jul 07, 2009 at 01:26:11PM -0400, Gregory Haskins wrote: > These patches pass checkpatch.pl and I happen to like the extra > whitespace for readability. I agree that a random isolated whitespace > hunk, or double whitespace in a row are probably inadvertent and should > be pointed out. But these little one liners in the middle of code I > generally do on purpose (for instance, [A]). Where it's on purpose, it's on purpose. I am just trying to convey a rather obvious point that each line of code should have a purpose in life, and that includes empty lines :) > I suppose its personal preference either way, so I guess unless Avi > objects lets just each have our own style in that regard. I think Avi already said we don't need to standardize everything. I hope at least some of the comments were helpful. > >> @@ -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) { > >> > > > > I thought the function returns 0 on success? > > If so can we just if (ret) all over? > > > > > > I guess, but what does that churn buy us? It's not that important really. I think we need to document the return value, and check it according to how it is documented. The reason I commented, I see < 0 and ask "what if it is > 0"? I look it up and it turns out it's never > 0. So why check < 0? > >> + ret = kvm_io_bus_register_dev(kvm, &kvm->mmio_bus, &dev->dev); > >> + if (ret < 0) > >> + kfree(dev); > >> + > >> > > > > kill empty line > > > > Why do you object here especially when there is precedence How do you mean, precedence? > with > something like the space before the return with [B]? I think big > mono-blocks of code are ugly and harder to read, personally. I don't intend to keep arguing and I agree it's a question of style. But since you ask why I'll try to answer. I think an empty line should help delimit a block of code with some common meaning, like a paragraph. But if overused it loses meaning and stops being helpful. E.g., it does not make sense to put it between every 2 lines of code in a function. It also does not make sense to put it after } which is already delimiting a block in a visible way; it does not make sense to put it around a multiline comment which is delimited by /**/. It does not make sense to put it around an idented block which is already delimited by indentation. > >> + if (bus->devs[i] == dev) { > >> + bus->devs[i] = bus->devs[--bus->dev_count]; > >> + break; > >> + } > >> + } > >> > > > > no {} around single statement This is actually part of style IMO, it is just hard for a perl script to catch :). > > > > > >> } > >> > >> 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
Michael S. Tsirkin wrote: > On Tue, Jul 07, 2009 at 01:26:11PM -0400, Gregory Haskins wrote: > > >> I suppose its personal preference either way, so I guess unless Avi >> objects lets just each have our own style in that regard. >> > > I think Avi already said we don't need to standardize everything. > I hope at least some of the comments were helpful. > > Yes, it was very helpful and I really *do* appreciate the time you take to review the code. I think you will see that aside from the things we agree are whitespace style, I addressed your feedback in v10. The one notable omission (and this was not intentional) was that I forgot to document the return value like you requested. If a v11 is needed after review comments, I will update it then. Otherwise, Ill just submit a trivial doc patch after v10 is accepted. Thanks again, Michael, -Greg
diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c index 8c3ac30..298312d 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,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 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..11595c7 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2533,21 +2533,52 @@ 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 = {
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 | 22 ++++++++++++++++++++-- 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 | 41 ++++++++++++++++++++++++++++++++++++----- 6 files changed, 83 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