Re: [PATCH 3/9] allow intersecting region to be on the boundary.

[Glauber Costa <glommer@redhat.com>]

On Tue, Sep 23, 2008 at 01:30:41PM +0300, Avi Kivity wrote:
> Glauber Costa wrote:
>> On Sat, Sep 20, 2008 at 11:32:48AM -0700, Avi Kivity wrote:
>>   
>>> Glauber Costa wrote:
>>>     
>>>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>>>> ---
>>>>  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.
I have an updated version that just gets rid of the check, and it seems to work well.
I'm still thinking about adding sanity checkings to make sure for example, that we never
span two slots, etc. But maybe this can be done later?

>
> Does no one read this code before merging it?!
>
> -- 
> error compiling committee.c: too many arguments to function
>