| Message ID | 20190311054252.6094-1-richardw.yang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] exec.c: refactor function flatview_add_to_dispatch() | expand |
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); > } > } > >
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 <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. > 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); >> } >> } >> >>
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 + */ 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); } }
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(-)