[Qemu-devel] [PATCH] Make page_find() return 0 for too-large addresses

[Qemu-devel] [PATCH] Make page_find() return 0 for too-large addresses

[Eduardo Habkost]

On some cases, such as under KVM, tb_invalidate_phys_page_range()
may be called for large addresses, when qemu is configured to more than
4GB of RAM.

On these cases, qemu was crashing because it was using an index too
large for l1_map[], that supports only 32-bit addresses when compiling
without CONFIG_USER_ONLY.

Below is a sample backtrace of this happening, under KVM. 'start' on
tb_invalidate_phys_page_range() is 0x132b1c812, beyond 2^32, making
page_find() return a bogus pointer.

 #0  tb_invalidate_phys_page_range (start=5145479186, end=5145479246, is_cpu_write_access=0) at /usr/src/debug/kvm-74/qemu/exec.c:877
 #1  0x000000000049cd28 in cpu_physical_memory_rw (addr=5682350098, buf=0x7389dd10 "RT", len=60, is_write=1) at /usr/src/debug/kvm-74/qemu/exec.c:2818
 #2  0x0000000000428f54 in rtl8139_do_receive (opaque=0x1f472240, buf=<value optimized out>, size=60, do_interrupt=1) at ../cpu-all.h:929
 #3  0x000000000040717b in qemu_send_packet (vc1=0x1e54f480, buf=0x7389ddc0 "RT", size=42) at /usr/src/debug/kvm-74/qemu/vl.c:4146
 #4  0x0000000000480990 in slirp_input (pkt=0x1f4a9010 "������RT", pkt_len=42) at slirp/slirp.c:597
 #5  0x000000000040717b in qemu_send_packet (vc1=0x1f475d70, buf=0x1f4a9010 "������RT", size=42) at /usr/src/debug/kvm-74/qemu/vl.c:4146
 #6  0x00000000004295bd in rtl8139_io_writeb (opaque=0x1f472240, addr=<value optimized out>, val=<value optimized out>) at /usr/src/debug/kvm-74/qemu/hw/rtl8139.c:2299
 #7  0x000000000049cb62 in cpu_physical_memory_rw (addr=4060090585, buf=0x2aaadcf11028 <Address 0x2aaadcf11028 out of bounds>, len=1, is_write=1)
     at /usr/src/debug/kvm-74/qemu/exec.c:2807
 #8  0x00000000004eebf8 in kvm_mmio_write (opaque=<value optimized out>, addr=5145479186, data=0x132b1c84e <Address 0x132b1c84e out of bounds>, len=707232)
     at /usr/src/debug/kvm-74/qemu/qemu-kvm.c:689
 #9  0x0000000000515646 in handle_mmio (kvm=0x1e54e040, kvm_run=0x2aaadcf11000) at libkvm.c:849
 #10 0x0000000000515b57 in kvm_run (kvm=0x1e54e040, vcpu=0) at libkvm.c:975


The following patch makes page_find() safe for >32-bit addresses, by
just returning 0 if the address is too large.

