On Mon, 24 Aug 2015 11:29:29 +0800
Jason Wang <jasowang@xxxxxxxxxx> wrote:
> On 08/21/2015 05:29 PM, Cornelia Huck wrote:
> > On Fri, 21 Aug 2015 16:03:52 +0800
> > Jason Wang <jasowang@xxxxxxxxxx> 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.
>
> >
> >> 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?
>
> >
> >> kvm->buses[bus_idx]->ioeventfd_count--;
> >> ioeventfd_release(p);
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
