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

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

[Glauber Costa]

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);
         } else {
             printf("Registering overlapping slot\n");
             abort();
-- 
1.5.6.5

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

[Jan Kiszka]

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.


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).

Thanks,
Jan

---------->

kvm: cleanup unmap condition in kvm_set_phys_mem
    
Testing for TLB_MMIO on unmap makes no sense as A) that flag belongs to
CPUTLBEntry and not to io_memory slots or physical addresses and B) we
already use a different condition before mapping. So make this test
consistent.

diff --git a/kvm-all.c b/kvm-all.c
index 9fb295c..c0481a0 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -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;

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

[Avi Kivity]

Jan Kiszka wrote:
>             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ...as long as this line exists, issues will remain. IIRC, the mapping
> the i440 tries to re-establish after reboot will hit this case.
>
>   

I agree.  Slot management is similar to vma management in the kernel.  
We're probably better off keeping the slot list sorted and implementing 
all possible map/unmap cases wrt partial overlap.

-- 
error compiling committee.c: too many arguments to function

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

[Glauber Costa]

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.
 

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

[Jan Kiszka]

Glauber Costa wrote:
> 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.
> 

Well, then "fix" all users...

> 
>>
>> 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.

What confused me most here - and still does - is that TLB_* flags make
semantically no sense to me in the physical memory
registering/unregistering context. They should only show up in
CPUTLBEntry, no? But maybe I'm overseeing some strange relation.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

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

[Jan Kiszka]

Jan Kiszka wrote:
> Glauber Costa wrote:
>> 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.
>>
> 
> Well, then "fix" all users...

BTW, if you want to play with some problematic case, apply this and
reboot a guest while using -enable-kvm:

diff --git a/vl.c b/vl.c
index ce80690..d53611e 100644
--- a/vl.c
+++ b/vl.c
@@ -3557,6 +3557,8 @@ void qemu_system_reset(void)
     for(re = first_reset_entry; re != NULL; re = re->next) {
         re->func(re->opaque);
     }
+    if (kvm_enabled())
+        kvm_sync_vcpus();
 }
 
 void qemu_system_reset_request(void)

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

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

[Glauber Costa]

> > 
> > 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.
> 
> What confused me most here - and still does - is that TLB_* flags make
> semantically no sense to me in the physical memory
> registering/unregistering context. They should only show up in
> CPUTLBEntry, no? But maybe I'm overseeing some strange relation.
> 
I might be wrong, but I believe it's used quite often in normal memory
to track pieces of memory in which we do mmio.

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

[Avi Kivity]

Jan Kiszka wrote:
>> 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.
>>
>>     
>
> Well, then "fix" all users...
>   

Making memory management in qemu symmetric (any 
cpu_register_physical_memory() must be followed by a 
cpu_unregister_physical_memory() with the same parameters) is a good idea.

-- 
error compiling committee.c: too many arguments to function

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

[Jan Kiszka]

Avi Kivity wrote:
> Jan Kiszka wrote:
>>> 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.
>>>
>>>     
>>
>> Well, then "fix" all users...
>>   
> 
> Making memory management in qemu symmetric (any
> cpu_register_physical_memory() must be followed by a
> cpu_unregister_physical_memory() with the same parameters) is a good idea.
> 

Then all devices should track their - potentially guest-defined -
mappings and revert them on reset? I think handling this in the lower
layer is more convenient.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

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

[Avi Kivity]

Jan Kiszka wrote:

  

>> Making memory management in qemu symmetric (any
>> cpu_register_physical_memory() must be followed by a
>> cpu_unregister_physical_memory() with the same parameters) is a good idea.
>>
>>     
>
> Then all devices should track their - potentially guest-defined -
> mappings and revert them on reset? I think handling this in the lower
> layer is more convenient.
>   

Most of these mappings (at least in the PC world) are PCI mappings, no?  
In which case the PCI layer already tracks everything.

There's an issue if the guest programs conflicting BARs.  Currently qemu 
has a latest-wins policy, but that's not necessarily the right thing.

-- 
error compiling committee.c: too many arguments to function