Urgent memory fix for kvm with unaligned memory slots

GIT PULL Avi Kivity 4 participants 7 messages Mar 1, 2012 – Mar 2, 2012 KVM

Share & tools

Avi Kivity

author

2012-03-01 13:40 UTC #0 lore raw

The memory core may generate RAM memory regions that are not page aligned, but the kvm code is not prepared to handle them well and will abort under certain conditions. This patch fixes the problem.

Please pull from:

git://git.kernel.org/pub/scm/virt/kvm/qemu-kvm.git memory/urgent

----------------------------------------------------------------
Avi Kivity (1):
kvm: fix unaligned slots

 kvm-all.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

kvm-all.c +0 -0

index c4babda..4b7a4ae 100644
@@ -541,17 +541,26 @@ static void kvm_set_phys_mem(MemoryRegionSection

*section, bool add)

     target_phys_addr_t start_addr = section->offset_within_address_space;
     ram_addr_t size = section->size;
     void *ram = NULL;

+ unsigned delta;

/* kvm works in page size chunks, but the function may be called
with sub-page size and unaligned start address. */

-    size = TARGET_PAGE_ALIGN(size);
-    start_addr = TARGET_PAGE_ALIGN(start_addr);
+    delta = TARGET_PAGE_ALIGN(size) - size;
+    if (delta > size) {
+        return;
+    }
+    start_addr += delta;
+    size -= delta;
+    size &= TARGET_PAGE_MASK;
+    if (!size || (start_addr & ~TARGET_PAGE_MASK)) {

+ return;
+ }

     if (!memory_region_is_ram(mr)) {
         return;
     }

- ram = memory_region_get_ram_ptr(mr) + section->offset_within_region;
+ ram = memory_region_get_ram_ptr(mr) + section->offset_within_region
+ delta;

while (1) {
mem = kvm_lookup_overlapping_slot(s, start_addr, start_addr +
size);

Signature · 1 line

error compiling committee.c: too many arguments to function

Bobby Powers

2012-03-01 16:51 UTC #1 lore raw

Quoted 41 lines · Avi Kivity

The memory core may generate RAM memory regions that are not page aligned, but the kvm code is not prepared to handle them well and will abort under certain conditions. This patch fixes the problem.
Please pull from:
git://git.kernel.org/pub/scm/virt/kvm/qemu-kvm.git memory/urgent
----------------------------------------------------------------
Avi Kivity (1):
kvm: fix unaligned slots
kvm-all.c | 15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)
kvm-all.c +1 -0
index c4babda..4b7a4ae 100644
@@ -541,17 +541,26 @@ static void kvm_set_phys_mem(MemoryRegionSection
 *section, bool add)
     target_phys_addr_t start_addr = section->offset_within_address_space;
     ram_addr_t size = section->size;
     void *ram = NULL;
+    unsigned delta;
 
     /* kvm works in page size chunks, but the function may be called
with sub-page size and unaligned start address. */
- size = TARGET_PAGE_ALIGN(size);
- start_addr = TARGET_PAGE_ALIGN(start_addr);
+ delta = TARGET_PAGE_ALIGN(size) - size;
+ if (delta > size) {
+ return;
+ }
+ start_addr += delta;
+ size -= delta;
+ size &= TARGET_PAGE_MASK;
+ if (!size || (start_addr & ~TARGET_PAGE_MASK)) {
+ return;
+ }

Quoted 7 lines

if (!memory_region_is_ram(mr)) {
return;
}
- ram = memory_region_get_ram_ptr(mr) + section->offset_within_region;
+ ram = memory_region_get_ram_ptr(mr) + section->offset_within_region
+ delta;

Am I crazy, or does this look wrong?

Quoted 12 lines

