Re: [Qemu-devel] Unknown command 0xffffff in SVGA command FIFO

[Janne Huttunen]

> I see no way to tell whether the guest is currently in the middle of
> writing a command.  So it seems the only way to check is to peek the
> first word in the fifo (which *is* written entirely before a NEXT_CMD
> update) and look up the expected command length, and then check
> whether enough data is in the FIFO.  Do you want to implement this?

It's not quite that simple. There are a bunch of command where
amount of arguments depends on other arguments.

Here's an experiment for sanity checking the lengths and leaving
the command in the FIFO if it is not complete. It fixes the
problem for me (running it right now), but I agree that it's not
very elegant to look at :-/ .


diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 12bff48..7730ae0 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -526,6 +526,17 @@ static inline int vmsvga_fifo_empty(struct vmsvga_state_s 
*s)
     return (s->cmd->next_cmd == s->cmd->stop);
 }

+static inline int vmsvga_fifo_items(struct vmsvga_state_s *s)
+{
+    int num;
+    if (!s->config || !s->enable)
+        return 0;
+    num = CMD(next_cmd) - CMD(stop);
+    if (num < 0)
+        num += (CMD(max) - CMD(min));
+    return (num >> 2);
+}
+
 static inline uint32_t vmsvga_fifo_read_raw(struct vmsvga_state_s *s)
 {
     uint32_t cmd = s->fifo[CMD(stop) >> 2];
@@ -540,6 +551,14 @@ static inline uint32_t vmsvga_fifo_read(struct 
vmsvga_state_s *s)
     return le32_to_cpu(vmsvga_fifo_read_raw(s));
 }

+static inline uint32_t vmsvga_fifo_peek(struct vmsvga_state_s *s, uint32_t 
offs)
+{
+    offs = (offs << 2) + CMD(stop);
+    if (offs >= CMD(max))
+        offs = offs - CMD(max) + CMD(min);
+    return le32_to_cpu(s->fifo[offs >> 2]);
+}
+
 static void vmsvga_fifo_run(struct vmsvga_state_s *s)
 {
     uint32_t cmd, colour;
@@ -547,9 +566,12 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
     int x, y, dx, dy, width, height;
     struct vmsvga_cursor_definition_s cursor;
     while (!vmsvga_fifo_empty(s))
-        switch (cmd = vmsvga_fifo_read(s)) {
+        switch (cmd = vmsvga_fifo_peek(s, 0)) {
         case SVGA_CMD_UPDATE:
         case SVGA_CMD_UPDATE_VERBOSE:
+            if (vmsvga_fifo_items(s) < 5)
+                break;
+            vmsvga_fifo_read(s);
             x = vmsvga_fifo_read(s);
             y = vmsvga_fifo_read(s);
             width = vmsvga_fifo_read(s);
@@ -558,6 +580,9 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             break;

         case SVGA_CMD_RECT_FILL:
+            if (vmsvga_fifo_items(s) < 6)
+                break;
+            vmsvga_fifo_read(s);
             colour = vmsvga_fifo_read(s);
             x = vmsvga_fifo_read(s);
             y = vmsvga_fifo_read(s);
@@ -571,6 +596,9 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
 #endif

         case SVGA_CMD_RECT_COPY:
+            if (vmsvga_fifo_items(s) < 7)
+                break;
+            vmsvga_fifo_read(s);
             x = vmsvga_fifo_read(s);
             y = vmsvga_fifo_read(s);
             dx = vmsvga_fifo_read(s);
@@ -585,13 +613,20 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
 #endif

         case SVGA_CMD_DEFINE_CURSOR:
-            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);
-            vmsvga_fifo_read(s);
-            cursor.bpp = vmsvga_fifo_read(s);
+            if (vmsvga_fifo_items(s) < 8)
+                break;
+            cursor.id = vmsvga_fifo_peek(s, 1);
+            cursor.hot_x = vmsvga_fifo_peek(s, 2);
+            cursor.hot_y = vmsvga_fifo_peek(s, 3);
+            cursor.width = x = vmsvga_fifo_peek(s, 4);
+            cursor.height = y = vmsvga_fifo_peek(s, 5);
+            cursor.bpp = vmsvga_fifo_peek(s, 7);
+
+            if (vmsvga_fifo_items(s) < SVGA_BITMAP_SIZE(x, y) + 
SVGA_PIXMAP_SIZE(x, y, cursor.bpp) + 8)
+                break;
+
+            for (args = 0; args < 8; args++)
+                vmsvga_fifo_read(s);

            if (SVGA_BITMAP_SIZE(x, y) > sizeof cursor.mask ||
                SVGA_PIXMAP_SIZE(x, y, cursor.bpp) > sizeof cursor.image) {
@@ -616,25 +651,43 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
          * for so we can avoid FIFO desync if driver uses them illegally.
          */
         case SVGA_CMD_DEFINE_ALPHA_CURSOR:
-            vmsvga_fifo_read(s);
-            vmsvga_fifo_read(s);
-            vmsvga_fifo_read(s);
-            x = vmsvga_fifo_read(s);
-            y = vmsvga_fifo_read(s);
+            if (vmsvga_fifo_items(s) < 6)
+                break;
+            x = vmsvga_fifo_peek(s, 4);
+            y = vmsvga_fifo_peek(s, 5);
+            if (vmsvga_fifo_items(s) < x * y + 6)
+                break;
+            for (args = 0; args < 6; args++)
+                vmsvga_fifo_read(s);
             args = x * y;
             goto badcmd;
         case SVGA_CMD_RECT_ROP_FILL:
+            if (vmsvga_fifo_items(s) < 7)
+                break;
+            vmsvga_fifo_read(s);
             args = 6;
             goto badcmd;
         case SVGA_CMD_RECT_ROP_COPY:
+            if (vmsvga_fifo_items(s) < 8)
+                break;
+            vmsvga_fifo_read(s);
             args = 7;
             goto badcmd;
         case SVGA_CMD_DRAW_GLYPH_CLIPPED:
+            if (vmsvga_fifo_items(s) < 4)
+                break;
+            args = 7 + (vmsvga_fifo_peek(s, 3) >> 2);
+            if (vmsvga_fifo_items(s) < args + 4)
+                break;
+            vmsvga_fifo_read(s);
+            vmsvga_fifo_read(s);
             vmsvga_fifo_read(s);
             vmsvga_fifo_read(s);
-            args = 7 + (vmsvga_fifo_read(s) >> 2);
             goto badcmd;
         case SVGA_CMD_SURFACE_ALPHA_BLEND:
+            if (vmsvga_fifo_items(s) < 13)
+                break;
+            vmsvga_fifo_read(s);
             args = 12;
             goto badcmd;