Message ID | 20190304065102.26447-1-richardw.yang@linux.intel.com |
---|---|
State | New |
Headers | show |
Series | exec.c: remove an unnecessary condition in flatview_add_to_dispatch() | expand |
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); > } >
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?
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); }
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(-)