[Qemu-devel] Regression: Xen guest with 5G of RAM on 32bit fail to boot

[Qemu-devel] Regression: Xen guest with 5G of RAM on 32bit fail to boot

[Anthony PERARD]

Hi,

Under Xen, a guest with 5G of RAM, with a 32bit binary QEMU (well, with a
32bit dom0) does not boot anymore. QEMU abort() with "Bad ram offset efffd000".

This issue first appear in 4ed023ce2a39ab5812d33cf4d819def168965a7f (Round
up RAMBlock sizes to host page sizes).

The problem is in qemu_ram_alloc_internal() where 'size' and 'maxsize' are
now been truncate to 32bit, due to 'qemu_host_page_size' been an uintptr_t
in the HOST_PAGE_ALIGN macro.

ram_add_t is uint64_t when compiled with --enable-xen.

Regards,

-- 
Anthony PERARD

Re: [Qemu-devel] Regression: Xen guest with 5G of RAM on 32bit fail to boot

[Dr. David Alan Gilbert]

* Anthony PERARD (anthony.perard@citrix.com) wrote:
> Hi,
> 
> Under Xen, a guest with 5G of RAM, with a 32bit binary QEMU (well, with a
> 32bit dom0) does not boot anymore. QEMU abort() with "Bad ram offset efffd000".
> 
> This issue first appear in 4ed023ce2a39ab5812d33cf4d819def168965a7f (Round
> up RAMBlock sizes to host page sizes).
> 
> The problem is in qemu_ram_alloc_internal() where 'size' and 'maxsize' are
> now been truncate to 32bit, due to 'qemu_host_page_size' been an uintptr_t
> in the HOST_PAGE_ALIGN macro.
> 
> ram_add_t is uint64_t when compiled with --enable-xen.

Hmm, that's a fun problem.
Would changing qemu_host_page_[size|mask] to ram_addr_t  work?

Dave

> 
> Regards,
> 
> -- 
> Anthony PERARD
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] Regression: Xen guest with 5G of RAM on 32bit fail to boot

[Anthony PERARD]

On Tue, Dec 01, 2015 at 06:37:36PM +0000, Dr. David Alan Gilbert wrote:
> * Anthony PERARD (anthony.perard@citrix.com) wrote:
> > Hi,
> > 
> > Under Xen, a guest with 5G of RAM, with a 32bit binary QEMU (well, with a
> > 32bit dom0) does not boot anymore. QEMU abort() with "Bad ram offset efffd000".
> > 
> > This issue first appear in 4ed023ce2a39ab5812d33cf4d819def168965a7f (Round
> > up RAMBlock sizes to host page sizes).
> > 
> > The problem is in qemu_ram_alloc_internal() where 'size' and 'maxsize' are
> > now been truncate to 32bit, due to 'qemu_host_page_size' been an uintptr_t
> > in the HOST_PAGE_ALIGN macro.
> > 
> > ram_add_t is uint64_t when compiled with --enable-xen.
> 
> Hmm, that's a fun problem.
> Would changing qemu_host_page_[size|mask] to ram_addr_t  work?

Yes, well, I did change the type to uint64_t and I could boot a guest. With
ram_addr_t, it works fine as well.

-- 
Anthony PERARD

Re: [Qemu-devel] Regression: Xen guest with 5G of RAM on 32bit fail to boot

[Paolo Bonzini]

On 01/12/2015 18:53, Anthony PERARD wrote:
> The problem is in qemu_ram_alloc_internal() where 'size' and 'maxsize' are
> now been truncate to 32bit, due to 'qemu_host_page_size' been an uintptr_t
> in the HOST_PAGE_ALIGN macro.

Isn't it qemu_host_page_mask that causes the problem?

This should also work, as it causes qemu_host_page_mask to sign-extend:

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index f9998b9..87a4145 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -174,11 +174,10 @@ extern unsigned long reserved_va;
 #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1)
 #define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK)
 
-/* ??? These should be the larger of uintptr_t and target_ulong.  */
 extern uintptr_t qemu_real_host_page_size;
-extern uintptr_t qemu_real_host_page_mask;
+extern intptr_t qemu_real_host_page_mask;
 extern uintptr_t qemu_host_page_size;
