[1/2] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN
diff mbox series

Message ID 20191013021145.16011-2-richardw.yang@linux.intel.com
State New
Headers show
Series
  • cleanup on page size
Related show

Commit Message

Wei Yang Oct. 13, 2019, 2:11 a.m. UTC
Use ROUND_UP() to define, which is a little bit easy to read.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 include/exec/cpu-all.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Michael S. Tsirkin Oct. 13, 2019, 6:34 a.m. UTC | #1
On Sun, Oct 13, 2019 at 10:11:44AM +0800, Wei Yang wrote:
> Use ROUND_UP() to define, which is a little bit easy to read.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  include/exec/cpu-all.h | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index ad9ab85eb3..255bb186ac 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -220,7 +220,7 @@ extern int target_page_bits;
>  
>  #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS)
>  #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1)
> -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK)
> +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE)
>  
>  /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even
>   * when intptr_t is 32-bit and we are aligning a long long.
> @@ -228,9 +228,8 @@ extern int target_page_bits;
>  extern uintptr_t qemu_host_page_size;
>  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) & \
> -                                    qemu_real_host_page_mask)
> +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size)
> +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_real_host_page_size)
>  
>  /* same as PROT_xxx */
>  #define PAGE_READ      0x0001
> -- 
> 2.17.1
David Gibson Oct. 13, 2019, 9:27 a.m. UTC | #2
On Sun, Oct 13, 2019 at 10:11:44AM +0800, Wei Yang wrote:
> Use ROUND_UP() to define, which is a little bit easy to read.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  include/exec/cpu-all.h | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index ad9ab85eb3..255bb186ac 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -220,7 +220,7 @@ extern int target_page_bits;
>  
>  #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS)
>  #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1)
> -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK)
> +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE)
>  
>  /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even
>   * when intptr_t is 32-bit and we are aligning a long long.
> @@ -228,9 +228,8 @@ extern int target_page_bits;
>  extern uintptr_t qemu_host_page_size;
>  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) & \
> -                                    qemu_real_host_page_mask)
> +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size)
> +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_real_host_page_size)
>  
>  /* same as PROT_xxx */
>  #define PAGE_READ      0x0001
Richard Henderson Oct. 13, 2019, 3:56 p.m. UTC | #3
On 10/12/19 10:11 PM, Wei Yang wrote:
> Use ROUND_UP() to define, which is a little bit easy to read.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  include/exec/cpu-all.h | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index ad9ab85eb3..255bb186ac 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -220,7 +220,7 @@ extern int target_page_bits;
>  
>  #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS)
>  #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1)
> -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK)
> +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE)
>  
>  /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even
>   * when intptr_t is 32-bit and we are aligning a long long.
> @@ -228,9 +228,8 @@ extern int target_page_bits;
>  extern uintptr_t qemu_host_page_size;
>  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) & \
> -                                    qemu_real_host_page_mask)
> +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size)
> +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_real_host_page_size)


No, please.

(1) The compiler does not know that qemu_*host_page_size is a power of 2, and
will generate a real division at runtime.  The same is true for
TARGET_PAGE_SIZE when TARGET_PAGE_BITS_VARY.

(2) The first hunk conflicts with an in-flight patch of mine:

https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04526.html


r~
David Gibson Oct. 13, 2019, 11:36 p.m. UTC | #4
On Sun, Oct 13, 2019 at 11:56:35AM -0400, Richard Henderson wrote:
> On 10/12/19 10:11 PM, Wei Yang wrote:
> > Use ROUND_UP() to define, which is a little bit easy to read.
> > 
> > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> > ---
> >  include/exec/cpu-all.h | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> > index ad9ab85eb3..255bb186ac 100644
> > --- a/include/exec/cpu-all.h
> > +++ b/include/exec/cpu-all.h
> > @@ -220,7 +220,7 @@ extern int target_page_bits;
> >  
> >  #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS)
> >  #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1)
> > -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK)
> > +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE)
> >  
> >  /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even
> >   * when intptr_t is 32-bit and we are aligning a long long.
> > @@ -228,9 +228,8 @@ extern int target_page_bits;
> >  extern uintptr_t qemu_host_page_size;
> >  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) & \
> > -                                    qemu_real_host_page_mask)
> > +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size)
> > +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_real_host_page_size)
> 
> 
> No, please.
> 
> (1) The compiler does not know that qemu_*host_page_size is a power of 2, and
> will generate a real division at runtime.  The same is true for
> TARGET_PAGE_SIZE when TARGET_PAGE_BITS_VARY.

Ouch, good point, I didn't think of that when I gave an R-b.

