[Qemu-devel] [PATCH 00/29]: vmsvga: Various fixes and enhancements

[Qemu-devel] [PATCH 00/29]: vmsvga: Various fixes and enhancements

[Liran Alon]

Hi,

This patch series aim to fix many issues in vmware-svga emulation
which have prevented it from being fully functional in a wide-variety
of guests.

Patches 1-5 are just code refactoring patches.

Patches 6-11 aim to fix multiple command parsing issues which caused
FIFO to desync and thus fail to parse commands posted after the
commands which failed to parse.

Patches 12-13 better define FIFO registers and add support for
extended FIFO registers.

Patches 14-17 add interrupt support.

Patches 18-19 utilize interrupt support to implement various interrupt
sources. Specifically, FIFO_PROGRESS interrupt and FIFO_FENCE related
interrupts.

Patches 20-24 fix various issues in cursor commands and improve
hardware cursor performance by utilizing CURSOR_BYPASS_3 capability
which allows getting cursor updates via FIFO registers.

Patch 25 add basic support for parsing GMR commands to avoid FIFO
desync.

Patches 26-27 add vmware-svga capabilities required by Linux
kernel vmware-svga driver.

Patch 28 is a small code refactoring change.

Patch 29 disallows setting screen size to zero width/height to avoid
VNC window surprisingly disappearing.

Regards,
-Liran Alon

[Qemu-devel] [PATCH 01/29] vmsvga: Stop using redundant fifo pointer variable

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 0bbb78b9a6fd..60a672530840 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -63,7 +63,6 @@ struct vmsvga_state_s {
     int syncing;
 
     MemoryRegion fifo_ram;
-    uint8_t *fifo_ptr;
     unsigned int fifo_size;
 
     uint32_t *fifo;
@@ -1022,7 +1021,6 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
 
     case SVGA_REG_CONFIG_DONE:
         if (value) {
-            s->fifo = (uint32_t *) s->fifo_ptr;
             vga_dirty_log_stop(&s->vga);
         }
         s->config = !!value;
@@ -1179,9 +1177,6 @@ static int vmsvga_post_load(void *opaque, int version_id)
     struct vmsvga_state_s *s = opaque;
 
     s->invalidated = 1;
-    if (s->config) {
-        s->fifo = (uint32_t *) s->fifo_ptr;
-    }
     return 0;
 }
 
@@ -1240,7 +1235,7 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s,
     s->fifo_size = SVGA_FIFO_SIZE;
     memory_region_init_ram(&s->fifo_ram, NULL, "vmsvga.fifo", s->fifo_size,
                            &error_fatal);
-    s->fifo_ptr = memory_region_get_ram_ptr(&s->fifo_ram);
+    s->fifo = (uint32_t *)memory_region_get_ram_ptr(&s->fifo_ram);
 
     vga_common_init(&s->vga, OBJECT(dev));
     vga_init(&s->vga, OBJECT(dev), address_space, io, true);
-- 
1.9.1

[Qemu-devel] [PATCH 02/29] vmsvga: Group together commands by their handling

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

