[PATCHEY 0/9] Rrrreplace the ol' scurvy memory registration

[PATCHEY 0/9] Rrrreplace the ol' scurvy memory registration

[Glauber Costa]

Yahoy mateys!

I be now presentin'ya the last scurvy version of the ol'memory registration
patches! He pilleage the ol'infrastructure and make me ship more consistent.

All'of the ol'references to kvm_cpu_register_physical_memory() be trow to the
salty sea, to the sharks! I be putin' all those scurvy dogs in cpu_register_physical_memory()

Cap'n, these be not much differing from the ol'version, so me say it be included if
no mateys say no

Yoo ho!

[PATCH 1/9] Don't separate registrations with IO_MEM_ROM set

[Glauber Costa]

Actually, all registrations are the same. If IO_MEM_ROM is set, we only
need to take care of not passing its value as the phys_offset.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu/qemu-kvm.c |   31 +++++++++++--------------------
 1 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index c522a28..58a6d4a 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -776,26 +776,17 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
                                       unsigned long phys_offset)
 {
     int r = 0;
-    if (!(phys_offset & ~TARGET_PAGE_MASK)) {
-        r = kvm_is_allocated_mem(kvm_context, start_addr, size);
-        if (r)
-            return;
-        r = kvm_is_intersecting_mem(kvm_context, start_addr);
-        if (r)
-            kvm_create_mem_hole(kvm_context, start_addr, size);
-        r = kvm_register_phys_mem(kvm_context, start_addr,
-                                            phys_ram_base + phys_offset,
-                                            size, 0);
-    }
-    if (phys_offset & IO_MEM_ROM) {
-        phys_offset &= ~IO_MEM_ROM;
-        r = kvm_is_intersecting_mem(kvm_context, start_addr);
-        if (r)
-            kvm_create_mem_hole(kvm_context, start_addr, size);
-        r = kvm_register_phys_mem(kvm_context, start_addr,
-                                            phys_ram_base + phys_offset,
-                                            size, 0);
-    }
+
+    phys_offset &= ~IO_MEM_ROM;
+    r = kvm_is_allocated_mem(kvm_context, start_addr, size);
+    if (r)
+        return;
+    r = kvm_is_intersecting_mem(kvm_context, start_addr);
+    if (r)
+        kvm_create_mem_hole(kvm_context, start_addr, size);
+    r = kvm_register_phys_mem(kvm_context, start_addr,
+                              phys_ram_base + phys_offset,
+                              size, 0);
     if (r < 0) {
         printf("kvm_cpu_register_physical_memory: failed\n");
         exit(1);
-- 
1.5.5.1

[PATCH 2/9] do not use mem_hole anymore.

[Glauber Costa]

memory holes are totally evil. Right now they work for some basic tests,
but had never been stressed enough. Using memory holes leaves open questions like:

* what happens if a area being registered span two slots?
* what happens if there is already data in the slots?

also, the code behaves badly if the piece to be removed lies in the boundaries of the
current slot. Luckily, we don't really need it. Remove it, and make sure we never hit it.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 libkvm/libkvm.c |   69 +-----------------------------------------------------
 qemu/qemu-kvm.c |   13 +++++----
 2 files changed, 9 insertions(+), 73 deletions(-)

diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c
index 63fbcba..e768e44 100644
--- a/libkvm/libkvm.c
+++ b/libkvm/libkvm.c
@@ -436,74 +436,9 @@ int kvm_is_allocated_mem(kvm_context_t kvm, unsigned long phys_start,
 	return 0;
 }
 
-int kvm_create_mem_hole(kvm_context_t kvm, unsigned long phys_start,
-			unsigned long len)
-{
-	int slot;
-	int r;
-	struct kvm_userspace_memory_region rmslot;
-	struct kvm_userspace_memory_region newslot1;
-	struct kvm_userspace_memory_region newslot2;
-
-	len = (len + PAGE_SIZE - 1) & PAGE_MASK;
-
-	slot = get_intersecting_slot(phys_start);
-	/* no need to create hole, as there is already hole */
-	if (slot == -1)
-		return 0;
-
-	memset(&rmslot, 0, sizeof(struct kvm_userspace_memory_region));
-	memset(&newslot1, 0, sizeof(struct kvm_userspace_memory_region));
-	memset(&newslot2, 0, sizeof(struct kvm_userspace_memory_region));
-
-	rmslot.guest_phys_addr = slots[slot].phys_addr;
-	rmslot.slot = slot;
-
-	newslot1.guest_phys_addr = slots[slot].phys_addr;
-	newslot1.memory_size = phys_start - slots[slot].phys_addr;
-	newslot1.slot = slot;
-	newslot1.userspace_addr = slots[slot].userspace_addr;
-	newslot1.flags = slots[slot].flags;
-
-	newslot2.guest_phys_addr = newslot1.guest_phys_addr +
-				   newslot1.memory_size + len;
-	newslot2.memory_size = slots[slot].phys_addr +
-			       slots[slot].len - newslot2.guest_phys_addr;
-	newslot2.userspace_addr = newslot1.userspace_addr +
-				  newslot1.memory_size;
-	newslot2.slot = get_free_slot(kvm);
-	newslot2.flags = newslot1.flags;
-
-	r = ioctl(kvm->vm_fd, KVM_SET_USER_MEMORY_REGION, &rmslot);
-	if (r == -1) {
-		fprintf(stderr, "kvm_create_mem_hole: %s\n", strerror(errno));
-		return -1;
-	}
-	free_slot(slot);
-
-	r = ioctl(kvm->vm_fd, KVM_SET_USER_MEMORY_REGION, &newslot1);
-	if (r == -1) {
-		fprintf(stderr, "kvm_create_mem_hole: %s\n", strerror(errno));
-		return -1;
-	}
-	register_slot(newslot1.slot, newslot1.guest_phys_addr,
-		      newslot1.memory_size, newslot1.userspace_addr,
-		      newslot1.flags);
-
-	r = ioctl(kvm->vm_fd, KVM_SET_USER_MEMORY_REGION, &newslot2);
-	if (r == -1) {
-		fprintf(stderr, "kvm_create_mem_hole: %s\n", strerror(errno));
-		return -1;
-	}
-	register_slot(newslot2.slot, newslot2.guest_phys_addr,
-		      newslot2.memory_size, newslot2.userspace_addr,
-		      newslot2.flags);
-	return 0;
-}
-
 int kvm_register_phys_mem(kvm_context_t kvm,
-			unsigned long phys_start, void *userspace_addr,
-			unsigned long len, int log)
+			  unsigned long phys_start, void *userspace_addr,
+			  unsigned long len, int log)
 {
 
 	struct kvm_userspace_memory_region memory = {
diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index 58a6d4a..cff04c5 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -781,12 +781,13 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
     r = kvm_is_allocated_mem(kvm_context, start_addr, size);
     if (r)
         return;
-    r = kvm_is_intersecting_mem(kvm_context, start_addr);
-    if (r)
-        kvm_create_mem_hole(kvm_context, start_addr, size);
-    r = kvm_register_phys_mem(kvm_context, start_addr,
-                              phys_ram_base + phys_offset,
-                              size, 0);
+        r = kvm_is_intersecting_mem(kvm_context, start_addr);
+    if (r) {
+        printf("Ignoring intersecting memory %llx (%lx)\n", start_addr, size);
+    } else
+        r = kvm_register_phys_mem(kvm_context, start_addr,
+                                  phys_ram_base + phys_offset,
+                                  size, 0);
     if (r < 0) {
         printf("kvm_cpu_register_physical_memory: failed\n");
         exit(1);
-- 
1.5.5.1

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

[Glauber Costa]

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;
 }
-- 
1.5.5.1

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

[Avi Kivity]

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.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

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

[Glauber Costa]

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?

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?

>
> -- 
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
>

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

[Avi Kivity]

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.

Does no one read this code before merging it?!

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

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

[Glauber Costa]

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
>

[PATCH 4/9] substitute is_allocated_mem with more general is_containing_region

[Glauber Costa]

is_allocated_mem is a function that checks if every relevant aspect of the memory slot
match (start and size). Replace it with a more generic function that checks if a memory
region is totally contained into another. The former case is also covered.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 libkvm/libkvm.c |   34 +++++++++++++++++++++-------------
 libkvm/libkvm.h |    2 +-
 qemu/qemu-kvm.c |    4 ++--
 3 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c
index fa65c30..a5e20bb 100644
--- a/libkvm/libkvm.c
+++ b/libkvm/libkvm.c
@@ -136,6 +136,27 @@ int get_intersecting_slot(unsigned long phys_addr)
 	return -1;
 }
 
+/* Returns -1 if this slot is not totally contained on any other,
+ * and the number of the slot otherwise */
+int get_container_slot(uint64_t phys_addr, unsigned long size)
+{
+	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 + size)
+			return i;
+	return -1;
+}
+
+int kvm_is_containing_region(kvm_context_t kvm, unsigned long phys_addr, unsigned long size)
+{
+	int slot = get_container_slot(phys_addr, size);
+	if (slot == -1)
+		return 0;
+	return 1;
+}
+
 /* 
  * dirty pages logging control 
  */
@@ -423,19 +444,6 @@ int kvm_is_intersecting_mem(kvm_context_t kvm, unsigned long phys_start)
 	return get_intersecting_slot(phys_start) != -1;
 }
 
