Re: [PATCH 2/3] kvm: don't register wildcard MMIO EVENTFD on two buses

[Jason Wang <jasowang@redhat.com>]

On 08/21/2015 05:29 PM, Cornelia Huck wrote:
> On Fri, 21 Aug 2015 16:03:52 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>
>> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
>> index 9ff4193..834a409 100644
>> --- a/virt/kvm/eventfd.c
>> +++ b/virt/kvm/eventfd.c
>> @@ -838,11 +838,6 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>>  
>>  	kvm_iodevice_init(&p->dev, &ioeventfd_ops);
>>  
>> -	ret = kvm_io_bus_register_dev(kvm, bus_idx, p->addr, p->length,
>> -				      &p->dev);
>> -	if (ret < 0)
>> -		goto unlock_fail;
>> -
>>  	/* When length is ignored, MMIO is also put on a separate bus, for
>>  	 * faster lookups.
> You probably want to change this comment as well?

Yes.

>
>>  	 */
>> @@ -850,9 +845,15 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> Unfortunately snipped by diff, but the check here is on !len && !PIO,
> which only does the desired thing as VIRTIO_CCW always uses len == 8.
> Should the check be for !len && MMIO instead?

I think the answer depends on whether len == 0 is valid for ccw. If not
we can fail the assign earlier. Since even without this patch, if
userspace tries to register a dev with len equals to zero, it will also
be registered to KVM_FAST_MMIO_BUS. If yes, we need check as you
suggested here.

>
>>  		ret = kvm_io_bus_register_dev(kvm, KVM_FAST_MMIO_BUS,
>>  					      p->addr, 0, &p->dev);
>>  		if (ret < 0)
>> -			goto register_fail;
>> +			goto unlock_fail;
>> +	} else {
>> +		ret = kvm_io_bus_register_dev(kvm, bus_idx, p->addr, p->length,
>> +					      &p->dev);
>> +		if (ret < 0)
>> +			goto unlock_fail;
>>  	}
> Hm... maybe the following would be more obvious:
>
> my_bus = (p->length == 0) && (bus_idx == KVM_MMIO_BUS) ? KVM_FAST_MMIO_BUS : bus_idx;
> ret = kvm_io_bus_register_dev(kvm, my_bus, p->addr, p->length, &pdev->dev); 
>
>>  
>> +
>>  	kvm->buses[bus_idx]->ioeventfd_count++;
>>  	list_add_tail(&p->list, &kvm->ioeventfds);
> (...)
>
>> @@ -900,10 +899,11 @@ kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>>  		if (!p->wildcard && p->datamatch != args->datamatch)
>>  			continue;
>>  
>> -		kvm_io_bus_unregister_dev(kvm, bus_idx, &p->dev);
>>  		if (!p->length) {
>>  			kvm_io_bus_unregister_dev(kvm, KVM_FAST_MMIO_BUS,
>>  						  &p->dev);
>> +		} else {
>> +			kvm_io_bus_unregister_dev(kvm, bus_idx, &p->dev);
>>  		}
> Similar comments here... do you want to check for bus_idx ==
> KVM_MMIO_BUS as well?

Good catch. I think keep the original code as is will be also ok to
solve this. (with changing the bus_idx to KVM_FAST_MMIO_BUS during
registering if it was an wildcard mmio).

>
>>  		kvm->buses[bus_idx]->ioeventfd_count--;
>>  		ioeventfd_release(p);