> (2) The first hunk conflicts with an in-flight patch of mine:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04526.html
> 
> 
> r~
>
Wei Yang Oct. 14, 2019, 1:01 a.m. UTC | #5
On Sun, Oct 13, 2019 at 11:56:35AM -0400, Richard Henderson wrote:
>On 10/12/19 10:11 PM, Wei Yang wrote:
>> Use ROUND_UP() to define, which is a little bit easy to read.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  include/exec/cpu-all.h | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>> 
>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
>> index ad9ab85eb3..255bb186ac 100644
>> --- a/include/exec/cpu-all.h
>> +++ b/include/exec/cpu-all.h
>> @@ -220,7 +220,7 @@ extern int target_page_bits;
>>  
>>  #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS)
>>  #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1)
>> -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK)
>> +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE)
>>  
>>  /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even
>>   * when intptr_t is 32-bit and we are aligning a long long.
>> @@ -228,9 +228,8 @@ extern int target_page_bits;
>>  extern uintptr_t qemu_host_page_size;
>>  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) & \
>> -                                    qemu_real_host_page_mask)
>> +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size)
>> +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_real_host_page_size)
>
>
>No, please.
>
>(1) The compiler does not know that qemu_*host_page_size is a power of 2, and
>will generate a real division at runtime.  The same is true for
>TARGET_PAGE_SIZE when TARGET_PAGE_BITS_VARY.
>

Confused

The definition of ROUND_UP is:

#define ROUND_UP(n, d) (((n) + (d) - 1) & -(0 ? (n) : (d)))

Why it will do division? This will be expanded to the same form as the
original code, if my understanding is correct. Would you mind telling me more?

>(2) The first hunk conflicts with an in-flight patch of mine:
>
>https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04526.html
>
>
>r~
Richard Henderson Oct. 14, 2019, 2:38 a.m. UTC | #6
On 10/13/19 6:01 PM, Wei Yang wrote:
>> No, please.
>>
>> (1) The compiler does not know that qemu_*host_page_size is a power of 2, and
>> will generate a real division at runtime.  The same is true for
>> TARGET_PAGE_SIZE when TARGET_PAGE_BITS_VARY.
>>
> 
> Confused
> 
> The definition of ROUND_UP is:
> 
> #define ROUND_UP(n, d) (((n) + (d) - 1) & -(0 ? (n) : (d)))

Ah, my bad, I did confuse this with QEMU_ALIGN_UP.

Hmm.

	lea	-1(n, size), t
	neg	size
	and	size, t

vs

	mov	mask, t
	not	t
	add	n, t
	and	mask, t

which is what I proposed here

>> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04526.html

I'm ok with your version.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
Wei Yang Oct. 14, 2019, 3:19 a.m. UTC | #7
On Sun, Oct 13, 2019 at 07:38:04PM -0700, Richard Henderson wrote:
>On 10/13/19 6:01 PM, Wei Yang wrote:
>>> No, please.
>>>
>>> (1) The compiler does not know that qemu_*host_page_size is a power of 2, and
>>> will generate a real division at runtime.  The same is true for
>>> TARGET_PAGE_SIZE when TARGET_PAGE_BITS_VARY.
>>>
>> 
>> Confused
>> 
>> The definition of ROUND_UP is:
>> 
>> #define ROUND_UP(n, d) (((n) + (d) - 1) & -(0 ? (n) : (d)))
>
>Ah, my bad, I did confuse this with QEMU_ALIGN_UP.
>
>Hmm.
>
>	lea	-1(n, size), t
>	neg	size
>	and	size, t
>
>vs
>
>	mov	mask, t
>	not	t
>	add	n, t
>	and	mask, t
>
>which is what I proposed here
>
>>> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04526.html
>
>I'm ok with your version.
>
>Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>

Thanks for your clarification.

Have a nice day

>
>r~
>
Juan Quintela Oct. 14, 2019, 4:25 p.m. UTC | #8
Wei Yang <richardw.yang@linux.intel.com> wrote:
> Use ROUND_UP() to define, which is a little bit easy to read.
>
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

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

Patch
diff mbox series

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index ad9ab85eb3..255bb186ac 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -220,7 +220,7 @@  extern int target_page_bits;
 
 #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS)
 #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1)
-#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK)
+#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE)
 
 /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even
  * when intptr_t is 32-bit and we are aligning a long long.
@@ -228,9 +228,8 @@  extern int target_page_bits;
 extern uintptr_t qemu_host_page_size;
 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) & \
-                                    qemu_real_host_page_mask)
+#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size)
+#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_real_host_page_size)
 
 /* same as PROT_xxx */
 #define PAGE_READ      0x0001