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@xxxxxxxxxx>
>> ---
>>
>> 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 = {
>>
Attachment:
signature.asc
Description: OpenPGP digital signature
