| Message ID | 20240530110417.1724583-1-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | stdvga: fix screen blanking | expand |
Hi On Thu, May 30, 2024 at 3:05 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > In case the display surface uses a shared buffer (i.e. uses vga vram > directly instead of a shadow) go unshare the buffer before clearing it. > > This avoids vga memory corruption, which in turn fixes unblanking not > working properly with X11. > > Cc: qemu-stable@nongnu.org > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2067 > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/display/vga.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/display/vga.c b/hw/display/vga.c > index 30facc6c8e33..34ab8eb9b745 100644 > --- a/hw/display/vga.c > +++ b/hw/display/vga.c > @@ -1762,6 +1762,12 @@ static void vga_draw_blank(VGACommonState *s, int > full_update) > if (s->last_scr_width <= 0 || s->last_scr_height <= 0) > return; > > + if (is_buffer_shared(surface)) { > It might be a good time to rename this function. surface_is_borrowed() ? > + /* unshare buffer, otherwise the blanking corrupts vga vram */ > + qemu_console_resize(s->con, s->last_scr_width, > s->last_scr_height); > If we want to guarantee that a new surface is created, we should leave a comment on qemu_console_resize(), or perhaps make it take a new/alloc argument? + surface = qemu_console_surface(s->con); > + } > + > w = s->last_scr_width * surface_bytes_per_pixel(surface); > d = surface_data(surface); > for(i = 0; i < s->last_scr_height; i++) { > -- > 2.45.1 > > > thanks
On Mon, Jun 03, 2024 at 02:24:52PM GMT, Marc-André Lureau wrote: > Hi > > On Thu, May 30, 2024 at 3:05 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > In case the display surface uses a shared buffer (i.e. uses vga vram > > directly instead of a shadow) go unshare the buffer before clearing it. > > > > This avoids vga memory corruption, which in turn fixes unblanking not > > working properly with X11. > > > > Cc: qemu-stable@nongnu.org > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2067 > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > --- > > hw/display/vga.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/hw/display/vga.c b/hw/display/vga.c > > index 30facc6c8e33..34ab8eb9b745 100644 > > --- a/hw/display/vga.c > > +++ b/hw/display/vga.c > > @@ -1762,6 +1762,12 @@ static void vga_draw_blank(VGACommonState *s, int > > full_update) > > if (s->last_scr_width <= 0 || s->last_scr_height <= 0) > > return; > > > > + if (is_buffer_shared(surface)) { > > > > It might be a good time to rename this function. surface_is_borrowed() ? "shared" means memory shared between guest and host (typically vga vram). I doubt using the term "borrowed" instead clarifies things much, especially as this isn't an rust-style "borrow" (which I guess you are referring to). Nothing prevents the host from writing to the surface as the bug clearly shows. Also qemu is a C project, so I wouldn't expect developers being familiar with rust semantics and terminology. > > + /* unshare buffer, otherwise the blanking corrupts vga vram */ > > + qemu_console_resize(s->con, s->last_scr_width, > > s->last_scr_height); > > If we want to guarantee that a new surface is created, we should leave a > comment on qemu_console_resize(), I left the comment there exactly because it isn't obvious that the qemu_console_resize() will create a new (not shared) surface. So not sure what exactly you are suggesting here? > or perhaps make it take a new/alloc argument? Right now qemu_console_resize() does a bunch of checks to figure whenever it can take a shortcut (because width + height didn't change) or not. We could certainly pass a boolean in instead and have the caller decide that way. Didn't check whenever that makes sense, and IMHO that is well beyond the scope of a 3-lines bugfix. kraxel@sirius ~/projects/qemu# git grep qemu_console_resize | wc -l 35 take care, Gerd
Hi On Mon, Jun 3, 2024 at 3:51 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > On Mon, Jun 03, 2024 at 02:24:52PM GMT, Marc-André Lureau wrote: > > Hi > > > > On Thu, May 30, 2024 at 3:05 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > > In case the display surface uses a shared buffer (i.e. uses vga vram > > > directly instead of a shadow) go unshare the buffer before clearing it. > > > > > > This avoids vga memory corruption, which in turn fixes unblanking not > > > working properly with X11. > > > > > > Cc: qemu-stable@nongnu.org > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2067 > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > > --- > > > hw/display/vga.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/hw/display/vga.c b/hw/display/vga.c > > > index 30facc6c8e33..34ab8eb9b745 100644 > > > --- a/hw/display/vga.c > > > +++ b/hw/display/vga.c > > > @@ -1762,6 +1762,12 @@ static void vga_draw_blank(VGACommonState *s, > int > > > full_update) > > > if (s->last_scr_width <= 0 || s->last_scr_height <= 0) > > > return; > > > > > > + if (is_buffer_shared(surface)) { > > > > > > > It might be a good time to rename this function. surface_is_borrowed() ? > > "shared" means memory shared between guest and host (typically vga vram). > > In this context, but this is now confusing because we also have shared memory surface support for win32. static inline int is_buffer_shared(DisplaySurface *surface) { return !(surface->flags & QEMU_ALLOCATED_FLAG); } !allocated = the surface memory is not owned. > I doubt using the term "borrowed" instead clarifies things much, > especially as this isn't an rust-style "borrow" (which I guess you are > referring to). Nothing prevents the host from writing to the surface as > the bug clearly shows. Also qemu is a C project, so I wouldn't expect > developers being familiar with rust semantics and terminology. > > Borrowing is not a term specific to Rust :) (and you can have mutable borrows btw) I'd rather use "shared" memory for IPC purposes. The lack of surface_ function prefix is also annoying. > > > + /* unshare buffer, otherwise the blanking corrupts vga vram */ > > > + qemu_console_resize(s->con, s->last_scr_width, > > > s->last_scr_height); > > > > If we want to guarantee that a new surface is created, we should leave a > > comment on qemu_console_resize(), > > I left the comment there exactly because it isn't obvious that the > qemu_console_resize() will create a new (not shared) surface. So not > sure what exactly you are suggesting here? > > I meant to document qemu_console_resize() function itself, as it would be too easy to miss and break this case. > > or perhaps make it take a new/alloc argument? > > Right now qemu_console_resize() does a bunch of checks to figure > whenever it can take a shortcut (because width + height didn't change) > or not. > > We could certainly pass a boolean in instead and have the caller decide > that way. Didn't check whenever that makes sense, and IMHO that is well > beyond the scope of a 3-lines bugfix. > > kraxel@sirius ~/projects/qemu# git grep qemu_console_resize | wc -l > 35 > Maybe introduce a new function then?
diff --git a/hw/display/vga.c b/hw/display/vga.c index 30facc6c8e33..34ab8eb9b745 100644 --- a/hw/display/vga.c +++ b/hw/display/vga.c @@ -1762,6 +1762,12 @@ static void vga_draw_blank(VGACommonState *s, int full_update) if (s->last_scr_width <= 0 || s->last_scr_height <= 0) return; + if (is_buffer_shared(surface)) { + /* unshare buffer, otherwise the blanking corrupts vga vram */ + qemu_console_resize(s->con, s->last_scr_width, s->last_scr_height); + surface = qemu_console_surface(s->con); + } + w = s->last_scr_width * surface_bytes_per_pixel(surface); d = surface_data(surface); for(i = 0; i < s->last_scr_height; i++) {
In case the display surface uses a shared buffer (i.e. uses vga vram directly instead of a shadow) go unshare the buffer before clearing it. This avoids vga memory corruption, which in turn fixes unblanking not working properly with X11. Cc: qemu-stable@nongnu.org Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2067 Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/display/vga.c | 6 ++++++ 1 file changed, 6 insertions(+)