diff mbox series

[v2,1/7] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN

Message ID 20191023154505.30521-2-richard.henderson@linaro.org
State New
Headers show
Series exec: Improve code for TARGET_PAGE_BITS_VARY | expand

Commit Message

Richard Henderson Oct. 23, 2019, 3:44 p.m. UTC 
From: Wei Yang <richardw.yang@linux.intel.com>

Use ROUND_UP() to define, which is a little bit easy to read.

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
Message-Id: <20191013021145.16011-2-richardw.yang@linux.intel.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/cpu-all.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Philippe Mathieu-Daudé Oct. 24, 2019, 11:52 a.m. UTC | #1 
On 10/23/19 5:44 PM, Richard Henderson wrote:
> From: Wei Yang <richardw.yang@linux.intel.com>
> 
> Use ROUND_UP() to define, which is a little bit easy to read.
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

^ one is enough ;)

> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> Message-Id: <20191013021145.16011-2-richardw.yang@linux.intel.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   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
>
Paolo Bonzini Oct. 24, 2019, 12:04 p.m. UTC | #2 
On 24/10/19 13:52, Philippe Mathieu-Daudé wrote:
>> 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
>>

Isn't this the patch where Richard pointed out that the compiler
generates worse code?

Paolo
Richard Henderson Oct. 24, 2019, 2:06 p.m. UTC | #3 
On 10/24/19 8:04 AM, Paolo Bonzini wrote:
> On 24/10/19 13:52, Philippe Mathieu-Daudé wrote:
>>> 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
>>>
> 
> Isn't this the patch where Richard pointed out that the compiler
> generates worse code?

Richard confused ROUND_UP with QEMU_ALIGN_UP.


r~
Paolo Bonzini Oct. 24, 2019, 2:14 p.m. UTC | #4 
On 24/10/19 16:06, Richard Henderson wrote:
> On 10/24/19 8:04 AM, Paolo Bonzini wrote:
>> On 24/10/19 13:52, Philippe Mathieu-Daudé wrote:
>>>> 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
>>>>
>>
>> Isn't this the patch where Richard pointed out that the compiler
>> generates worse code?
> 
> Richard confused ROUND_UP with QEMU_ALIGN_UP.

Uh, those are both really badly named.  Especially considering that
DIV_ROUND_UP divides the result of QEMU_ALIGN_UP.  Should we consider
swapping them?!?

Paolo
Richard Henderson Oct. 25, 2019, 11:48 a.m. UTC | #5 
On 10/24/19 10:14 AM, Paolo Bonzini wrote:
> On 24/10/19 16:06, Richard Henderson wrote:
>> Richard confused ROUND_UP with QEMU_ALIGN_UP.
> 
> Uh, those are both really badly named.  Especially considering that
> DIV_ROUND_UP divides the result of QEMU_ALIGN_UP.  Should we consider
> swapping them?!?

Perhaps not swapping, but perhaps renaming ROUND_UP to POW2_ROUND_UP or
something.  I dunno, naming is hard.  :-P


r~
diff mbox series

Patch

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