[Qemu-devel] [PATCH] exec.c: remove an unnecessary condition in flatview_add_to_dispatch()

[Qemu-devel] [PATCH] exec.c: remove an unnecessary condition in flatview_add_to_dispatch()

[Wei Yang]

flatview_add_to_dispatch() registers page based on the condition of
*section*, which may looks like this:

    |s|PPPPPPP|s|

where s stands for subpage and P for page.

The procedure of this function could be described as:

    - register first subpage
    - register page
    - register last subpage

This means only the first offset_within_address_space could be not
TARGET_PAGE_SIZE aligned. During the wile loop, this will not happen.

This patch just removes the unnecessary condition and adds some comment
to clarify it.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 exec.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/exec.c b/exec.c
index 518064530b..e6221d52ba 100644
--- a/exec.c
+++ b/exec.c
@@ -1599,12 +1599,26 @@ static void register_multipage(FlatView *fv,
     phys_page_set(d, start_addr >> TARGET_PAGE_BITS, num_pages, section_index);
 }
 
+/*
+ * The range in *section* may look like this:
+ *
+ *      |s|PPPPPPP|s|
+ *
+ * where s stands for subpage and P for page.
+ *
+ * The procedure in following function could be described as:
+ *
+ * - register first subpage
+ * - register page
+ * - register last subpage
+ */
 void flatview_add_to_dispatch(FlatView *fv, MemoryRegionSection *section)
 {
     MemoryRegionSection now = *section, remain = *section;
     Int128 page_size = int128_make64(TARGET_PAGE_SIZE);
 
     if (now.offset_within_address_space & ~TARGET_PAGE_MASK) {
+        /* register first subpage */
         uint64_t left = TARGET_PAGE_ALIGN(now.offset_within_address_space)
                        - now.offset_within_address_space;
 
@@ -1619,11 +1633,10 @@ void flatview_add_to_dispatch(FlatView *fv, MemoryRegionSection *section)
         remain.offset_within_region += int128_get64(now.size);
         now = remain;
         if (int128_lt(remain.size, page_size)) {
-            register_subpage(fv, &now);
-        } else if (remain.offset_within_address_space & ~TARGET_PAGE_MASK) {
-            now.size = page_size;
+            /* register last subpage */
             register_subpage(fv, &now);
         } else {
+            /* register page */
             now.size = int128_and(now.size, int128_neg(page_size));
             register_multipage(fv, &now);
         }
-- 
2.19.1

Re: [Qemu-devel] [PATCH] exec.c: remove an unnecessary condition in flatview_add_to_dispatch()

[Paolo Bonzini]

On 04/03/19 07:51, Wei Yang wrote:
> flatview_add_to_dispatch() registers page based on the condition of
> *section*, which may looks like this:
> 
>     |s|PPPPPPP|s|
> 
> where s stands for subpage and P for page.
> 
> The procedure of this function could be described as:
> 
>     - register first subpage
>     - register page
>     - register last subpage
> 
> This means only the first offset_within_address_space could be not
> TARGET_PAGE_SIZE aligned. During the wile loop, this will not happen.
> 
> This patch just removes the unnecessary condition and adds some comment
> to clarify it.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Good point!

Here's another way to write it without a while loop at all:

diff --git a/exec.c b/exec.c
index cb09c531a7..e85e3d9fb8 100644
--- a/exec.c
+++ b/exec.c
@@ -1601,33 +1601,37 @@ static void register_multipage(FlatView *fv,
 
 void flatview_add_to_dispatch(FlatView *fv, MemoryRegionSection *section)
 {
-    MemoryRegionSection now = *section, remain = *section;
+    MemoryRegionSection remain = *section;
     Int128 page_size = int128_make64(TARGET_PAGE_SIZE);
 
     if (now.offset_within_address_space & ~TARGET_PAGE_MASK) {
+        MemoryRegionSection now = remain;
         uint64_t left = TARGET_PAGE_ALIGN(now.offset_within_address_space)
                        - now.offset_within_address_space;
 
         now.size = int128_min(int128_make64(left), now.size);
         register_subpage(fv, &now);
-    } else {
-        now.size = int128_zero();
-    }
-    while (int128_ne(remain.size, now.size)) {
+        if (int128_eq(remain.size, now.size)) {
+            return;
+        }
         remain.size = int128_sub(remain.size, now.size);
         remain.offset_within_address_space += int128_get64(now.size);
         remain.offset_within_region += int128_get64(now.size);
-        now = remain;
-        if (int128_lt(remain.size, page_size)) {
-            register_subpage(fv, &now);
-        } else if (remain.offset_within_address_space & ~TARGET_PAGE_MASK) {
-            now.size = page_size;
-            register_subpage(fv, &now);
-        } else {
-            now.size = int128_and(now.size, int128_neg(page_size));
-            register_multipage(fv, &now);
+    }
+
+    if (int128_ge(remain.size, page_size)) {
+        MemoryRegionSection now = remain;
+        now.size = int128_and(now.size, int128_neg(page_size));
+        register_multipage(fv, &now);
+        if (int128_eq(remain.size, now.size)) {
+            return;
         }
+        remain.size = int128_sub(remain.size, now.size);
+        remain.offset_within_address_space += int128_get64(now.size);
+        remain.offset_within_region += int128_get64(now.size);
     }
+
+    register_subpage(fv, &remain);
 }
 
 void qemu_flush_coalesced_mmio_buffer(void)

There are a handful of duplicated lines, but overall I think it's
even clearer.

Paolo

> ---
>  exec.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 518064530b..e6221d52ba 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1599,12 +1599,26 @@ static void register_multipage(FlatView *fv,
>      phys_page_set(d, start_addr >> TARGET_PAGE_BITS, num_pages, section_index);
>  }
>  
> +/*
> + * The range in *section* may look like this:
> + *
> + *      |s|PPPPPPP|s|
> + *
> + * where s stands for subpage and P for page.
> + *
> + * The procedure in following function could be described as:
> + *
> + * - register first subpage
> + * - register page
> + * - register last subpage
> + */
>  void flatview_add_to_dispatch(FlatView *fv, MemoryRegionSection *section)
>  {
>      MemoryRegionSection now = *section, remain = *section;
>      Int128 page_size = int128_make64(TARGET_PAGE_SIZE);
>  
>      if (now.offset_within_address_space & ~TARGET_PAGE_MASK) {
> +        /* register first subpage */
>          uint64_t left = TARGET_PAGE_ALIGN(now.offset_within_address_space)
>                         - now.offset_within_address_space;
>  
> @@ -1619,11 +1633,10 @@ void flatview_add_to_dispatch(FlatView *fv, MemoryRegionSection *section)
>          remain.offset_within_region += int128_get64(now.size);
>          now = remain;
>          if (int128_lt(remain.size, page_size)) {
> -            register_subpage(fv, &now);
> -        } else if (remain.offset_within_address_space & ~TARGET_PAGE_MASK) {
> -            now.size = page_size;
> +            /* register last subpage */
>              register_subpage(fv, &now);
>          } else {
> +            /* register page */
>              now.size = int128_and(now.size, int128_neg(page_size));
>              register_multipage(fv, &now);
>          }
> 

Re: [Qemu-devel] [PATCH] exec.c: remove an unnecessary condition in flatview_add_to_dispatch()

[Wei Yang]

On Mon, Mar 04, 2019 at 10:55:39AM +0100, Paolo Bonzini wrote:
>On 04/03/19 07:51, Wei Yang wrote:
>> flatview_add_to_dispatch() registers page based on the condition of
>> *section*, which may looks like this:
>> 
>>     |s|PPPPPPP|s|
>> 
>> where s stands for subpage and P for page.
>> 
>> The procedure of this function could be described as:
>> 
>>     - register first subpage
>>     - register page
>>     - register last subpage
>> 
>> This means only the first offset_within_address_space could be not
>> TARGET_PAGE_SIZE aligned. During the wile loop, this will not happen.
>> 
>> This patch just removes the unnecessary condition and adds some comment
>> to clarify it.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>
>Good point!
>
>Here's another way to write it without a while loop at all:
>

Yes, you are right. We can do this without loop.

>diff --git a/exec.c b/exec.c
>index cb09c531a7..e85e3d9fb8 100644
>--- a/exec.c
>+++ b/exec.c
>@@ -1601,33 +1601,37 @@ static void register_multipage(FlatView *fv,
> 
> void flatview_add_to_dispatch(FlatView *fv, MemoryRegionSection *section)
> {
>-    MemoryRegionSection now = *section, remain = *section;
>+    MemoryRegionSection remain = *section;
>     Int128 page_size = int128_make64(TARGET_PAGE_SIZE);
> 
>     if (now.offset_within_address_space & ~TARGET_PAGE_MASK) {
>+        MemoryRegionSection now = remain;
>         uint64_t left = TARGET_PAGE_ALIGN(now.offset_within_address_space)
>                        - now.offset_within_address_space;
> 
>         now.size = int128_min(int128_make64(left), now.size);
>         register_subpage(fv, &now);
>-    } else {
>-        now.size = int128_zero();
>-    }
>-    while (int128_ne(remain.size, now.size)) {
>+        if (int128_eq(remain.size, now.size)) {
>+            return;
>+        }
>         remain.size = int128_sub(remain.size, now.size);
>         remain.offset_within_address_space += int128_get64(now.size);
>         remain.offset_within_region += int128_get64(now.size);
>-        now = remain;
>-        if (int128_lt(remain.size, page_size)) {
>-            register_subpage(fv, &now);
>-        } else if (remain.offset_within_address_space & ~TARGET_PAGE_MASK) {
>-            now.size = page_size;
>-            register_subpage(fv, &now);
>-        } else {
>-            now.size = int128_and(now.size, int128_neg(page_size));
>-            register_multipage(fv, &now);
>+    }
>+
>+    if (int128_ge(remain.size, page_size)) {
>+        MemoryRegionSection now = remain;
>+        now.size = int128_and(now.size, int128_neg(page_size));
>+        register_multipage(fv, &now);
>+        if (int128_eq(remain.size, now.size)) {
>+            return;
>         }
>+        remain.size = int128_sub(remain.size, now.size);
>+        remain.offset_within_address_space += int128_get64(now.size);
>+        remain.offset_within_region += int128_get64(now.size);
>     }
>+
>+    register_subpage(fv, &remain);
> }
> 
> void qemu_flush_coalesced_mmio_buffer(void)
>
>There are a handful of duplicated lines, but overall I think it's
>even clearer.
>
>Paolo
>
>> ---
>>  exec.c | 19 ++++++++++++++++---
>>  1 file changed, 16 insertions(+), 3 deletions(-)
>> 
>> diff --git a/exec.c b/exec.c
>> index 518064530b..e6221d52ba 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1599,12 +1599,26 @@ static void register_multipage(FlatView *fv,
>>      phys_page_set(d, start_addr >> TARGET_PAGE_BITS, num_pages, section_index);
>>  }
>>  
>> +/*
>> + * The range in *section* may look like this:
>> + *
>> + *      |s|PPPPPPP|s|
>> + *
>> + * where s stands for subpage and P for page.
>> + *
>> + * The procedure in following function could be described as:
>> + *
>> + * - register first subpage
>> + * - register page
>> + * - register last subpage
>> + */
>>  void flatview_add_to_dispatch(FlatView *fv, MemoryRegionSection *section)
>>  {
>>      MemoryRegionSection now = *section, remain = *section;
>>      Int128 page_size = int128_make64(TARGET_PAGE_SIZE);
>>  
>>      if (now.offset_within_address_space & ~TARGET_PAGE_MASK) {
>> +        /* register first subpage */
>>          uint64_t left = TARGET_PAGE_ALIGN(now.offset_within_address_space)
>>                         - now.offset_within_address_space;
>>  
>> @@ -1619,11 +1633,10 @@ void flatview_add_to_dispatch(FlatView *fv, MemoryRegionSection *section)
>>          remain.offset_within_region += int128_get64(now.size);
>>          now = remain;
>>          if (int128_lt(remain.size, page_size)) {
>> -            register_subpage(fv, &now);
>> -        } else if (remain.offset_within_address_space & ~TARGET_PAGE_MASK) {
>> -            now.size = page_size;
>> +            /* register last subpage */
>>              register_subpage(fv, &now);
>>          } else {
>> +            /* register page */
>>              now.size = int128_and(now.size, int128_neg(page_size));
>>              register_multipage(fv, &now);
>>          }
>> 
>

You want me to send v2 with this version? or you would do it by
yourself?

-- 
Wei Yang
Help you, Help me