From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: KVM: PCIPT: direct mmio Date: Thu, 05 Jun 2008 16:20:09 +0300 Message-ID: <4847E809.1070607@qumranet.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Muli Ben-Yehuda , Amit Shah To: Ben-Ami Yassour Return-path: Received: from il.qumranet.com ([212.179.150.194]:21116 "EHLO il.qumranet.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754202AbYFENUO (ORCPT ); Thu, 5 Jun 2008 09:20:14 -0400 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: Ben-Ami Yassour wrote: > >>> + if (len > 0) { >>> + ptr = mmap(NULL, len, prot, MAP_ANONYMOUS | MAP_SHARED, -1, >>> 0); >>> + if (ptr == MAP_FAILED) { >>> + fprintf(stderr, "create_userspace_phys_mem: %s", >>> + strerror(errno)); >>> + return 0; >>> + } >> >> You're using 'len == 0' here to change the semantics of the function. >> It would be better to have two different APIs (perhaps sharing some of >> the implementation by calling a helper). >> > > Actually, this is a fix of a bug that is probably exposed only by the > direct mmio code. > Here is the problem (in the existing code): > kvm_destroy_phys_mem calls kvm_create_phys_mem(kvm, phys_start, 0, 0, 0); > kvm_create_phys_mem calls kvm_create_userspace_phys_mem which is calling > > mmap(NULL, len, prot, MAP_ANONYMOUS | MAP_SHARED, -1, 0); > if (ptr == MAP_FAILED) > > now, if len = 0 it fails. > What's the point of a zero length memslot? I seem to remember this conversation in the past. > We could have sent is as a separate patch, > and we could have made more changes to fix it differently, > for example, not to let kvm_destroy_phys_mem call kvm_create_phys_mem > (which seems strange in general...). Yeah. Perhaps a common helper. > We wanted to minimize the amount of changes that we make, so we choose > this option. > > What do you recommend? Send patches that fix the current broken code and prepare the way for the real changes. This way its easy to review both the fixes to the existing code and the new functionality. -- error compiling committee.c: too many arguments to function