-int kvm_is_allocated_mem(kvm_context_t kvm, unsigned long phys_start,
-			 unsigned long len)
-{
-	int slot;
-
-	slot = get_slot(phys_start);
-	if (slot == -1)
-		return 0;
-	if (slots[slot].len == len)
-		return 1;
-	return 0;
-}
-
 int kvm_register_phys_mem(kvm_context_t kvm,
 			  unsigned long phys_start, void *userspace_addr,
 			  unsigned long len, int log)
diff --git a/libkvm/libkvm.h b/libkvm/libkvm.h
index 79dd769..1e89993 100644
--- a/libkvm/libkvm.h
+++ b/libkvm/libkvm.h
@@ -454,7 +454,7 @@ void *kvm_create_phys_mem(kvm_context_t, unsigned long phys_start,
 			  unsigned long len, int log, int writable);
 void kvm_destroy_phys_mem(kvm_context_t, unsigned long phys_start, 
 			  unsigned long len);
-int kvm_is_intersecting_mem(kvm_context_t kvm, unsigned long phys_start);
+int kvm_is_containing_region(kvm_context_t kvm, unsigned long phys_start, unsigned long size);
 int kvm_is_allocated_mem(kvm_context_t kvm, unsigned long phys_start,
 			 unsigned long len);
 int kvm_create_mem_hole(kvm_context_t kvm, unsigned long phys_start,
diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index cff04c5..e0b114a 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -778,10 +778,10 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
     int r = 0;
 
     phys_offset &= ~IO_MEM_ROM;
-    r = kvm_is_allocated_mem(kvm_context, start_addr, size);
+    r = kvm_is_containing_region(kvm_context, start_addr, size);
     if (r)
         return;
-        r = kvm_is_intersecting_mem(kvm_context, start_addr);
+    r = kvm_is_intersecting_mem(kvm_context, start_addr);
     if (r) {
         printf("Ignoring intersecting memory %llx (%lx)\n", start_addr, size);
     } else
-- 
1.5.5.1

Re: [PATCH 4/9] substitute is_allocated_mem with more general is_containing_region

[Avi Kivity]

Glauber Costa wrote:
> is_allocated_mem is a function that checks if every relevant aspect of the memory slot
> match (start and size). Replace it with a more generic function that checks if a memory
> region is totally contained into another. The former case is also covered.
>   

I think enabling dirty page tracking requires the slot to match exactly.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 4/9] substitute is_allocated_mem with more general is_containing_region

[Glauber Costa]

On Sat, Sep 20, 2008 at 11:33:56AM -0700, Avi Kivity wrote:
> Glauber Costa wrote:
>> is_allocated_mem is a function that checks if every relevant aspect of the memory slot
>> match (start and size). Replace it with a more generic function that checks if a memory
>> region is totally contained into another. The former case is also covered.
>>   
>
> I think enabling dirty page tracking requires the slot to match exactly.

The registering function was the only caller for that. As dirty tracking was happening
_inside_ it, so we're not really losing anything here.

That said, I believe in the future, it will. (would be a logical next step, and start
addressing the problem that Jan raised). Maybe we can keep the function that checks for
exact matching, but another alternative is to pass a phys_addr, and turn dirty tracking on/off
for whatever slot that contains that phys_addr. What do you think?

>
> -- 
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
>

Re: [PATCH 4/9] substitute is_allocated_mem with more general is_containing_region

[Avi Kivity]

Glauber Costa wrote:
> On Sat, Sep 20, 2008 at 11:33:56AM -0700, Avi Kivity wrote:
>   
>> Glauber Costa wrote:
>>     
>>> is_allocated_mem is a function that checks if every relevant aspect of the memory slot
>>> match (start and size). Replace it with a more generic function that checks if a memory
>>> region is totally contained into another. The former case is also covered.
>>>   
>>>       
>> I think enabling dirty page tracking requires the slot to match exactly.
>>     
>
> The registering function was the only caller for that. As dirty tracking was happening
> _inside_ it, so we're not really losing anything here.
>
> That said, I believe in the future, it will. (would be a logical next step, and start
> addressing the problem that Jan raised). Maybe we can keep the function that checks for
> exact matching, but another alternative is to pass a phys_addr, and turn dirty tracking on/off
> for whatever slot that contains that phys_addr. What do you think?
>   

Exact matching would avoid surprises (catch errors early).

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

Re: [PATCH 4/9] substitute is_allocated_mem with more general is_containing_region

