Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932190AbbHXD3g (ORCPT ); Sun, 23 Aug 2015 23:29:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57188 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753803AbbHXD3f (ORCPT ); Sun, 23 Aug 2015 23:29:35 -0400 Message-ID: <55DA8F99.106@redhat.com> Date: Mon, 24 Aug 2015 11:29:29 +0800 From: Jason Wang User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: Cornelia Huck CC: gleb@kernel.org, pbonzini@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, "Michael S. Tsirkin" Subject: Re: [PATCH 2/3] kvm: don't register wildcard MMIO EVENTFD on two buses References: <1440144233-29748-1-git-send-email-jasowang@redhat.com> <1440144233-29748-2-git-send-email-jasowang@redhat.com> <20150821112946.64ae6f7b.cornelia.huck@de.ibm.com> In-Reply-To: <20150821112946.64ae6f7b.cornelia.huck@de.ibm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2970 Lines: 89 On 08/21/2015 05:29 PM, Cornelia Huck wrote: > On Fri, 21 Aug 2015 16:03:52 +0800 > Jason Wang 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); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/