Re: [Qemu-devel] [PATCH 3/6] Fix last page errors in page_set_flags and page_check_range.

[Blue Swirl]

On Fri, Feb 12, 2010 at 12:57 AM, Richard Henderson <address@hidden> wrote:
> The addr < end comparison prevents the last page from being
> iterated; an iteration based on length avoids this problem.

Please make separate patches for unrelated changes. Now the essence of
the patch is very hard to see. Also pure formatting changes are not
very useful.

> ---
>  exec.c |   54 +++++++++++++++++++++++++++---------------------------
>  1 files changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 766568b..ebbe6d0 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2222,35 +2222,31 @@ void page_dump(FILE *f)
>
>  int page_get_flags(target_ulong address)
>  {
> -    PageDesc *p;
> -
> -    p = page_find(address >> TARGET_PAGE_BITS);
> -    if (!p)
> -        return 0;
> -    return p->flags;
> +    PageDesc *p = page_find(address >> TARGET_PAGE_BITS);
> +    return p ? p->flags : 0;
>  }

For example this change does not make any difference but just add confusion.

>
> -/* modify the flags of a page and invalidate the code if
> -   necessary. The flag PAGE_WRITE_ORG is positioned automatically
> -   depending on PAGE_WRITE */
> +/* Modify the flags of a page and invalidate the code if necessary.
> +   The flag PAGE_WRITE_ORG is positioned automatically depending
> +   on PAGE_WRITE.  The mmap_lock should already be held.  */
>  void page_set_flags(target_ulong start, target_ulong end, int flags)
>  {
> -    PageDesc *p;
> -    target_ulong addr;
> +    target_ulong addr, len;
>
> -    /* mmap_lock should already be held.  */
>     start = start & TARGET_PAGE_MASK;
>     end = TARGET_PAGE_ALIGN(end);
> -    if (flags & PAGE_WRITE)
> +
> +    if (flags & PAGE_WRITE) {
>         flags |= PAGE_WRITE_ORG;
> -    for(addr = start; addr < end; addr += TARGET_PAGE_SIZE) {
> -        p = page_find_alloc(addr >> TARGET_PAGE_BITS);
> -        /* We may be called for host regions that are outside guest
> -           address space.  */

Why remove the comment, is it no longer true?

> -        if (!p)
> -            return;
> -        /* if the write protection is set, then we invalidate the code
> -           inside */
> +    }
> +
> +    for (addr = start, len = end - start;
> +         len != 0;
> +         len -= TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) {
> +        PageDesc *p = page_find_alloc(addr >> TARGET_PAGE_BITS, 1);
> +
> +        /* If the write protection bit is set, then we invalidate
> +           the code inside.  */
>         if (!(p->flags & PAGE_WRITE) &&
>             (flags & PAGE_WRITE) &&
>             p->first_tb) {
> @@ -2266,18 +2262,22 @@ int page_check_range(target_ulong start, target_ulong 
> len, int flags)
>     target_ulong end;
>     target_ulong addr;
>
> -    if (start + len < start)
> -        /* we've wrapped around */
> +    if (start + len - 1 < start) {
> +        /* We've wrapped around.  */
>         return -1;
> +    }
>
> -    end = TARGET_PAGE_ALIGN(start+len); /* must do before we loose bits in 
> the next step */
> +    /* END must be computed before we drop bits from START.  */
> +    end = TARGET_PAGE_ALIGN(start + len);
>     start = start & TARGET_PAGE_MASK;
>
> -    for(addr = start; addr < end; addr += TARGET_PAGE_SIZE) {
> +    for (addr = start, len = end - start;
> +         len != 0;
> +         len -= TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) {
>         p = page_find(addr >> TARGET_PAGE_BITS);
> -        if( !p )
> +        if (!p)
>             return -1;
> -        if( !(p->flags & PAGE_VALID) )
> +        if (!(p->flags & PAGE_VALID))
>             return -1;
>
>         if ((flags & PAGE_READ) && !(p->flags & PAGE_READ))
> --
> 1.6.6
>
>
>
>