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

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

[Janne Huttunen]

Hi!

I'm trying to run a Linux guest on top of QEMU (kvm). The only VGA
emulation that seems to give any kind of usable performance is the
vmware SVGA adapter, but that in turn is very unstable. It usually
freezes the guest display within a minute or two and starts printing
an error like "vmsvga_fifo_run: Unknown command 0xffffff in SVGA
command FIFO". A bit of googling tells that I'm not the only one
with this problem (1).

Now, correct me if I'm wrong, but isn't vmsvga_fifo_run() called
from an asynchronous context (wrt the guest)? If that indeed is
so, it may very well be, that it is run while the guest is
modifying the FIFO. This means, that a command may found in the
FIFO, but its arguments may not be there yet.

AFAICT the code just seems to check that the FIFO is not empty
before reading the command, but then assumes that the rest of
the command arguments are also there and reads the FIFO without
further checks. If it is possible that part of the command is
missing, this will desynchronize the FIFO. As there seems to be
no mechanism for re-syncing it, we're toast.

Also, if the FIFO gets full, the guest will force the FIFO to
be run. Now, AFAICT there is no guarantee on the guest side
that the last command in the FIFO won't be incomplete when this
happens. This will desynchronize the FIFO the same way and
can happen even if all the calls to vmsvga_fifo_run() are
synchronized.

It seems to me that the FIFO handling probably needs some way to
"peek" into the FIFO to see if the command in there is complete
and delay executing it if parts of it are still missing. That
should work in all cases except if a single command can be so big
that it won't fit in the FIFO. The other alternative I can think
of is to implement the vmsvga_fifo_run() as a state machine that
can read a partial command from the FIFO and continue it on the
next call.

Finally, if it indeed is true that the FIFO can be run both
asynchronously and forced by the guest, shouldn't the access to
the FIFO also be protected between these two somehow? At least I
couldn't find any such mechanism, but I must admit that I may
have easily just missed it.


(1): https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/595427

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

[andrzej zaborowski]

Hi,

On 21 July 2010 13:17, Janne Huttunen <jahuttun@gmail.com> wrote:
> Now, correct me if I'm wrong, but isn't vmsvga_fifo_run() called
> from an asynchronous context (wrt the guest)? If that indeed is
> so, it may very well be, that it is run while the guest is
> modifying the FIFO. This means, that a command may found in the
> FIFO, but its arguments may not be there yet.

No, I think that can't happen, but it would be interesting to bisect
what the guest is doing exactly when this happens.  The guest should
not move the "next command" pointer before if has written the command
entirely, this should be enough to guard against executing a partial
command.  Unless there's another timing issue somewhere obviously.

Can you check whether this also happens without kvm?

>
> AFAICT the code just seems to check that the FIFO is not empty
> before reading the command, but then assumes that the rest of
> the command arguments are also there and reads the FIFO without
> further checks. If it is possible that part of the command is
> missing, this will desynchronize the FIFO. As there seems to be
> no mechanism for re-syncing it, we're toast.
>
> Also, if the FIFO gets full, the guest will force the FIFO to
> be run. Now, AFAICT there is no guarantee on the guest side
> that the last command in the FIFO won't be incomplete when this
> happens. This will desynchronize the FIFO the same way and
> can happen even if all the calls to vmsvga_fifo_run() are
> synchronized.

Hmm, I don't know about the guest.. would be good to check too, but it
should be only fixable in the guest if it is so.

I'm not sure if it's likely that the FIFO is really getting full?
Most guest software will not write too many commands without knowing
that the previous content has appeared on the screen, right?

>
> It seems to me that the FIFO handling probably needs some way to
> "peek" into the FIFO to see if the command in there is complete
> and delay executing it if parts of it are still missing. That
> should work in all cases except if a single command can be so big
> that it won't fit in the FIFO. The other alternative I can think
> of is to implement the vmsvga_fifo_run() as a state machine that
> can read a partial command from the FIFO and continue it on the
> next call.
>
> Finally, if it indeed is true that the FIFO can be run both
> asynchronously and forced by the guest, shouldn't the access to
> the FIFO also be protected between these two somehow? At least I
> couldn't find any such mechanism, but I must admit that I may
> have easily just missed it.

The FIFO can run at any moment but everything else stops until all the
commands currently in the FIFO have been executed.  vmware_fifo_run is
called by the UI update which in turn is called from the main select()
loop.  Guest code is also executed in that loop.

Cheers

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

[Janne Huttunen]

> No, I think that can't happen, but it would be interesting to bisect
> what the guest is doing exactly when this happens.  The guest should
> not move the "next command" pointer before if has written the command
> entirely, this should be enough to guard against executing a partial
> command.  Unless there's another timing issue somewhere obviously.

Well, the guest driver is essentially the one in X.Org git tree here:

http://cgit.freedesktop.org/xorg/driver/xf86-video-vmware/tree/src

