Re: [Qemu-devel] [PULL 1/1] ui/vnc: fix vmware VGA incompatiblities

[Peter Maydell]

On Tue, 18 Mar 2014 at 07:26, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> From: Peter Lieven <pl@kamp.de>
>
> this fixes invalid rectangle updates observed after commit 12b316d
> with the vmware VGA driver. The issues occured because the server
> and client surface update seems to be out of sync at some points
> and the max width of the surface is not dividable by
> VNC_DIRTY_BITS_PER_PIXEL (16).
>
> Reported-by: Serge Hallyn <serge.hallyn@ubuntu.com>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Hi all; I know this is a 6-year-old patch (now commit
2f487a3d40faff1 in git), but I was just looking
through some header file dependencies, and I noticed that
hw/display/vmware_vga.c includes ui/vnc.h, and that struck
me as pretty odd... I'm sending this email in the hope that
people might remember something of the situation beyond what's
described in the commit message.

> ---
>  hw/display/vmware_vga.c |  3 ++-
>  ui/vnc.c                | 10 +++++++---
>  2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
> index bd2c108..6ae3348 100644
> --- a/hw/display/vmware_vga.c
> +++ b/hw/display/vmware_vga.c
> @@ -25,6 +25,7 @@
>  #include "hw/loader.h"
>  #include "trace.h"
>  #include "ui/console.h"
> +#include "ui/vnc.h"
>  #include "hw/pci/pci.h"
>
>  #undef VERBOSE
> @@ -218,7 +219,7 @@ enum {
>
>  /* These values can probably be changed arbitrarily.  */
>  #define SVGA_SCRATCH_SIZE               0x8000
> -#define SVGA_MAX_WIDTH                  2360
> +#define SVGA_MAX_WIDTH                  ROUND_UP(2360, 
> VNC_DIRTY_PIXELS_PER_BIT)
>  #define SVGA_MAX_HEIGHT                 1770
>
>  #ifdef VERBOSE

Here we pull in the VNC header in order to get the definition
of the VNC_DIRTY_PIXELS_PER_BIT constant, but I don't understand
why. The hw/display code should be agnostic of whatever the
UI display front-end is. Why does vmware_vga.c need to care
but not any other device?

Moreover, looking at the vmware_vga.c code, although the
SVGA_MAX_WIDTH value is made to be a multiple of VNC_DIRTY_PIXELS_PER_BIT
(which is 16), there seems to be nothing preventing the guest itself
from programming the device to some other width that isn't
a multiple of 16. So either this device needs to guard against
bad widths generally, or the problem is really in the VNC code.
I can't find anything in the vmware VGA device docs (though they
are pretty meagre) suggesting that there's a requirement for
the surface to be a multiple of 16, so I think that the VNC code
needs to be able to cope. (This should be no different from any other
display device model setting a non-multiple-of-16 width.)

So my feeling is that this vmware_vga.c portion of this commit
should be reverted, and if there's still a problem with non-multiple-of-16
surface widths then it needs to be handled in the common or VNC
specific UI code...

thanks
-- PMM