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.y...@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