Looking at e.g. vmwareSendSVGACmdUpdate and vmwareWriteWordToFIFO
in vmware.c, the command seems to be inserted into the FIFO one
value at a time. Now, is the whole sequence somehow atomic wrt the
host FIFO access or not?

> Hmm, I don't know about the guest.. would be good to check too, but it
> should be only fixable in the guest if it is so.

Is the FIFO protocol documented somewhere? Like what kind of
atomicity is expected?

> I'm not sure if it's likely that the FIFO is really getting full?
> Most guest software will not write too many commands without knowing
> that the previous content has appeared on the screen, right?

Yes, I agree. That's more of a theoretical issue.

> The FIFO can run at any moment but everything else stops until all the
> commands currently in the FIFO have been executed.  vmware_fifo_run is
> called by the UI update which in turn is called from the main select()
> loop.  Guest code is also executed in that loop.

"At any time" as in between guest calls to vmwareWriteWordToFIFO?
Or not? It seems to me that the GUI is updated from a timer, but
can it go off at any time?

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

[andrzej zaborowski]

On 21 July 2010 14:14, Janne Huttunen <jahuttun@gmail.com> wrote:
>> No, I think that can't happen, but it would be interesting to bisect
>> what the guest is doing exactly when this happens.  The guest should
>> not move the "next command" pointer before if has written the command
>> entirely, this should be enough to guard against executing a partial
>> command.  Unless there's another timing issue somewhere obviously.
>
> Well, the guest driver is essentially the one in X.Org git tree here:
>
> http://cgit.freedesktop.org/xorg/driver/xf86-video-vmware/tree/src
>
> Looking at e.g. vmwareSendSVGACmdUpdate and vmwareWriteWordToFIFO
> in vmware.c, the command seems to be inserted into the FIFO one
> value at a time. Now, is the whole sequence somehow atomic wrt the
> host FIFO access or not?

Hmm, okay, I had expected the driver to not update NEXT_CMD until it's
done with writing the current command, but it does update after every
word.  I'd say this is wrong, but we probably need to handle it in
qemu.

The sequence is not atomic, although with TCG it may turn out to be
atomic depending on how the code was compiled and where the page
boundary is... there are only RAM accesses so they shouldn't cause a
vmexit.  No idea about KVM.

>
>> Hmm, I don't know about the guest.. would be good to check too, but it
>> should be only fixable in the guest if it is so.
>
> Is the FIFO protocol documented somewhere? Like what kind of
> atomicity is expected?

Not that I know.

>
> "At any time" as in between guest calls to vmwareWriteWordToFIFO?
> Or not? It seems to me that the GUI is updated from a timer, but
> can it go off at any time?

The timers are also updated from the select loop.. but in theory yes,
it can go off in between the WriteWordToFIFO calls and we need to take
that into account.

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?

Cheers

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;

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

[Janne Huttunen]

> 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 :-/ .

And here's another version with couple of stupid bugs removed
(it obviously is not a good idea to busyloop if the command is
incomplete).

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 12bff48..839f715 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)
+                goto out;
+            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)
+                goto out;
+            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)
+                goto out;
+            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)
+                goto out;
+            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)
+                goto out;
+
+            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)
+                goto out;
+            x = vmsvga_fifo_peek(s, 4);
+            y = vmsvga_fifo_peek(s, 5);
+            if (vmsvga_fifo_items(s) < x * y + 6)
+                goto out;
+            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)
+                goto out;
+            vmsvga_fifo_read(s);
              args = 6;
              goto badcmd;
          case SVGA_CMD_RECT_ROP_COPY:
+            if (vmsvga_fifo_items(s) < 8)
+                goto out;
+            vmsvga_fifo_read(s);
              args = 7;
              goto badcmd;
          case SVGA_CMD_DRAW_GLYPH_CLIPPED:
+            if (vmsvga_fifo_items(s) < 4)
+                goto out;
+            args = 7 + (vmsvga_fifo_peek(s, 3) >> 2);
+            if (vmsvga_fifo_items(s) < args + 4)
+                goto out;
+            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)
+                goto out;
+            vmsvga_fifo_read(s);
              args = 12;
              goto badcmd;

@@ -647,6 +700,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
          case SVGA_CMD_FRONT_ROP_FILL:
          case SVGA_CMD_FENCE:
          case SVGA_CMD_INVALID_CMD:
+            vmsvga_fifo_read(s);
              break; /* Nop */

          default:
@@ -658,6 +712,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
              break;
          }

+out:
      s->syncing = 0;
  }

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

[balrog]

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.

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;
 }

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

[andrzej zaborowski]

Sorry, tried to use get-send-email but haven't tested first.

On 23 July 2010 03:35,  <balrog@openstreetmap.pl> wrote:
> From: Andrzej Zaborowski <balrogg@gmail.com>
...

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

[Janne Huttunen]

> 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.

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

[andrzej zaborowski]

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