The translation block handling code on exec.c still doesn't support
addresses >32-bit if CONFIG_USER_ONLY is not defined (KVM qemu currently
crashes immediately when using -no-kvm with more than 4 GB of memory),
but this is another issue.


Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
diff -purN qemu.orig/exec.c qemu/exec.c
--- qemu.orig/exec.c	2008-09-12 15:40:37.000000000 -0300
+++ qemu/exec.c	2008-09-12 15:40:37.000000000 -0300
@@ -317,8 +317,11 @@ static inline PageDesc *page_find_alloc(
 static inline PageDesc *page_find(target_ulong index)
 {
     PageDesc *p;
+    target_ulong l1_index = (index >> L2_BITS);
+    if (l1_index > L1_SIZE)
+        return 0;
 
-    p = l1_map[index >> L2_BITS];
+    p = l1_map[l1_index];
     if (!p)
         return 0;
     return p + (index & (L2_SIZE - 1));


-- 
Eduardo

[Qemu-devel] Re: [PATCH] Make page_find() return 0 for too-large addresses

[Anthony Liguori]

Eduardo Habkost wrote:
> On some cases, such as under KVM, tb_invalidate_phys_page_range()
> may be called for large addresses, when qemu is configured to more than
> 4GB of RAM.
>
> On these cases, qemu was crashing because it was using an index too
> large for l1_map[], that supports only 32-bit addresses when compiling
> without CONFIG_USER_ONLY.
>   

Did you have kqemu enabled in the build?  l1_map should be sufficiently 
large when you have kqemu disabled.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH] Make page_find() return 0 for too-large addresses

[Eduardo Habkost]

On Fri, Sep 12, 2008 at 02:50:33PM -0500, Anthony Liguori wrote:
> Eduardo Habkost wrote:
>> On some cases, such as under KVM, tb_invalidate_phys_page_range()
>> may be called for large addresses, when qemu is configured to more than
>> 4GB of RAM.
>>
>> On these cases, qemu was crashing because it was using an index too
>> large for l1_map[], that supports only 32-bit addresses when compiling
>> without CONFIG_USER_ONLY.
>>   
>
> Did you have kqemu enabled in the build?  l1_map should be sufficiently  
> large when you have kqemu disabled.

KVM uses './configure --disable-kqemu', but exec.c has this:

#if defined(CONFIG_USER_ONLY) && defined(TARGET_VIRT_ADDR_SPACE_BITS)
/* XXX: this is a temporary hack for alpha target.
 *      In the future, this is to be replaced by a multi-level table
 *      to actually be able to handle the complete 64 bits address space.
 */
#define L1_BITS (TARGET_VIRT_ADDR_SPACE_BITS - L2_BITS - TARGET_PAGE_BITS)
#else
#define L1_BITS (32 - L2_BITS - TARGET_PAGE_BITS)
#endif


And CONFIG_USER_ONLY is not defined, making l1_map work only for 32-bit
addresses.


BTW, I've just noticed page_find_alloc() has this:

#if TARGET_LONG_BITS > 32
    /* Host memory outside guest VM.  For 32-bit targets we have already
       excluded high addresses.  */
    if (index > ((target_ulong)L2_SIZE * L1_SIZE))
        return NULL;
#endif

So, we can just use a similar check on page_find().


Side note: the check on the kvm git tree looks broken: it is checking
for (L2_SIZE * L1_SIZE * TARGET_PAGE_SIZE) instead.

-- 
Eduardo

Re: [Qemu-devel] Re: [PATCH] Make page_find() return 0 for too-large addresses

[Eduardo Habkost]

On Fri, Sep 12, 2008 at 05:14:06PM -0300, Eduardo Habkost wrote:
> On Fri, Sep 12, 2008 at 02:50:33PM -0500, Anthony Liguori wrote:
> > Eduardo Habkost wrote:
> >> On some cases, such as under KVM, tb_invalidate_phys_page_range()
> >> may be called for large addresses, when qemu is configured to more than
> >> 4GB of RAM.
> >>
> >> On these cases, qemu was crashing because it was using an index too
> >> large for l1_map[], that supports only 32-bit addresses when compiling
> >> without CONFIG_USER_ONLY.
> >>   
<snip>
> 
> BTW, I've just noticed page_find_alloc() has this:
> 
> #if TARGET_LONG_BITS > 32
>     /* Host memory outside guest VM.  For 32-bit targets we have already
>        excluded high addresses.  */
>     if (index > ((target_ulong)L2_SIZE * L1_SIZE))
>         return NULL;
> #endif
> 
> So, we can just use a similar check on page_find().

New patch, reusing the range check from page_find_alloc() on
page_find(). Untested.


Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Index: qemu/exec.c
===================================================================
--- qemu/exec.c	(revisão 5200)
+++ qemu/exec.c	(cópia de trabalho)
@@ -279,17 +279,24 @@ static void page_init(void)
 #endif
 }
 
