diff mbox

Regression: Xen guest with 5G of RAM on 32bit fail to boot

Message ID 565EC83D.9020907@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini Dec. 2, 2015, 10:30 a.m. UTC 
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:

Comments

Dr. David Alan Gilbert Dec. 2, 2015, 10:44 a.m. UTC | #1 
* 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
Paolo Bonzini Dec. 2, 2015, 11:59 a.m. UTC | #2 
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
Dr. David Alan Gilbert Dec. 2, 2015, 12:05 p.m. UTC | #3 
* 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
Juan Quintela Dec. 2, 2015, 12:11 p.m. UTC | #4 
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.
diff mbox

Patch

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 */