Should not change semantics.

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 60 ++++++++++++++++++++++++++++---------------------
 1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 60a672530840..f8c5b64cfd7c 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -607,6 +607,8 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
         cmd_start = s->fifo_stop;
 
         switch (cmd = vmsvga_fifo_read(s)) {
+
+        /* Implemented commands */
         case SVGA_CMD_UPDATE:
         case SVGA_CMD_UPDATE_VERBOSE:
             len -= 5;
@@ -621,25 +623,6 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             vmsvga_update_rect_delayed(s, x, y, width, height);
             break;
 
-        case SVGA_CMD_RECT_FILL:
-            len -= 6;
-            if (len < 0) {
-                goto rewind;
-            }
-
-            colour = vmsvga_fifo_read(s);
-            x = vmsvga_fifo_read(s);
-            y = vmsvga_fifo_read(s);
-            width = vmsvga_fifo_read(s);
-            height = vmsvga_fifo_read(s);
-#ifdef HW_FILL_ACCEL
-            if (vmsvga_fill_rect(s, colour, x, y, width, height) == 0) {
-                break;
-            }
-#endif
-            args = 0;
-            goto badcmd;
-
         case SVGA_CMD_RECT_COPY:
             len -= 7;
             if (len < 0) {
@@ -704,27 +687,52 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
 #endif
 
         /*
-         * Other commands that we at least know the number of arguments
-         * for so we can avoid FIFO desync if driver uses them illegally.
+         * Deprecated commands are neither documented in VMware SVGA development kit
+         * nor in Linux kernel vmware-svga driver source code.
+         * If they are not encountered in real world scenarious, they should be
+         * completely removed.
          */
-        case SVGA_CMD_DEFINE_ALPHA_CURSOR:
+        case SVGA_CMD_RECT_FILL:
             len -= 6;
             if (len < 0) {
                 goto rewind;
             }
-            vmsvga_fifo_read(s);
-            vmsvga_fifo_read(s);
-            vmsvga_fifo_read(s);
+
+            colour = vmsvga_fifo_read(s);
             x = vmsvga_fifo_read(s);
             y = vmsvga_fifo_read(s);
-            args = x * y;
+            width = vmsvga_fifo_read(s);
+            height = vmsvga_fifo_read(s);
+#ifdef HW_FILL_ACCEL
+            if (vmsvga_fill_rect(s, colour, x, y, width, height) == 0) {
+                break;
+            }
+#endif
+            args = 0;
             goto badcmd;
+
+        /*
+         * Unimplemented commands that we gracefully skip their
+         * arguments so we can avoid FIFO desync
+         */
         case SVGA_CMD_RECT_ROP_FILL:
             args = 6;
             goto badcmd;
         case SVGA_CMD_RECT_ROP_COPY:
             args = 7;
             goto badcmd;
+        case SVGA_CMD_DEFINE_ALPHA_CURSOR:
+            len -= 6;
+            if (len < 0) {
+                goto rewind;
+            }
+            vmsvga_fifo_read(s);
+            vmsvga_fifo_read(s);
+            vmsvga_fifo_read(s);
+            x = vmsvga_fifo_read(s);
+            y = vmsvga_fifo_read(s);
+            args = x * y;
+            goto badcmd;
         case SVGA_CMD_DRAW_GLYPH_CLIPPED:
             len -= 4;
             if (len < 0) {
-- 
1.9.1

[Qemu-devel] [PATCH 03/29] vmsvga: Explictly mark deprecated commands

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

These commands are neither mentioned nor documented in VMware SVGA
development-kit and Linux vmware-svga driver source code.
Thus, they should be subject to future deletion, if not encountered in practice.

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 56 ++++++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index f8c5b64cfd7c..a244f43a866f 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -248,33 +248,33 @@ static const char *vmsvga_guest_id[] = {
 enum {
     SVGA_CMD_INVALID_CMD = 0,
     SVGA_CMD_UPDATE = 1,
-    SVGA_CMD_RECT_FILL = 2,
+    SVGA_CMD_RECT_FILL = 2,               /* deprecated */
     SVGA_CMD_RECT_COPY = 3,
-    SVGA_CMD_DEFINE_BITMAP = 4,
-    SVGA_CMD_DEFINE_BITMAP_SCANLINE = 5,
-    SVGA_CMD_DEFINE_PIXMAP = 6,
-    SVGA_CMD_DEFINE_PIXMAP_SCANLINE = 7,
-    SVGA_CMD_RECT_BITMAP_FILL = 8,
-    SVGA_CMD_RECT_PIXMAP_FILL = 9,
-    SVGA_CMD_RECT_BITMAP_COPY = 10,
-    SVGA_CMD_RECT_PIXMAP_COPY = 11,
-    SVGA_CMD_FREE_OBJECT = 12,
-    SVGA_CMD_RECT_ROP_FILL = 13,
-    SVGA_CMD_RECT_ROP_COPY = 14,
-    SVGA_CMD_RECT_ROP_BITMAP_FILL = 15,
-    SVGA_CMD_RECT_ROP_PIXMAP_FILL = 16,
-    SVGA_CMD_RECT_ROP_BITMAP_COPY = 17,
-    SVGA_CMD_RECT_ROP_PIXMAP_COPY = 18,
+    SVGA_CMD_DEFINE_BITMAP = 4,           /* deprecated */
+    SVGA_CMD_DEFINE_BITMAP_SCANLINE = 5,  /* deprecated */
+    SVGA_CMD_DEFINE_PIXMAP = 6,           /* deprecated */
+    SVGA_CMD_DEFINE_PIXMAP_SCANLINE = 7,  /* deprecated */
+    SVGA_CMD_RECT_BITMAP_FILL = 8,        /* deprecated */
+    SVGA_CMD_RECT_PIXMAP_FILL = 9,        /* deprecated */
+    SVGA_CMD_RECT_BITMAP_COPY = 10,       /* deprecated */
+    SVGA_CMD_RECT_PIXMAP_COPY = 11,       /* deprecated */
+    SVGA_CMD_FREE_OBJECT = 12,            /* deprecated */
+    SVGA_CMD_RECT_ROP_FILL = 13,          /* deprecated */
+    SVGA_CMD_RECT_ROP_COPY = 14,          /* deprecated */
+    SVGA_CMD_RECT_ROP_BITMAP_FILL = 15,   /* deprecated */
+    SVGA_CMD_RECT_ROP_PIXMAP_FILL = 16,   /* deprecated */
+    SVGA_CMD_RECT_ROP_BITMAP_COPY = 17,   /* deprecated */
+    SVGA_CMD_RECT_ROP_PIXMAP_COPY = 18,   /* deprecated */
     SVGA_CMD_DEFINE_CURSOR = 19,
-    SVGA_CMD_DISPLAY_CURSOR = 20,
-    SVGA_CMD_MOVE_CURSOR = 21,
+    SVGA_CMD_DISPLAY_CURSOR = 20,         /* deprecated */
+    SVGA_CMD_MOVE_CURSOR = 21,            /* deprecated */
     SVGA_CMD_DEFINE_ALPHA_CURSOR = 22,
-    SVGA_CMD_DRAW_GLYPH = 23,
-    SVGA_CMD_DRAW_GLYPH_CLIPPED = 24,
+    SVGA_CMD_DRAW_GLYPH = 23,             /* deprecated */
+    SVGA_CMD_DRAW_GLYPH_CLIPPED = 24,     /* deprecated */
     SVGA_CMD_UPDATE_VERBOSE = 25,
-    SVGA_CMD_SURFACE_FILL = 26,
-    SVGA_CMD_SURFACE_COPY = 27,
-    SVGA_CMD_SURFACE_ALPHA_BLEND = 28,
+    SVGA_CMD_SURFACE_FILL = 26,           /* deprecated */
+    SVGA_CMD_SURFACE_COPY = 27,           /* deprecated */
+    SVGA_CMD_SURFACE_ALPHA_BLEND = 28,    /* deprecated */
     SVGA_CMD_FRONT_ROP_FILL = 29,
     SVGA_CMD_FENCE = 30,
 };
@@ -692,7 +692,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
          * If they are not encountered in real world scenarious, they should be
          * completely removed.
          */
-        case SVGA_CMD_RECT_FILL:
+        case SVGA_CMD_RECT_FILL: /* deprecated */
             len -= 6;
             if (len < 0) {
                 goto rewind;
@@ -715,10 +715,10 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
          * Unimplemented commands that we gracefully skip their
          * arguments so we can avoid FIFO desync
          */
-        case SVGA_CMD_RECT_ROP_FILL:
+        case SVGA_CMD_RECT_ROP_FILL: /* deprecated */
             args = 6;
             goto badcmd;
-        case SVGA_CMD_RECT_ROP_COPY:
+        case SVGA_CMD_RECT_ROP_COPY: /* deprecated */
             args = 7;
             goto badcmd;
         case SVGA_CMD_DEFINE_ALPHA_CURSOR:
@@ -733,7 +733,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             y = vmsvga_fifo_read(s);
             args = x * y;
             goto badcmd;
-        case SVGA_CMD_DRAW_GLYPH_CLIPPED:
+        case SVGA_CMD_DRAW_GLYPH_CLIPPED: /* deprecated */
             len -= 4;
             if (len < 0) {
                 goto rewind;
@@ -742,7 +742,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             vmsvga_fifo_read(s);
             args = 7 + (vmsvga_fifo_read(s) >> 2);
             goto badcmd;
-        case SVGA_CMD_SURFACE_ALPHA_BLEND:
+        case SVGA_CMD_SURFACE_ALPHA_BLEND: /* deprecated */
             args = 12;
             goto badcmd;
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH 03/29] vmsvga: Explictly mark deprecated commands

[Gerd Hoffmann]

On Thu, Aug 09, 2018 at 02:46:16PM +0300, Liran Alon wrote:
> From: Leonid Shatz <leonid.shatz@oracle.com>
> 
> These commands are neither mentioned nor documented in VMware SVGA
> development-kit and Linux vmware-svga driver source code.
> Thus, they should be subject to future deletion, if not encountered in practice.

Well, as far I know qemu is compatible to a rather old version of the
vmware svga.  The xorg-x11-drv-vmware driver can handle the device just
fine, so I guess it is worth checking whenever there is more information
about these commands in the driver source code.

First, maybe the driver actually uses these commands on old devices
(when the kms driver doesn't load due to missing capabilities).

Second, being more specific than just "deprecated" would be useful if
possible (command replaced by $foo in device revision $bar).  That goes
into the "nice to have" bucket though.

cheers,
  Gerd

[Qemu-devel] [PATCH 04/29] vmsvga: Do not print error message for ignored commands

[Liran Alon]

Future patches will add handling of commands that are parsed but
deliberately ignored. This change adds required framework for
avoiding printing parsing error messages for these commands,
when we encounter them in the FIFO.

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index a244f43a866f..2e6ac5dfad8a 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -600,11 +600,13 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
     int x, y, dx, dy, width, height;
     struct vmsvga_cursor_definition_s cursor;
     uint32_t cmd_start;
+    bool cmd_ignored;
 
     len = vmsvga_fifo_length(s);
     while (len > 0 && --maxloop > 0) {
         /* May need to go back to the start of the command if incomplete */
         cmd_start = s->fifo_stop;
+        cmd_ignored = false;
 
         switch (cmd = vmsvga_fifo_read(s)) {
 
@@ -759,6 +761,9 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
 
         default:
             args = 0;
+            goto badcmd;
+        ignoredcmd:
+            cmd_ignored = true;
         badcmd:
             len -= args;
             if (len < 0) {
@@ -767,8 +772,10 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             while (args--) {
                 vmsvga_fifo_read(s);
             }
-            printf("%s: Unknown command 0x%02x in SVGA command FIFO\n",
-                   __func__, cmd);
+            if (!cmd_ignored) {
+                printf("%s: Unknown command 0x%02x in SVGA command FIFO\n",
+                       __func__, cmd);
+            }
             break;
 
         rewind:
-- 
1.9.1

[Qemu-devel] [PATCH 05/29] vmsvga: Show registers and commands on debug output as decimals

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

For better readability of debug output, show these values in decimal notation,
as they are defined in source by decimal integers.

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 2e6ac5dfad8a..2fbb9e7f6c9f 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -773,7 +773,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
                 vmsvga_fifo_read(s);
             }
             if (!cmd_ignored) {
-                printf("%s: Unknown command 0x%02x in SVGA command FIFO\n",
+                printf("%s: Unknown command %d in SVGA command FIFO\n",
                        __func__, cmd);
             }
             break;
@@ -964,7 +964,7 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
             ret = s->scratch[s->index - SVGA_SCRATCH_BASE];
             break;
         }
-        printf("%s: Bad register %02x\n", __func__, s->index);
+        printf("%s: Bad register %d\n", __func__, s->index);
         ret = 0;
         break;
     }
@@ -1092,7 +1092,7 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
             s->scratch[s->index - SVGA_SCRATCH_BASE] = value;
             break;
         }
-        printf("%s: Bad register %02x\n", __func__, s->index);
+        printf("%s: Bad register %d\n", __func__, s->index);
     }
 }
 
-- 
1.9.1

[Qemu-devel] [PATCH 06/29] vmsvga: Fix parse of SVGA_CMD_UPDATE_VERBOSE to consider additional opaque cookie

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

SVGA_CMD_UPDATE_VERBOSE has one more extra argument to fetch from command fifo,
as compared to SVGA_CMD_UPDATE command. From Linux kernel
drivers/gpu/drm/vmwgfx/device_include/svga_reg.h:
"Just like SVGA_CMD_UPDATE, but also provide a per-rectangle
'reason' value, an opaque cookie which is used by internal
debugging tools. Third party drivers should not use this
command."

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 2fbb9e7f6c9f..d3a78809673d 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -611,8 +611,11 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
         switch (cmd = vmsvga_fifo_read(s)) {
 
         /* Implemented commands */
-        case SVGA_CMD_UPDATE:
         case SVGA_CMD_UPDATE_VERBOSE:
+            /* One extra word: an opaque cookie which is used for debugging */
+            len -= 1;
+            /* fall through */
+        case SVGA_CMD_UPDATE:
             len -= 5;
             if (len < 0) {
                 goto rewind;
@@ -622,6 +625,8 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             y = vmsvga_fifo_read(s);
             width = vmsvga_fifo_read(s);
             height = vmsvga_fifo_read(s);
+            if (cmd == SVGA_CMD_UPDATE_VERBOSE)
+                vmsvga_fifo_read(s);
             vmsvga_update_rect_delayed(s, x, y, width, height);
             break;
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH 06/29] vmsvga: Fix parse of SVGA_CMD_UPDATE_VERBOSE to consider additional opaque cookie

[Gerd Hoffmann]

  Hi,

> +            if (cmd == SVGA_CMD_UPDATE_VERBOSE)
> +                vmsvga_fifo_read(s);

Code style.  qemu wants braces unconditionally.  There might be more of
this in following patches.  You can use scripts/checkpatch.pl to check.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 06/29] vmsvga: Fix parse of SVGA_CMD_UPDATE_VERBOSE to consider additional opaque cookie

[Liran Alon]

> On 10 Aug 2018, at 13:00, Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
>  Hi,
> 
>> +            if (cmd == SVGA_CMD_UPDATE_VERBOSE)
>> +                vmsvga_fifo_read(s);
> 
> Code style.  qemu wants braces unconditionally.  There might be more of
> this in following patches.  You can use scripts/checkpatch.pl to check.
> 
> cheers,
>  Gerd
> 

Oops. Will fix for v2 of patch series.

Thanks,
-Liran

[Qemu-devel] [PATCH 07/29] vmsvga: Handle SVGA_CMD_FRONT_ROP_FILL command

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

We parse the command structure, but ignore the hint given by command.
Without doing so, command FIFO could get out of sync and cause
vmware-svga device to hang.

>From Linux kernel drivers/gpu/drm/vmwgfx/device_include/svga_reg.h:

    /*
     * SVGA_CMD_FRONT_ROP_FILL --
     *
     *    This is a hint which tells the SVGA device that the driver has
     *    just filled a rectangular region of the GFB with a solid
     *    color. Instead of reading these pixels from the GFB, the device
     *    can assume that they all equal 'color'. This is primarily used
     *    for remote desktop protocols.
     *
     * Availability:
     *    SVGA_FIFO_CAP_ACCELFRONT
     */

    typedef
    struct {
       uint32 color;     /* In the same format as the GFB */
       uint32 x;
       uint32 y;
       uint32 width;
       uint32 height;
       uint32 rop;       /* Must be SVGA_ROP_COPY */
    }
    SVGAFifoCmdFrontRopFill;

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index d3a78809673d..fab6443a87e2 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -693,6 +693,14 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             goto badcmd;
 #endif
 
+        case SVGA_CMD_FRONT_ROP_FILL:
+            len -= 1;
+            if (len < 0) {
+                goto rewind;
+            }
+            args = 6;
+            goto ignoredcmd;
+
         /*
          * Deprecated commands are neither documented in VMware SVGA development kit
          * nor in Linux kernel vmware-svga driver source code.
@@ -759,7 +767,6 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
          */
         case SVGA_CMD_SURFACE_FILL:
         case SVGA_CMD_SURFACE_COPY:
-        case SVGA_CMD_FRONT_ROP_FILL:
         case SVGA_CMD_FENCE:
         case SVGA_CMD_INVALID_CMD:
             break; /* Nop */
-- 
1.9.1

[Qemu-devel] [PATCH 08/29] vmsvga: Parse SVGA_CMD_FENCE command to avoid FIFO desync

[Liran Alon]

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index fab6443a87e2..675c8755ab48 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -760,6 +760,9 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
         case SVGA_CMD_SURFACE_ALPHA_BLEND: /* deprecated */
             args = 12;
             goto badcmd;
+        case SVGA_CMD_FENCE:
+            args = 1;
+            goto badcmd;
 
         /*
          * Other commands that are not listed as depending on any
@@ -767,7 +770,6 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
          */
         case SVGA_CMD_SURFACE_FILL:
         case SVGA_CMD_SURFACE_COPY:
-        case SVGA_CMD_FENCE:
         case SVGA_CMD_INVALID_CMD:
             break; /* Nop */
 
-- 
1.9.1

[Qemu-devel] [PATCH 09/29] vmsvga: Account for length of command word when parsing commands

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

While we continue to ignore SVGA_CMD_RECT_ROP_FILL, SVGA_CMD_RECT_ROP_COPY
and SVGA_CMD_FENCE commands, we should account for command length, not only
arguments following command code.

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 675c8755ab48..b32a625ae9c2 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -731,9 +731,17 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
          * arguments so we can avoid FIFO desync
          */
         case SVGA_CMD_RECT_ROP_FILL: /* deprecated */
+            len -= 1;
+            if (len < 0) {
+                goto rewind;
+            }
             args = 6;
             goto badcmd;
         case SVGA_CMD_RECT_ROP_COPY: /* deprecated */
+            len -= 1;
+            if (len < 0) {
+                goto rewind;
+            }
             args = 7;
             goto badcmd;
         case SVGA_CMD_DEFINE_ALPHA_CURSOR:
@@ -761,6 +769,10 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             args = 12;
             goto badcmd;
         case SVGA_CMD_FENCE:
+            len -= 1;
+            if (len < 0) {
+                goto rewind;
+            }
             args = 1;
             goto badcmd;
 
-- 
1.9.1

[Qemu-devel] [PATCH 10/29] vmsvga: Remove treatment of deprecated commands as Nop

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

Remove switch-case invalid handling of SVGA_CMD_SURFACE_FILL
and SVGA_CMD_SURFACE_COPY deprecated commands as their
handling is obviously not complete. We'd rather leave it to default
(unknown) command handling and have an error message displayed.

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index b32a625ae9c2..c30ae9b4b204 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -780,8 +780,6 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
          * Other commands that are not listed as depending on any
          * CAPABILITIES bits, but are not described in the README either.
          */
-        case SVGA_CMD_SURFACE_FILL:
-        case SVGA_CMD_SURFACE_COPY:
         case SVGA_CMD_INVALID_CMD:
             break; /* Nop */
 
-- 
1.9.1

[Qemu-devel] [PATCH 11/29] vmsvga: Remove handler of SVGA_CMD_INVALID_CMD

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

This should be better handled by switch default case, which will output debugging
message about encountering this command, instead of silently discarding.
If such command is ever encountered, it serves as indicator of broken FIFO
command decoding chain.

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index c30ae9b4b204..f0e6b4bc74ba 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -776,13 +776,6 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             args = 1;
             goto badcmd;
 
-        /*
-         * Other commands that are not listed as depending on any
-         * CAPABILITIES bits, but are not described in the README either.
-         */
-        case SVGA_CMD_INVALID_CMD:
-            break; /* Nop */
-
         default:
             args = 0;
             goto badcmd;
-- 
1.9.1

[Qemu-devel] [PATCH 12/29] vmsvga: Add definitions of FIFO registers and report their number

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

Import FIFO register definitions from VMware SVGA Device Developer Kit.
Report number of available registers so that guest device driver can
reserve enough space for registers before command FIFO.

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 126 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 121 insertions(+), 5 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index f0e6b4bc74ba..8eeb0efc9cab 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -76,6 +76,8 @@ struct vmsvga_state_s {
         int x, y, w, h;
     } redraw_fifo[REDRAW_FIFO_LEN];
     int redraw_fifo_first, redraw_fifo_last;
+
+    uint32_t num_fifo_regs;
 };
 
 #define TYPE_VMWARE_SVGA "vmware-svga"
@@ -194,14 +196,124 @@ enum {
     SVGA_FIFO_NEXT,
     SVGA_FIFO_STOP,
 
-    /*
-     * Additional offsets added as of SVGA_CAP_EXTENDED_FIFO
-     */
+   /*
+    * Block 2 (extended registers): Mandatory registers for the extended
+    * FIFO.  These exist if the SVGA caps register includes
+    * SVGA_CAP_EXTENDED_FIFO; some of them are valid only if their
+    * associated capability bit is enabled.
+    *
+    * Note that when originally defined, SVGA_CAP_EXTENDED_FIFO implied
+    * support only for (FIFO registers) CAPABILITIES, FLAGS, and FENCE.
+    * This means that the guest has to test individually (in most cases
+    * using FIFO caps) for the presence of registers after this; the VMX
+    * can define "extended FIFO" to mean whatever it wants, and currently
+    * won't enable it unless there's room for that set and much more.
+    */
+
     SVGA_FIFO_CAPABILITIES = 4,
     SVGA_FIFO_FLAGS,
     SVGA_FIFO_FENCE,
-    SVGA_FIFO_3D_HWVERSION,
-    SVGA_FIFO_PITCHLOCK,
+
+   /*
+    * Block 3a (optional extended registers): Additional registers for the
+    * extended FIFO, whose presence isn't actually implied by
+    * SVGA_CAP_EXTENDED_FIFO; these exist if SVGA_FIFO_MIN is high enough to
+    * leave room for them.
+    *
+    * These in block 3a, the VMX currently considers mandatory for the
+    * extended FIFO.
+    */
+
+   /* Valid if exists (i.e. if extended FIFO enabled): */
+   SVGA_FIFO_3D_HWVERSION,       /* See SVGA3dHardwareVersion in svga3d_reg.h */
+   /* Valid with SVGA_FIFO_CAP_PITCHLOCK: */
+   SVGA_FIFO_PITCHLOCK,
+
+   /* Valid with SVGA_FIFO_CAP_CURSOR_BYPASS_3: */
+   SVGA_FIFO_CURSOR_ON,          /* Cursor bypass 3 show/hide register */
+   SVGA_FIFO_CURSOR_X,           /* Cursor bypass 3 x register */
+   SVGA_FIFO_CURSOR_Y,           /* Cursor bypass 3 y register */
+   SVGA_FIFO_CURSOR_COUNT,       /* Incremented when any of the other 3 change */
+   SVGA_FIFO_CURSOR_LAST_UPDATED,/* Last time the host updated the cursor */
+
+   /* Valid with SVGA_FIFO_CAP_RESERVE: */
+   SVGA_FIFO_RESERVED,           /* Bytes past NEXT_CMD with real contents */
+
+   /*
+    * Valid with SVGA_FIFO_CAP_SCREEN_OBJECT or SVGA_FIFO_CAP_SCREEN_OBJECT_2:
+    *
+    * By default this is SVGA_ID_INVALID, to indicate that the cursor
+    * coordinates are specified relative to the virtual root. If this
+    * is set to a specific screen ID, cursor position is reinterpreted
+    * as a signed offset relative to that screen's origin.
+    */
+   SVGA_FIFO_CURSOR_SCREEN_ID,
+
+   /*
+    * Valid with SVGA_FIFO_CAP_DEAD
+    *
+    * An arbitrary value written by the host, drivers should not use it.
+    */
+   SVGA_FIFO_DEAD,
+
+   /*
+    * Valid with SVGA_FIFO_CAP_3D_HWVERSION_REVISED:
+    *
+    * Contains 3D HWVERSION (see SVGA3dHardwareVersion in svga3d_reg.h)
+    * on platforms that can enforce graphics resource limits.
+    */
+   SVGA_FIFO_3D_HWVERSION_REVISED,
+
+   /*
+    * XXX: The gap here, up until SVGA_FIFO_3D_CAPS, can be used for new
+    * registers, but this must be done carefully and with judicious use of
+    * capability bits, since comparisons based on SVGA_FIFO_MIN aren't
+    * enough to tell you whether the register exists: we've shipped drivers
+    * and products that used SVGA_FIFO_3D_CAPS but didn't know about some of
+    * the earlier ones.  The actual order of introduction was:
+    * - PITCHLOCK
+    * - 3D_CAPS
+    * - CURSOR_* (cursor bypass 3)
+    * - RESERVED
+    * So, code that wants to know whether it can use any of the
+    * aforementioned registers, or anything else added after PITCHLOCK and
+    * before 3D_CAPS, needs to reason about something other than
+    * SVGA_FIFO_MIN.
+    */
+
+   /*
+    * 3D caps block space; valid with 3D hardware version >=
+    * SVGA3D_HWVERSION_WS6_B1.
+    */
+   SVGA_FIFO_3D_CAPS      = 32,
+   SVGA_FIFO_3D_CAPS_LAST = 32 + 255,
+
+   /*
+    * End of VMX's current definition of "extended-FIFO registers".
+    * Registers before here are always enabled/disabled as a block; either
+    * the extended FIFO is enabled and includes all preceding registers, or
+    * it's disabled entirely.
+    *
+    * Block 3b (truly optional extended registers): Additional registers for
+    * the extended FIFO, which the VMX already knows how to enable and
+    * disable with correct granularity.
+    *
+    * Registers after here exist if and only if the guest SVGA driver
+    * sets SVGA_FIFO_MIN high enough to leave room for them.
+    */
+
+   /* Valid if register exists: */
+   SVGA_FIFO_GUEST_3D_HWVERSION, /* Guest driver's 3D version */
+   SVGA_FIFO_FENCE_GOAL,         /* Matching target for SVGA_IRQFLAG_FENCE_GOAL */
+   SVGA_FIFO_BUSY,               /* See "FIFO Synchronization Registers" */
+
+   /*
+    * Always keep this last.  This defines the maximum number of
+    * registers we know about.  At power-on, this value is placed in
+    * the SVGA_REG_MEM_REGS register, and we expect the guest driver
+    * to allocate this much space in FIFO memory for registers.
+    */
+    SVGA_FIFO_NUM_REGS
 };
 
 #define SVGA_FIFO_CAP_NONE              0
@@ -969,6 +1081,9 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         break;
 
     case SVGA_REG_MEM_REGS:
+        ret = s->num_fifo_regs;
+        break;
+
     case SVGA_REG_NUM_DISPLAYS:
     case SVGA_REG_PITCHLOCK:
     case SVGA_PALETTE_BASE ... SVGA_PALETTE_END:
@@ -1268,6 +1383,7 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s,
     memory_region_init_ram(&s->fifo_ram, NULL, "vmsvga.fifo", s->fifo_size,
                            &error_fatal);
     s->fifo = (uint32_t *)memory_region_get_ram_ptr(&s->fifo_ram);
+    s->num_fifo_regs = SVGA_FIFO_NUM_REGS;
 
     vga_common_init(&s->vga, OBJECT(dev));
     vga_init(&s->vga, OBJECT(dev), address_space, io, true);
-- 
1.9.1

[Qemu-devel] [PATCH 13/29] vmsvga: Add support for extended FIFO registers

[Liran Alon]

Supporting extended FIFO registers is required to support
SVGA_FIFO_FENCE which allows guest to receive interrupt when FIFO is
processed up to a specified fence.

Thus, as a preperation for supporting SVGA_FIFO_FENCE, add extened
FIFO registers support. Note that exposing SVGA_CAP_EXTENDED_FIFO
requires to support the following registers: SVGA_FIFO_CAPABILITIES,
SVGA_FIFO_FLAGS and SVGA_FIFO_3D_HWVERSION.

For more information on how SVGA_FIFO_3D_HWVERSION is negoitated, see
SVGA3D_Init() in VMware SVGA development kit.

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 8eeb0efc9cab..91f990544e14 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1033,6 +1033,7 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
                     SVGA_CAP_CURSOR_BYPASS;
         }
 #endif
+        caps |= SVGA_CAP_EXTENDED_FIFO;
         ret = caps;
         break;
 
@@ -1138,6 +1139,8 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
         } else {
             vga_dirty_log_start(&s->vga);
         }
+        if (s->enable)
+            s->fifo[SVGA_FIFO_3D_HWVERSION] = 0;    /* 3D disabled */
         break;
 
     case SVGA_REG_WIDTH:
@@ -1384,6 +1387,8 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s,
                            &error_fatal);
     s->fifo = (uint32_t *)memory_region_get_ram_ptr(&s->fifo_ram);
     s->num_fifo_regs = SVGA_FIFO_NUM_REGS;
