[Restoring Davide's proper email. I had a typo in the original v4
announcement. Sorry Davide]
Avi Kivity wrote:
> 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) Refactor io_bus to allow "holes" in the array so device hotplug
>> can add/remove devices arbitrarily.
>> 4) Add an unregister function
>>
>> Signed-off-by: Gregory Haskins <ghaskins@xxxxxxxxxx>
>> ---
>>
>> arch/x86/kvm/i8254.c | 34 ++++++++++++++++++++++---------
>> arch/x86/kvm/i8259.c | 9 +++++++-
>> include/linux/kvm_host.h | 8 +++++--
>> virt/kvm/coalesced_mmio.c | 8 ++++++-
>> virt/kvm/ioapic.c | 9 ++++++--
>> virt/kvm/kvm_main.c | 49
>> +++++++++++++++++++++++++++++++++++++++------
>> 6 files changed, 92 insertions(+), 25 deletions(-)
>>
>> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
>> index 584e3d3..6cf84d4 100644
>> --- a/arch/x86/kvm/i8254.c
>> +++ b/arch/x86/kvm/i8254.c
>> @@ -564,36 +564,40 @@ 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)
>> return NULL;
>>
>> pit->irq_source_id = kvm_request_irq_source_id(kvm);
>> - if (pit->irq_source_id < 0) {
>> - kfree(pit);
>> - return NULL;
>> - }
>> -
>> - mutex_init(&pit->pit_state.lock);
>> - mutex_lock(&pit->pit_state.lock);
>> - spin_lock_init(&pit->pit_state.inject_lock);
>> + if (pit->irq_source_id < 0)
>> + goto fail;
>>
>> /* Initialize PIO device */
>> pit->dev.read = pit_ioport_read;
>> pit->dev.write = pit_ioport_write;
>> pit->dev.in_range = pit_in_range;
>> pit->dev.private = pit;
>> - 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) {
>> pit->speaker_dev.read = speaker_ioport_read;
>> pit->speaker_dev.write = speaker_ioport_write;
>> pit->speaker_dev.in_range = speaker_in_range;
>> pit->speaker_dev.private = pit;
>> - 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;
>> }
>>
>> + mutex_init(&pit->pit_state.lock);
>> + mutex_lock(&pit->pit_state.lock);
>> + spin_lock_init(&pit->pit_state.inject_lock);
>> +
>>
>
> You are registering the PIT before it is initialized. That exposes a
> race. The original code also did that, but at least the pit lock was
> held while this was done.
Doh! Will fix.
>
>> kvm->arch.vpit = pit;
>> pit->kvm = kvm;
>>
>> @@ -613,6 +617,16 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm,
>> u32 flags)
>> kvm_register_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
>>
>> return pit;
>> +
>> +fail:
>> + kvm_io_bus_unregister_dev(&kvm->pio_bus, &pit->speaker_dev);
>>
>
> There's an option now to avoid speaker_dev, so this needs to be
> conditional.
I was intentionally simple here based on the fact that the unregister
can silently/harmlessly fail. (Another scenario is that we never tried
to register the speaker_dev before we hit the fail: path.
>
>> + 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;
>> }
>>
>> @@ -52,7 +52,7 @@ extern struct kmem_cache *kvm_vcpu_cache;
>> * in one place.
>> */
>> struct kvm_io_bus {
>> - int dev_count;
>> + spinlock_t lock;
>> #define NR_IOBUS_DEVS 6
>> struct kvm_io_device *devs[NR_IOBUS_DEVS];
>> };
>> @@ -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);
>> +int kvm_io_bus_unregister_dev(struct kvm_io_bus *bus,
>> + struct kvm_io_device *dev);
>>
>>
>
> unregister() should return void. There's really nothing you can do to
> recover from a failure.
Yeah, good point.
>
>>
>> @@ -2453,21 +2455,54 @@ struct kvm_io_device
>> *kvm_io_bus_find_dev(struct kvm_io_bus *bus,
>> {
>> int i;
>>
>> - for (i = 0; i < bus->dev_count; i++) {
>> + for (i = 0; i < NR_IOBUS_DEVS; i++) {
>> struct kvm_io_device *pos = bus->devs[i];
>>
>> - if (pos->in_range(pos, addr, len, is_write))
>> + if (pos && pos->in_range(pos, addr, len, is_write))
>> return pos;
>> }
>>
>
> Let's keep dev_count, and just move things around on unregister.
Ok
-Greg
Attachment:
signature.asc
Description: OpenPGP digital signature