-extern uintptr_t qemu_host_page_mask;
+extern intptr_t qemu_host_page_mask;
 
 #define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) & qemu_host_page_mask)
 #define REAL_HOST_PAGE_ALIGN(addr) (((addr) + qemu_real_host_page_size - 1) & \
diff --git a/translate-all.c b/translate-all.c
index a940bd2..7a15109 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -118,7 +118,7 @@ typedef struct PageDesc {
 #define V_L1_SHIFT (L1_MAP_ADDR_SPACE_BITS - TARGET_PAGE_BITS - V_L1_BITS)
 
 uintptr_t qemu_host_page_size;
-uintptr_t qemu_host_page_mask;
+intptr_t qemu_host_page_mask;
 
 /* The bottom level has pointers to PageDesc */
 static void *l1_map[V_L1_SIZE];
@@ -326,14 +326,14 @@ void page_size_init(void)
     /* NOTE: we can always suppose that qemu_host_page_size >=
        TARGET_PAGE_SIZE */
     qemu_real_host_page_size = getpagesize();
-    qemu_real_host_page_mask = ~(qemu_real_host_page_size - 1);
+    qemu_real_host_page_mask = -(intptr_t)qemu_real_host_page_size;
     if (qemu_host_page_size == 0) {
         qemu_host_page_size = qemu_real_host_page_size;
     }
     if (qemu_host_page_size < TARGET_PAGE_SIZE) {
         qemu_host_page_size = TARGET_PAGE_SIZE;
     }
-    qemu_host_page_mask = ~(qemu_host_page_size - 1);
+    qemu_host_page_mask = -(intptr_t)qemu_host_page_size;
 }
 
 static void page_init(void)
diff --git a/translate-common.c b/translate-common.c
index 619feb4..171222d 100644
--- a/translate-common.c
+++ b/translate-common.c
@@ -21,7 +21,7 @@
 #include "qom/cpu.h"
 
 uintptr_t qemu_real_host_page_size;
-uintptr_t qemu_real_host_page_mask;
+intptr_t qemu_real_host_page_mask;
 
 #ifndef CONFIG_USER_ONLY
 /* mask must never be zero, except for A20 change call */

Re: [Qemu-devel] Regression: Xen guest with 5G of RAM on 32bit fail to boot

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 01/12/2015 18:53, Anthony PERARD wrote:
> > The problem is in qemu_ram_alloc_internal() where 'size' and 'maxsize' are
> > now been truncate to 32bit, due to 'qemu_host_page_size' been an uintptr_t
> > in the HOST_PAGE_ALIGN macro.
> 
> Isn't it qemu_host_page_mask that causes the problem?
> 
> This should also work, as it causes qemu_host_page_mask to sign-extend:

I've just posted a set that just flips them to ram_addr_t (and removes the 10 year
old warning that questions whether the type is right - which it obviously wasn't);
see '[For 2.5?? PATCH 1/1] qemu_{real_}host_page_[size|mask] change types to
 ram_addr_t'

Anthony: I'd appreciate if you could give it a Xen test.

Dave

> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index f9998b9..87a4145 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -174,11 +174,10 @@ extern unsigned long reserved_va;
>  #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1)
>  #define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK)
>  
> -/* ??? These should be the larger of uintptr_t and target_ulong.  */
>  extern uintptr_t qemu_real_host_page_size;
> -extern uintptr_t qemu_real_host_page_mask;
> +extern intptr_t qemu_real_host_page_mask;
>  extern uintptr_t qemu_host_page_size;
> -extern uintptr_t qemu_host_page_mask;
> +extern intptr_t qemu_host_page_mask;
>  
>  #define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) & qemu_host_page_mask)
>  #define REAL_HOST_PAGE_ALIGN(addr) (((addr) + qemu_real_host_page_size - 1) & \
> diff --git a/translate-all.c b/translate-all.c
> index a940bd2..7a15109 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -118,7 +118,7 @@ typedef struct PageDesc {
>  #define V_L1_SHIFT (L1_MAP_ADDR_SPACE_BITS - TARGET_PAGE_BITS - V_L1_BITS)
>  
>  uintptr_t qemu_host_page_size;
> -uintptr_t qemu_host_page_mask;
> +intptr_t qemu_host_page_mask;
>  
>  /* The bottom level has pointers to PageDesc */
>  static void *l1_map[V_L1_SIZE];
> @@ -326,14 +326,14 @@ void page_size_init(void)
>      /* NOTE: we can always suppose that qemu_host_page_size >=
>         TARGET_PAGE_SIZE */
>      qemu_real_host_page_size = getpagesize();
> -    qemu_real_host_page_mask = ~(qemu_real_host_page_size - 1);
> +    qemu_real_host_page_mask = -(intptr_t)qemu_real_host_page_size;
>      if (qemu_host_page_size == 0) {
>          qemu_host_page_size = qemu_real_host_page_size;
>      }
>      if (qemu_host_page_size < TARGET_PAGE_SIZE) {
>          qemu_host_page_size = TARGET_PAGE_SIZE;
>      }
> -    qemu_host_page_mask = ~(qemu_host_page_size - 1);
> +    qemu_host_page_mask = -(intptr_t)qemu_host_page_size;
>  }
>  
>  static void page_init(void)
> diff --git a/translate-common.c b/translate-common.c
> index 619feb4..171222d 100644
> --- a/translate-common.c
> +++ b/translate-common.c
> @@ -21,7 +21,7 @@
>  #include "qom/cpu.h"
>  
>  uintptr_t qemu_real_host_page_size;
> -uintptr_t qemu_real_host_page_mask;
> +intptr_t qemu_real_host_page_mask;
>  
>  #ifndef CONFIG_USER_ONLY
>  /* mask must never be zero, except for A20 change call */
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] Regression: Xen guest with 5G of RAM on 32bit fail to boot

[Paolo Bonzini]

On 02/12/2015 11:30, Paolo Bonzini wrote:
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index f9998b9..87a4145 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -174,11 +174,10 @@ extern unsigned long reserved_va;
>  #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1)
>  #define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK)
>  
> -/* ??? These should be the larger of uintptr_t and target_ulong.  */
>  extern uintptr_t qemu_real_host_page_size;
> -extern uintptr_t qemu_real_host_page_mask;
> +extern intptr_t qemu_real_host_page_mask;
>  extern uintptr_t qemu_host_page_size;
> -extern uintptr_t qemu_host_page_mask;
> +extern intptr_t qemu_host_page_mask;
>  
>  #define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) & qemu_host_page_mask)
>  #define REAL_HOST_PAGE_ALIGN(addr) (((addr) + qemu_real_host_page_size - 1) & \
> diff --git a/translate-all.c b/translate-all.c
> index a940bd2..7a15109 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -118,7 +118,7 @@ typedef struct PageDesc {
>  #define V_L1_SHIFT (L1_MAP_ADDR_SPACE_BITS - TARGET_PAGE_BITS - V_L1_BITS)
>  
>  uintptr_t qemu_host_page_size;
> -uintptr_t qemu_host_page_mask;
> +intptr_t qemu_host_page_mask;
>  
>  /* The bottom level has pointers to PageDesc */
>  static void *l1_map[V_L1_SIZE];
> @@ -326,14 +326,14 @@ void page_size_init(void)
>      /* NOTE: we can always suppose that qemu_host_page_size >=
>         TARGET_PAGE_SIZE */
>      qemu_real_host_page_size = getpagesize();
> -    qemu_real_host_page_mask = ~(qemu_real_host_page_size - 1);
> +    qemu_real_host_page_mask = -(intptr_t)qemu_real_host_page_size;
>      if (qemu_host_page_size == 0) {
>          qemu_host_page_size = qemu_real_host_page_size;
>      }
>      if (qemu_host_page_size < TARGET_PAGE_SIZE) {
>          qemu_host_page_size = TARGET_PAGE_SIZE;
>      }
> -    qemu_host_page_mask = ~(qemu_host_page_size - 1);
> +    qemu_host_page_mask = -(intptr_t)qemu_host_page_size;
>  }
>  
>  static void page_init(void)
> diff --git a/translate-common.c b/translate-common.c
> index 619feb4..171222d 100644
> --- a/translate-common.c
> +++ b/translate-common.c
> @@ -21,7 +21,7 @@
>  #include "qom/cpu.h"
>  
>  uintptr_t qemu_real_host_page_size;
> -uintptr_t qemu_real_host_page_mask;
> +intptr_t qemu_real_host_page_mask;
>  
>  #ifndef CONFIG_USER_ONLY
>  /* mask must never be zero, except for A20 change call */
> 
> 

