diff mbox

Unknown command 0xffffff in SVGA command FIFO

Message ID 12798489222610-git-send-email-
State New
Headers show

Commit Message

balrog@openstreetmap.pl July 23, 2010, 1:35 a.m. UTC 
From: Andrzej Zaborowski <balrogg@gmail.com>

Hi Janne,
I came up with this version, it kind of reverses the logic of your
patch but reuses the _items function (renamed _length), please
see if it looks ok and possibly even works.

Comments

Janne Huttunen Aug. 16, 2010, 8:26 p.m. UTC | #1 
> I came up with this version, it kind of reverses the logic of your
> patch but reuses the _items function (renamed _length), please
> see if it looks ok and possibly even works.

[sorry about the delay, I was out of office for a while]

Yes, your version works (both on paper and in practice). I'm not
quite sure I like the way it breaches the apparent abstraction
of the FIFO handling routines (if you can call it that) or the
way it first gives FIFO slots back to the guest but then rewinds
them back. Not that either of those concerns necessarily matter
much.

BTW, now that I look at it, if either HW_FILL_ACCEL or HW_RECT_ACCEL
is not set 'badcmd' will be called, but args won't be set (as far
as I can see). Isn't that wrong? Although I think the bug was there
even before your changes.
andrzej zaborowski Sept. 10, 2010, 1:34 a.m. UTC | #2 
Hi,

On 16 August 2010 22:26, Janne Huttunen <jahuttun@gmail.com> wrote:
> Yes, your version works (both on paper and in practice). I'm not
> quite sure I like the way it breaches the apparent abstraction
> of the FIFO handling routines (if you can call it that) or the
> way it first gives FIFO slots back to the guest but then rewinds
> them back. Not that either of those concerns necessarily matter
> much.

Since the incomplete commands are expected to be very rare compared to
normal handling, I think the rewinding makes more sense logically.  We
also save some cycles this way.

>
> BTW, now that I look at it, if either HW_FILL_ACCEL or HW_RECT_ACCEL
> is not set 'badcmd' will be called, but args won't be set (as far
> as I can see). Isn't that wrong? Although I think the bug was there
> even before your changes.

Yeah, you're right.  I added the missing args = 0; for those two cases
and pushed the change.

Cheers
diff mbox

Patch

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 12bff48..464f8bc 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -519,11 +519,15 @@  static inline void vmsvga_cursor_define(struct vmsvga_state_s *s,
 
 #define CMD(f)	le32_to_cpu(s->cmd->f)
 
-static inline int vmsvga_fifo_empty(struct vmsvga_state_s *s)
+static inline int vmsvga_fifo_length(struct vmsvga_state_s *s)
 {
+    int num;
     if (!s->config || !s->enable)
-        return 1;
-    return (s->cmd->next_cmd == s->cmd->stop);
+        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)
@@ -543,13 +547,23 @@  static inline uint32_t vmsvga_fifo_read(struct vmsvga_state_s *s)
 static void vmsvga_fifo_run(struct vmsvga_state_s *s)
 {
     uint32_t cmd, colour;
-    int args = 0;
+    int args, len;
     int x, y, dx, dy, width, height;
     struct vmsvga_cursor_definition_s cursor;
-    while (!vmsvga_fifo_empty(s))
+    uint32_t cmd_start;
+
+    len = vmsvga_fifo_length(s);
+    while (len > 0) {
+        /* May need to go back to the start of the command if incomplete */
+        cmd_start = s->cmd->stop;
+
         switch (cmd = vmsvga_fifo_read(s)) {
         case SVGA_CMD_UPDATE:
         case SVGA_CMD_UPDATE_VERBOSE:
+            len -= 5;
+            if (len < 0)
+                goto rewind;
+
             x = vmsvga_fifo_read(s);
             y = vmsvga_fifo_read(s);
             width = vmsvga_fifo_read(s);
@@ -558,6 +572,10 @@  static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             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);
@@ -571,6 +589,10 @@  static void vmsvga_fifo_run(struct vmsvga_state_s *s)
 #endif
 
         case SVGA_CMD_RECT_COPY:
+            len -= 7;
+            if (len < 0)
+                goto rewind;
+
             x = vmsvga_fifo_read(s);
             y = vmsvga_fifo_read(s);
             dx = vmsvga_fifo_read(s);
@@ -585,6 +607,10 @@  static void vmsvga_fifo_run(struct vmsvga_state_s *s)
 #endif
 
         case SVGA_CMD_DEFINE_CURSOR:
+            len -= 8;
+            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);
@@ -593,11 +619,14 @@  static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             vmsvga_fifo_read(s);
             cursor.bpp = vmsvga_fifo_read(s);
 
+            args = SVGA_BITMAP_SIZE(x, y) + SVGA_PIXMAP_SIZE(x, y, cursor.bpp);
 	    if (SVGA_BITMAP_SIZE(x, y) > sizeof cursor.mask ||
-		SVGA_PIXMAP_SIZE(x, y, cursor.bpp) > sizeof cursor.image) {
-		    args = SVGA_BITMAP_SIZE(x, y) + SVGA_PIXMAP_SIZE(x, y, cursor.bpp);
+		SVGA_PIXMAP_SIZE(x, y, cursor.bpp) > sizeof cursor.image)
 		    goto badcmd;
-	    }
+
+            len -= args;
+            if (len < 0)
+                goto rewind;
 
             for (args = 0; args < SVGA_BITMAP_SIZE(x, y); args ++)
                 cursor.mask[args] = vmsvga_fifo_read_raw(s);
@@ -616,6 +645,10 @@  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:
+            len -= 6;
+            if (len < 0)
+                goto rewind;
+
             vmsvga_fifo_read(s);
             vmsvga_fifo_read(s);
             vmsvga_fifo_read(s);
@@ -630,6 +663,10 @@  static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             args = 7;
             goto badcmd;
         case SVGA_CMD_DRAW_GLYPH_CLIPPED:
+            len -= 4;
+            if (len < 0)
+                goto rewind;
+
             vmsvga_fifo_read(s);
             vmsvga_fifo_read(s);
             args = 7 + (vmsvga_fifo_read(s) >> 2);
@@ -650,13 +687,22 @@  static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             break; /* Nop */
 
         default:
+            args = 0;
         badcmd:
+            len -= args;
+            if (len < 0)
+                goto rewind;
             while (args --)
                 vmsvga_fifo_read(s);
             printf("%s: Unknown command 0x%02x in SVGA command FIFO\n",
                             __FUNCTION__, cmd);
             break;
+
+        rewind:
+            s->cmd->stop = cmd_start;
+            break;
         }
+    }
 
     s->syncing = 0;
 }