[Qemu-devel] Re: [PATCH 5/5] cache slot lookup

[Glauber Costa]

On Thu, Dec 18, 2008 at 11:41:24AM +0200, Avi Kivity wrote:
> Glauber Costa wrote:
>> record slot used in last lookup. For the common mmio case,
>> we'll usually access the same memory slot repeatedly.
>>   
>
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -75,16 +75,25 @@ static KVMSlot *kvm_alloc_slot(KVMState *s)
>>      return NULL;
>>  }
>>  +static KVMSlot *last_slot = NULL;
>> +
>>  static KVMSlot *kvm_lookup_slot(KVMState *s, target_phys_addr_t start_addr)
>>  {
>>      int i;
>>  +
>> +    if (last_slot && (start_addr >= last_slot->start_addr &&
>> +            start_addr < (last_slot->start_addr + last_slot->memory_size)))
>> +        return last_slot;
>> +
>>      for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
>>          KVMSlot *mem = &s->slots[i];
>>           if (start_addr >= mem->start_addr &&
>> -            start_addr < (mem->start_addr + mem->memory_size))
>> +            start_addr < (mem->start_addr + mem->memory_size)) {
>> +            last_slot = mem;
>>              return mem;
>> +        }
>>      }
>>  
>
> This wasn't introduced by this patch, but the comparison is broken ion  
> i386 hosts, where target_phys_addr_t is 32 bits wide.  mem->start_addr +  
> mem->memory_size can overflow (this in fact happens for the bios slot at  
> 4G-128K)
AFAIK, the assumption is that kvm will always be qemu-system-x86_64, due to
migration issues. Then, _target_ phys_addr_t is always 64 bit wide.
If it's not the case, then this is really a problem.