while (1) {
mem = kvm_lookup_overlapping_slot(s, start_addr, start_addr + size);
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Avi Kivity

author

2012-03-01 17:03 UTC #2 lore raw

Quoted 19 lines · Bobby Powers

Quoted 19 lines

/* kvm works in page size chunks, but the function may be called
with sub-page size and unaligned start address. */

(quoted diff) +10 -2

-    size = TARGET_PAGE_ALIGN(size);
-    start_addr = TARGET_PAGE_ALIGN(start_addr);
+    delta = TARGET_PAGE_ALIGN(size) - size;
+    if (delta > size) {
+        return;
+    }
+    start_addr += delta;
+    size -= delta;
+    size &= TARGET_PAGE_MASK;
+    if (!size || (start_addr & ~TARGET_PAGE_MASK)) {
+        return;
+    }
 
    if (!memory_region_is_ram(mr)) {
        return;
    }

Quoted 5 lines

Review
Quoted 3 lines
(quoted diff) +2 -1
-    ram = memory_region_get_ram_ptr(mr) + section->offset_within_region;
+    ram = memory_region_get_ram_ptr(mr) + section->offset_within_region
+ delta;
Am I crazy, or does this look wrong?

Could be both. Why do you thing it is wrong?

Signature · 1 line

error compiling committee.c: too many arguments to function

Eric Blake

2012-03-01 17:08 UTC #3 lore raw

Quoted 8 lines · Avi Kivity

Quoted 6 lines
Review
Quoted 4 lines
(quoted diff) +2 -1
-    ram = memory_region_get_ram_ptr(mr) + section->offset_within_region;
+    ram = memory_region_get_ram_ptr(mr) + section->offset_within_region
+ delta;
Am I crazy, or does this look wrong?
Could be both. Why do you thing it is wrong?

Line wrapping makes it look like we are adding two lines, one line ending in 'section->offset_within_region', and the next line starting with 'delta;', which is a syntax error.

But without line wrapping, we are adding just one line with 'offset_within_region + delta;' at the end of that line.

Signature · 2 lines

Eric Blake   [email protected]    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Bobby Powers

2012-03-01 17:11 UTC #4 lore raw

Quoted 6 lines · Eric Blake

Quoted 5 lines · Avi Kivity
Quoted 5 lines
Quoted 4 lines
- ram = memory_region_get_ram_ptr(mr) + section->offset_within_region;
+ ram = memory_region_get_ram_ptr(mr) + section->offset_within_region
+ delta;

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Am I crazy, or does this look wrong?
Could be both. Why do you thing it is wrong?
Line wrapping makes it look like we are adding two lines, one line ending in 'section->offset_within_region', and the next line starting with 'delta;', which is a syntax error.

yea, patch line wrapping was making my eyes see things. okay, sorry for the noise.

Quoted 8 lines

But without line wrapping, we are adding just one line with 'offset_within_region + delta;' at the end of that line.
--
Eric Blake [email protected] +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Avi Kivity

author

2012-03-01 17:13 UTC #5 lore raw

In reply to Eric Blake

Quoted 8 lines · Eric Blake

Quoted 7 lines · Avi Kivity
Quoted 6 lines
Review
Quoted 4 lines
(quoted diff) +2 -1
-    ram = memory_region_get_ram_ptr(mr) + section->offset_within_region;
+    ram = memory_region_get_ram_ptr(mr) + section->offset_within_region
+ delta;
Am I crazy, or does this look wrong?

Quoted 9 lines

Quoted 1 line
Could be both. Why do you thing it is wrong?
Line wrapping makes it look like we are adding two lines, one line ending in 'section->offset_within_region', and the next line starting with 'delta;', which is a syntax error.
But without line wrapping, we are adding just one line with 'offset_within_region + delta;' at the end of that line.

Ah, of course. I just copy/pasted this into thunderbird, this is not a proper patch but a pull request. Sorry about the confusion.

Bobby: so it does indeed look wrong. This says nothing about your sanity though, for that consult a qualified professional instead of asking on the mailing list.

Signature · 1 line

error compiling committee.c: too many arguments to function

Anthony Liguori

2012-03-02 13:23 UTC #6 lore raw

In reply to Avi Kivity

Quoted 7 lines · Avi Kivity

The memory core may generate RAM memory regions that are not page aligned, but the kvm code is not prepared to handle them well and will abort under certain conditions. This patch fixes the problem.
Please pull from:
git://git.kernel.org/pub/scm/virt/kvm/qemu-kvm.git memory/urgent

Pulled. Thanks.

Regards,

Anthony Liguori

Quoted 46 lines

----------------------------------------------------------------
Avi Kivity (1):
kvm: fix unaligned slots

  kvm-all.c |   15 ++++++++++++---
  1 files changed, 12 insertions(+), 3 deletions(-)

kvm-all.c +13 -3

index c4babda..4b7a4ae 100644
@@ -541,17 +541,26 @@ static void kvm_set_phys_mem(MemoryRegionSection
 *section, bool add)
      target_phys_addr_t start_addr = section->offset_within_address_space;
      ram_addr_t size = section->size;
      void *ram = NULL;
+    unsigned delta;
 
      /* kvm works in page size chunks, but the function may be called
         with sub-page size and unaligned start address. */
-    size = TARGET_PAGE_ALIGN(size);
-    start_addr = TARGET_PAGE_ALIGN(start_addr);
+    delta = TARGET_PAGE_ALIGN(size) - size;
+    if (delta>  size) {
+        return;
+    }
+    start_addr += delta;
+    size -= delta;
+    size&= TARGET_PAGE_MASK;
+    if (!size || (start_addr&  ~TARGET_PAGE_MASK)) {
+        return;
+    }
 
      if (!memory_region_is_ram(mr)) {
          return;
      }
 
-    ram = memory_region_get_ram_ptr(mr) + section->offset_within_region;
+    ram = memory_region_get_ram_ptr(mr) + section->offset_within_region
+ delta;
 
      while (1) {
          mem = kvm_lookup_overlapping_slot(s, start_addr, start_addr +

size);