[Glauber Costa]

On Tue, Sep 23, 2008 at 10:35:24AM +0300, Avi Kivity wrote:
> Glauber Costa wrote:
>> On Sat, Sep 20, 2008 at 11:33:56AM -0700, Avi Kivity wrote:
>>   
>>> Glauber Costa wrote:
>>>     
>>>> is_allocated_mem is a function that checks if every relevant aspect of the memory slot
>>>> match (start and size). Replace it with a more generic function that checks if a memory
>>>> region is totally contained into another. The former case is also covered.
>>>>         
>>> I think enabling dirty page tracking requires the slot to match exactly.
>>>     
>>
>> The registering function was the only caller for that. As dirty tracking was happening
>> _inside_ it, so we're not really losing anything here.
>>
>> That said, I believe in the future, it will. (would be a logical next step, and start
>> addressing the problem that Jan raised). Maybe we can keep the function that checks for
>> exact matching, but another alternative is to pass a phys_addr, and turn dirty tracking on/off
>> for whatever slot that contains that phys_addr. What do you think?
>>   
>
> Exact matching would avoid surprises (catch errors early).
Agreed, you're right. However, as I said, this is for future reference, because right now, we don't
change slots properties via the memory registration infrastructure once it's in place.

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

[PATCH 5/9] add debuging facilities to memory registration at libkvm

[Glauber Costa]

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 libkvm/libkvm.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c
index a5e20bb..222c858 100644
--- a/libkvm/libkvm.c
+++ b/libkvm/libkvm.c
@@ -52,6 +52,8 @@
 #include "kvm-s390.h"
 #endif
 
+//#define DEBUG_MEMREG
+
 int kvm_abi = EXPECTED_KVM_API_VERSION;
 int kvm_page_size;
 
@@ -458,6 +460,11 @@ int kvm_register_phys_mem(kvm_context_t kvm,
 	int r;
 
 	memory.slot = get_free_slot(kvm);
+#ifdef DEBUG_MEMREG
+	fprintf(stderr, "%s, memory: gpa: %llx, size: %llx, uaddr: %llx, slot: %x, flags: %lx\n",
+		__func__, memory.guest_phys_addr, memory.memory_size,
+		memory.userspace_addr, memory.slot, memory.flags);
+#endif
 	r = ioctl(kvm->vm_fd, KVM_SET_USER_MEMORY_REGION, &memory);
 	if (r == -1) {
 		fprintf(stderr, "create_userspace_phys_mem: %s\n", strerror(errno));
@@ -996,6 +1003,9 @@ int kvm_unregister_coalesced_mmio(kvm_context_t kvm, uint64_t addr, uint32_t siz
 			perror("kvm_unregister_coalesced_mmio_zone");
 			return -errno;
 		}
+#ifdef DEBUG_MEMREG
+		fprintf(stderr, "Unregistered coalesced mmio region for %llx (%lx)\n", addr, size);
+#endif
 		return 0;
 	}
 #endif
-- 
1.5.5.1

Re: [PATCH 5/9] add debuging facilities to memory registration at libkvm

[Avi Kivity]

Glauber Costa wrote:
> +#ifdef DEBUG_MEMREG
> +	fprintf(stderr, "%s, memory: gpa: %llx, size: %llx, uaddr: %llx, slot: %x, flags: %lx\n",
> +		__func__, memory.guest_phys_addr, memory.memory_size,
> +		memory.userspace_addr, memory.slot, memory.flags);
> +#endif
>   

Please wrap in a dprintf() macro or similar.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 5/9] add debuging facilities to memory registration at libkvm

[Glauber Costa]

On Sat, Sep 20, 2008 at 11:34:55AM -0700, Avi Kivity wrote:
> Glauber Costa wrote:
>> +#ifdef DEBUG_MEMREG
>> +	fprintf(stderr, "%s, memory: gpa: %llx, size: %llx, uaddr: %llx, slot: %x, flags: %lx\n",
>> +		__func__, memory.guest_phys_addr, memory.memory_size,
>> +		memory.userspace_addr, memory.slot, memory.flags);
>> +#endif
>>   
>
> Please wrap in a dprintf() macro or similar.
Consider it done.

>
>
> -- 
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
>

[PATCH 6/9] unregister memory area depending on their flags

[Glauber Costa]

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 libkvm/libkvm.c |   14 ++++++++++++++
 libkvm/libkvm.h |    3 +++
 qemu/qemu-kvm.c |    7 +++++++
 3 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c
index 222c858..6ebdc52 100644
--- a/libkvm/libkvm.c
+++ b/libkvm/libkvm.c
@@ -516,6 +516,20 @@ void kvm_destroy_phys_mem(kvm_context_t kvm, unsigned long phys_start,
 	free_slot(memory.slot);
 }
 
+void kvm_unregister_memory_area(kvm_context_t kvm, uint64_t phys_addr, unsigned long size)
+{
+
+	int slot = get_container_slot(phys_addr, size);
+
+	if (slot != -1) {
+#ifdef DEBUG_MEMREG
+		fprintf(stderr, "Unregistering memory region %llx (%lx)\n", phys_addr, size);
+#endif
+		kvm_destroy_phys_mem(kvm, phys_addr, size);
+		return;
+	}
+}
+
 static int kvm_get_map(kvm_context_t kvm, int ioctl_num, int slot, void *buf)
 {
 	int r;
diff --git a/libkvm/libkvm.h b/libkvm/libkvm.h
index 1e89993..fae4e0b 100644
--- a/libkvm/libkvm.h
+++ b/libkvm/libkvm.h
@@ -454,6 +454,9 @@ void *kvm_create_phys_mem(kvm_context_t, unsigned long phys_start,
 			  unsigned long len, int log, int writable);
 void kvm_destroy_phys_mem(kvm_context_t, unsigned long phys_start, 
 			  unsigned long len);
+void kvm_unregister_memory_area(kvm_context_t, uint64_t phys_start,
+                                unsigned long len);
+
 int kvm_is_containing_region(kvm_context_t kvm, unsigned long phys_start, unsigned long size);
 int kvm_is_allocated_mem(kvm_context_t kvm, unsigned long phys_start,
 			 unsigned long len);
diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index e0b114a..d9fb499 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -776,8 +776,15 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
                                       unsigned long phys_offset)
 {
     int r = 0;
+    unsigned long area_flags = phys_offset & ~TARGET_PAGE_MASK;
 
     phys_offset &= ~IO_MEM_ROM;
+
+    if (area_flags == IO_MEM_UNASSIGNED) {
+        kvm_unregister_memory_area(kvm_context, start_addr, size);
+        return;
+    }
+
     r = kvm_is_containing_region(kvm_context, start_addr, size);
     if (r)
         return;
-- 
1.5.5.1

[PATCH 7/9] register mmio slots

[Glauber Costa]

By analysing phys_offset, we know whether a region is an mmio region
or not. If it is, register it as so. We don't reuse the same slot
infrastructure already existant, because there is a relationship between
the slot number for kvm the kernel module, and the index in the slots vector
for libkvm. However, we can do best in the future and use only a single data structure
for both.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 libkvm/libkvm.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 qemu/qemu-kvm.c |   12 ++++++++-
 2 files changed, 76 insertions(+), 6 deletions(-)

diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c
index 6ebdc52..dbc1b62 100644
--- a/libkvm/libkvm.c
+++ b/libkvm/libkvm.c
@@ -64,14 +64,22 @@ struct slot_info {
 	unsigned flags;
 };
 
+struct mmio_slot_info {
+    uint64_t phys_addr;
+    unsigned int len;
+};
+
 struct slot_info slots[KVM_MAX_NUM_MEM_REGIONS];
+struct mmio_slot_info mmio_slots[KVM_MAX_NUM_MEM_REGIONS];
 
 void init_slots(void)
 {
 	int i;
 
-	for (i = 0; i < KVM_MAX_NUM_MEM_REGIONS; ++i)
+	for (i = 0; i < KVM_MAX_NUM_MEM_REGIONS; ++i) {
 		slots[i].len = 0;
+		mmio_slots[i].len = 0;
+	}
 }
 
 int get_free_slot(kvm_context_t kvm)
@@ -101,6 +109,16 @@ int get_free_slot(kvm_context_t kvm)
 	return -1;
 }
 
