Re: [Qemu-devel] [PATCH v3] exec.c: refactor function flatview_add_to_dispatch()

[Paolo Bonzini <pbonzini@redhat.com>]

On 11/03/19 06:42, 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 the procedure could be simplified into these three steps
> instead of a loop iteration.
> 
> This patch refactors the function into three corresponding steps and
> adds some comment to clarify it.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> 
> ---
> v3:
>   * pass int128_make64() instead of 0 to int128_gt()
> v2:
>   * removes the loop iteration as suggested by Paolo
> ---
>  exec.c | 50 +++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 17 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 0959b00c06..79cd561b3b 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1598,34 +1598,50 @@ 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

The last paragraph is a duplicate of the comments in the function, so it
can be deleted.  I did that and queued the patch.

Paolo

> + */
>  void flatview_add_to_dispatch(FlatView *fv, MemoryRegionSection *section)
>  {
> -    MemoryRegionSection now = *section, remain = *section;
> +    MemoryRegionSection now, remain = *section;
>      Int128 page_size = int128_make64(TARGET_PAGE_SIZE);
>  
> -    if (now.offset_within_address_space & ~TARGET_PAGE_MASK) {
> -        uint64_t left = TARGET_PAGE_ALIGN(now.offset_within_address_space)
> -                       - now.offset_within_address_space;
> +    /* register first subpage */
> +    if (remain.offset_within_address_space & ~TARGET_PAGE_MASK) {
> +        uint64_t left = TARGET_PAGE_ALIGN(remain.offset_within_address_space)
> +                        - remain.offset_within_address_space;
>  
> +        now = remain;
>          now.size = int128_min(int128_make64(left), now.size);
> +        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, &now);
> -    } else {
> -        now.size = int128_zero();
>      }
> -    while (int128_ne(remain.size, now.size)) {
> +
> +    /* register page */
> +    if (int128_ge(remain.size, page_size)) {
> +        now = remain;
> +        now.size = int128_and(now.size, int128_neg(page_size));
>          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);
> -        }
> +        register_multipage(fv, &now);
> +    }
> +
> +    /* register last subpage */
> +    if (int128_gt(remain.size, int128_make64(0))) {
> +        register_subpage(fv, &remain);
>      }
>  }
>  
>