Hi On Mon, Jun 3, 2024 at 3:51 PM Gerd Hoffmann <kra...@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 <kra...@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-sta...@nongnu.org > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2067 > > > Signed-off-by: Gerd Hoffmann <kra...@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
