[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] exec.c: remove an unnecessary condition in flat
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH] exec.c: remove an unnecessary condition in flatview_add_to_dispatch() |
Date: |
Mon, 4 Mar 2019 10:55:39 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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 <address@hidden>
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);
> }
>