[Qemu-devel] Re: [PATCH] remove smaller slots if registering a bigger one

[Glauber Costa <glommer@redhat.com>]

On Wed, Feb 11, 2009 at 03:23:07PM +0100, Jan Kiszka wrote:
> Glauber Costa wrote:
> > It's like a shark eating a bunch of small fishes:
> > in some situations (vga linear frame buffer mapping,
> > for example), we need to register a new slot in place
> > of older, smaller ones. This patch handles this case
> > 
> > Signed-off-by: Glauber Costa <glommer@redhat.com>
> > ---
> >  kvm-all.c |   10 ++++++++++
> >  1 files changed, 10 insertions(+), 0 deletions(-)
> > 
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 9fb295c..53aca0a 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -582,6 +582,16 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
> >                  kvm_set_phys_mem(mem_start, mem_size, mem_offset);
> >  
> >              return;
> > +        } else if (start_addr <= mem->start_addr &&
> > +                   (start_addr + size) >= (mem->start_addr +
> > +                                           mem->memory_size)) {
> > +            KVMSlot slot;
> > +            /* unregister whole slot */
> > +            memcpy(&slot, mem, sizeof(slot));
> > +            mem->memory_size = 0;
> > +            kvm_set_user_memory_region(s, mem);
> > +
> > +            kvm_set_phys_mem(start_addr, size, phys_offset);
> 
> That may solve some problems, but...
> 
> >          } else {
> >              printf("Registering overlapping slot\n");
> >              abort();
>             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ...as long as this line exists, issues will remain. IIRC, the mapping
> the i440 tries to re-establish after reboot will hit this case.
Which is fine. I'd prefer it to be here, so we can analyse it case by case.
The old memory code for kvm was totally messy, in part because we tried to
hug the world at once, with some code paths that were almost never hit.

Slot management can easily get very complicated. and trying to come up
with a solution that accounts for all problems at once may backfire on us.


> 
> 
> BTW, I found the unposted patch below in my attic, maybe you can comment
> on it (if it makes sense, I'll properly repost with signed-off).
I don't believe it makes much sense.

> @@ -540,7 +540,7 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
>  
>      mem = kvm_lookup_slot(s, start_addr);
>      if (mem) {
> -        if ((flags == IO_MEM_UNASSIGNED) || (flags >= TLB_MMIO)) {
> +        if (flags >= IO_MEM_UNASSIGNED) {
>              mem->memory_size = 0;
>              mem->start_addr = start_addr;
>              mem->phys_offset = 0;
vga seem to be a heavy user of this kind of construct. We map some piece
of memory as RAM, and later on, it becomes an mmio region again. In this
case, we have to delete it from kvm slot list.

Now, if you remember the last memory patches I sent, it actually removes
this line. However, this is because in that alternative, we were tracking
mmio regions in qemu, but not in kvm. so flags >= TLB_MMIO would just
delete it from the kernel mapping, but qemu would not forget about it.

This is to show that I believe that it might be possible to handle mmio
regions slightly different, but I don't think just dropping this line would
help much.