Ok, I tested this by adding

+     assert(HOST_PAGE_ALIGN(0x123456700ll) == 0x123457000ll);
+     assert(REAL_HOST_PAGE_ALIGN(0x123456700ll) == 0x123457000ll);

and doing a 32-bit x86_64-linux-user build.  Since Dave's patch does not
compile for user-mode emulation (ram_addr_t is a softmmu concept), I'm
queuing my patch for 2.5.

Paolo

Re: [Qemu-devel] Regression: Xen guest with 5G of RAM on 32bit fail to boot

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 02/12/2015 11:30, Paolo Bonzini wrote:
> > diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> > index f9998b9..87a4145 100644
> > --- a/include/exec/cpu-all.h
> > +++ b/include/exec/cpu-all.h
> > @@ -174,11 +174,10 @@ extern unsigned long reserved_va;
> >  #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1)
> >  #define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK)
> >  
> > -/* ??? These should be the larger of uintptr_t and target_ulong.  */
> >  extern uintptr_t qemu_real_host_page_size;
> > -extern uintptr_t qemu_real_host_page_mask;
> > +extern intptr_t qemu_real_host_page_mask;
> >  extern uintptr_t qemu_host_page_size;
> > -extern uintptr_t qemu_host_page_mask;
> > +extern intptr_t qemu_host_page_mask;
> >  
> >  #define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) & qemu_host_page_mask)
> >  #define REAL_HOST_PAGE_ALIGN(addr) (((addr) + qemu_real_host_page_size - 1) & \
> > diff --git a/translate-all.c b/translate-all.c
> > index a940bd2..7a15109 100644
> > --- a/translate-all.c
> > +++ b/translate-all.c
> > @@ -118,7 +118,7 @@ typedef struct PageDesc {
> >  #define V_L1_SHIFT (L1_MAP_ADDR_SPACE_BITS - TARGET_PAGE_BITS - V_L1_BITS)
> >  
> >  uintptr_t qemu_host_page_size;
> > -uintptr_t qemu_host_page_mask;
> > +intptr_t qemu_host_page_mask;
> >  
> >  /* The bottom level has pointers to PageDesc */
> >  static void *l1_map[V_L1_SIZE];
> > @@ -326,14 +326,14 @@ void page_size_init(void)
> >      /* NOTE: we can always suppose that qemu_host_page_size >=
> >         TARGET_PAGE_SIZE */
> >      qemu_real_host_page_size = getpagesize();
> > -    qemu_real_host_page_mask = ~(qemu_real_host_page_size - 1);
> > +    qemu_real_host_page_mask = -(intptr_t)qemu_real_host_page_size;
> >      if (qemu_host_page_size == 0) {
> >          qemu_host_page_size = qemu_real_host_page_size;
> >      }
> >      if (qemu_host_page_size < TARGET_PAGE_SIZE) {
> >          qemu_host_page_size = TARGET_PAGE_SIZE;
> >      }
> > -    qemu_host_page_mask = ~(qemu_host_page_size - 1);
> > +    qemu_host_page_mask = -(intptr_t)qemu_host_page_size;
> >  }
> >  
> >  static void page_init(void)
> > diff --git a/translate-common.c b/translate-common.c
> > index 619feb4..171222d 100644
> > --- a/translate-common.c
> > +++ b/translate-common.c
> > @@ -21,7 +21,7 @@
> >  #include "qom/cpu.h"
> >  
> >  uintptr_t qemu_real_host_page_size;
> > -uintptr_t qemu_real_host_page_mask;
> > +intptr_t qemu_real_host_page_mask;
> >  
> >  #ifndef CONFIG_USER_ONLY
> >  /* mask must never be zero, except for A20 change call */
> > 
> > 
> 
> Ok, I tested this by adding
> 
> +     assert(HOST_PAGE_ALIGN(0x123456700ll) == 0x123457000ll);
> +     assert(REAL_HOST_PAGE_ALIGN(0x123456700ll) == 0x123457000ll);
> 
> and doing a 32-bit x86_64-linux-user build.  Since Dave's patch does not
> compile for user-mode emulation (ram_addr_t is a softmmu concept), I'm
> queuing my patch for 2.5.

