From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 3/9] allow intersecting region to be on the boundary. Date: Tue, 23 Sep 2008 13:30:41 +0300 Message-ID: <48D8C551.6000207@redhat.com> References: <1221840506-22996-1-git-send-email-glommer@redhat.com> <1221840506-22996-4-git-send-email-glommer@redhat.com> <48D541D0.7090701@redhat.com> <20080922134804.GB3618@poweredge.glommer> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, aliguori@us.ibm.com To: Glauber Costa Return-path: Received: from mx2.redhat.com ([66.187.237.31]:56114 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750839AbYIWKaq (ORCPT ); Tue, 23 Sep 2008 06:30:46 -0400 In-Reply-To: <20080922134804.GB3618@poweredge.glommer> Sender: kvm-owner@vger.kernel.org List-ID: Glauber Costa wrote: > On Sat, Sep 20, 2008 at 11:32:48AM -0700, Avi Kivity wrote: > >> Glauber Costa wrote: >> >>> Signed-off-by: Glauber Costa >>> --- >>> libkvm/libkvm.c | 4 ++-- >>> 1 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c >>> index e768e44..fa65c30 100644 >>> --- a/libkvm/libkvm.c >>> +++ b/libkvm/libkvm.c >>> @@ -130,8 +130,8 @@ int get_intersecting_slot(unsigned long phys_addr) >>> int i; >>> for (i = 0; i < KVM_MAX_NUM_MEM_REGIONS ; ++i) >>> - if (slots[i].len && slots[i].phys_addr < phys_addr && >>> - (slots[i].phys_addr + slots[i].len) > phys_addr) >>> + if (slots[i].len && slots[i].phys_addr <= phys_addr && >>> + (slots[i].phys_addr + slots[i].len) >= phys_addr) >>> return i; >>> return -1; >>> >>> >> consider >> >> slots[i].phys_addr = 0 >> slots[i].len = 1 >> phys_addr = 1 >> >> with the new calculation, i (well, not me personally) will be considered >> an intersecting slot. >> >> Not that I (me this time) can understand how you can calculate interval >> intersection without the entire interval. >> > would you be fine with checking only the left interval? > > You mean the left edge? That's what we're doing now. No checking or complete checking are understandable, but partial checking seems an invitation for something to break. > But to be honest, look at: > > r = kvm_is_containing_region(kvm_context, start_addr, size); > if (r) > return; > > [ sip ] > > r = kvm_is_intersecting_mem(kvm_context, start_addr); > if (r) { > printf("Ignoring intersecting memory %llx (%lx)\n", start_addr, size); > > We don't really do anything, which is the same action as the containing region case. > So maybe we should just merge the two checks, and do the same thing (nothing) on both? > Yes. So long as it's self-consistent, which the current code (before your patches) isn't. Does no one read this code before merging it?! -- error compiling committee.c: too many arguments to function