Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933191AbbHXOFo (ORCPT ); Mon, 24 Aug 2015 10:05:44 -0400 Received: from e06smtp15.uk.ibm.com ([195.75.94.111]:58680 "EHLO e06smtp15.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932184AbbHXOFn (ORCPT ); Mon, 24 Aug 2015 10:05:43 -0400 X-Helo: d06dlp01.portsmouth.uk.ibm.com X-MailFrom: cornelia.huck@de.ibm.com X-RcptTo: linux-kernel@vger.kernel.org Date: Mon, 24 Aug 2015 16:05:20 +0200 From: Cornelia Huck To: Jason Wang 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 Message-ID: <20150824160520.583c3a74.cornelia.huck@de.ibm.com> In-Reply-To: <55DA8F99.106@redhat.com> 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> <55DA8F99.106@redhat.com> Organization: IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz =?UTF-8?B?R2VzY2jDpGZ0c2bDvGhydW5nOg==?= Dirk Wittkopp Sitz der Gesellschaft: =?UTF-8?B?QsO2Ymxpbmdlbg==?= Registergericht: Amtsgericht Stuttgart, HRB 243294 X-Mailer: Claws Mail 3.8.0 (GTK+ 2.24.10; i686-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15082414-0021-0000-0000-0000050F6EDB Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2943 Lines: 81 On Mon, 24 Aug 2015 11:29:29 +0800 Jason Wang wrote: > On 08/21/2015 05:29 PM, Cornelia Huck wrote: > > On Fri, 21 Aug 2015 16:03:52 +0800 > > Jason Wang 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 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/