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

[Daniel P. Berrange]

On Thu, Dec 18, 2008 at 08:48:50AM -0200, Glauber Costa wrote:
> 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.

Migration compatability is a problem for mgmt apps to solve & merely
saying to use qemu-system-x86_64 everywhere is a tiny insignificant 
piece of the problem. AFAIK, current Fedora packages build a 32-bit 
'qemu' for KVM on i386  and a qemu-system-x86_64' for x86_64, which
happens to be called 'qemu-kvm' on both, to avoid clash with the base
QEMU binary names.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|