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

[Wei Yang]

On Mon, Mar 11, 2019 at 02:42:58PM +0100, Paolo Bonzini wrote:
>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 <address@hidden>
>> 
>> ---
>> 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.
>

Thanks :-)

>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);
>>      }
>>  }
>>  
>> 

-- 
Wei Yang
Help you, Help me