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

[Jason Wang <jasowang@redhat.com>]

On 08/24/2015 10:05 PM, Cornelia Huck wrote:
> On Mon, 24 Aug 2015 11:29:29 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> 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:
>>>> @@ -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.
> I don't think len != 8 makes much sense for the way ioeventfd is
> defined for ccw (we handle hypercalls with a payload specifying the
> device), but we currently don't actively fence it.
>
> But regardless, I'd prefer to decide directly upon whether userspace
> actually tried to register for the mmio bus.

Ok.

>
>>>>  		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).
> Do you need to handle the ioeventfd_count changes on the fast mmio bus
> as well?

Yes. So actually, it needs some changes: checking the return value of
kvm_io_bus_unregister_dev() and decide which bus does the device belongs to.

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