+int get_free_mmio_slot(kvm_context_t kvm)
+{
+
+       unsigned int i;
+       for (i = 0; i < KVM_MAX_NUM_MEM_REGIONS; ++i)
+		if (!mmio_slots[i].len)
+			return i;
+       return -1;
+}
+
 void register_slot(int slot, unsigned long phys_addr, unsigned long len,
 		   unsigned long userspace_addr, unsigned flags)
 {
@@ -151,14 +169,47 @@ int get_container_slot(uint64_t phys_addr, unsigned long size)
 	return -1;
 }
 
+int get_container_mmio_slot(kvm_context_t kvm, uint64_t phys_addr, unsigned long size)
+{
+       int i;
+
+       for (i = 0; i < KVM_MAX_NUM_MEM_REGIONS ; ++i)
+               if (mmio_slots[i].len && mmio_slots[i].phys_addr <= phys_addr &&
+                   (mmio_slots[i].phys_addr + mmio_slots[i].len) >= phys_addr + size)
+                       return i;
+       return -1;
+}
+
+int kvm_register_mmio_slot(kvm_context_t kvm, uint64_t phys_addr, unsigned int size)
+{
+       int slot = get_free_mmio_slot(kvm);
+
+       if (slot == -1)
+               goto out;
+
+#ifdef DEBUG_MEMREG
+	fprintf(stderr, "Registering mmio region %llx (%lx)\n", phys_addr, size);
+#endif
+       mmio_slots[slot].phys_addr = phys_addr;
+       mmio_slots[slot].len = size;
+out:
+       return slot;
+}
+
 int kvm_is_containing_region(kvm_context_t kvm, unsigned long phys_addr, unsigned long size)
 {
 	int slot = get_container_slot(phys_addr, size);
-	if (slot == -1)
-		return 0;
-	return 1;
+
+	if (slot != -1)
+		return 1;
+	slot = get_container_mmio_slot(kvm, phys_addr, size);
+	if (slot != -1)
+		return 1;
+
+	return 0;
 }
 
+
 /* 
  * dirty pages logging control 
  */
@@ -528,6 +579,17 @@ void kvm_unregister_memory_area(kvm_context_t kvm, uint64_t phys_addr, unsigned
 		kvm_destroy_phys_mem(kvm, phys_addr, size);
 		return;
 	}
+
+       slot = get_container_mmio_slot(kvm, phys_addr, size);
+       if (slot != -1) {
+#ifdef DEBUG_MEMREG
+               fprintf(stderr, "Unregistering mmio region %llx (%lx)\n", phys_addr, size);
+#endif
+               kvm_unregister_coalesced_mmio(kvm, phys_addr, size);
+               mmio_slots[slot].len = 0;
+       }
+
+       return;
 }
 
 static int kvm_get_map(kvm_context_t kvm, int ioctl_num, int slot, void *buf)
diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index d9fb499..721a9dc 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -788,6 +788,16 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
     r = kvm_is_containing_region(kvm_context, start_addr, size);
     if (r)
         return;
+
+    if (area_flags >= TLB_MMIO) {
+        r = kvm_register_mmio_slot(kvm_context, start_addr, size);
+        if (r < 0) {
+            printf("No free mmio slots\n");
+            exit(1);
+        }
+        return;
+    }
+
     r = kvm_is_intersecting_mem(kvm_context, start_addr);
     if (r) {
         printf("Ignoring intersecting memory %llx (%lx)\n", start_addr, size);
@@ -1032,11 +1042,9 @@ void kvm_mutex_lock(void)
 
 int qemu_kvm_register_coalesced_mmio(target_phys_addr_t addr, unsigned int size)
 {
-    return kvm_register_coalesced_mmio(kvm_context, addr, size);
 }
 
 int qemu_kvm_unregister_coalesced_mmio(target_phys_addr_t addr,
 				       unsigned int size)
 {
-    return kvm_unregister_coalesced_mmio(kvm_context, addr, size);
 }
-- 
1.5.5.1

Re: [PATCH 7/9] register mmio slots

[Avi Kivity]

Glauber Costa wrote:
> By analysing phys_offset, we know whether a region is an mmio region
> or not. If it is, register it as so. We don't reuse the same slot
> infrastructure already existant, because there is a relationship between
> the slot number for kvm the kernel module, and the index in the slots vector
> for libkvm. However, we can do best in the future and use only a single data structure
> for both.
>
>   

Why is kvm interested in emulated mmio regions, at all?


> @@ -1032,11 +1042,9 @@ void kvm_mutex_lock(void)
>  
>  int qemu_kvm_register_coalesced_mmio(target_phys_addr_t addr, unsigned int size)
>  {
> -    return kvm_register_coalesced_mmio(kvm_context, addr, size);
>  }
>  
>  int qemu_kvm_unregister_coalesced_mmio(target_phys_addr_t addr,
>  				       unsigned int size)
>  {
> -    return kvm_unregister_coalesced_mmio(kvm_context, addr, size);
>  }
>   

Why?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 7/9] register mmio slots

[Glauber Costa]

On Sat, Sep 20, 2008 at 11:38:22AM -0700, Avi Kivity wrote:
> Glauber Costa wrote:
>> By analysing phys_offset, we know whether a region is an mmio region
>> or not. If it is, register it as so. We don't reuse the same slot
>> infrastructure already existant, because there is a relationship between
>> the slot number for kvm the kernel module, and the index in the slots vector
>> for libkvm. However, we can do best in the future and use only a single data structure
>> for both.
>>
>>   
>
> Why is kvm interested in emulated mmio regions, at all?
We don't need to. If the region is an mmio region, we do nothing (well, later on, I'm
coalescing the accesses). But still, we need to keep track. Otherwise, qemu can (and it will)
try to register subsets of that memory, but without any indication that this is part of an mmio region

