[3/6] Fix last page errors in page_set_flags and page_check_range.

The addr < end comparison prevents the last page from being
iterated; an iteration based on length avoids this problem.
---
 exec.c |   54 +++++++++++++++++++++++++++---------------------------
 1 files changed, 27 insertions(+), 27 deletions(-)

On Fri, Feb 12, 2010 at 12:57 AM, Richard Henderson <rth@twiddle.net> 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
>
>
>
>

On 02/12/2010 11:47 AM, Blue Swirl wrote:
> 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.

Is this just about page_get_flags, or was there some other pure 
formatting change to which you object?  For instance:

>> -    if (start + len < start)
>> -        /* we've wrapped around */
>> +    if (start + len - 1 < start) {
>> +        /* We've wrapped around.  */
>>         return -1;
>> +    }

Only the first line is required to fix an off-by-one error.  But if I 
don't add the braces, generally the patch will be bounced for that.

>> -        /* We may be called for host regions that are outside guest
>> -           address space.  */
>
> Why remove the comment, is it no longer true?

Yes, after the entire patch series is applied.  Indeed, I believe that 
many of the addresses rejected here were *not* in fact outside the guest 
address space, merely that page_find_alloc did not accurately know what 
the guest address space was.

I think it was a mistake to ever consider usage of this function with 
out-of-band addresses as valid -- it adds a great deal of confusion.

I could add an assertion here to make sure, if you like.

If this were C++, it might have been interesting to try to use the type 
system to keep the host and guest address spaces forcibly separate.  But 
since this is plain C, I think wrapping everything in structures and 
access macros would just be too ugly.


r~

On Fri, Feb 12, 2010 at 10:16 PM, Richard Henderson <rth@twiddle.net> wrote:
> On 02/12/2010 11:47 AM, Blue Swirl wrote:
>>
>> 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.
>
> Is this just about page_get_flags, or was there some other pure formatting
> change to which you object?  For instance:

Also this one:
-    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;

and these:

-        if( !p )
+        if (!p)
            return -1;
-        if( !(p->flags & PAGE_VALID) )
+        if (!(p->flags & PAGE_VALID))

>
>>> -    if (start + len < start)
>>> -        /* we've wrapped around */
>>> +    if (start + len - 1 < start) {
>>> +        /* We've wrapped around.  */
>>>        return -1;
>>> +    }
>
> Only the first line is required to fix an off-by-one error.  But if I don't
> add the braces, generally the patch will be bounced for that.

That change is OK.

>>> -        /* We may be called for host regions that are outside guest
>>> -           address space.  */
>>
>> Why remove the comment, is it no longer true?
>
> Yes, after the entire patch series is applied.  Indeed, I believe that many
> of the addresses rejected here were *not* in fact outside the guest address
> space, merely that page_find_alloc did not accurately know what the guest
> address space was.
>
> I think it was a mistake to ever consider usage of this function with
> out-of-band addresses as valid -- it adds a great deal of confusion.
>
> I could add an assertion here to make sure, if you like.
>
> If this were C++, it might have been interesting to try to use the type
> system to keep the host and guest address spaces forcibly separate.  But
> since this is plain C, I think wrapping everything in structures and access
> macros would just be too ugly.
>
>
> r~
>