diff mbox

exec-obsolete: fix length handling

Message ID CAAu8pHtQYSXayKD72jcEdbEPiiA_ucD9UWVoT=h=5sOpXWN3Sw@mail.gmail.com
State New
Headers show

Commit Message

Blue Swirl Jan. 28, 2012, 6:13 p.m. UTC 
Fix suspend/resume broken by off-by-one error in
59abb06198ee9471e29c970f294eae80c0b39be1.

Adjust the loop so that it handles correctly the case
start = (ram_addr_t)-TARGET_PAGE_SIZE, length = TARGET_PAGE_SIZE.

Reported-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 exec-obsolete.h |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

 }
@@ -96,12 +95,11 @@ static inline void
cpu_physical_memory_mask_dirty_range(ram_addr_t start,
 {
     int mask;
     uint8_t *p;
-    ram_addr_t addr, end;
+    ram_addr_t cur;

-    end = start + length;
     mask = ~dirty_flags;
     p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
-    for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
+    for (cur = 0; cur < length; cur += TARGET_PAGE_SIZE) {
         *p++ &= mask;
     }
 }

Comments

Avi Kivity Jan. 29, 2012, 10:22 a.m. UTC | #1 
On 01/28/2012 08:13 PM, Blue Swirl wrote:
> Fix suspend/resume broken by off-by-one error in
> 59abb06198ee9471e29c970f294eae80c0b39be1.
>
> Adjust the loop so that it handles correctly the case
> start = (ram_addr_t)-TARGET_PAGE_SIZE, length = TARGET_PAGE_SIZE.

Is the ram_addr_t even legal? ram addresses start from 0 and end up
around ~(ram_addr_t)0 >> 1, max.


> Reported-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  exec-obsolete.h |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/exec-obsolete.h b/exec-obsolete.h
> index 03cf35e..1bba970 100644
> --- a/exec-obsolete.h
> +++ b/exec-obsolete.h
> @@ -81,11 +81,10 @@ static inline void
> cpu_physical_memory_set_dirty_range(ram_addr_t start,
>                                                         int dirty_flags)
>  {
>      uint8_t *p;
> -    ram_addr_t addr, end;
> +    ram_addr_t cur;
>
> -    end = start + length;
>      p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
> -    for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
> +    for (cur = 0; cur < length; cur += TARGET_PAGE_SIZE) {
>          *p++ |= dirty_flags;
>      }
>  }
> @@ -96,12 +95,11 @@ static inline void
> cpu_physical_memory_mask_dirty_range(ram_addr_t start,
>  {
>      int mask;
>      uint8_t *p;
> -    ram_addr_t addr, end;
> +    ram_addr_t cur;
>
> -    end = start + length;
>      mask = ~dirty_flags;
>      p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
> -    for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
> +    for (cur = 0; cur < length; cur += TARGET_PAGE_SIZE) {
>          *p++ &= mask;
>      }
>  }
Blue Swirl Jan. 29, 2012, 11:37 a.m. UTC | #2 
On Sun, Jan 29, 2012 at 10:22, Avi Kivity <avi@redhat.com> wrote:
> On 01/28/2012 08:13 PM, Blue Swirl wrote:
>> Fix suspend/resume broken by off-by-one error in
>> 59abb06198ee9471e29c970f294eae80c0b39be1.
>>
>> Adjust the loop so that it handles correctly the case
>> start = (ram_addr_t)-TARGET_PAGE_SIZE, length = TARGET_PAGE_SIZE.
>
> Is the ram_addr_t even legal? ram addresses start from 0 and end up
> around ~(ram_addr_t)0 >> 1, max.

Is that defined somewhere? I think only the size of host virtual
address space should limit the range, for example on a 32 bit host,
near to 3GB should be possible as limited by host OS.

Anyway, this version is closer to your original code and should be
equal or better otherwise to current broken code.

>> Reported-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>  exec-obsolete.h |   10 ++++------
>>  1 files changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/exec-obsolete.h b/exec-obsolete.h
>> index 03cf35e..1bba970 100644
>> --- a/exec-obsolete.h
>> +++ b/exec-obsolete.h
>> @@ -81,11 +81,10 @@ static inline void
>> cpu_physical_memory_set_dirty_range(ram_addr_t start,
>>                                                         int dirty_flags)
>>  {
>>      uint8_t *p;
>> -    ram_addr_t addr, end;
>> +    ram_addr_t cur;
>>
>> -    end = start + length;
>>      p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
>> -    for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
>> +    for (cur = 0; cur < length; cur += TARGET_PAGE_SIZE) {
>>          *p++ |= dirty_flags;
>>      }
>>  }
>> @@ -96,12 +95,11 @@ static inline void
>> cpu_physical_memory_mask_dirty_range(ram_addr_t start,
>>  {
>>      int mask;
>>      uint8_t *p;
>> -    ram_addr_t addr, end;
>> +    ram_addr_t cur;
>>
>> -    end = start + length;
>>      mask = ~dirty_flags;
>>      p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
>> -    for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
>> +    for (cur = 0; cur < length; cur += TARGET_PAGE_SIZE) {
>>          *p++ &= mask;
>>      }
>>  }
>
>
> --
> error compiling committee.c: too many arguments to function
>
Avi Kivity Jan. 29, 2012, 11:53 a.m. UTC | #3 
On 01/29/2012 01:37 PM, Blue Swirl wrote:
> On Sun, Jan 29, 2012 at 10:22, Avi Kivity <avi@redhat.com> wrote:
> > On 01/28/2012 08:13 PM, Blue Swirl wrote:
> >> Fix suspend/resume broken by off-by-one error in
> >> 59abb06198ee9471e29c970f294eae80c0b39be1.
> >>
> >> Adjust the loop so that it handles correctly the case
> >> start = (ram_addr_t)-TARGET_PAGE_SIZE, length = TARGET_PAGE_SIZE.
> >
> > Is the ram_addr_t even legal? ram addresses start from 0 and end up
> > around ~(ram_addr_t)0 >> 1, max.
>
> Is that defined somewhere? I think only the size of host virtual
> address space should limit the range, for example on a 32 bit host,
> near to 3GB should be possible as limited by host OS.

Correct, but the code limits it to 2GB.

> Anyway, this version is closer to your original code and should be
> equal or better otherwise to current broken code.

I prefer the earlier patch, but both should work.
Avi Kivity Jan. 29, 2012, 12:08 p.m. UTC | #4 
On 01/28/2012 08:13 PM, Blue Swirl wrote:
> Fix suspend/resume broken by off-by-one error in
> 59abb06198ee9471e29c970f294eae80c0b39be1.
>
> Adjust the loop so that it handles correctly the case
> start = (ram_addr_t)-TARGET_PAGE_SIZE, length = TARGET_PAGE_SIZE.
>
> Reported-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  exec-obsolete.h |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/exec-obsolete.h b/exec-obsolete.h
> index 03cf35e..1bba970 100644
> --- a/exec-obsolete.h
> +++ b/exec-obsolete.h
> @@ -81,11 +81,10 @@ static inline void
> cpu_physical_memory_set_dirty_range(ram_addr_t start,
>                                                         int dirty_flags)
>  {
>      uint8_t *p;
> -    ram_addr_t addr, end;
> +    ram_addr_t cur;
>
> -    end = start + length;
>      p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
> -    for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
> +    for (cur = 0; cur < length; cur += TARGET_PAGE_SIZE) {
>          *p++ |= dirty_flags;
>      }

I think this is still wrong - if length == 2 it will iterate once, but
we need two iterations if start == 0xfff.
Blue Swirl Jan. 29, 2012, 1:16 p.m. UTC | #5 
On Sun, Jan 29, 2012 at 12:08, Avi Kivity <avi@redhat.com> wrote:
> On 01/28/2012 08:13 PM, Blue Swirl wrote:
>> Fix suspend/resume broken by off-by-one error in
>> 59abb06198ee9471e29c970f294eae80c0b39be1.
>>
>> Adjust the loop so that it handles correctly the case
>> start = (ram_addr_t)-TARGET_PAGE_SIZE, length = TARGET_PAGE_SIZE.
>>
>> Reported-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>  exec-obsolete.h |   10 ++++------
>>  1 files changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/exec-obsolete.h b/exec-obsolete.h
>> index 03cf35e..1bba970 100644
>> --- a/exec-obsolete.h
>> +++ b/exec-obsolete.h
>> @@ -81,11 +81,10 @@ static inline void
>> cpu_physical_memory_set_dirty_range(ram_addr_t start,
>>                                                         int dirty_flags)
>>  {
>>      uint8_t *p;
>> -    ram_addr_t addr, end;
>> +    ram_addr_t cur;
>>
>> -    end = start + length;
>>      p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
>> -    for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
>> +    for (cur = 0; cur < length; cur += TARGET_PAGE_SIZE) {
>>          *p++ |= dirty_flags;
>>      }
>
> I think this is still wrong - if length == 2 it will iterate once, but
> we need two iterations if start == 0xfff.

Yes, tricky. We could do something like
for (cur = start & TARGET_PAGE_MASK; cur < length; cur += TARGET_PAGE_SIZE) {
but I'll send a new patch with just s/<=/</.

>
>
> --
> error compiling committee.c: too many arguments to function
>
Avi Kivity Jan. 29, 2012, 1:20 p.m. UTC | #6 
On 01/29/2012 03:16 PM, Blue Swirl wrote:
> On Sun, Jan 29, 2012 at 12:08, Avi Kivity <avi@redhat.com> wrote:
> > On 01/28/2012 08:13 PM, Blue Swirl wrote:
> >> Fix suspend/resume broken by off-by-one error in
> >> 59abb06198ee9471e29c970f294eae80c0b39be1.
> >>
> >> Adjust the loop so that it handles correctly the case
> >> start = (ram_addr_t)-TARGET_PAGE_SIZE, length = TARGET_PAGE_SIZE.
> >>
> >> Reported-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> >> ---
> >>  exec-obsolete.h |   10 ++++------
> >>  1 files changed, 4 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/exec-obsolete.h b/exec-obsolete.h
> >> index 03cf35e..1bba970 100644
> >> --- a/exec-obsolete.h
> >> +++ b/exec-obsolete.h
> >> @@ -81,11 +81,10 @@ static inline void
> >> cpu_physical_memory_set_dirty_range(ram_addr_t start,
> >>                                                         int dirty_flags)
> >>  {
> >>      uint8_t *p;
> >> -    ram_addr_t addr, end;
> >> +    ram_addr_t cur;
> >>
> >> -    end = start + length;
> >>      p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
> >> -    for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
> >> +    for (cur = 0; cur < length; cur += TARGET_PAGE_SIZE) {
> >>          *p++ |= dirty_flags;
> >>      }
> >
> > I think this is still wrong - if length == 2 it will iterate once, but
> > we need two iterations if start == 0xfff.
>
> Yes, tricky. We could do something like
> for (cur = start & TARGET_PAGE_MASK; cur < length; cur += TARGET_PAGE_SIZE) {
> but I'll send a new patch with just s/<=/</.

That's broken too.

I have:

     uint8_t *p;
     ram_addr_t addr, end;
 
-    end = start + length;
+    end = (start + length - 1) | (TARGET_PAGE_SIZE - 1);
+    start &= TARGET_PAGE_MASK;
     p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
     for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
         *p++ |= dirty_flags;
@@ -98,7 +99,8 @@ static inline void
cpu_physical_memory_mask_dirty_range(ram_addr_t start,
     uint8_t *p;
     ram_addr_t addr, end;
 
-    end = start + length;
+    end = (start + length - 1) | (TARGET_PAGE_SIZE - 1);
+    start &= TARGET_PAGE_MASK;
     mask = ~dirty_flags;
     p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
     for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {


And a non-terminating migration - not sure if this is the cause.
Blue Swirl Jan. 29, 2012, 1:39 p.m. UTC | #7 
On Sun, Jan 29, 2012 at 13:20, Avi Kivity <avi@redhat.com> wrote:
> On 01/29/2012 03:16 PM, Blue Swirl wrote:
>> On Sun, Jan 29, 2012 at 12:08, Avi Kivity <avi@redhat.com> wrote:
>> > On 01/28/2012 08:13 PM, Blue Swirl wrote:
>> >> Fix suspend/resume broken by off-by-one error in
>> >> 59abb06198ee9471e29c970f294eae80c0b39be1.
>> >>
>> >> Adjust the loop so that it handles correctly the case
>> >> start = (ram_addr_t)-TARGET_PAGE_SIZE, length = TARGET_PAGE_SIZE.
>> >>
>> >> Reported-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> >> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> >> ---
>> >>  exec-obsolete.h |   10 ++++------
>> >>  1 files changed, 4 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/exec-obsolete.h b/exec-obsolete.h
>> >> index 03cf35e..1bba970 100644
>> >> --- a/exec-obsolete.h
>> >> +++ b/exec-obsolete.h
>> >> @@ -81,11 +81,10 @@ static inline void
>> >> cpu_physical_memory_set_dirty_range(ram_addr_t start,
>> >>                                                         int dirty_flags)
>> >>  {
>> >>      uint8_t *p;
>> >> -    ram_addr_t addr, end;
>> >> +    ram_addr_t cur;
>> >>
>> >> -    end = start + length;
>> >>      p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
>> >> -    for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
>> >> +    for (cur = 0; cur < length; cur += TARGET_PAGE_SIZE) {
>> >>          *p++ |= dirty_flags;
>> >>      }
>> >
>> > I think this is still wrong - if length == 2 it will iterate once, but
>> > we need two iterations if start == 0xfff.
>>
>> Yes, tricky. We could do something like
>> for (cur = start & TARGET_PAGE_MASK; cur < length; cur += TARGET_PAGE_SIZE) {
>> but I'll send a new patch with just s/<=/</.
>
> That's broken too.

Because length should be adjusted by -1?

> I have:
>
>     uint8_t *p;
>     ram_addr_t addr, end;
>
> -    end = start + length;
> +    end = (start + length - 1) | (TARGET_PAGE_SIZE - 1);

Why  | (TARGET_PAGE_SIZE - 1), for length == 1? TARGET_PAGE_ALIGN()
could be useful here.

> +    start &= TARGET_PAGE_MASK;
>     p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
>     for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
>         *p++ |= dirty_flags;
> @@ -98,7 +99,8 @@ static inline void
> cpu_physical_memory_mask_dirty_range(ram_addr_t start,
>     uint8_t *p;
>     ram_addr_t addr, end;
>
> -    end = start + length;
> +    end = (start + length - 1) | (TARGET_PAGE_SIZE - 1);
> +    start &= TARGET_PAGE_MASK;
>     mask = ~dirty_flags;
>     p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
>     for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
>
>
> And a non-terminating migration - not sure if this is the cause.
>
> --
> error compiling committee.c: too many arguments to function
>
Avi Kivity Jan. 29, 2012, 2:21 p.m. UTC | #8 
On 01/29/2012 03:39 PM, Blue Swirl wrote:
> >> >> +++ b/exec-obsolete.h
> >> >> @@ -81,11 +81,10 @@ static inline void
> >> >> cpu_physical_memory_set_dirty_range(ram_addr_t start,
> >> >>                                                         int dirty_flags)
> >> >>  {
> >> >>      uint8_t *p;
> >> >> -    ram_addr_t addr, end;
> >> >> +    ram_addr_t cur;
> >> >>
> >> >> -    end = start + length;
> >> >>      p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
> >> >> -    for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
> >> >> +    for (cur = 0; cur < length; cur += TARGET_PAGE_SIZE) {
> >> >>          *p++ |= dirty_flags;
> >> >>      }
> >> >
> >> > I think this is still wrong - if length == 2 it will iterate once, but
> >> > we need two iterations if start == 0xfff.
> >>
> >> Yes, tricky. We could do something like
> >> for (cur = start & TARGET_PAGE_MASK; cur < length; cur += TARGET_PAGE_SIZE) {
> >> but I'll send a new patch with just s/<=/</.
> >
> > That's broken too.
>
> Because length should be adjusted by -1?

0xfff/2 breaks.

More generally, you can't have a loop that just looks at length, since
0/2 wants one iteration, and 0xfff/2 wants two.

>
> > I have:
> >
> >     uint8_t *p;
> >     ram_addr_t addr, end;
> >
> > -    end = start + length;
> > +    end = (start + length - 1) | (TARGET_PAGE_SIZE - 1);
>
> Why  | (TARGET_PAGE_SIZE - 1), for length == 1? 

Yes.  More generally, I wanted something that is easily understood -
start/end addresses without trickery, given all the broken patches for
fixing this.

> TARGET_PAGE_ALIGN()
> could be useful here.

True, I'll respin.
diff mbox

Patch

From fb378616b9aeab9032fa3c2341724529002fcea9 Mon Sep 17 00:00:00 2001
Message-Id: <fb378616b9aeab9032fa3c2341724529002fcea9.1327774266.git.blauwirbel@gmail.com>
From: Blue Swirl <blauwirbel@gmail.com>
Date: Sat, 28 Jan 2012 18:02:08 +0000
Subject: [PATCH] exec-obsolete: fix length handling

Fix suspend/resume broken by off-by-one error in
59abb06198ee9471e29c970f294eae80c0b39be1.

Adjust the loop so that it handles correctly the case
start = (ram_addr_t)-TARGET_PAGE_SIZE, length = TARGET_PAGE_SIZE.

Reported-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 exec-obsolete.h |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/exec-obsolete.h b/exec-obsolete.h
index 03cf35e..1bba970 100644
--- a/exec-obsolete.h
+++ b/exec-obsolete.h
@@ -81,11 +81,10 @@  static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
                                                        int dirty_flags)
 {
     uint8_t *p;
-    ram_addr_t addr, end;
+    ram_addr_t cur;
 
-    end = start + length;
     p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
-    for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
+    for (cur = 0; cur < length; cur += TARGET_PAGE_SIZE) {
         *p++ |= dirty_flags;
     }
 }
@@ -96,12 +95,11 @@  static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
 {
     int mask;
     uint8_t *p;
-    ram_addr_t addr, end;
+    ram_addr_t cur;
 
-    end = start + length;
     mask = ~dirty_flags;
     p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
-    for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
+    for (cur = 0; cur < length; cur += TARGET_PAGE_SIZE) {
         *p++ &= mask;
     }
 }
-- 
1.7.2.5