Our algorithm will fail in this case, since we will then register a memory area we should have left blank.
So think of mmio in this case as a "please leave it blank".

>
>
>> @@ -1032,11 +1042,9 @@ void kvm_mutex_lock(void)
>>   int qemu_kvm_register_coalesced_mmio(target_phys_addr_t addr, 
>> unsigned int size)
>>  {
>> -    return kvm_register_coalesced_mmio(kvm_context, addr, size);
>>  }
>>   int qemu_kvm_unregister_coalesced_mmio(target_phys_addr_t addr,
>>  				       unsigned int size)
>>  {
>> -    return kvm_unregister_coalesced_mmio(kvm_context, addr, size);
>>  }
>>   
>
> Why?
Because I'm doing automatic mmio coalescing in memory registration. It's the way I saw to remove 
all the calls spread through the code, for merging purposes. Any better idea?

>
> -- 
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
>

Re: [PATCH 7/9] register mmio slots

[Avi Kivity]

Glauber Costa wrote:
> On Sat, Sep 20, 2008 at 11:38:22AM -0700, Avi Kivity wrote:
>   
>> Glauber Costa wrote:
>>     
>>> By analysing phys_offset, we know whether a region is an mmio region
>>> or not. If it is, register it as so. We don't reuse the same slot
>>> infrastructure already existant, because there is a relationship between
>>> the slot number for kvm the kernel module, and the index in the slots vector
>>> for libkvm. However, we can do best in the future and use only a single data structure
>>> for both.
>>>
>>>   
>>>       
>> Why is kvm interested in emulated mmio regions, at all?
>>     
> We don't need to. If the region is an mmio region, we do nothing (well, later on, I'm
> coalescing the accesses). 

Oh no you don't.

> But still, we need to keep track. Otherwise, qemu can (and it will)
> try to register subsets of that memory, but without any indication that this is part of an mmio region
>   

Why do we care?

> Our algorithm will fail in this case, since we will then register a memory area we should have left blank.
> So think of mmio in this case as a "please leave it blank".
>   

Confused.  If qemu knows about the mmio regions, surely it won't try to 
overlay them with RAM?

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

Re: [PATCH 7/9] register mmio slots

[Glauber Costa]

On Tue, Sep 23, 2008 at 10:31:30AM +0300, Avi Kivity wrote:
> Glauber Costa wrote:
>> On Sat, Sep 20, 2008 at 11:38:22AM -0700, Avi Kivity wrote:
>>   
>>> Glauber Costa wrote:
>>>     
>>>> By analysing phys_offset, we know whether a region is an mmio region
>>>> or not. If it is, register it as so. We don't reuse the same slot
>>>> infrastructure already existant, because there is a relationship between
>>>> the slot number for kvm the kernel module, and the index in the slots vector
>>>> for libkvm. However, we can do best in the future and use only a single data structure
>>>> for both.
>>>>
>>>>         
>>> Why is kvm interested in emulated mmio regions, at all?
>>>     
>> We don't need to. If the region is an mmio region, we do nothing (well, later on, I'm
>> coalescing the accesses). 
>
> Oh no you don't.
>
>> But still, we need to keep track. Otherwise, qemu can (and it will)
>> try to register subsets of that memory, but without any indication that this is part of an mmio region
>>   
>
> Why do we care?
>
>> Our algorithm will fail in this case, since we will then register a memory area we should have left blank.
>> So think of mmio in this case as a "please leave it blank".
>>   
>
> Confused.  If qemu knows about the mmio regions, surely it won't try to  
> overlay them with RAM?

Oh, you're right. I thought a lot of cases of subregions being registered lacked the proper MMIO flags.
But I think I was confused by an earlier issue. It'll get even simpler!

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

[PATCH 8/9] coalesce mmio regions with an explicit call

[Glauber Costa]

Remove explicit calls to mmio coalescing. Rather,
include it in the registration functions.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu/hw/cirrus_vga.c |    2 --
 qemu/hw/e1000.c      |   12 ------------
 qemu/hw/pci.c        |    3 ---
 qemu/hw/vga.c        |    4 ----
 qemu/qemu-kvm.c      |   10 +---------
 qemu/qemu-kvm.h      |    4 ----
 6 files changed, 1 insertions(+), 34 deletions(-)

diff --git a/qemu/hw/cirrus_vga.c b/qemu/hw/cirrus_vga.c
index 0cf5b24..5919732 100644
--- a/qemu/hw/cirrus_vga.c
+++ b/qemu/hw/cirrus_vga.c
@@ -3291,8 +3291,6 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci)
                                            cirrus_vga_mem_write, s);
     cpu_register_physical_memory(isa_mem_base + 0x000a0000, 0x20000,
                                  vga_io_memory);
-    if (kvm_enabled())
-        qemu_kvm_register_coalesced_mmio(isa_mem_base + 0x000a0000, 0x20000);
 
     s->sr[0x06] = 0x0f;
     if (device_id == CIRRUS_ID_CLGD5446) {
diff --git a/qemu/hw/e1000.c b/qemu/hw/e1000.c
index 5ae3960..2d97b34 100644
--- a/qemu/hw/e1000.c
+++ b/qemu/hw/e1000.c
@@ -942,18 +942,6 @@ e1000_mmio_map(PCIDevice *pci_dev, int region_num,
 
     d->mmio_base = addr;
     cpu_register_physical_memory(addr, PNPMMIO_SIZE, d->mmio_index);
-
-    if (kvm_enabled()) {
-	int i;
-        uint32_t excluded_regs[] = {
-            E1000_MDIC, E1000_ICR, E1000_ICS, E1000_IMS,
-            E1000_IMC, E1000_TCTL, E1000_TDT, PNPMMIO_SIZE
-        };
-        qemu_kvm_register_coalesced_mmio(addr, excluded_regs[0]);
-        for (i = 0; excluded_regs[i] != PNPMMIO_SIZE; i++)
-            qemu_kvm_register_coalesced_mmio(addr + excluded_regs[i] + 4,
-                             excluded_regs[i + 1] - excluded_regs[i] - 4);
-    }
 }
 
 static int
diff --git a/qemu/hw/pci.c b/qemu/hw/pci.c
index 07d37a8..2e4ec92 100644
--- a/qemu/hw/pci.c
+++ b/qemu/hw/pci.c
@@ -324,9 +324,6 @@ static void pci_update_mappings(PCIDevice *d)
                         cpu_register_physical_memory(pci_to_cpu_addr(r->addr),
                                                      r->size,
                                                      IO_MEM_UNASSIGNED);
-                        if (kvm_enabled())
-                                qemu_kvm_unregister_coalesced_mmio(r->addr,
-                                                                 r->size);
                     }
                 }
                 r->addr = new_addr;
