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?