+    s->fifo[SVGA_FIFO_CAPABILITIES] = 0;
+    s->fifo[SVGA_FIFO_FLAGS] = 0;
 
     vga_common_init(&s->vga, OBJECT(dev));
     vga_init(&s->vga, OBJECT(dev), address_space, io, true);
-- 
1.9.1

[Qemu-devel] [PATCH 14/29] vmsvga: Setup interrupt pin

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

This is necessary in order for device to raise interrupts.
Future patches will add functionality to device which will
need this ability.

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 91f990544e14..eae3f1455445 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1447,6 +1447,7 @@ static void pci_vmsvga_realize(PCIDevice *dev, Error **errp)
     dev->config[PCI_CACHE_LINE_SIZE] = 0x08;
     dev->config[PCI_LATENCY_TIMER] = 0x40;
     dev->config[PCI_INTERRUPT_LINE] = 0xff;          /* End */
+    dev->config[PCI_INTERRUPT_PIN] = 1;  /* interrupt pin A */
 
     memory_region_init_io(&s->io_bar, NULL, &vmsvga_io_ops, &s->chip,
                           "vmsvga-io", 0x10);
-- 
1.9.1

[Qemu-devel] [PATCH 15/29] vmsvga: Add interrupt mask and status registers

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

Add missing functionality of interrupt mask and status registers.
Writing to interrupt status register clears interrupt request.

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index eae3f1455445..4e4f6f8eec42 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -78,6 +78,8 @@ struct vmsvga_state_s {
     int redraw_fifo_first, redraw_fifo_last;
 
     uint32_t num_fifo_regs;
+    uint32_t irq_mask;
+    uint32_t irq_status;
 };
 
 #define TYPE_VMWARE_SVGA "vmware-svga"