diff --git a/qemu/hw/vga.c b/qemu/hw/vga.c
index 3a5dcbc..ba0dec4 100644
--- a/qemu/hw/vga.c
+++ b/qemu/hw/vga.c
@@ -2259,8 +2259,6 @@ void vga_init(VGAState *s)
     vga_io_memory = cpu_register_io_memory(0, vga_mem_read, vga_mem_write, s);
     cpu_register_physical_memory(isa_mem_base + 0x000a0000, 0x20000,
                                  vga_io_memory);
-    if (kvm_enabled())
-        qemu_kvm_register_coalesced_mmio(isa_mem_base + 0x000a0000, 0x20000);
 }
 
 /* Memory mapped interface */
@@ -2336,8 +2334,6 @@ static void vga_mm_init(VGAState *s, target_phys_addr_t vram_base,
     cpu_register_physical_memory(ctrl_base, 0x100000, s_ioport_ctrl);
     s->bank_offset = 0;
     cpu_register_physical_memory(vram_base + 0x000a0000, 0x20000, vga_io_memory);
-    if (kvm_enabled())
-        qemu_kvm_register_coalesced_mmio(vram_base + 0x000a0000, 0x20000);
 }
 
 int isa_vga_init(DisplayState *ds, uint8_t *vga_ram_base,
diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index 721a9dc..660e11f 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -795,6 +795,7 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
             printf("No free mmio slots\n");
             exit(1);
         }
+        kvm_register_coalesced_mmio(kvm_context, start_addr, size);
         return;
     }
 
@@ -1039,12 +1040,3 @@ void kvm_mutex_lock(void)
     pthread_mutex_lock(&qemu_mutex);
     cpu_single_env = NULL;
 }
-
-int qemu_kvm_register_coalesced_mmio(target_phys_addr_t addr, unsigned int size)
-{
-}
-
-int qemu_kvm_unregister_coalesced_mmio(target_phys_addr_t addr,
-				       unsigned int size)
-{
-}
diff --git a/qemu/qemu-kvm.h b/qemu/qemu-kvm.h
index 3e40a7d..4308e18 100644
--- a/qemu/qemu-kvm.h
+++ b/qemu/qemu-kvm.h
@@ -75,10 +75,6 @@ int handle_tpr_access(void *opaque, int vcpu,
 void kvm_tpr_vcpu_start(CPUState *env);
 
 int qemu_kvm_get_dirty_pages(unsigned long phys_addr, void *buf);
-int qemu_kvm_register_coalesced_mmio(target_phys_addr_t addr,
-				     unsigned int size);
-int qemu_kvm_unregister_coalesced_mmio(target_phys_addr_t addr,
-				       unsigned int size);
 
 void qemu_kvm_system_reset_request(void);
 
-- 
1.5.5.1

Re: [PATCH 8/9] coalesce mmio regions with an explicit call

[Avi Kivity]

Glauber Costa wrote:
> Remove explicit calls to mmio coalescing. Rather,
> include it in the registration functions.
>
> index 5ae3960..2d97b34 100644
> --- a/qemu/hw/e1000.c
> +++ b/qemu/hw/e1000.c
> @@ -942,18 +942,6 @@ e1000_mmio_map(PCIDevice *pci_dev, int region_num,
>  
>      d->mmio_base = addr;
>      cpu_register_physical_memory(addr, PNPMMIO_SIZE, d->mmio_index);
> -
> -    if (kvm_enabled()) {
> -	int i;
> -        uint32_t excluded_regs[] = {
> -            E1000_MDIC, E1000_ICR, E1000_ICS, E1000_IMS,
> -            E1000_IMC, E1000_TCTL, E1000_TDT, PNPMMIO_SIZE
> -        };
> -        qemu_kvm_register_coalesced_mmio(addr, excluded_regs[0]);
> -        for (i = 0; excluded_regs[i] != PNPMMIO_SIZE; i++)
> -            qemu_kvm_register_coalesced_mmio(addr + excluded_regs[i] + 4,
> -                             excluded_regs[i + 1] - excluded_regs[i] - 4);
> -    }
>  }
>  
>   

Where did all of this go?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 8/9] coalesce mmio regions with an explicit call

[Glauber Costa]

On Sat, Sep 20, 2008 at 11:39:44AM -0700, Avi Kivity wrote:
> Glauber Costa wrote:
>> Remove explicit calls to mmio coalescing. Rather,
>> include it in the registration functions.
>>
>> index 5ae3960..2d97b34 100644
>> --- a/qemu/hw/e1000.c
>> +++ b/qemu/hw/e1000.c
>> @@ -942,18 +942,6 @@ e1000_mmio_map(PCIDevice *pci_dev, int region_num,
>>       d->mmio_base = addr;
>>      cpu_register_physical_memory(addr, PNPMMIO_SIZE, d->mmio_index);
>> -
>> -    if (kvm_enabled()) {
>> -	int i;
>> -        uint32_t excluded_regs[] = {
>> -            E1000_MDIC, E1000_ICR, E1000_ICS, E1000_IMS,
>> -            E1000_IMC, E1000_TCTL, E1000_TDT, PNPMMIO_SIZE
>> -        };
>> -        qemu_kvm_register_coalesced_mmio(addr, excluded_regs[0]);
>> -        for (i = 0; excluded_regs[i] != PNPMMIO_SIZE; i++)
>> -            qemu_kvm_register_coalesced_mmio(addr + excluded_regs[i] + 4,
>> -                             excluded_regs[i + 1] - excluded_regs[i] - 4);
>> -    }
>>  }
>>    
>
> Where did all of this go?

All the region is coalesced (not just the pieces) automatically during memory registration.
Or not at all, in case coalescing is disabled.

>
> -- 
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
>

Re: [PATCH 8/9] coalesce mmio regions with an explicit call

[Avi Kivity]

Glauber Costa wrote:
> On Sat, Sep 20, 2008 at 11:39:44AM -0700, Avi Kivity wrote:
>   
>> Glauber Costa wrote:
>>     
>>> Remove explicit calls to mmio coalescing. Rather,
>>> include it in the registration functions.
>>>
>>> index 5ae3960..2d97b34 100644
>>> --- a/qemu/hw/e1000.c
>>> +++ b/qemu/hw/e1000.c
>>> @@ -942,18 +942,6 @@ e1000_mmio_map(PCIDevice *pci_dev, int region_num,
>>>       d->mmio_base = addr;
>>>      cpu_register_physical_memory(addr, PNPMMIO_SIZE, d->mmio_index);
>>> -
>>> -    if (kvm_enabled()) {
>>> -	int i;
>>> -        uint32_t excluded_regs[] = {
>>> -            E1000_MDIC, E1000_ICR, E1000_ICS, E1000_IMS,
>>> -            E1000_IMC, E1000_TCTL, E1000_TDT, PNPMMIO_SIZE
>>> -        };
>>> -        qemu_kvm_register_coalesced_mmio(addr, excluded_regs[0]);
>>> -        for (i = 0; excluded_regs[i] != PNPMMIO_SIZE; i++)
>>> -            qemu_kvm_register_coalesced_mmio(addr + excluded_regs[i] + 4,
>>> -                             excluded_regs[i + 1] - excluded_regs[i] - 4);
>>> -    }
>>>  }
>>>    
>>>       
>> Where did all of this go?
>>     
>
> All the region is coalesced (not just the pieces) automatically during memory registration.
> Or not at all, in case coalescing is disabled.
>   