-static inline PageDesc *page_find_alloc(target_ulong index)
+static inline PageDesc **page_l1_map(target_ulong index)
 {
-    PageDesc **lp, *p;
-
 #if TARGET_LONG_BITS > 32
     /* Host memory outside guest VM.  For 32-bit targets we have already
        excluded high addresses.  */
     if (index > ((target_ulong)L2_SIZE * L1_SIZE))
         return NULL;
 #endif
-    lp = &l1_map[index >> L2_BITS];
+    return &l1_map[index >> L2_BITS];
+}
+
+static inline PageDesc *page_find_alloc(target_ulong index)
+{
+    PageDesc **lp, *p;
+    lp = page_l1_map(index);
+    if (!lp)
+        return NULL;
+
     p = *lp;
     if (!p) {
         /* allocate if not found */
@@ -316,9 +323,12 @@ static inline PageDesc *page_find_alloc(
 
 static inline PageDesc *page_find(target_ulong index)
 {
-    PageDesc *p;
+    PageDesc **lp, *p;
+    lp = page_l1_map(index);
+    if (!lp)
+        return NULL;
 
-    p = l1_map[index >> L2_BITS];
+    p = *lp;
     if (!p)
         return 0;
     return p + (index & (L2_SIZE - 1));


-- 
Eduardo

Re: [Qemu-devel] Re: [PATCH] Make page_find() return 0 for too-large addresses

[Anthony Liguori]

Eduardo Habkost wrote:
> On Fri, Sep 12, 2008 at 05:14:06PM -0300, Eduardo Habkost wrote:
>   
>> On Fri, Sep 12, 2008 at 02:50:33PM -0500, Anthony Liguori wrote:
>>     
>>> Eduardo Habkost wrote:
>>>       
>>>> On some cases, such as under KVM, tb_invalidate_phys_page_range()
>>>> may be called for large addresses, when qemu is configured to more than
>>>> 4GB of RAM.
>>>>
>>>> On these cases, qemu was crashing because it was using an index too
>>>> large for l1_map[], that supports only 32-bit addresses when compiling
>>>> without CONFIG_USER_ONLY.
>>>>   
>>>>         
> <snip>
>   

So... are you building with kqemu enabled?  If so, 
TARGET_PHYS_ADDR_SPACE_BITS will be 32.  So I don't think this sort of 
work-around is correct.  A better solution would be to validate ram_size 
against TARGET_PHYS_ADDR_SPACE_BITS.

To put it another way, if you have kqemu enabled (at build time), you 
cannot use > ~4GB of memory for the guest.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH] Make page_find() return 0 for too-large addresses

[Eduardo Habkost]

On Fri, Sep 12, 2008 at 04:27:30PM -0500, Anthony Liguori wrote:
> Eduardo Habkost wrote:
>> On Fri, Sep 12, 2008 at 05:14:06PM -0300, Eduardo Habkost wrote:
>>   
>>> On Fri, Sep 12, 2008 at 02:50:33PM -0500, Anthony Liguori wrote:
>>>     
>>>> Eduardo Habkost wrote:
>>>>       
>>>>> On some cases, such as under KVM, tb_invalidate_phys_page_range()
>>>>> may be called for large addresses, when qemu is configured to more than
>>>>> 4GB of RAM.
>>>>>
>>>>> On these cases, qemu was crashing because it was using an index too
>>>>> large for l1_map[], that supports only 32-bit addresses when compiling
>>>>> without CONFIG_USER_ONLY.
>>>>>           
>> <snip>
>>   
>
> So... are you building with kqemu enabled?

No, as I've stated in the reply I've sent before the patch.


> If so,  
> TARGET_PHYS_ADDR_SPACE_BITS will be 32.  So I don't think this sort of  
> work-around is correct.  A better solution would be to validate ram_size  
> against TARGET_PHYS_ADDR_SPACE_BITS.

This is a good idea (maybe it is already checked?), but it is not the
case I am dealing with.


>
> To put it another way, if you have kqemu enabled (at build time), you  
> cannot use > ~4GB of memory for the guest.

Right.

-- 
Eduardo

Re: [Qemu-devel] Re: [PATCH] Make page_find() return 0 for too-large addresses

[Glauber Costa]

On Fri, Sep 12, 2008 at 04:27:30PM -0500, Anthony Liguori wrote:
> Eduardo Habkost wrote:
>> On Fri, Sep 12, 2008 at 05:14:06PM -0300, Eduardo Habkost wrote:
>>   
>>> On Fri, Sep 12, 2008 at 02:50:33PM -0500, Anthony Liguori wrote:
>>>     
>>>> Eduardo Habkost wrote:
>>>>       
>>>>> On some cases, such as under KVM, tb_invalidate_phys_page_range()
>>>>> may be called for large addresses, when qemu is configured to more than
>>>>> 4GB of RAM.
>>>>>
>>>>> On these cases, qemu was crashing because it was using an index too
>>>>> large for l1_map[], that supports only 32-bit addresses when compiling
>>>>> without CONFIG_USER_ONLY.
>>>>>           
>> <snip>
>>   
>
> So... are you building with kqemu enabled?  If so,  
> TARGET_PHYS_ADDR_SPACE_BITS will be 32.  So I don't think this sort of  
> work-around is correct.  A better solution would be to validate ram_size  
> against TARGET_PHYS_ADDR_SPACE_BITS.
>
> To put it another way, if you have kqemu enabled (at build time), you  
> cannot use > ~4GB of memory for the guest.
I can be very wrong here, but page_find() does not seem to limit our ability to
run guests if > 4GB, as it is only concerned about the code translation layer.
Differently from phys_page_find(), that does manage physical memory, and even
presented problems about it in the past.

>
> Regards,
>
> Anthony Liguori
>

Re: [Qemu-devel] Re: [PATCH] Make page_find() return 0 for too-large addresses

[Anthony Liguori]

Eduardo Habkost wrote:
> On Fri, Sep 12, 2008 at 05:14:06PM -0300, Eduardo Habkost wrote:
>   
>> On Fri, Sep 12, 2008 at 02:50:33PM -0500, Anthony Liguori wrote:
>>     
>>> Eduardo Habkost wrote:
>>>       
>>>> On some cases, such as under KVM, tb_invalidate_phys_page_range()
>>>> may be called for large addresses, when qemu is configured to more than
>>>> 4GB of RAM.
>>>>
>>>> On these cases, qemu was crashing because it was using an index too
>>>> large for l1_map[], that supports only 32-bit addresses when compiling
>>>> without CONFIG_USER_ONLY.
>>>>   
>>>>         
> <snip>
>   
>> BTW, I've just noticed page_find_alloc() has this:
>>
>> #if TARGET_LONG_BITS > 32
>>     /* Host memory outside guest VM.  For 32-bit targets we have already
>>        excluded high addresses.  */
>>     if (index > ((target_ulong)L2_SIZE * L1_SIZE))
>>         return NULL;
>> #endif
>>
>> So, we can just use a similar check on page_find().
>>     
>
> New patch, reusing the range check from page_find_alloc() on
> page_find(). Untested.
>   

Have you tested this patch yet?  I like to avoid being the first one to 
test something when it's not my code :-)

Regards,

Anthony Liguori

> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Index: qemu/exec.c
> ===================================================================
> --- qemu/exec.c	(revisão 5200)
> +++ qemu/exec.c	(cópia de trabalho)
> @@ -279,17 +279,24 @@ static void page_init(void)
>  #endif
>  }
>  
> -static inline PageDesc *page_find_alloc(target_ulong index)
> +static inline PageDesc **page_l1_map(target_ulong index)
>  {
> -    PageDesc **lp, *p;
> -
>  #if TARGET_LONG_BITS > 32
>      /* Host memory outside guest VM.  For 32-bit targets we have already
>         excluded high addresses.  */
>      if (index > ((target_ulong)L2_SIZE * L1_SIZE))
>          return NULL;
>  #endif
> -    lp = &l1_map[index >> L2_BITS];
> +    return &l1_map[index >> L2_BITS];
> +}
> +
> +static inline PageDesc *page_find_alloc(target_ulong index)
> +{
> +    PageDesc **lp, *p;
> +    lp = page_l1_map(index);
> +    if (!lp)
> +        return NULL;
> +
>      p = *lp;
>      if (!p) {
>          /* allocate if not found */
> @@ -316,9 +323,12 @@ static inline PageDesc *page_find_alloc(
>  
>  static inline PageDesc *page_find(target_ulong index)
>  {
> -    PageDesc *p;
> +    PageDesc **lp, *p;
> +    lp = page_l1_map(index);
> +    if (!lp)
> +        return NULL;
>  
> -    p = l1_map[index >> L2_BITS];
> +    p = *lp;
>      if (!p)
>          return 0;
>      return p + (index & (L2_SIZE - 1));
>
>
>   

Re: [Qemu-devel] Re: [PATCH] Make page_find() return 0 for too-large addresses

[Eduardo Habkost]

On Mon, Sep 15, 2008 at 10:29:14AM -0500, Anthony Liguori wrote:
> Eduardo Habkost wrote:
<snip>
>>
>> New patch, reusing the range check from page_find_alloc() on
>> page_find(). Untested.
>>   
>
> Have you tested this patch yet?  I like to avoid being the first one to  
> test something when it's not my code :-)

I've tested it now, both with KVM enabled and with KVM disabled. My
machine didn't explode yet, so it seems to be fine.  :)

>
> Regards,
>
> Anthony Liguori
>
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>> Index: qemu/exec.c
>> ===================================================================
>> --- qemu/exec.c	(revisão 5200)
>> +++ qemu/exec.c	(cópia de trabalho)
>> @@ -279,17 +279,24 @@ static void page_init(void)
>>  #endif
>>  }
>>  -static inline PageDesc *page_find_alloc(target_ulong index)
>> +static inline PageDesc **page_l1_map(target_ulong index)
>>  {
>> -    PageDesc **lp, *p;
>> -
>>  #if TARGET_LONG_BITS > 32
>>      /* Host memory outside guest VM.  For 32-bit targets we have already
>>         excluded high addresses.  */
>>      if (index > ((target_ulong)L2_SIZE * L1_SIZE))
>>          return NULL;
>>  #endif
>> -    lp = &l1_map[index >> L2_BITS];
>> +    return &l1_map[index >> L2_BITS];
>> +}
>> +
>> +static inline PageDesc *page_find_alloc(target_ulong index)
>> +{
>> +    PageDesc **lp, *p;
>> +    lp = page_l1_map(index);
>> +    if (!lp)
>> +        return NULL;
>> +
>>      p = *lp;
>>      if (!p) {
>>          /* allocate if not found */
>> @@ -316,9 +323,12 @@ static inline PageDesc *page_find_alloc(
>>   static inline PageDesc *page_find(target_ulong index)
>>  {
>> -    PageDesc *p;
>> +    PageDesc **lp, *p;
>> +    lp = page_l1_map(index);
>> +    if (!lp)
>> +        return NULL;
>>  -    p = l1_map[index >> L2_BITS];
>> +    p = *lp;
>>      if (!p)
>>          return 0;
>>      return p + (index & (L2_SIZE - 1));
>>
>>
>>   
>
>

-- 
Eduardo

Re: [Qemu-devel] Re: [PATCH] Make page_find() return 0 for too-large addresses

[Anthony Liguori]

Eduardo Habkost wrote:
> On Mon, Sep 15, 2008 at 10:29:14AM -0500, Anthony Liguori wrote:
>   
>> Eduardo Habkost wrote:
>>     
> <snip>
>   
>>> New patch, reusing the range check from page_find_alloc() on
>>> page_find(). Untested.
>>>   
>>>       
>> Have you tested this patch yet?  I like to avoid being the first one to  
>> test something when it's not my code :-)
>>     
>
> I've tested it now, both with KVM enabled and with KVM disabled. My
> machine didn't explode yet, so it seems to be fine.  :)
>   

Applied.  Thanks.

Regards,

Anthony Liguori