@@ -104,6 +106,7 @@ struct pci_vmsvga_state_s {
 #define SVGA_INDEX_PORT         0x0
 #define SVGA_VALUE_PORT         0x1
 #define SVGA_BIOS_PORT          0x2
+#define SVGA_IRQSTATUS_PORT     0x8
 
 #define SVGA_VERSION_2
 
@@ -158,6 +161,7 @@ enum {
     SVGA_REG_MEM_REGS = 30,             /* Number of FIFO registers */
     SVGA_REG_NUM_DISPLAYS = 31,         /* Number of guest displays */
     SVGA_REG_PITCHLOCK = 32,            /* Fixed pitch for all modes */
+    SVGA_REG_IRQMASK = 33,              /* Interrupt mask */
 
     SVGA_PALETTE_BASE = 1024,           /* Base of SVGA color map */
     SVGA_PALETTE_END  = SVGA_PALETTE_BASE + 767,
@@ -183,6 +187,7 @@ enum {
 #define SVGA_CAP_EXTENDED_FIFO          (1 << 15)
 #define SVGA_CAP_MULTIMON               (1 << 16)
 #define SVGA_CAP_PITCHLOCK              (1 << 17)
+#define SVGA_CAP_IRQMASK                (1 << 18)
 
 /*
  * FIFO offsets (seen as an array of 32-bit words)
@@ -1034,6 +1039,7 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         }
 #endif
         caps |= SVGA_CAP_EXTENDED_FIFO;
+        caps |= SVGA_CAP_IRQMASK;
         ret = caps;
         break;
 
@@ -1091,6 +1097,10 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         ret = 0;
         break;
 
+    case SVGA_REG_IRQMASK:
+        ret = s->irq_mask;
+        break;
+
     default:
         if (s->index >= SVGA_SCRATCH_BASE &&
             s->index < SVGA_SCRATCH_BASE + s->scratch_size) {
@@ -1221,6 +1231,10 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
     case SVGA_PALETTE_BASE ... SVGA_PALETTE_END:
         break;
 
+    case SVGA_REG_IRQMASK:
+        s->irq_mask = value;
+        break;
+
     default:
         if (s->index >= SVGA_SCRATCH_BASE &&
                 s->index < SVGA_SCRATCH_BASE + s->scratch_size) {
@@ -1231,6 +1245,28 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
     }
 }
 
+static uint32_t vmsvga_irqstatus_read(void *opaque, uint32_t address)
+{
+    struct vmsvga_state_s *s = opaque;
+    return s->irq_status;
+}
+
+static void vmsvga_irqstatus_write(void *opaque, uint32_t address, uint32_t data)
+{
+    struct vmsvga_state_s *s = opaque;
+    struct pci_vmsvga_state_s *pci_vmsvga =
+        container_of(s, struct pci_vmsvga_state_s, chip);
+    PCIDevice *pci_dev = PCI_DEVICE(pci_vmsvga);
+
+    /*
+     * Clear selected interrupt sources and lower
+     * interrupt request when none are left active
+     */
+    s->irq_status &= ~data;
+    if (!s->irq_status)
+        pci_set_irq(pci_dev, 0);
+}
+
 static uint32_t vmsvga_bios_read(void *opaque, uint32_t address)
 {
     printf("%s: what are we supposed to return?\n", __func__);
@@ -1298,6 +1334,8 @@ static void vmsvga_reset(DeviceState *dev)
     s->redraw_fifo_first = 0;
     s->redraw_fifo_last = 0;
     s->syncing = 0;
+    s->irq_mask = 0;
+    s->irq_status = 0;
 
     vga_dirty_log_start(&s->vga);
 }
@@ -1327,12 +1365,18 @@ static int vmsvga_post_load(void *opaque, int version_id)
     struct vmsvga_state_s *s = opaque;
 
     s->invalidated = 1;
+
+    if (version_id < 1) {
+        s->irq_mask = 0;
+        s->irq_status = 0;
+    }
+
     return 0;
 }
 
 static const VMStateDescription vmstate_vmware_vga_internal = {
     .name = "vmware_vga_internal",
-    .version_id = 0,
+    .version_id = 1,
     .minimum_version_id = 0,
     .post_load = vmsvga_post_load,
     .fields = (VMStateField[]) {
@@ -1352,6 +1396,8 @@ static const VMStateDescription vmstate_vmware_vga_internal = {
         VMSTATE_UINT32(svgaid, struct vmsvga_state_s),
         VMSTATE_INT32(syncing, struct vmsvga_state_s),
         VMSTATE_UNUSED(4), /* was fb_size */
+        VMSTATE_UINT32_V(irq_mask, struct vmsvga_state_s, 1),
+        VMSTATE_UINT32_V(irq_status, struct vmsvga_state_s, 1),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -1404,6 +1450,7 @@ static uint64_t vmsvga_io_read(void *opaque, hwaddr addr, unsigned size)
     case SVGA_IO_MUL * SVGA_INDEX_PORT: return vmsvga_index_read(s, addr);
     case SVGA_IO_MUL * SVGA_VALUE_PORT: return vmsvga_value_read(s, addr);
     case SVGA_IO_MUL * SVGA_BIOS_PORT: return vmsvga_bios_read(s, addr);
+    case SVGA_IO_MUL * SVGA_IRQSTATUS_PORT: return vmsvga_irqstatus_read(s, addr);
     default: return -1u;
     }
 }
@@ -1423,6 +1470,9 @@ static void vmsvga_io_write(void *opaque, hwaddr addr,
     case SVGA_IO_MUL * SVGA_BIOS_PORT:
         vmsvga_bios_write(s, addr, data);
         break;
+    case SVGA_IO_MUL * SVGA_IRQSTATUS_PORT:
+        vmsvga_irqstatus_write(s, addr, data);
+        break;
     }
 }
 
-- 
1.9.1

[Qemu-devel] [PATCH 16/29] vmsvga: Add framework code for SVGA command to raise interrupt

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

Should not change semantics.
This is done as a preparation for future patches.

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 4e4f6f8eec42..ce5b8814ac91 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -718,6 +718,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
     struct vmsvga_cursor_definition_s cursor;
     uint32_t cmd_start;
     bool cmd_ignored;
+    bool irq_pending = false;
 
     len = vmsvga_fifo_length(s);
     while (len > 0 && --maxloop > 0) {
@@ -920,6 +921,13 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
     }
 
     s->syncing = 0;
+
+    /* Need to raise irq ? */
+    if (irq_pending && (s->irq_status & s->irq_mask)) {
+        struct pci_vmsvga_state_s *pci_vmsvga
+            = container_of(s, struct pci_vmsvga_state_s, chip);
+        pci_set_irq(PCI_DEVICE(pci_vmsvga), 1);
+    }
 }
 
 static uint32_t vmsvga_index_read(void *opaque, uint32_t address)
-- 
1.9.1

[Qemu-devel] [PATCH 17/29] vmsvga: Define interrupt source flags for interrupt status and mask registers

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

