[v1] stdvga: fix screen blanking

[PATCH] stdvga: fix screen blanking

Posted by Gerd Hoffmann 5 months, 4 weeks ago

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)) {
+        /* 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++) {
-- 
2.45.1

Re: [PATCH] stdvga: fix screen blanking

Posted by Marc-André Lureau 5 months, 3 weeks ago

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

-- 
Marc-André Lureau

Re: [PATCH] stdvga: fix screen blanking

Posted by Gerd Hoffmann 5 months, 3 weeks ago

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

Re: [PATCH] stdvga: fix screen blanking

Posted by Marc-André Lureau 5 months, 3 weeks ago

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?



-- 
Marc-André Lureau