* [Qemu-devel] [PATCH] exec-obsolete: fix length handling
@ 2012-01-28 18:13 Blue Swirl
2012-01-29 10:22 ` Avi Kivity
2012-01-29 12:08 ` Avi Kivity
0 siblings, 2 replies; 10+ messages in thread
From: Blue Swirl @ 2012-01-28 18:13 UTC (permalink / raw)
To: Avi Kivity, Stefan Berger, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1479 bytes --]
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.9.rc0
[-- Attachment #2: 0001-exec-obsolete-fix-length-handling.patch --]
[-- Type: text/x-patch, Size: 1773 bytes --]
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] exec-obsolete: fix length handling
2012-01-28 18:13 [Qemu-devel] [PATCH] exec-obsolete: fix length handling Blue Swirl
@ 2012-01-29 10:22 ` Avi Kivity
2012-01-29 11:37 ` Blue Swirl
2012-01-29 12:08 ` Avi Kivity
1 sibling, 1 reply; 10+ messages in thread
From: Avi Kivity @ 2012-01-29 10:22 UTC (permalink / raw)
To: Blue Swirl; +Cc: qemu-devel, Stefan Berger
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;
> }
> }
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] exec-obsolete: fix length handling
2012-01-29 10:22 ` Avi Kivity
@ 2012-01-29 11:37 ` Blue Swirl
2012-01-29 11:53 ` Avi Kivity
0 siblings, 1 reply; 10+ messages in thread
From: Blue Swirl @ 2012-01-29 11:37 UTC (permalink / raw)
To: Avi Kivity; +Cc: qemu-devel, Stefan Berger
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] exec-obsolete: fix length handling
2012-01-29 11:37 ` Blue Swirl
@ 2012-01-29 11:53 ` Avi Kivity
0 siblings, 0 replies; 10+ messages in thread
From: Avi Kivity @ 2012-01-29 11:53 UTC (permalink / raw)
To: Blue Swirl; +Cc: qemu-devel, Stefan Berger
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.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] exec-obsolete: fix length handling
2012-01-28 18:13 [Qemu-devel] [PATCH] exec-obsolete: fix length handling Blue Swirl
2012-01-29 10:22 ` Avi Kivity
@ 2012-01-29 12:08 ` Avi Kivity
2012-01-29 13:16 ` Blue Swirl
1 sibling, 1 reply; 10+ messages in thread
From: Avi Kivity @ 2012-01-29 12:08 UTC (permalink / raw)
To: Blue Swirl; +Cc: qemu-devel, Stefan Berger
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.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] exec-obsolete: fix length handling
2012-01-29 12:08 ` Avi Kivity
@ 2012-01-29 13:16 ` Blue Swirl
2012-01-29 13:20 ` Avi Kivity
0 siblings, 1 reply; 10+ messages in thread
From: Blue Swirl @ 2012-01-29 13:16 UTC (permalink / raw)
To: Avi Kivity; +Cc: qemu-devel, Stefan Berger
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] exec-obsolete: fix length handling
2012-01-29 13:16 ` Blue Swirl
@ 2012-01-29 13:20 ` Avi Kivity
2012-01-29 13:39 ` Blue Swirl
0 siblings, 1 reply; 10+ messages in thread
From: Avi Kivity @ 2012-01-29 13:20 UTC (permalink / raw)
To: Blue Swirl; +Cc: qemu-devel, Stefan Berger
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.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] exec-obsolete: fix length handling
2012-01-29 13:20 ` Avi Kivity
@ 2012-01-29 13:39 ` Blue Swirl
2012-01-29 14:21 ` Avi Kivity
0 siblings, 1 reply; 10+ messages in thread
From: Blue Swirl @ 2012-01-29 13:39 UTC (permalink / raw)
To: Avi Kivity; +Cc: qemu-devel, Stefan Berger
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] exec-obsolete: fix length handling
2012-01-29 13:39 ` Blue Swirl
@ 2012-01-29 14:21 ` Avi Kivity
0 siblings, 0 replies; 10+ messages in thread
From: Avi Kivity @ 2012-01-29 14:21 UTC (permalink / raw)
To: Blue Swirl; +Cc: qemu-devel, Stefan Berger
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.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH] exec-obsolete: fix length handling
@ 2012-01-29 13:17 Blue Swirl
0 siblings, 0 replies; 10+ messages in thread
From: Blue Swirl @ 2012-01-29 13:17 UTC (permalink / raw)
To: Avi Kivity, Stefan Berger, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]
Fix suspend/resume broken by off-by-one error in
59abb06198ee9471e29c970f294eae80c0b39be1, based on patch by
Stefan Berger.
Reported-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
exec-obsolete.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/exec-obsolete.h b/exec-obsolete.h
index 03cf35e..5a1e468 100644
--- a/exec-obsolete.h
+++ b/exec-obsolete.h
@@ -85,7 +85,7 @@ static inline void
cpu_physical_memory_set_dirty_range(ram_addr_t start,
end = start + length;
p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
- for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
+ for (addr = start; addr < end; addr += TARGET_PAGE_SIZE) {
*p++ |= dirty_flags;
}
}
@@ -101,7 +101,7 @@ static inline void
cpu_physical_memory_mask_dirty_range(ram_addr_t start,
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 (addr = start; addr < end; addr += TARGET_PAGE_SIZE) {
*p++ &= mask;
}
}
--
1.7.9.rc0
[-- Attachment #2: 0001-exec-obsolete-fix-length-handling.patch --]
[-- Type: text/x-patch, Size: 1459 bytes --]
From bd2cf8f212fe5b683f24c978f8844e93efab2aca Mon Sep 17 00:00:00 2001
Message-Id: <bd2cf8f212fe5b683f24c978f8844e93efab2aca.1327842793.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, based on patch by
Stefan Berger.
Reported-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
exec-obsolete.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/exec-obsolete.h b/exec-obsolete.h
index 03cf35e..5a1e468 100644
--- a/exec-obsolete.h
+++ b/exec-obsolete.h
@@ -85,7 +85,7 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
end = start + length;
p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS);
- for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) {
+ for (addr = start; addr < end; addr += TARGET_PAGE_SIZE) {
*p++ |= dirty_flags;
}
}
@@ -101,7 +101,7 @@ static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
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 (addr = start; addr < end; addr += TARGET_PAGE_SIZE) {
*p++ &= mask;
}
}
--
1.7.2.5
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-01-29 14:21 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-28 18:13 [Qemu-devel] [PATCH] exec-obsolete: fix length handling Blue Swirl
2012-01-29 10:22 ` Avi Kivity
2012-01-29 11:37 ` Blue Swirl
2012-01-29 11:53 ` Avi Kivity
2012-01-29 12:08 ` Avi Kivity
2012-01-29 13:16 ` Blue Swirl
2012-01-29 13:20 ` Avi Kivity
2012-01-29 13:39 ` Blue Swirl
2012-01-29 14:21 ` Avi Kivity
-- strict thread matches above, loose matches on Subject: below --
2012-01-29 13:17 Blue Swirl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).