You can't coalesce the registers which trigger device action.  You'll 
destroy latency and/or functionality.

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

Re: [PATCH 8/9] coalesce mmio regions with an explicit call

[Glauber Costa]

On Tue, Sep 23, 2008 at 10:29:48AM +0300, Avi Kivity wrote:
> Glauber Costa wrote:
>> On Sat, Sep 20, 2008 at 11:39:44AM -0700, Avi Kivity wrote:
>>   
>>> Glauber Costa wrote:
>>>     
>>>> Remove explicit calls to mmio coalescing. Rather,
>>>> include it in the registration functions.
>>>>
>>>> index 5ae3960..2d97b34 100644
>>>> --- a/qemu/hw/e1000.c
>>>> +++ b/qemu/hw/e1000.c
>>>> @@ -942,18 +942,6 @@ e1000_mmio_map(PCIDevice *pci_dev, int region_num,
>>>>       d->mmio_base = addr;
>>>>      cpu_register_physical_memory(addr, PNPMMIO_SIZE, d->mmio_index);
>>>> -
>>>> -    if (kvm_enabled()) {
>>>> -	int i;
>>>> -        uint32_t excluded_regs[] = {
>>>> -            E1000_MDIC, E1000_ICR, E1000_ICS, E1000_IMS,
>>>> -            E1000_IMC, E1000_TCTL, E1000_TDT, PNPMMIO_SIZE
>>>> -        };
>>>> -        qemu_kvm_register_coalesced_mmio(addr, excluded_regs[0]);
>>>> -        for (i = 0; excluded_regs[i] != PNPMMIO_SIZE; i++)
>>>> -            qemu_kvm_register_coalesced_mmio(addr + excluded_regs[i] + 4,
>>>> -                             excluded_regs[i + 1] - excluded_regs[i] - 4);
>>>> -    }
>>>>  }
>>>>          
>>> Where did all of this go?
>>>     
>>
>> All the region is coalesced (not just the pieces) automatically during memory registration.
>> Or not at all, in case coalescing is disabled.
>>   
>
> You can't coalesce the registers which trigger device action.  You'll  
> destroy latency and/or functionality.

which kills the goal of getting rid of explicit kvm code.

So maybe the solution here is to add calls in qemu to a memory
coalescing function that in the raw qemu / kqemu case just don't
do anything?

Anthony, do you have an opinion about it ?

Re: [PATCH 8/9] coalesce mmio regions with an explicit call

[Avi Kivity]

Glauber Costa wrote:
>> You can't coalesce the registers which trigger device action.  You'll  
>> destroy latency and/or functionality.
>>     
>
> which kills the goal of getting rid of explicit kvm code.
>
>   

It's a fact that coalescing helps kvm but not qemu.

> So maybe the solution here is to add calls in qemu to a memory
> coalescing function that in the raw qemu / kqemu case just don't
> do anything?
>   

That's just word games.  s/kvm/qemu/ won't change the fact that this is 
a kvm specific hook.

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

Re: [PATCH 8/9] coalesce mmio regions with an explicit call

[Glauber Costa]

On Wed, Sep 24, 2008 at 02:10:26PM +0300, Avi Kivity wrote:
> Glauber Costa wrote:
>>> You can't coalesce the registers which trigger device action.  You'll 
>>>  destroy latency and/or functionality.
>>>     
>>
>> which kills the goal of getting rid of explicit kvm code.
>>
>>   
>
> It's a fact that coalescing helps kvm but not qemu.
>
>> So maybe the solution here is to add calls in qemu to a memory
>> coalescing function that in the raw qemu / kqemu case just don't
>> do anything?
>>   
>
> That's just word games.  s/kvm/qemu/ won't change the fact that this is  
> a kvm specific hook.
Any ideas about what's up for the other hypervisors that may (we hope) be integrated
in the future? Xen?

Re: [PATCH 8/9] coalesce mmio regions with an explicit call

[Avi Kivity]

Glauber Costa wrote:
>
> Any ideas about what's up for the other hypervisors that may (we hope) be integrated
> in the future? Xen?
>   

Xen should benefit even more (much more).  IIRC Windows wouldn't boot
since it was spending all its time context switching when the splash
screen with its KITT bar was displayed, so they hacked something for
vga, but nothing generic.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 8/9] coalesce mmio regions with an explicit call

[Glauber Costa]

On Thu, Sep 25, 2008 at 10:08:53AM +0300, Avi Kivity wrote:
> Glauber Costa wrote:
> >
> > Any ideas about what's up for the other hypervisors that may (we hope) be integrated
> > in the future? Xen?
> >   
> 
> Xen should benefit even more (much more).  IIRC Windows wouldn't boot
> since it was spending all its time context switching when the splash
> screen with its KITT bar was displayed, so they hacked something for
> vga, but nothing generic.
That's the point. It's not just a word play between qemu and kvm, because
if we introduce generic hooks that kvm happens to fill, but qemu not, other hypervirors
may (we hope) fill it in the future.

Re: [PATCH 8/9] coalesce mmio regions with an explicit call

[Dmitry Eremin-Solenikov]

Glauber Costa wrote:

> Remove explicit calls to mmio coalescing. Rather, include it in the
> registration functions.
> 

OK. On real SVM HW this seems to work. However now i'm stumbled upon another
problem wrt. NMI. See another mail.

-- 
With best wishes
Dmitry

se

[PATCH 9/9] move kvm memory registration inside qemu's

[Glauber Costa]

Remove explicit calls to kvm_cpu_register_physical_memory,
and bundle it together with qemu's memory registration function.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu/exec.c             |    5 +++++
 qemu/hw/ipf.c           |    8 --------
 qemu/hw/pc.c            |   23 ++---------------------
 qemu/hw/ppc440_bamboo.c |    2 --
 4 files changed, 7 insertions(+), 31 deletions(-)

diff --git a/qemu/exec.c b/qemu/exec.c
index bf037f0..b32f2ff 100644
--- a/qemu/exec.c
+++ b/qemu/exec.c
@@ -2203,6 +2203,11 @@ void cpu_register_physical_memory(target_phys_addr_t start_addr,
         kqemu_set_phys_mem(start_addr, size, phys_offset);
     }
 #endif
