* [Qemu-devel] [PATCH RESEND v2 1/2] fix vnc regression
@ 2011-03-02 3:46 Wen Congyang
2011-03-02 3:58 ` [Qemu-devel] [PATCH RESEND 2/2] vnc: Fix heap corruption Wen Congyang
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Wen Congyang @ 2011-03-02 3:46 UTC (permalink / raw)
To: qemu-devel, Stefan Weil, Anthony Liguori, Corentin Chary
This patch fix the following two regressions:
1. we should use bitmap_set() and bitmap_clear() to replace vnc_set_bits().
2. The unit of bitmap_intersects()'third parameter is bit, not words.
But we pass the num of words to bitmap_intersects().
Changes from v1 to v2:
1. fix the third argument of bitmap_clear()
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
ui/vnc.c | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index 610f884..e3761b0 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1645,17 +1645,21 @@ static void framebuffer_update_request(VncState *vs, int incremental,
int x_position, int y_position,
int w, int h)
{
+ int i;
+ const size_t width = ds_get_width(vs->ds) / 16;
+
if (y_position > ds_get_height(vs->ds))
y_position = ds_get_height(vs->ds);
if (y_position + h >= ds_get_height(vs->ds))
h = ds_get_height(vs->ds) - y_position;
- int i;
vs->need_update = 1;
if (!incremental) {
vs->force_update = 1;
for (i = 0; i < h; i++) {
- bitmap_set(vs->dirty[y_position + i], x_position / 16, w / 16);
+ bitmap_set(vs->dirty[y_position + i], 0, width);
+ bitmap_clear(vs->dirty[y_position + i], width,
+ VNC_DIRTY_WORDS * BITS_PER_LONG - width);
}
}
}
@@ -2406,7 +2410,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
guest_row = vd->guest.ds->data;
server_row = vd->server->data;
for (y = 0; y < vd->guest.ds->height; y++) {
- if (bitmap_intersects(vd->guest.dirty[y], width_mask, VNC_DIRTY_WORDS)) {
+ if (bitmap_intersects(vd->guest.dirty[y], width_mask,
+ VNC_DIRTY_WORDS * BITS_PER_LONG)) {
int x;
uint8_t *guest_ptr;
uint8_t *server_ptr;
--
1.7.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH RESEND 2/2] vnc: Fix heap corruption
2011-03-02 3:46 [Qemu-devel] [PATCH RESEND v2 1/2] fix vnc regression Wen Congyang
@ 2011-03-02 3:58 ` Wen Congyang
2011-03-02 10:57 ` [Qemu-devel] " Corentin Chary
2011-03-02 10:56 ` [Qemu-devel] Re: [PATCH RESEND v2 1/2] fix vnc regression Corentin Chary
2011-03-03 2:44 ` [Qemu-devel] [PATCH 3/3] correct VNC_DIRTY_WORDS on 64 bit machine Wen Congyang
2 siblings, 1 reply; 14+ messages in thread
From: Wen Congyang @ 2011-03-02 3:58 UTC (permalink / raw)
To: qemu-devel, Stefan Weil, Anthony Liguori, Corentin Chary
This bug is reported by Stefan Weil:
========
Commit bc2429b9174ac2d3c56b7fd35884b0d89ec7fb02 introduced
a severe bug (heap corruption).
bitmap_clear was called with a wrong argument
which caused out-of-bound writes to width_mask.
This bug was detected with QEMU running on windows.
It also occurs with wine:
*** stack smashing detected ***: terminated
wine: Unhandled illegal instruction at address 0x6115c7 (thread 0009), starting debugger...
The bug is not windows specific!
========
The third argument of bitmap_clear() is number of bits to be cleared, but we pass
the end bits to be cleared to bitmap_clear().
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Reported-by: Stefan Weil <weil@mail.berlios.de>
---
ui/vnc.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index e3761b0..e7d0b5b 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2390,6 +2390,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
unsigned long width_mask[VNC_DIRTY_WORDS];
VncState *vs;
int has_dirty = 0;
+ const size_t width = ds_get_width(vd->ds) / 16;
struct timeval tv = { 0, 0 };
@@ -2403,9 +2404,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
* Check and copy modified bits from guest to server surface.
* Update server dirty map.
*/
- bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
- bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
- VNC_DIRTY_WORDS * BITS_PER_LONG);
+ bitmap_set(width_mask, 0, width);
+ bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - width);
cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds);
guest_row = vd->guest.ds->data;
server_row = vd->server->data;
--
1.7.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] Re: [PATCH RESEND v2 1/2] fix vnc regression
2011-03-02 3:46 [Qemu-devel] [PATCH RESEND v2 1/2] fix vnc regression Wen Congyang
2011-03-02 3:58 ` [Qemu-devel] [PATCH RESEND 2/2] vnc: Fix heap corruption Wen Congyang
@ 2011-03-02 10:56 ` Corentin Chary
2011-03-03 2:44 ` [Qemu-devel] [PATCH 3/3] correct VNC_DIRTY_WORDS on 64 bit machine Wen Congyang
2 siblings, 0 replies; 14+ messages in thread
From: Corentin Chary @ 2011-03-02 10:56 UTC (permalink / raw)
To: Anthony Liguori; +Cc: qemu-devel
On Wed, Mar 2, 2011 at 3:46 AM, Wen Congyang <wency@cn.fujitsu.com> wrote:
> This patch fix the following two regressions:
> 1. we should use bitmap_set() and bitmap_clear() to replace vnc_set_bits().
> 2. The unit of bitmap_intersects()'third parameter is bit, not words.
> But we pass the num of words to bitmap_intersects().
>
> Changes from v1 to v2:
> 1. fix the third argument of bitmap_clear()
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Acked-by: Corentin Chary <corentin.chary@gmail.com>
> ---
> ui/vnc.c | 11 ++++++++---
> 1 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 610f884..e3761b0 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -1645,17 +1645,21 @@ static void framebuffer_update_request(VncState *vs, int incremental,
> int x_position, int y_position,
> int w, int h)
> {
> + int i;
> + const size_t width = ds_get_width(vs->ds) / 16;
> +
> if (y_position > ds_get_height(vs->ds))
> y_position = ds_get_height(vs->ds);
> if (y_position + h >= ds_get_height(vs->ds))
> h = ds_get_height(vs->ds) - y_position;
>
> - int i;
> vs->need_update = 1;
> if (!incremental) {
> vs->force_update = 1;
> for (i = 0; i < h; i++) {
> - bitmap_set(vs->dirty[y_position + i], x_position / 16, w / 16);
> + bitmap_set(vs->dirty[y_position + i], 0, width);
> + bitmap_clear(vs->dirty[y_position + i], width,
> + VNC_DIRTY_WORDS * BITS_PER_LONG - width);
> }
> }
> }
> @@ -2406,7 +2410,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
> guest_row = vd->guest.ds->data;
> server_row = vd->server->data;
> for (y = 0; y < vd->guest.ds->height; y++) {
> - if (bitmap_intersects(vd->guest.dirty[y], width_mask, VNC_DIRTY_WORDS)) {
> + if (bitmap_intersects(vd->guest.dirty[y], width_mask,
> + VNC_DIRTY_WORDS * BITS_PER_LONG)) {
> int x;
> uint8_t *guest_ptr;
> uint8_t *server_ptr;
> --
> 1.7.1
>
--
Corentin Chary
http://xf.iksaif.net
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
2011-03-02 3:58 ` [Qemu-devel] [PATCH RESEND 2/2] vnc: Fix heap corruption Wen Congyang
@ 2011-03-02 10:57 ` Corentin Chary
2011-03-02 18:36 ` Stefan Weil
0 siblings, 1 reply; 14+ messages in thread
From: Corentin Chary @ 2011-03-02 10:57 UTC (permalink / raw)
To: Anthony Liguori; +Cc: qemu-devel
On Wed, Mar 2, 2011 at 3:58 AM, Wen Congyang <wency@cn.fujitsu.com> wrote:
> This bug is reported by Stefan Weil:
> ========
> Commit bc2429b9174ac2d3c56b7fd35884b0d89ec7fb02 introduced
> a severe bug (heap corruption).
>
> bitmap_clear was called with a wrong argument
> which caused out-of-bound writes to width_mask.
>
> This bug was detected with QEMU running on windows.
> It also occurs with wine:
>
> *** stack smashing detected ***: terminated
> wine: Unhandled illegal instruction at address 0x6115c7 (thread 0009), starting debugger...
>
> The bug is not windows specific!
> ========
>
> The third argument of bitmap_clear() is number of bits to be cleared, but we pass
> the end bits to be cleared to bitmap_clear().
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Reported-by: Stefan Weil <weil@mail.berlios.de>
Acked-by: Corentin Chary <corentin.chary@gmail.com>
> ---
> ui/vnc.c | 6 +++---
> 1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index e3761b0..e7d0b5b 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2390,6 +2390,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
> unsigned long width_mask[VNC_DIRTY_WORDS];
> VncState *vs;
> int has_dirty = 0;
> + const size_t width = ds_get_width(vd->ds) / 16;
>
> struct timeval tv = { 0, 0 };
>
> @@ -2403,9 +2404,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
> * Check and copy modified bits from guest to server surface.
> * Update server dirty map.
> */
> - bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
> - bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
> - VNC_DIRTY_WORDS * BITS_PER_LONG);
> + bitmap_set(width_mask, 0, width);
> + bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - width);
> cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds);
> guest_row = vd->guest.ds->data;
> server_row = vd->server->data;
> --
> 1.7.1
>
>
--
Corentin Chary
http://xf.iksaif.net
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
2011-03-02 10:57 ` [Qemu-devel] " Corentin Chary
@ 2011-03-02 18:36 ` Stefan Weil
2011-03-02 18:47 ` Peter Maydell
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Weil @ 2011-03-02 18:36 UTC (permalink / raw)
To: Corentin Chary; +Cc: Anthony Liguori, qemu-devel
Am 02.03.2011 11:57, schrieb Corentin Chary:
> On Wed, Mar 2, 2011 at 3:58 AM, Wen Congyang <wency@cn.fujitsu.com> wrote:
>> This bug is reported by Stefan Weil:
>> ========
>> Commit bc2429b9174ac2d3c56b7fd35884b0d89ec7fb02 introduced
>> a severe bug (heap corruption).
>>
>> bitmap_clear was called with a wrong argument
>> which caused out-of-bound writes to width_mask.
>>
>> This bug was detected with QEMU running on windows.
>> It also occurs with wine:
>>
>> *** stack smashing detected ***: terminated
>> wine: Unhandled illegal instruction at address 0x6115c7 (thread
>> 0009), starting debugger...
>>
>> The bug is not windows specific!
>> ========
>>
>> The third argument of bitmap_clear() is number of bits to be cleared,
>> but we pass
>> the end bits to be cleared to bitmap_clear().
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Reported-by: Stefan Weil <weil@mail.berlios.de>
>
> Acked-by: Corentin Chary <corentin.chary@gmail.com>
>
No. I dont't think that the third parameter of bitmap_clear is
ok like that. See my patch for the correct value.
My own patch is also incomplete, so I'll send an update.
Stefan
>> ---
>> ui/vnc.c | 6 +++---
>> 1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index e3761b0..e7d0b5b 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -2390,6 +2390,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
>> unsigned long width_mask[VNC_DIRTY_WORDS];
>> VncState *vs;
>> int has_dirty = 0;
>> + const size_t width = ds_get_width(vd->ds) / 16;
>>
>> struct timeval tv = { 0, 0 };
>>
>> @@ -2403,9 +2404,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
>> * Check and copy modified bits from guest to server surface.
>> * Update server dirty map.
>> */
>> - bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
>> - bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
>> - VNC_DIRTY_WORDS * BITS_PER_LONG);
>> + bitmap_set(width_mask, 0, width);
>> + bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - width);
>> cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds);
>> guest_row = vd->guest.ds->data;
>> server_row = vd->server->data;
>> --
>> 1.7.1
>>
>>
>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
2011-03-02 18:36 ` Stefan Weil
@ 2011-03-02 18:47 ` Peter Maydell
2011-03-02 22:01 ` Stefan Weil
0 siblings, 1 reply; 14+ messages in thread
From: Peter Maydell @ 2011-03-02 18:47 UTC (permalink / raw)
To: Stefan Weil; +Cc: Anthony Liguori, qemu-devel, Corentin Chary
On 2 March 2011 18:36, Stefan Weil <weil@mail.berlios.de> wrote:
> No. I dont't think that the third parameter of bitmap_clear is
> ok like that. See my patch for the correct value.
Wen's patch:
+ const size_t width = ds_get_width(vd->ds) / 16;
[...]
- bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
- bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
- VNC_DIRTY_WORDS * BITS_PER_LONG);
+ bitmap_set(width_mask, 0, width);
+ bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - width);
Your patch:
bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
- VNC_DIRTY_WORDS * BITS_PER_LONG);
+ (VNC_MAX_WIDTH - ds_get_width(vd->ds)) / 16);
Since ui/vnc.h has:
#define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
the third parameter to bitmap_clear is the same value in
both cases, isn't it? Or is this a rounding bug?
-- PMM
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
2011-03-02 18:47 ` Peter Maydell
@ 2011-03-02 22:01 ` Stefan Weil
2011-03-02 22:27 ` Stefan Weil
2011-03-02 22:40 ` Peter Maydell
0 siblings, 2 replies; 14+ messages in thread
From: Stefan Weil @ 2011-03-02 22:01 UTC (permalink / raw)
To: Peter Maydell; +Cc: Anthony Liguori, qemu-devel, Corentin Chary
Am 02.03.2011 19:47, schrieb Peter Maydell:
> On 2 March 2011 18:36, Stefan Weil <weil@mail.berlios.de> wrote:
>> No. I dont't think that the third parameter of bitmap_clear is
>> ok like that. See my patch for the correct value.
>
> Wen's patch:
>
> + const size_t width = ds_get_width(vd->ds) / 16;
> [...]
> - bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
> - bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
> - VNC_DIRTY_WORDS * BITS_PER_LONG);
> + bitmap_set(width_mask, 0, width);
> + bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG -
> width);
>
> Your patch:
>
> bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
> - VNC_DIRTY_WORDS * BITS_PER_LONG);
> + (VNC_MAX_WIDTH - ds_get_width(vd->ds)) / 16);
>
> Since ui/vnc.h has:
>
> #define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
>
> the third parameter to bitmap_clear is the same value in
> both cases, isn't it? Or is this a rounding bug?
>
> -- PMM
Because of rounding effects, both values can be different.
The part missing in my patch is correct handling of another
rounding effect:
VNC_DIRTY_WORDS is exact for 32 bit long values (and the
"old" code which used uint32_t until some weeks ago), where
VNC_DIRTY_WORDS = 2560/16/32 = 5.
For 64 bit values, VNC_DIRTY_WORDS = 2560/16/64 = 2 (rounded)!
Stefan W.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
2011-03-02 22:01 ` Stefan Weil
@ 2011-03-02 22:27 ` Stefan Weil
2011-03-03 1:37 ` Wen Congyang
2011-03-02 22:40 ` Peter Maydell
1 sibling, 1 reply; 14+ messages in thread
From: Stefan Weil @ 2011-03-02 22:27 UTC (permalink / raw)
To: Peter Maydell; +Cc: Anthony Liguori, qemu-devel, Corentin Chary
Am 02.03.2011 23:01, schrieb Stefan Weil:
> Am 02.03.2011 19:47, schrieb Peter Maydell:
>> On 2 March 2011 18:36, Stefan Weil <weil@mail.berlios.de> wrote:
>>> No. I dont't think that the third parameter of bitmap_clear is
>>> ok like that. See my patch for the correct value.
>>
>> Wen's patch:
>>
>> + const size_t width = ds_get_width(vd->ds) / 16;
>> [...]
>> - bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
>> - bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
>> - VNC_DIRTY_WORDS * BITS_PER_LONG);
>> + bitmap_set(width_mask, 0, width);
>> + bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG
>> - width);
>>
>> Your patch:
>>
>> bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
>> - VNC_DIRTY_WORDS * BITS_PER_LONG);
>> + (VNC_MAX_WIDTH - ds_get_width(vd->ds)) / 16);
>>
>> Since ui/vnc.h has:
>>
>> #define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
>>
>> the third parameter to bitmap_clear is the same value in
>> both cases, isn't it? Or is this a rounding bug?
>>
>> -- PMM
>
> Because of rounding effects, both values can be different.
>
> The part missing in my patch is correct handling of another
> rounding effect:
>
> VNC_DIRTY_WORDS is exact for 32 bit long values (and the
> "old" code which used uint32_t until some weeks ago), where
> VNC_DIRTY_WORDS = 2560/16/32 = 5.
>
> For 64 bit values, VNC_DIRTY_WORDS = 2560/16/64 = 2 (rounded)!
>
> Stefan W.
Is bitmap_clear() really needed here? Meanwhile I think it is not,
so this might be a new patch variant...
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
2011-03-02 22:01 ` Stefan Weil
2011-03-02 22:27 ` Stefan Weil
@ 2011-03-02 22:40 ` Peter Maydell
1 sibling, 0 replies; 14+ messages in thread
From: Peter Maydell @ 2011-03-02 22:40 UTC (permalink / raw)
To: Stefan Weil; +Cc: Anthony Liguori, qemu-devel, Corentin Chary
On 2 March 2011 22:01, Stefan Weil <weil@mail.berlios.de> wrote:
> The part missing in my patch is correct handling of another
> rounding effect:
>
> VNC_DIRTY_WORDS is exact for 32 bit long values (and the
> "old" code which used uint32_t until some weeks ago), where
> VNC_DIRTY_WORDS = 2560/16/32 = 5.
>
> For 64 bit values, VNC_DIRTY_WORDS = 2560/16/64 = 2 (rounded)!
Yes, I noticed that after I posted. Given that we have arrays
like
unsigned long dirty[VNC_MAX_HEIGHT][VNC_DIRTY_WORDS];
rounding down rather than up looks a bit suspicious...
(Can we just make VNC_MAX_WIDTH a multiple of 64, or is it
baked into the VNC protocol?)
-- PMM
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
2011-03-02 22:27 ` Stefan Weil
@ 2011-03-03 1:37 ` Wen Congyang
0 siblings, 0 replies; 14+ messages in thread
From: Wen Congyang @ 2011-03-03 1:37 UTC (permalink / raw)
To: Stefan Weil; +Cc: Peter Maydell, Anthony Liguori, qemu-devel, Corentin Chary
At 03/03/2011 06:27 AM, Stefan Weil Write:
> Am 02.03.2011 23:01, schrieb Stefan Weil:
>> Am 02.03.2011 19:47, schrieb Peter Maydell:
>>> On 2 March 2011 18:36, Stefan Weil <weil@mail.berlios.de> wrote:
>>>> No. I dont't think that the third parameter of bitmap_clear is
>>>> ok like that. See my patch for the correct value.
>>>
>>> Wen's patch:
>>>
>>> + const size_t width = ds_get_width(vd->ds) / 16;
>>> [...]
>>> - bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
>>> - bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
>>> - VNC_DIRTY_WORDS * BITS_PER_LONG);
>>> + bitmap_set(width_mask, 0, width);
>>> + bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG
>>> - width);
>>>
>>> Your patch:
>>>
>>> bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
>>> - VNC_DIRTY_WORDS * BITS_PER_LONG);
>>> + (VNC_MAX_WIDTH - ds_get_width(vd->ds)) / 16);
>>>
>>> Since ui/vnc.h has:
>>>
>>> #define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
>>>
>>> the third parameter to bitmap_clear is the same value in
>>> both cases, isn't it? Or is this a rounding bug?
>>>
>>> -- PMM
>>
>> Because of rounding effects, both values can be different.
>>
>> The part missing in my patch is correct handling of another
>> rounding effect:
>>
>> VNC_DIRTY_WORDS is exact for 32 bit long values (and the
>> "old" code which used uint32_t until some weeks ago), where
>> VNC_DIRTY_WORDS = 2560/16/32 = 5.
>>
>> For 64 bit values, VNC_DIRTY_WORDS = 2560/16/64 = 2 (rounded)!
>>
>> Stefan W.
>
>
> Is bitmap_clear() really needed here? Meanwhile I think it is not,
> so this might be a new patch variant...
I do not know why we call bitmap_clear() hear. I only know it is
the same as the old code:
-static inline void vnc_set_bits(uint32_t *d, int n, int nb_words)
-{
- int j;
-
- j = 0;
- while (n >= 32) {
- d[j++] = -1;
- n -= 32;
- }
- if (n > 0)
- d[j++] = (1 << n) - 1;
- while (j < nb_words) <=== bitmap_clear()
- d[j++] = 0;
-}
- vnc_set_bits(width_mask, (ds_get_width(vd->ds) / 16), VNC_DIRTY_WORDS);
+ bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
+ bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
+ VNC_DIRTY_WORDS * BITS_PER_LONG);
>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 3/3] correct VNC_DIRTY_WORDS on 64 bit machine
2011-03-02 3:46 [Qemu-devel] [PATCH RESEND v2 1/2] fix vnc regression Wen Congyang
2011-03-02 3:58 ` [Qemu-devel] [PATCH RESEND 2/2] vnc: Fix heap corruption Wen Congyang
2011-03-02 10:56 ` [Qemu-devel] Re: [PATCH RESEND v2 1/2] fix vnc regression Corentin Chary
@ 2011-03-03 2:44 ` Wen Congyang
2011-03-03 6:41 ` [Qemu-devel] " Corentin Chary
2 siblings, 1 reply; 14+ messages in thread
From: Wen Congyang @ 2011-03-03 2:44 UTC (permalink / raw)
To: qemu-devel, Stefan Weil, Anthony Liguori, Corentin Chary
VNC_DIRTY_WORDS is wrong on 64 bit long machine.
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
ui/vnc.h | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/ui/vnc.h b/ui/vnc.h
index 8a1e7b9..239a7a7 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -81,7 +81,8 @@ typedef void VncSendHextileTile(VncState *vs,
#define VNC_MAX_WIDTH 2560
#define VNC_MAX_HEIGHT 2048
-#define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
+#define divideup(x, y) (((x) + ((y) - 1)) / (y))
+#define VNC_DIRTY_WORDS (divideup(VNC_MAX_WIDTH, (16 * BITS_PER_LONG)))
#define VNC_STAT_RECT 64
#define VNC_STAT_COLS (VNC_MAX_WIDTH / VNC_STAT_RECT)
--
1.7.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] Re: [PATCH 3/3] correct VNC_DIRTY_WORDS on 64 bit machine
2011-03-03 2:44 ` [Qemu-devel] [PATCH 3/3] correct VNC_DIRTY_WORDS on 64 bit machine Wen Congyang
@ 2011-03-03 6:41 ` Corentin Chary
2011-03-03 6:42 ` Wen Congyang
2011-03-03 6:49 ` [Qemu-devel] [PATCH v2 " Wen Congyang
0 siblings, 2 replies; 14+ messages in thread
From: Corentin Chary @ 2011-03-03 6:41 UTC (permalink / raw)
To: Wen Congyang; +Cc: Anthony Liguori, qemu-devel
On Thu, Mar 3, 2011 at 3:44 AM, Wen Congyang <wency@cn.fujitsu.com> wrote:
> VNC_DIRTY_WORDS is wrong on 64 bit long machine.
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>
> ---
> ui/vnc.h | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/ui/vnc.h b/ui/vnc.h
> index 8a1e7b9..239a7a7 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -81,7 +81,8 @@ typedef void VncSendHextileTile(VncState *vs,
>
> #define VNC_MAX_WIDTH 2560
> #define VNC_MAX_HEIGHT 2048
> -#define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
> +#define divideup(x, y) (((x) + ((y) - 1)) / (y))
osdep.h:#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
--
Corentin Chary
http://xf.iksaif.net
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] Re: [PATCH 3/3] correct VNC_DIRTY_WORDS on 64 bit machine
2011-03-03 6:41 ` [Qemu-devel] " Corentin Chary
@ 2011-03-03 6:42 ` Wen Congyang
2011-03-03 6:49 ` [Qemu-devel] [PATCH v2 " Wen Congyang
1 sibling, 0 replies; 14+ messages in thread
From: Wen Congyang @ 2011-03-03 6:42 UTC (permalink / raw)
To: Corentin Chary; +Cc: Anthony Liguori, qemu-devel
At 03/03/2011 02:41 PM, Corentin Chary Write:
> On Thu, Mar 3, 2011 at 3:44 AM, Wen Congyang <wency@cn.fujitsu.com> wrote:
>> VNC_DIRTY_WORDS is wrong on 64 bit long machine.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>
>> ---
>> ui/vnc.h | 3 ++-
>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/ui/vnc.h b/ui/vnc.h
>> index 8a1e7b9..239a7a7 100644
>> --- a/ui/vnc.h
>> +++ b/ui/vnc.h
>> @@ -81,7 +81,8 @@ typedef void VncSendHextileTile(VncState *vs,
>>
>> #define VNC_MAX_WIDTH 2560
>> #define VNC_MAX_HEIGHT 2048
>> -#define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
>> +#define divideup(x, y) (((x) + ((y) - 1)) / (y))
>
> osdep.h:#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
Thank you for pointing out it. I will update my patch.
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v2 3/3] correct VNC_DIRTY_WORDS on 64 bit machine
2011-03-03 6:41 ` [Qemu-devel] " Corentin Chary
2011-03-03 6:42 ` Wen Congyang
@ 2011-03-03 6:49 ` Wen Congyang
1 sibling, 0 replies; 14+ messages in thread
From: Wen Congyang @ 2011-03-03 6:49 UTC (permalink / raw)
To: Corentin Chary; +Cc: Anthony Liguori, qemu-devel
VNC_DIRTY_WORDS is wrong on 64 bit long machine.
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
ui/vnc.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/ui/vnc.h b/ui/vnc.h
index 8a1e7b9..5fc54e5 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -81,7 +81,7 @@ typedef void VncSendHextileTile(VncState *vs,
#define VNC_MAX_WIDTH 2560
#define VNC_MAX_HEIGHT 2048
-#define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
+#define VNC_DIRTY_WORDS (DIV_ROUND_UP(VNC_MAX_WIDTH, (16 * BITS_PER_LONG)))
#define VNC_STAT_RECT 64
#define VNC_STAT_COLS (VNC_MAX_WIDTH / VNC_STAT_RECT)
--
1.7.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2011-03-03 6:50 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-02 3:46 [Qemu-devel] [PATCH RESEND v2 1/2] fix vnc regression Wen Congyang
2011-03-02 3:58 ` [Qemu-devel] [PATCH RESEND 2/2] vnc: Fix heap corruption Wen Congyang
2011-03-02 10:57 ` [Qemu-devel] " Corentin Chary
2011-03-02 18:36 ` Stefan Weil
2011-03-02 18:47 ` Peter Maydell
2011-03-02 22:01 ` Stefan Weil
2011-03-02 22:27 ` Stefan Weil
2011-03-03 1:37 ` Wen Congyang
2011-03-02 22:40 ` Peter Maydell
2011-03-02 10:56 ` [Qemu-devel] Re: [PATCH RESEND v2 1/2] fix vnc regression Corentin Chary
2011-03-03 2:44 ` [Qemu-devel] [PATCH 3/3] correct VNC_DIRTY_WORDS on 64 bit machine Wen Congyang
2011-03-03 6:41 ` [Qemu-devel] " Corentin Chary
2011-03-03 6:42 ` Wen Congyang
2011-03-03 6:49 ` [Qemu-devel] [PATCH v2 " Wen Congyang
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).