Hmm yes OK; my alternate was just making ram_addr_t being always defined.
I'm not sure we have any other type that's more suitable than ram_addr_t
but I guess the intptr_t will work for the mask.

Dave

> 
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] Regression: Xen guest with 5G of RAM on 32bit fail to boot

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 02/12/2015 11:30, Paolo Bonzini wrote:
>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
>> index f9998b9..87a4145 100644
>> --- a/include/exec/cpu-all.h
>> +++ b/include/exec/cpu-all.h
>> @@ -174,11 +174,10 @@ extern unsigned long reserved_va;
>>  #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1)
>>  #define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK)
>>  
>> -/* ??? These should be the larger of uintptr_t and target_ulong.  */
>>  extern uintptr_t qemu_real_host_page_size;
>> -extern uintptr_t qemu_real_host_page_mask;
>> +extern intptr_t qemu_real_host_page_mask;
>>  extern uintptr_t qemu_host_page_size;
>> -extern uintptr_t qemu_host_page_mask;
>> +extern intptr_t qemu_host_page_mask;
>>  
>>  #define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) & qemu_host_page_mask)
>>  #define REAL_HOST_PAGE_ALIGN(addr) (((addr) + qemu_real_host_page_size - 1) & \
>> diff --git a/translate-all.c b/translate-all.c
>> index a940bd2..7a15109 100644
>> --- a/translate-all.c
>> +++ b/translate-all.c
>> @@ -118,7 +118,7 @@ typedef struct PageDesc {
>>  #define V_L1_SHIFT (L1_MAP_ADDR_SPACE_BITS - TARGET_PAGE_BITS - V_L1_BITS)
>>  
>>  uintptr_t qemu_host_page_size;
>> -uintptr_t qemu_host_page_mask;
>> +intptr_t qemu_host_page_mask;
>>  
>>  /* The bottom level has pointers to PageDesc */
>>  static void *l1_map[V_L1_SIZE];
>> @@ -326,14 +326,14 @@ void page_size_init(void)
>>      /* NOTE: we can always suppose that qemu_host_page_size >=
>>         TARGET_PAGE_SIZE */
>>      qemu_real_host_page_size = getpagesize();
>> -    qemu_real_host_page_mask = ~(qemu_real_host_page_size - 1);
>> +    qemu_real_host_page_mask = -(intptr_t)qemu_real_host_page_size;
>>      if (qemu_host_page_size == 0) {
>>          qemu_host_page_size = qemu_real_host_page_size;
>>      }
>>      if (qemu_host_page_size < TARGET_PAGE_SIZE) {
>>          qemu_host_page_size = TARGET_PAGE_SIZE;
>>      }
>> -    qemu_host_page_mask = ~(qemu_host_page_size - 1);
>> +    qemu_host_page_mask = -(intptr_t)qemu_host_page_size;
>>  }
>>  
>>  static void page_init(void)
>> diff --git a/translate-common.c b/translate-common.c
>> index 619feb4..171222d 100644
>> --- a/translate-common.c
>> +++ b/translate-common.c
>> @@ -21,7 +21,7 @@
>>  #include "qom/cpu.h"
>>  
>>  uintptr_t qemu_real_host_page_size;
>> -uintptr_t qemu_real_host_page_mask;
>> +intptr_t qemu_real_host_page_mask;
>>  
>>  #ifndef CONFIG_USER_ONLY
>>  /* mask must never be zero, except for A20 change call */
>> 
>> 
>
> Ok, I tested this by adding
>
> +     assert(HOST_PAGE_ALIGN(0x123456700ll) == 0x123457000ll);
> +     assert(REAL_HOST_PAGE_ALIGN(0x123456700ll) == 0x123457000ll);
>
> and doing a 32-bit x86_64-linux-user build.  Since Dave's patch does not
> compile for user-mode emulation (ram_addr_t is a softmmu concept), I'm
> queuing my patch for 2.5.
>
> Paolo

Reviewed-by: Juan Quintela <quintela@redhat.com>

Dave patch massively broke linux-user.  Going that route can make sense,
but not so late on the cycle.