+#ifdef USE_KVM
+    if (kvm_enabled())
+        kvm_cpu_register_physical_memory(start_addr, size, phys_offset);
+#endif
+
     size = (size + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK;
     end_addr = start_addr + (target_phys_addr_t)size;
     for(addr = start_addr; addr != end_addr; addr += TARGET_PAGE_SIZE) {
diff --git a/qemu/hw/ipf.c b/qemu/hw/ipf.c
index d70af90..5227385 100644
--- a/qemu/hw/ipf.c
+++ b/qemu/hw/ipf.c
@@ -420,18 +420,14 @@ static void ipf_init1(ram_addr_t ram_size, int vga_ram_size,
     if (kvm_enabled()) {
 		ram_addr = qemu_ram_alloc(0xa0000);
 		cpu_register_physical_memory(0, 0xa0000, ram_addr);
-		kvm_cpu_register_physical_memory(0, 0xa0000, ram_addr);
 
 		ram_addr = qemu_ram_alloc(0x20000); // Workaround 0xa0000-0xc0000
 
 		ram_addr = qemu_ram_alloc(0x40000);
 		cpu_register_physical_memory(0xc0000, 0x40000, ram_addr);
-		kvm_cpu_register_physical_memory(0xc0000, 0x40000, ram_addr);
 
 		ram_addr = qemu_ram_alloc(ram_size - 0x100000);
 		cpu_register_physical_memory(0x100000, ram_size - 0x100000, ram_addr);
-		kvm_cpu_register_physical_memory(0x100000, ram_size - 0x100000,
-				ram_addr);
 	} else
     {
         ram_addr = qemu_ram_alloc(ram_size);
@@ -444,9 +440,6 @@ static void ipf_init1(ram_addr_t ram_size, int vga_ram_size,
 	if (above_4g_mem_size > 0) {
 		ram_addr = qemu_ram_alloc(above_4g_mem_size);
 		cpu_register_physical_memory(0x100000000, above_4g_mem_size, ram_addr);
-	if (kvm_enabled())
-		kvm_cpu_register_physical_memory(0x100000000, above_4g_mem_size,
-					ram_addr);
 	}
 
 	/*Load firware to its proper position.*/
@@ -468,7 +461,6 @@ static void ipf_init1(ram_addr_t ram_size, int vga_ram_size,
 		fw_image_start = fw_start + GFW_SIZE - image_size;
 
         cpu_register_physical_memory(GFW_START, GFW_SIZE, fw_offset);
-        kvm_cpu_register_physical_memory(GFW_START,GFW_SIZE, fw_offset);
         memcpy(fw_image_start, image, image_size);
 
 		free(image);
diff --git a/qemu/hw/pc.c b/qemu/hw/pc.c
index 8a50096..bc4585c 100644
--- a/qemu/hw/pc.c
+++ b/qemu/hw/pc.c
@@ -769,9 +769,7 @@ static int load_option_rom(const char *filename, int offset, int type)
     cpu_register_physical_memory(0xd0000 + offset,
 				 size, option_rom_offset | type);
     option_rom_setup_reset(0xd0000 + offset, size);
-    if (kvm_enabled())
-	    kvm_cpu_register_physical_memory(0xd0000 + offset,
-                                             size, option_rom_offset | type);
+
     return size;
 }
 
@@ -845,16 +843,13 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
     if (kvm_enabled()) {
         ram_addr = qemu_ram_alloc(0xa0000);
         cpu_register_physical_memory(0, 0xa0000, ram_addr);
-        kvm_cpu_register_physical_memory(0, 0xa0000, ram_addr);
 
         ram_addr = qemu_ram_alloc(0x100000 - 0xa0000);   // hole
         ram_addr = qemu_ram_alloc(below_4g_mem_size - 0x100000);
         cpu_register_physical_memory(0x100000,
 				     below_4g_mem_size - 0x100000,
 				     ram_addr);
-        kvm_cpu_register_physical_memory(0x100000,
-					 below_4g_mem_size - 0x100000,
-                                         ram_addr);
+
         /* above 4giga memory allocation */
         if (above_4g_mem_size > 0) {
             ram_addr = qemu_ram_alloc(above_4g_mem_size);
@@ -870,9 +865,6 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
             cpu_register_physical_memory(0x100000000ULL,
                                          above_4g_mem_size,
                                          ram_addr);
-            kvm_cpu_register_physical_memory(0x100000000ULL,
-					     above_4g_mem_size,
-                                             ram_addr);
         }
     } else
     {
@@ -926,9 +918,6 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
     /* setup basic memory access */
     cpu_register_physical_memory(0xc0000, 0x10000,
                                  vga_bios_offset | IO_MEM_ROM);
-    if (kvm_enabled())
-        kvm_cpu_register_physical_memory(0xc0000, 0x10000,
-                                         vga_bios_offset | IO_MEM_ROM);
 
     /* map the last 128KB of the BIOS in ISA space */
     isa_bios_size = bios_size;
@@ -940,10 +929,6 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
     cpu_register_physical_memory(0x100000 - isa_bios_size,
                                  isa_bios_size,
                                  (bios_offset + bios_size - isa_bios_size) /* | IO_MEM_ROM */);
-    if (kvm_enabled())
-        kvm_cpu_register_physical_memory(0x100000 - isa_bios_size,
-                                         isa_bios_size,
-                                         (bios_offset + bios_size - isa_bios_size) | IO_MEM_ROM);
 
     /* XXX: for DDIM support, "ROM space" should be writable during
        initialization, and (optionally) marked readonly by the BIOS
@@ -962,10 +947,6 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
     /* map all the bios at the top of memory */
     cpu_register_physical_memory((uint32_t)(-bios_size),
                                  bios_size, bios_offset | IO_MEM_ROM);
-    if (kvm_enabled()) {
-        kvm_cpu_register_physical_memory((uint32_t)(-bios_size),
-                                           bios_size, bios_offset | IO_MEM_ROM);
-    }
 
     bochs_bios_init();
 
diff --git a/qemu/hw/ppc440_bamboo.c b/qemu/hw/ppc440_bamboo.c
index 9ff6f7d..deafc5f 100644
--- a/qemu/hw/ppc440_bamboo.c
+++ b/qemu/hw/ppc440_bamboo.c
@@ -93,8 +93,6 @@ void bamboo_init(ram_addr_t ram_size, int vga_ram_size,
 
 	/* Register mem */
 	cpu_register_physical_memory(0, ram_size, 0);
-	if (kvm_enabled())
-	  kvm_cpu_register_physical_memory(0, ram_size, 0);
 
 	/* load kernel with uboot loader */
 	printf("%s: load kernel\n", __func__);
-- 
1.5.5.1

Re: [PATCH 9/9] move kvm memory registration inside qemu's

[Jan Kiszka]

Glauber Costa wrote:
> Remove explicit calls to kvm_cpu_register_physical_memory,
> and bundle it together with qemu's memory registration function.

Fine, this also fixes -no-kvm for current userspace git - as I just
noticed. :)

Jan

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