Based on Linux kernel
drivers/gpu/drm/vmwgfx/device_include/svga_reg.h.

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index ce5b8814ac91..597051ec5c92 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -334,6 +334,18 @@ enum {
 #define SVGA_MAX_WIDTH                  ROUND_UP(2360, VNC_DIRTY_PIXELS_PER_BIT)
 #define SVGA_MAX_HEIGHT                 1770
 
+/*
+ * Interrupt source flags for IRQSTATUS_PORT and IRQMASK.
+ *
+ * Interrupts are only supported when the
+ * SVGA_CAP_IRQMASK capability is present.
+ */
+#define SVGA_IRQFLAG_ANY_FENCE            0x1    /* Any fence was passed */
+#define SVGA_IRQFLAG_FIFO_PROGRESS        0x2    /* Made forward progress in the FIFO */
+#define SVGA_IRQFLAG_FENCE_GOAL           0x4    /* SVGA_FIFO_FENCE_GOAL reached */
+#define SVGA_IRQFLAG_COMMAND_BUFFER       0x8    /* Command buffer completed */
+#define SVGA_IRQFLAG_ERROR                0x10   /* Error while processing commands */
+
 #ifdef VERBOSE
 # define GUEST_OS_BASE          0x5001
 static const char *vmsvga_guest_id[] = {
-- 
1.9.1

[Qemu-devel] [PATCH 18/29] vmsvga: Add support for SVGA_IRQFLAG_FIFO_PROGRESS

[Liran Alon]

If vmsvga supports interrupts (SVGA_CAP_IRQMASK), some guests wait for
FIFO to become not full by sleeping until FIFO_PROGRESS interrupt occurs.
This is the most efficient thing to do when the FIFO fills up.

To support these guests, add support for SVGA_IRQFLAG_FIFO_PROGRESS.

See usage example by guest in VMware SVGA development kit
SVGAFIFOFull().

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 597051ec5c92..dc5f4681f0d3 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -731,6 +731,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
     uint32_t cmd_start;
     bool cmd_ignored;
     bool irq_pending = false;
+    bool fifo_progress = false;
 
     len = vmsvga_fifo_length(s);
     while (len > 0 && --maxloop > 0) {
@@ -930,6 +931,15 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             s->fifo[SVGA_FIFO_STOP] = cpu_to_le32(s->fifo_stop);
             break;
         }
+
+        if (s->fifo_stop != cmd_start)
+            fifo_progress = true;
+    }
+
+    if ((s->irq_mask & SVGA_IRQFLAG_FIFO_PROGRESS) &&
+        fifo_progress) {
+        s->irq_status |= SVGA_IRQFLAG_FIFO_PROGRESS;
+        irq_pending = true;
     }
 
     s->syncing = 0;
-- 
1.9.1

[Qemu-devel] [PATCH 19/29] vmsvga: Handle SVGA_CMD_FENCE command

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

According to Linux kernel
drivers/gpu/drm/vmwgfx/device_include/svga_reg.h:

/*
 * SVGA_CMD_FENCE --
 *
 *    Insert a synchronization fence.  When the SVGA device reaches
 *    this command, it will copy the 'fence' value into the
 *    SVGA_FIFO_FENCE register. It will also compare the fence against
 *    SVGA_FIFO_FENCE_GOAL. If the fence matches the goal and the
 *    SVGA_IRQFLAG_FENCE_GOAL interrupt is enabled, the device will
 *    raise this interrupt.
 *
 * Availability:
 *    SVGA_FIFO_FENCE for this command,
 *    SVGA_CAP_IRQMASK for SVGA_FIFO_FENCE_GOAL.
 */

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index dc5f4681f0d3..73e373665bdb 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -217,6 +217,7 @@ enum {
 
     SVGA_FIFO_CAPABILITIES = 4,
     SVGA_FIFO_FLAGS,
+    /* Valid with SVGA_FIFO_CAP_FENCE */
     SVGA_FIFO_FENCE,
 
    /*
@@ -729,6 +730,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
     int x, y, dx, dy, width, height;
     struct vmsvga_cursor_definition_s cursor;
     uint32_t cmd_start;
+    uint32_t fence_arg;
     bool cmd_ignored;
     bool irq_pending = false;
     bool fifo_progress = false;
@@ -832,6 +834,28 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             args = 6;
             goto ignoredcmd;
 
+        case SVGA_CMD_FENCE:
+            len -= 2;
+            if (len < 0) {
+                goto rewind;
+            }
+
+            fence_arg = vmsvga_fifo_read(s);
+            s->fifo[SVGA_FIFO_FENCE] = cpu_to_le32(fence_arg);
+
+            if (s->irq_mask & SVGA_IRQFLAG_ANY_FENCE) {
+                s->irq_status |= SVGA_IRQFLAG_ANY_FENCE;
+                irq_pending = true;
+            }
+            if ((s->irq_mask & SVGA_IRQFLAG_FENCE_GOAL)
+               && (s->fifo_min > SVGA_FIFO_FENCE_GOAL)
+               && (s->fifo[SVGA_FIFO_FENCE_GOAL] == fence_arg)) {
+                s->irq_status |= SVGA_IRQFLAG_FENCE_GOAL;
+                irq_pending = true;
+            }
+
+            break;
+
         /*
          * Deprecated commands are neither documented in VMware SVGA development kit
          * nor in Linux kernel vmware-svga driver source code.
@@ -899,13 +923,6 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
         case SVGA_CMD_SURFACE_ALPHA_BLEND: /* deprecated */
             args = 12;
             goto badcmd;
-        case SVGA_CMD_FENCE:
-            len -= 1;
-            if (len < 0) {
-                goto rewind;
-            }
-            args = 1;
-            goto badcmd;
 
         default:
             args = 0;
@@ -1463,7 +1480,7 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s,
                            &error_fatal);
     s->fifo = (uint32_t *)memory_region_get_ram_ptr(&s->fifo_ram);
     s->num_fifo_regs = SVGA_FIFO_NUM_REGS;
-    s->fifo[SVGA_FIFO_CAPABILITIES] = 0;
+    s->fifo[SVGA_FIFO_CAPABILITIES] = SVGA_FIFO_CAP_FENCE;
     s->fifo[SVGA_FIFO_FLAGS] = 0;
 
     vga_common_init(&s->vga, OBJECT(dev));
-- 
1.9.1

[Qemu-devel] [PATCH 20/29] vmsvga: Use standard names for params defining hardware cursor image

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

AND/XOR mask is a standard method for defining hardware cursor images.
These are also the names suggested by VMware SVGA DevKit.

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 73e373665bdb..d79b3400452b 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -619,8 +619,8 @@ struct vmsvga_cursor_definition_s {
     uint32_t bpp;
     int hot_x;
     int hot_y;
-    uint32_t mask[1024];
-    uint32_t image[4096];
+    uint32_t and_mask[1024];
+    uint32_t xor_mask[4096];
 };
 
 #define SVGA_BITMAP_SIZE(w, h)          ((((w) + 31) >> 5) * (h))
@@ -638,20 +638,20 @@ static inline void vmsvga_cursor_define(struct vmsvga_state_s *s,
     qc->hot_y = c->hot_y;
     switch (c->bpp) {
     case 1:
-        cursor_set_mono(qc, 0xffffff, 0x000000, (void *)c->image,
-                        1, (void *)c->mask);
+        cursor_set_mono(qc, 0xffffff, 0x000000, (void *)c->xor_mask,
+                        1, (void *)c->and_mask);
 #ifdef DEBUG
         cursor_print_ascii_art(qc, "vmware/mono");
 #endif
         break;
     case 32:
         /* fill alpha channel from mask, set color to zero */
-        cursor_set_mono(qc, 0x000000, 0x000000, (void *)c->mask,
-                        1, (void *)c->mask);
+        cursor_set_mono(qc, 0x000000, 0x000000, (void *)c->and_mask,
+                        1, (void *)c->and_mask);
         /* add in rgb values */
         pixels = c->width * c->height;
         for (i = 0; i < pixels; i++) {
-            qc->data[i] |= c->image[i] & 0xffffff;
+            qc->data[i] |= c->xor_mask[i] & 0xffffff;
         }
 #ifdef DEBUG
         cursor_print_ascii_art(qc, "vmware/32bit");
@@ -801,9 +801,9 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             if (cursor.width > 256
                 || cursor.height > 256
                 || cursor.bpp > 32
-                || SVGA_BITMAP_SIZE(x, y) > ARRAY_SIZE(cursor.mask)
+                || SVGA_BITMAP_SIZE(x, y) > ARRAY_SIZE(cursor.and_mask)
                 || SVGA_PIXMAP_SIZE(x, y, cursor.bpp)
-                    > ARRAY_SIZE(cursor.image)) {
+                    > ARRAY_SIZE(cursor.xor_mask)) {
                     goto badcmd;
             }
 
@@ -813,10 +813,10 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             }
 
             for (args = 0; args < SVGA_BITMAP_SIZE(x, y); args++) {
-                cursor.mask[args] = vmsvga_fifo_read_raw(s);
+                cursor.and_mask[args] = vmsvga_fifo_read_raw(s);
             }
             for (args = 0; args < SVGA_PIXMAP_SIZE(x, y, cursor.bpp); args++) {
-                cursor.image[args] = vmsvga_fifo_read_raw(s);
+                cursor.xor_mask[args] = vmsvga_fifo_read_raw(s);
             }
 #ifdef HW_MOUSE_ACCEL
             vmsvga_cursor_define(s, &cursor);
-- 
1.9.1

[Qemu-devel] [PATCH 21/29] vmsvga: Use AND mask bpp parameter in SVGA_CMD_DEFINE_CURSOR

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

Prefer variable names of cursor structure from VMware SVGA DevKit.
In addition, make sure to use the AND mask bpp parameter in
SVGA_CMD_DEFINE_CURSOR.

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index d79b3400452b..ef2c8bdbf5be 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -616,9 +616,10 @@ struct vmsvga_cursor_definition_s {
     uint32_t width;
     uint32_t height;
     int id;
-    uint32_t bpp;
     int hot_x;
     int hot_y;
+    uint32_t and_mask_bpp; // Value must be 1 or equal to BITS_PER_PIXEL
+    uint32_t xor_mask_bpp; // Value must be 1 or equal to BITS_PER_PIXEL
     uint32_t and_mask[1024];
     uint32_t xor_mask[4096];
 };
@@ -636,7 +637,7 @@ static inline void vmsvga_cursor_define(struct vmsvga_state_s *s,
     qc = cursor_alloc(c->width, c->height);
     qc->hot_x = c->hot_x;
     qc->hot_y = c->hot_y;
-    switch (c->bpp) {
+    switch (c->xor_mask_bpp) {
     case 1:
         cursor_set_mono(qc, 0xffffff, 0x000000, (void *)c->xor_mask,
                         1, (void *)c->and_mask);
@@ -659,7 +660,7 @@ static inline void vmsvga_cursor_define(struct vmsvga_state_s *s,
         break;
     default:
         fprintf(stderr, "%s: unhandled bpp %d, using fallback cursor\n",
-                __func__, c->bpp);
+                __func__, c->xor_mask_bpp);
         cursor_put(qc);
         qc = cursor_builtin_left_ptr();
     }
@@ -794,15 +795,18 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             cursor.hot_y = vmsvga_fifo_read(s);
             cursor.width = x = vmsvga_fifo_read(s);
             cursor.height = y = vmsvga_fifo_read(s);
-            vmsvga_fifo_read(s);
-            cursor.bpp = vmsvga_fifo_read(s);
+            cursor.and_mask_bpp = vmsvga_fifo_read(s);
+            cursor.xor_mask_bpp = vmsvga_fifo_read(s);
 
-            args = SVGA_BITMAP_SIZE(x, y) + SVGA_PIXMAP_SIZE(x, y, cursor.bpp);
+            args = SVGA_PIXMAP_SIZE(x, y, cursor.and_mask_bpp) +
+                SVGA_PIXMAP_SIZE(x, y, cursor.xor_mask_bpp);
             if (cursor.width > 256
                 || cursor.height > 256
-                || cursor.bpp > 32
-                || SVGA_BITMAP_SIZE(x, y) > ARRAY_SIZE(cursor.and_mask)
-                || SVGA_PIXMAP_SIZE(x, y, cursor.bpp)
+                || cursor.and_mask_bpp > 32
+                || cursor.xor_mask_bpp > 32
+                || SVGA_PIXMAP_SIZE(x, y, cursor.and_mask_bpp)
+                    > ARRAY_SIZE(cursor.and_mask)
+                || SVGA_PIXMAP_SIZE(x, y, cursor.xor_mask_bpp)
                     > ARRAY_SIZE(cursor.xor_mask)) {
                     goto badcmd;
             }
@@ -812,10 +816,10 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
                 goto rewind;
             }
 
-            for (args = 0; args < SVGA_BITMAP_SIZE(x, y); args++) {
+            for (args = 0; args < SVGA_PIXMAP_SIZE(x, y, cursor.and_mask_bpp); args++) {
                 cursor.and_mask[args] = vmsvga_fifo_read_raw(s);
             }
-            for (args = 0; args < SVGA_PIXMAP_SIZE(x, y, cursor.bpp); args++) {
+            for (args = 0; args < SVGA_PIXMAP_SIZE(x, y, cursor.xor_mask_bpp); args++) {
                 cursor.xor_mask[args] = vmsvga_fifo_read_raw(s);
             }
 #ifdef HW_MOUSE_ACCEL
-- 
1.9.1

[Qemu-devel] [PATCH 22/29] vmsvga: Increase size of cursor AND bitmask

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

Some guests are using 64x64 32bpp cursor pixel array, the old size of 1024 integers
was not sufficient to store such large masks.

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index ef2c8bdbf5be..46f03fe90ac0 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -620,7 +620,7 @@ struct vmsvga_cursor_definition_s {
     int hot_y;
     uint32_t and_mask_bpp; // Value must be 1 or equal to BITS_PER_PIXEL
     uint32_t xor_mask_bpp; // Value must be 1 or equal to BITS_PER_PIXEL
-    uint32_t and_mask[1024];
+    uint32_t and_mask[4096];
     uint32_t xor_mask[4096];
 };
 
-- 
1.9.1

[Qemu-devel] [PATCH 23/29] vmsvga: Implement initial support for rgb-alpha cursors

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

This adds missing implementation of cursors having rgb and alpha-blending component.
Although more work is required in QEMU framework to fully support alpha-blending
functionality for cursors, the suggested fix shows well defined cursor shape instead
of crashing video device or no hint of visible mouse cursor.

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 85 +++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 72 insertions(+), 13 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 46f03fe90ac0..49b46938207e 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -668,6 +668,38 @@ static inline void vmsvga_cursor_define(struct vmsvga_state_s *s,
     dpy_cursor_define(s->vga.con, qc);
     cursor_put(qc);
 }
+
+static inline void vmsvga_rgba_cursor_define(struct vmsvga_state_s *s,
+                struct vmsvga_cursor_definition_s *c)
+{
+    QEMUCursor *qc;
+    int i, pixels = c->width * c->height;
+
+    qc = cursor_alloc(c->width, c->height);
+    qc->hot_x = c->hot_x;
+    qc->hot_y = c->hot_y;
+
+    /* fill alpha channel and rgb values */
+    for (i = 0; i < pixels; i++) {
+        qc->data[i] = c->xor_mask[i];
+        /*
+         * Turn semi-transparent pixels to fully opaque
+         * (opaque pixels stay opaque), due to lack of
+         * alpha-blending support in QEMU framework.
+         * This is a trade-off between cursor completely
+         * missing and cursors with some minor artifacts
+         * (such as Windows Aero style cursors).
+         */
+        if (c->and_mask[i]) {
+            qc->data[i] |= 0xff000000;
+        }
+    }
+#ifdef DEBUG
+    cursor_print_ascii_art(qc, "rgba");
+#endif
+    dpy_cursor_define(s->vga.con, qc);
+    cursor_put(qc);
+}
 #endif
 
 static inline int vmsvga_fifo_length(struct vmsvga_state_s *s)
@@ -728,7 +760,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
 {
     uint32_t cmd, colour;
     int args, len, maxloop = 1024;
-    int x, y, dx, dy, width, height;
+    int i, x, y, dx, dy, width, height;
     struct vmsvga_cursor_definition_s cursor;
     uint32_t cmd_start;
     uint32_t fence_arg;
@@ -830,6 +862,44 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             goto badcmd;
 #endif
 
+        case SVGA_CMD_DEFINE_ALPHA_CURSOR:
+            len -= 6;
+            if (len < 0) {
+                goto rewind;
+            }
+
+            cursor.id = vmsvga_fifo_read(s);
+            cursor.hot_x = vmsvga_fifo_read(s);
+            cursor.hot_y = vmsvga_fifo_read(s);
+            cursor.width  = x = vmsvga_fifo_read(s);
+            cursor.height = y = vmsvga_fifo_read(s);
+            cursor.and_mask_bpp = 32;
+            cursor.xor_mask_bpp = 32;
+            args = x * y;
+            if ((SVGA_PIXMAP_SIZE(x, y, 32) > ARRAY_SIZE(cursor.and_mask))
+               || (SVGA_PIXMAP_SIZE(x, y, 32) > ARRAY_SIZE(cursor.xor_mask))) {
+                    goto badcmd;
+            }
+
+            len -= args;
+            if (len < 0) {
+                goto rewind;
+            }
+
+            for (i = 0; i < args; i++) {
+                uint32_t rgba = vmsvga_fifo_read_raw(s);
+                cursor.xor_mask[i] = rgba & 0x00ffffff;
+                cursor.and_mask[i] = rgba & 0xff000000;
+            }
+
+#ifdef HW_MOUSE_ACCEL
+            vmsvga_rgba_cursor_define(s, &cursor);
+            break;
+#else
+            args = 0;
+            goto badcmd;
+#endif
+
         case SVGA_CMD_FRONT_ROP_FILL:
             len -= 1;
             if (len < 0) {
@@ -903,18 +973,6 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             }
             args = 7;
             goto badcmd;
-        case SVGA_CMD_DEFINE_ALPHA_CURSOR:
-            len -= 6;
-            if (len < 0) {
-                goto rewind;
-            }
-            vmsvga_fifo_read(s);
-            vmsvga_fifo_read(s);
-            vmsvga_fifo_read(s);
-            x = vmsvga_fifo_read(s);
-            y = vmsvga_fifo_read(s);
-            args = x * y;
-            goto badcmd;
         case SVGA_CMD_DRAW_GLYPH_CLIPPED: /* deprecated */
             len -= 4;
             if (len < 0) {
@@ -1085,6 +1143,7 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
 #endif
 #ifdef HW_MOUSE_ACCEL
         if (dpy_cursor_define_supported(s->vga.con)) {
+            caps |= SVGA_CAP_ALPHA_CURSOR;
             caps |= SVGA_CAP_CURSOR | SVGA_CAP_CURSOR_BYPASS_2 |
                     SVGA_CAP_CURSOR_BYPASS;
         }
-- 
1.9.1

Re: [Qemu-devel] [PATCH 23/29] vmsvga: Implement initial support for rgb-alpha cursors

[Gerd Hoffmann]

> +#ifdef HW_MOUSE_ACCEL
> +            vmsvga_rgba_cursor_define(s, &cursor);
> +            break;
> +#else
> +            args = 0;
> +            goto badcmd;
> +#endif

Hmm, do we want keep this #ifdef ?

In case it is useful to disable hardware cursor support for some reason
(which I doubt) this should be a runtime not a compile time option.  So,
feel free to drop this to simplify the code.

cheers,
  Gerd

[Qemu-devel] [PATCH 24/29] vmsvga: Add support for SVGA_FIFO_CAP_CURSOR_BYPASS_3

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

This adds tracking of guest cursor position with the help of FIFO
registers reporting pointing device coordindates.

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 49b46938207e..1db8f92f053b 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -51,6 +51,7 @@ struct vmsvga_state_s {
         int y;
         int on;
     } cursor;
+    uint32_t last_fifo_cursor_count;
 
     int index;
     int scratch_size;
@@ -326,6 +327,7 @@ enum {
 #define SVGA_FIFO_CAP_FENCE             (1 << 0)
 #define SVGA_FIFO_CAP_ACCELFRONT        (1 << 1)
 #define SVGA_FIFO_CAP_PITCHLOCK         (1 << 2)
+#define SVGA_FIFO_CAP_CURSOR_BYPASS_3   (1 << 4)
 
 #define SVGA_FIFO_FLAG_NONE             0
 #define SVGA_FIFO_FLAG_ACCELFRONT       (1 << 0)
@@ -417,6 +419,35 @@ enum {
     SVGA_CURSOR_ON_RESTORE_TO_FB = 3,
 };
 
+/* Update cursor position from SVGA_FIFO_CURSOR registers */
+static void cursor_update_from_fifo(struct vmsvga_state_s *s)
+{
+    uint32_t fifo_cursor_count;
+    uint32_t on_off;
+
+    if (!s->config || !s->enable) {
+        return;
+    }
+
+    if (s->fifo_min <= SVGA_FIFO_CURSOR_LAST_UPDATED) {
+        return;
+    }
+
+    fifo_cursor_count = s->fifo[SVGA_FIFO_CURSOR_COUNT];
+    if (fifo_cursor_count == s->last_fifo_cursor_count)
+        return;
+
+    s->last_fifo_cursor_count = fifo_cursor_count;
+    on_off = s->fifo[SVGA_FIFO_CURSOR_ON] ? SVGA_CURSOR_ON_SHOW : SVGA_CURSOR_ON_HIDE;
+    s->cursor.on = on_off;
+    s->cursor.x = s->fifo[SVGA_FIFO_CURSOR_X];
+    s->cursor.y = s->fifo[SVGA_FIFO_CURSOR_Y];
+
+#ifdef HW_MOUSE_ACCEL
+    dpy_mouse_set(s->vga.con, s->cursor.x, s->cursor.y, s->cursor.on);
+#endif
+}
+
 static inline bool vmsvga_verify_rect(DisplaySurface *surface,
                                       const char *name,
                                       int x, int y, int w, int h)
@@ -1423,6 +1454,7 @@ static void vmsvga_update_display(void *opaque)
 
     vmsvga_fifo_run(s);
     vmsvga_update_rect_flush(s);
+    cursor_update_from_fifo(s);
 
     if (s->invalidated) {
         s->invalidated = 0;
@@ -1446,6 +1478,7 @@ static void vmsvga_reset(DeviceState *dev)
     s->syncing = 0;
     s->irq_mask = 0;
     s->irq_status = 0;
+    s->last_fifo_cursor_count = 0;
 
     vga_dirty_log_start(&s->vga);
 }
@@ -1479,6 +1512,7 @@ static int vmsvga_post_load(void *opaque, int version_id)
     if (version_id < 1) {
         s->irq_mask = 0;
         s->irq_status = 0;
+        s->last_fifo_cursor_count = 0;
     }
 
     return 0;
@@ -1508,6 +1542,7 @@ static const VMStateDescription vmstate_vmware_vga_internal = {
         VMSTATE_UNUSED(4), /* was fb_size */
         VMSTATE_UINT32_V(irq_mask, struct vmsvga_state_s, 1),
         VMSTATE_UINT32_V(irq_status, struct vmsvga_state_s, 1),
+        VMSTATE_UINT32_V(last_fifo_cursor_count, struct vmsvga_state_s, 1),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -1543,7 +1578,8 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s,
                            &error_fatal);
     s->fifo = (uint32_t *)memory_region_get_ram_ptr(&s->fifo_ram);
     s->num_fifo_regs = SVGA_FIFO_NUM_REGS;
-    s->fifo[SVGA_FIFO_CAPABILITIES] = SVGA_FIFO_CAP_FENCE;
+    s->fifo[SVGA_FIFO_CAPABILITIES] =
+        SVGA_FIFO_CAP_FENCE | SVGA_FIFO_CAP_CURSOR_BYPASS_3;
     s->fifo[SVGA_FIFO_FLAGS] = 0;
 
     vga_common_init(&s->vga, OBJECT(dev));
-- 
1.9.1

[Qemu-devel] [PATCH 25/29] vmsvga: Add basic support for GMR registers and FIFO commands

[Liran Alon]

We don't support GMR regions while reporting caps, but some guests may try
to send us some GMR queries and we do our best to ignore them while avoiding
FIFO command crash.

Reported-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 1db8f92f053b..b2f3456357bd 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -164,6 +164,12 @@ enum {
     SVGA_REG_PITCHLOCK = 32,            /* Fixed pitch for all modes */
     SVGA_REG_IRQMASK = 33,              /* Interrupt mask */
 
+    /* Guest memory regions */
+    SVGA_REG_GMR_ID = 41,
+    SVGA_REG_GMR_DESCRIPTOR = 42,
+    SVGA_REG_GMR_MAX_IDS = 43,
+    SVGA_REG_GMR_MAX_DESCRIPTOR_LENGTH = 44,
+
     SVGA_PALETTE_BASE = 1024,           /* Base of SVGA color map */
     SVGA_PALETTE_END  = SVGA_PALETTE_BASE + 767,
     SVGA_SCRATCH_BASE = SVGA_PALETTE_BASE + 768,
@@ -409,6 +415,8 @@ enum {
     SVGA_CMD_SURFACE_ALPHA_BLEND = 28,    /* deprecated */
     SVGA_CMD_FRONT_ROP_FILL = 29,
     SVGA_CMD_FENCE = 30,
+    SVGA_CMD_DEFINE_GMR2 = 41,
+    SVGA_CMD_REMAP_GMR2 = 42,
 };
 
 /* Legal values for the SVGA_REG_CURSOR_ON register in cursor bypass mode */
@@ -419,6 +427,13 @@ enum {
     SVGA_CURSOR_ON_RESTORE_TO_FB = 3,
 };
 
+enum {
+   SVGA_REMAP_GMR2_PPN32         = 0,
+   SVGA_REMAP_GMR2_VIA_GMR       = (1 << 0),
+   SVGA_REMAP_GMR2_PPN64         = (1 << 1),
+   SVGA_REMAP_GMR2_SINGLE_PPN    = (1 << 2),
+};
+
 /* Update cursor position from SVGA_FIFO_CURSOR registers */
 static void cursor_update_from_fifo(struct vmsvga_state_s *s)
 {
@@ -795,6 +810,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
     struct vmsvga_cursor_definition_s cursor;
     uint32_t cmd_start;
     uint32_t fence_arg;
+    uint32_t flags, num_pages;
     bool cmd_ignored;
     bool irq_pending = false;
     bool fifo_progress = false;
@@ -961,6 +977,36 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
 
             break;
 
+        case SVGA_CMD_DEFINE_GMR2:
+            len -= 1;
+            if (len < 0) {
+                goto rewind;
+            }
+            args = 2;
+            goto badcmd;
+
+        case SVGA_CMD_REMAP_GMR2:
+            len -= 5;
+            if (len < 0) {
+                goto rewind;
+            }
+
+            vmsvga_fifo_read(s);            /* gmrId */
+            flags = vmsvga_fifo_read(s);
+            vmsvga_fifo_read(s);            /* offsetPages */
+            num_pages = vmsvga_fifo_read(s);
+
+            if (flags & SVGA_REMAP_GMR2_VIA_GMR) {
+                /* Read single struct SVGAGuestPtr */
+                args = 2;
+            } else {
+                args = (flags & SVGA_REMAP_GMR2_SINGLE_PPN) ? 1 : num_pages;
+                if (flags & SVGA_REMAP_GMR2_PPN64)
+                    args *= 2;
+            }
+
+            goto badcmd;
+
         /*
          * Deprecated commands are neither documented in VMware SVGA development kit
          * nor in Linux kernel vmware-svga driver source code.
@@ -1242,6 +1288,15 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         ret = s->irq_mask;
         break;
 
+    /* Guest memory regions */
+    case SVGA_REG_GMR_ID:
+    case SVGA_REG_GMR_DESCRIPTOR:
+    case SVGA_REG_GMR_MAX_IDS:
+    case SVGA_REG_GMR_MAX_DESCRIPTOR_LENGTH:
+        /* We don't support GMRs */
+        ret = 0;
+        break;
+
     default:
         if (s->index >= SVGA_SCRATCH_BASE &&
             s->index < SVGA_SCRATCH_BASE + s->scratch_size) {
-- 
1.9.1

[Qemu-devel] [PATCH 26/29] vmsvga: Add basic support for display topology

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

Report SVGA_CAP_DISPLAY_TOPOLOGY capability and support single
legacy screen at fixed offset of (0,0).

This is a pre-requesite for supporting PITCHLOCK feature which is
required by Linux kernel vmsvga driver (See Linux kernel
vmw_driver_load()).

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index b2f3456357bd..edd336e65005 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -81,6 +81,7 @@ struct vmsvga_state_s {
     uint32_t num_fifo_regs;
     uint32_t irq_mask;
     uint32_t irq_status;
+    uint32_t display_id;
 };
 
 #define TYPE_VMWARE_SVGA "vmware-svga"
@@ -103,6 +104,9 @@ struct pci_vmsvga_state_s {
 #define SVGA_ID_1               SVGA_MAKE_ID(1)
 #define SVGA_ID_2               SVGA_MAKE_ID(2)
 
+/* "Invalid" value for all SVGA IDs. (Version ID, screen object ID, surface ID...) */
+#define SVGA_ID_INVALID         0xFFFFFFFF
+
 #define SVGA_LEGACY_BASE_PORT   0x4560
 #define SVGA_INDEX_PORT         0x0
 #define SVGA_VALUE_PORT         0x1
@@ -164,6 +168,15 @@ enum {
     SVGA_REG_PITCHLOCK = 32,            /* Fixed pitch for all modes */
     SVGA_REG_IRQMASK = 33,              /* Interrupt mask */
 
+    /* Legacy multi-monitor support */
+    SVGA_REG_NUM_GUEST_DISPLAYS = 34,   /* Number of guest displays in X/Y direction */
+    SVGA_REG_DISPLAY_ID = 35,           /* Display ID for the following display attributes */
+    SVGA_REG_DISPLAY_IS_PRIMARY = 36,   /* Whether this is a primary display */
+    SVGA_REG_DISPLAY_POSITION_X = 37,   /* The display position x */
+    SVGA_REG_DISPLAY_POSITION_Y = 38,   /* The display position y */
+    SVGA_REG_DISPLAY_WIDTH = 39,        /* The display's width */
+    SVGA_REG_DISPLAY_HEIGHT = 40,       /* The display's height */
+
     /* Guest memory regions */
     SVGA_REG_GMR_ID = 41,
     SVGA_REG_GMR_DESCRIPTOR = 42,
@@ -195,6 +208,7 @@ enum {
 #define SVGA_CAP_MULTIMON               (1 << 16)
 #define SVGA_CAP_PITCHLOCK              (1 << 17)
 #define SVGA_CAP_IRQMASK                (1 << 18)
+#define SVGA_CAP_DISPLAY_TOPOLOGY       (1 << 19)   // Legacy multi-monitor support
 
 /*
  * FIFO offsets (seen as an array of 32-bit words)
@@ -1227,6 +1241,7 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
 #endif
         caps |= SVGA_CAP_EXTENDED_FIFO;
         caps |= SVGA_CAP_IRQMASK;
+        caps |= SVGA_CAP_DISPLAY_TOPOLOGY;
         ret = caps;
         break;
 
@@ -1288,6 +1303,32 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         ret = s->irq_mask;
         break;
 
+    case SVGA_REG_NUM_GUEST_DISPLAYS:
+        ret = 1;
+        break;
+    case SVGA_REG_DISPLAY_ID:
+        ret = s->display_id;
+        break;
+    case SVGA_REG_DISPLAY_IS_PRIMARY:
+        ret = s->display_id == 0 ? 1 : 0;
+        break;
+    case SVGA_REG_DISPLAY_POSITION_X:
+    case SVGA_REG_DISPLAY_POSITION_Y:
+        ret = 0;
+        break;
+    case SVGA_REG_DISPLAY_WIDTH:
+        if ((s->display_id == 0) || (s->display_id == SVGA_ID_INVALID))
+            ret = s->new_width ? s->new_width : surface_width(surface);
+        else
+            ret = 0;
+        break;
+    case SVGA_REG_DISPLAY_HEIGHT:
+        if ((s->display_id == 0) || (s->display_id == SVGA_ID_INVALID))
+            ret = s->new_height ? s->new_height : surface_height(surface);
+        else
+            ret = 0;
+        break;
+
     /* Guest memory regions */
     case SVGA_REG_GMR_ID:
     case SVGA_REG_GMR_DESCRIPTOR:
@@ -1431,6 +1472,28 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
         s->irq_mask = value;
         break;
 
+    /* We support single legacy screen at fixed offset of (0,0) */
+    case SVGA_REG_DISPLAY_ID:
+        s->display_id = value;
+        break;
+    case SVGA_REG_NUM_GUEST_DISPLAYS:
+    case SVGA_REG_DISPLAY_IS_PRIMARY:
+    case SVGA_REG_DISPLAY_POSITION_X:
+    case SVGA_REG_DISPLAY_POSITION_Y:
+        break;
+    case SVGA_REG_DISPLAY_WIDTH:
+        if ((s->display_id == 0) && (value <= SVGA_MAX_WIDTH)) {
+            s->new_width = value;
+            s->invalidated = 1;
+        }
+        break;
+    case SVGA_REG_DISPLAY_HEIGHT:
+        if ((s->display_id == 0) && (value <= SVGA_MAX_HEIGHT)) {
+            s->new_height = value;
+            s->invalidated = 1;
+        }
+        break;
+
     default:
         if (s->index >= SVGA_SCRATCH_BASE &&
                 s->index < SVGA_SCRATCH_BASE + s->scratch_size) {
@@ -1534,6 +1597,7 @@ static void vmsvga_reset(DeviceState *dev)
     s->irq_mask = 0;
     s->irq_status = 0;
     s->last_fifo_cursor_count = 0;
+    s->display_id = SVGA_ID_INVALID;
 
     vga_dirty_log_start(&s->vga);
 }
@@ -1568,6 +1632,7 @@ static int vmsvga_post_load(void *opaque, int version_id)
         s->irq_mask = 0;
         s->irq_status = 0;
         s->last_fifo_cursor_count = 0;
+        s->display_id = SVGA_ID_INVALID;
     }
 
     return 0;
@@ -1598,6 +1663,7 @@ static const VMStateDescription vmstate_vmware_vga_internal = {
         VMSTATE_UINT32_V(irq_mask, struct vmsvga_state_s, 1),
         VMSTATE_UINT32_V(irq_status, struct vmsvga_state_s, 1),
         VMSTATE_UINT32_V(last_fifo_cursor_count, struct vmsvga_state_s, 1),
+        VMSTATE_UINT32_V(display_id, struct vmsvga_state_s, 1),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.9.1

Re: [Qemu-devel] [PATCH 26/29] vmsvga: Add basic support for display topology

[Gerd Hoffmann]

  Hi,

> +    case SVGA_REG_DISPLAY_WIDTH:
> +        if ((s->display_id == 0) || (s->display_id == SVGA_ID_INVALID))
> +            ret = s->new_width ? s->new_width : surface_width(surface);
> +        else
> +            ret = 0;
> +        break;
> +    case SVGA_REG_DISPLAY_HEIGHT:
> +        if ((s->display_id == 0) || (s->display_id == SVGA_ID_INVALID))
> +            ret = s->new_height ? s->new_height : surface_height(surface);
> +        else
> +            ret = 0;
> +        break;

What is the purpose of these registers?  Hint for the guest about the
host display size?  If so you probably want wire up a callback for
GraphicHwOps->ui_info.  This will be called on display changes (i.e.
user resizes qemu gtk window).  See virtio_gpu_ui_info() for an example.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 26/29] vmsvga: Add basic support for display topology

[Liran Alon]

> On 10 Aug 2018, at 12:56, Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
>  Hi,
> 
>> +    case SVGA_REG_DISPLAY_WIDTH:
>> +        if ((s->display_id == 0) || (s->display_id == SVGA_ID_INVALID))
>> +            ret = s->new_width ? s->new_width : surface_width(surface);
>> +        else
>> +            ret = 0;
>> +        break;
>> +    case SVGA_REG_DISPLAY_HEIGHT:
>> +        if ((s->display_id == 0) || (s->display_id == SVGA_ID_INVALID))
>> +            ret = s->new_height ? s->new_height : surface_height(surface);
>> +        else
>> +            ret = 0;
>> +        break;
> 
> What is the purpose of these registers?  Hint for the guest about the
> host display size?  If so you probably want wire up a callback for
> GraphicHwOps->ui_info.  This will be called on display changes (i.e.
> user resizes qemu gtk window).  See virtio_gpu_ui_info() for an example.
> 
> cheers,
>  Gerd
> 

These registers are suppose to indicate to guest the display monitor size (width & height).

When the QEMU console framework timer will run gui_update()->dpy_refresh()->VNC dpy_refresh()->graphics_hw_update()->VMware-SVGA gfx_update()
Then vmsvga_update_display()->vmsvga_check_size() makes sure that host display surface is set to have width/height as specified in vmsvga_state_s->{new_width, new_height}.
Therefore, if new_width have been set, then host display will be changed to the value set by the guest.

Thus, wiring up GraphicsHwOps->ui_info callback to return new info on SVGA_REG_DISPLAY_{WIDTH, HEIGHT} registers may be useful only for case that we want guest to respond to the fact that the host display have been resized. However, I am not sure there is a mechanism to notify guest from vmware-svga device that this even has occurred for guest to reread these registers. Both in Linux vmware-svga driver code and VMware SVGA development kit, the SVGA_IRQFLAG_* flags don’t indicate such an interrupt source. In addition, it seems that Linux vmware-svga driver code only reads these registers at vmw_kms_save_vga() which weirdly enough, seems to be unreachable code (not called from anywhere…).
Therefore, I’m not sure it is important to do this change at this patch series.

But I am not 100% familiar with the entire QEMU graphics stack so maybe I’m missing something trivial here.

Regards,
-Liran

Re: [Qemu-devel] [PATCH 26/29] vmsvga: Add basic support for display topology

[Gerd Hoffmann]

  Hi,

> These registers are suppose to indicate to guest the display monitor
> size (width & height).

> Thus, wiring up GraphicsHwOps->ui_info callback to return new info on
> SVGA_REG_DISPLAY_{WIDTH, HEIGHT} registers may be useful only for case
> that we want guest to respond to the fact that the host display have
> been resized.

Yes, that is the purpose of the ui_info callback.  virtio-vga is the
easiest way to play with this:  Resize the gtk window, and gnome should
adapt the guest desktop to the new size, so you don't have a scaled
guest desktop.

> However, I am not sure there is a mechanism to notify guest from
> vmware-svga device that this even has occurred for guest to reread
> these registers.  Both in Linux vmware-svga driver code and VMware
> SVGA development kit, the SVGA_IRQFLAG_* flags don’t indicate such an
> interrupt source. In addition, it seems that Linux vmware-svga driver
> code only reads these registers at vmw_kms_save_vga() which weirdly
> enough, seems to be unreachable code (not called from anywhere…).

Hmm, that is strange ...

> Therefore, I’m not sure it is important to do this change at this
> patch series.

It isn't important for the series anyway.  Was just meant as hint where
the vmware-svga emulation could possibly improved even more, now that
you are at it.  It can certainly go either later as separate series or
not at all if it turns out it doesn't work anyway because the guests
drivers don't check the registers.

cheers,
  Gerd

[Qemu-devel] [PATCH 27/29] vmsvga: Add support for pitchlock register (a display line stride)

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

Indicate support of SVGA_REG_PITCHLOCK in device capabilities.
Value written by driver to SVGA_REG_PITCHLOCK is used to overwrite
the default line length derived from display width and bpp. Due to
lack of clear documentation, we resume using default formula for
line length when screen width is updated more than once without
update to SVGA_REG_PITCHLOCK register, which works for all tested
guests using vmsvga.

SVGA_REG_PITCHLOCK is required by Linux kernel vmsvga driver
(See Linux kernel vmw_driver_load()).

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 43 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index edd336e65005..bd7202833081 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -82,6 +82,8 @@ struct vmsvga_state_s {
     uint32_t irq_mask;
     uint32_t irq_status;
     uint32_t display_id;
+    uint32_t pitchlock;
+    int use_pitchlock;
 };
 
 #define TYPE_VMWARE_SVGA "vmware-svga"
@@ -1198,9 +1200,10 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         break;
 
     case SVGA_REG_BYTES_PER_LINE:
-        if (s->new_width) {
-            ret = (s->new_depth * s->new_width) / 8;
-        } else {
+        ret = (s->use_pitchlock >= 0) ?
+                s->pitchlock :
+                ((s->new_depth * s->new_width) / 8);
+        if (!ret) {
             ret = surface_stride(surface);
         }
         break;
@@ -1242,6 +1245,7 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         caps |= SVGA_CAP_EXTENDED_FIFO;
         caps |= SVGA_CAP_IRQMASK;
         caps |= SVGA_CAP_DISPLAY_TOPOLOGY;
+        caps |= SVGA_CAP_PITCHLOCK;
         ret = caps;
         break;
 
@@ -1294,11 +1298,14 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         break;
 
     case SVGA_REG_NUM_DISPLAYS:
-    case SVGA_REG_PITCHLOCK:
     case SVGA_PALETTE_BASE ... SVGA_PALETTE_END:
         ret = 0;
         break;
 
+    case SVGA_REG_PITCHLOCK:
+       ret = s->pitchlock;
+       break;
+
     case SVGA_REG_IRQMASK:
         ret = s->irq_mask;
         break;
@@ -1394,6 +1401,13 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
         if (value <= SVGA_MAX_WIDTH) {
             s->new_width = value;
             s->invalidated = 1;
+            /* This is a hack used to drop effective pitchlock setting
+             * when guest writes screen width without prior write to
+             * the pitchlock register.
+             */
+            if (s->use_pitchlock >= 0) {
+                s->use_pitchlock--;
+            }
         } else {
             printf("%s: Bad width: %i\n", __func__, value);
         }
@@ -1464,10 +1478,15 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
     case SVGA_REG_DEPTH:
     case SVGA_REG_MEM_REGS:
     case SVGA_REG_NUM_DISPLAYS:
-    case SVGA_REG_PITCHLOCK:
     case SVGA_PALETTE_BASE ... SVGA_PALETTE_END:
         break;
 
+    case SVGA_REG_PITCHLOCK:
+       s->pitchlock = value;
+       s->use_pitchlock = (value > 0) ? 1 : -1;
+       s->invalidated = 1;
+       break;
+
     case SVGA_REG_IRQMASK:
         s->irq_mask = value;
         break;
@@ -1540,16 +1559,20 @@ static void vmsvga_bios_write(void *opaque, uint32_t address, uint32_t data)
 static inline void vmsvga_check_size(struct vmsvga_state_s *s)
 {
     DisplaySurface *surface = qemu_console_surface(s->vga.con);
+    uint32_t new_stride;
 
+    new_stride = (s->use_pitchlock >= 0) ?
+        s->pitchlock :
+        ((s->new_depth * s->new_width) / 8);
     if (s->new_width != surface_width(surface) ||
         s->new_height != surface_height(surface) ||
+        (new_stride != surface_stride(surface)) ||
         s->new_depth != surface_bits_per_pixel(surface)) {
-        int stride = (s->new_depth * s->new_width) / 8;
         pixman_format_code_t format =
             qemu_default_pixman_format(s->new_depth, true);
         trace_vmware_setmode(s->new_width, s->new_height, s->new_depth);
         surface = qemu_create_displaysurface_from(s->new_width, s->new_height,
-                                                  format, stride,
+                                                  format, new_stride,
                                                   s->vga.vram_ptr);
         dpy_gfx_replace_surface(s->vga.con, surface);
         s->invalidated = 1;
@@ -1598,6 +1621,8 @@ static void vmsvga_reset(DeviceState *dev)
     s->irq_status = 0;
     s->last_fifo_cursor_count = 0;
     s->display_id = SVGA_ID_INVALID;
+    s->pitchlock = 0;
+    s->use_pitchlock = -1;
 
     vga_dirty_log_start(&s->vga);
 }
@@ -1633,6 +1658,8 @@ static int vmsvga_post_load(void *opaque, int version_id)
         s->irq_status = 0;
         s->last_fifo_cursor_count = 0;
         s->display_id = SVGA_ID_INVALID;
+        s->pitchlock = 0;
+        s->use_pitchlock = -1;
     }
 
     return 0;
@@ -1664,6 +1691,8 @@ static const VMStateDescription vmstate_vmware_vga_internal = {
         VMSTATE_UINT32_V(irq_status, struct vmsvga_state_s, 1),
         VMSTATE_UINT32_V(last_fifo_cursor_count, struct vmsvga_state_s, 1),
         VMSTATE_UINT32_V(display_id, struct vmsvga_state_s, 1),
+        VMSTATE_UINT32_V(pitchlock, struct vmsvga_state_s, 1),
+        VMSTATE_INT32_V(use_pitchlock, struct vmsvga_state_s, 1),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.9.1

[Qemu-devel] [PATCH 28/29] vmsvga: Introduce new function for reporting device capabilities

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

This in only code refactoring without change in semantics.

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 45 +++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index bd7202833081..389248b4badf 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -479,6 +479,30 @@ static void cursor_update_from_fifo(struct vmsvga_state_s *s)
 #endif
 }
 
+/* Report device capabilities */
+static uint32_t vmsvga_device_caps(struct vmsvga_state_s *s)
+{
+    uint32_t caps = SVGA_CAP_NONE;
+#ifdef HW_RECT_ACCEL
+    caps |= SVGA_CAP_RECT_COPY;
+#endif
+#ifdef HW_FILL_ACCEL
+    caps |= SVGA_CAP_RECT_FILL;
+#endif
+#ifdef HW_MOUSE_ACCEL
+    if (dpy_cursor_define_supported(s->vga.con)) {
+        caps |= SVGA_CAP_ALPHA_CURSOR;
+        caps |= SVGA_CAP_CURSOR | SVGA_CAP_CURSOR_BYPASS_2 |
+                SVGA_CAP_CURSOR_BYPASS;
+    }
+#endif
+    caps |= SVGA_CAP_IRQMASK;
+    caps |= SVGA_CAP_EXTENDED_FIFO;
+    caps |= SVGA_CAP_DISPLAY_TOPOLOGY;
+    caps |= SVGA_CAP_PITCHLOCK;
+    return caps;
+}
+
 static inline bool vmsvga_verify_rect(DisplaySurface *surface,
                                       const char *name,
                                       int x, int y, int w, int h)
@@ -1140,7 +1164,6 @@ static void vmsvga_index_write(void *opaque, uint32_t address, uint32_t index)
 
 static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
 {
-    uint32_t caps;
     struct vmsvga_state_s *s = opaque;
     DisplaySurface *surface = qemu_console_surface(s->vga.con);
     PixelFormat pf;
@@ -1228,25 +1251,7 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         break;
 
     case SVGA_REG_CAPABILITIES:
-        caps = SVGA_CAP_NONE;
-#ifdef HW_RECT_ACCEL
-        caps |= SVGA_CAP_RECT_COPY;
-#endif
-#ifdef HW_FILL_ACCEL
-        caps |= SVGA_CAP_RECT_FILL;
-#endif
-#ifdef HW_MOUSE_ACCEL
-        if (dpy_cursor_define_supported(s->vga.con)) {
-            caps |= SVGA_CAP_ALPHA_CURSOR;
-            caps |= SVGA_CAP_CURSOR | SVGA_CAP_CURSOR_BYPASS_2 |
-                    SVGA_CAP_CURSOR_BYPASS;
-        }
-#endif
-        caps |= SVGA_CAP_EXTENDED_FIFO;
-        caps |= SVGA_CAP_IRQMASK;
-        caps |= SVGA_CAP_DISPLAY_TOPOLOGY;
-        caps |= SVGA_CAP_PITCHLOCK;
-        ret = caps;
+        ret = vmsvga_device_caps(s);
         break;
 
     case SVGA_REG_MEM_START: {
-- 
1.9.1

Re: [Qemu-devel] [PATCH 28/29] vmsvga: Introduce new function for reporting device capabilities

[Gerd Hoffmann]

> +#ifdef HW_RECT_ACCEL
> +    caps |= SVGA_CAP_RECT_COPY;
> +#endif
> +#ifdef HW_FILL_ACCEL
> +    caps |= SVGA_CAP_RECT_FILL;
> +#endif

Same with these #ifdefs.  I think we should either make them runtime
options or just drop them.

cheers,
  Gerd

[Qemu-devel] [PATCH 29/29] vmsvga: Don't allow setting screen size with zero width or height

[Liran Alon]

From: Leonid Shatz <leonid.shatz@oracle.com>

Signed-off-by: Leonid Shatz <leonid.shatz@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 hw/display/vmware_vga.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 389248b4badf..1803a565fa07 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1566,6 +1566,9 @@ static inline void vmsvga_check_size(struct vmsvga_state_s *s)
     DisplaySurface *surface = qemu_console_surface(s->vga.con);
     uint32_t new_stride;
 
+    /* Don't allow setting uninitialized 0-size screen */
+    if ((s->new_width == 0) || (s->new_height == 0)) return;
+
     new_stride = (s->use_pitchlock >= 0) ?
         s->pitchlock :
         ((s->new_depth * s->new_width) / 8);
-- 
1.9.1

Re: [Qemu-devel] [PATCH 00/29]: vmsvga: Various fixes and enhancements

[Gerd Hoffmann]

On Thu, Aug 09, 2018 at 02:46:13PM +0300, Liran Alon wrote:
> Hi,
> 
> This patch series aim to fix many issues in vmware-svga emulation
> which have prevented it from being fully functional in a wide-variety
> of guests.

Nice job in modernizing the vmware svga emulation.

What guests this was tested with?  I've tried Fedora 28.  The vmware kms
driver loads fine, fbcon runs fine.  Nice.

But gdm doesn't start, neither with X11 nor with wayland.  Which is a
regression, because the xorg vmware driver manages to handle the current
qemu emulation just fine so gnome @ X11 works.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 00/29]: vmsvga: Various fixes and enhancements

[Liran Alon]

> On 10 Aug 2018, at 13:14, Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> On Thu, Aug 09, 2018 at 02:46:13PM +0300, Liran Alon wrote:
>> Hi,
>> 
>> This patch series aim to fix many issues in vmware-svga emulation
>> which have prevented it from being fully functional in a wide-variety
>> of guests.
> 
> Nice job in modernizing the vmware svga emulation.
> 
> What guests this was tested with?  I've tried Fedora 28.  The vmware kms
> driver loads fine, fbcon runs fine.  Nice.

This was tested with a wide-variety of various Windows versions & Linux distributions
which we have encountered that had issues with current vmware-svga emulation.
If it’s important, we can provide the exact test-matrix we have run against this code.

> 
> But gdm doesn't start, neither with X11 nor with wayland.  Which is a
> regression, because the xorg vmware driver manages to handle the current
> qemu emulation just fine so gnome @ X11 works.

Interesting. Thanks for finding this out. Will have a look.
Can you give more exact details about how you reproduced this issue?
I prefer to have a similar setup to make sure we have fixed the regression in this series.

> 
> cheers,
>  Gerd
> 

Thanks for the detailed review and feedback.
-Liran

Re: [Qemu-devel] [PATCH 00/29]: vmsvga: Various fixes and enhancements

[Gerd Hoffmann]

  Hi,

> > But gdm doesn't start, neither with X11 nor with wayland.  Which is a
> > regression, because the xorg vmware driver manages to handle the current
> > qemu emulation just fine so gnome @ X11 works.
> 
> Interesting. Thanks for finding this out. Will have a look.
> Can you give more exact details about how you reproduced this issue?

xorg is easiest, because Fedora 28 uses that by default for vmware svga:

  (1) download Fedora 28 workstation live iso.
  (2) boot in qemu with -vga vmware.

To try wayland you have to install fedora to disk (can be done using the
workstation live iso).

You can switch between xorg and wayland using /etc/gdm/custom.conf then,
by commenting/uncommenting the WaylandEnable line ...

   [daemon]
   # Uncoment the line below to force the login screen to use Xorg
   #WaylandEnable=false

... and rebooting.

> I prefer to have a similar setup to make sure we have fixed the
> regression in this series.

xorg regression needs fixing.

Having wayland working too would be nice, but wayland doesn't work with
the current emulation either.  So it isn't a regression and can be done
as separate patch series later on.

cheers,
  Gerd