Re: [Qemu-devel] Unknown command 0xffffff in SVGA command FIFO
Hi, On 16 August 2010 22:26, Janne Huttunen jahut...@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
Re: [Qemu-devel] Unknown command 0xffffff in SVGA command FIFO
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
From: Andrzej Zaborowski balr...@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
Sorry, tried to use get-send-email but haven't tested first. On 23 July 2010 03:35, bal...@openstreetmap.pl wrote: From: Andrzej Zaborowski balr...@gmail.com ...
Re: [Qemu-devel] Unknown command 0xffffff in SVGA command FIFO
Hi, On 21 July 2010 13:17, Janne Huttunen jahut...@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
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
On 21 July 2010 14:14, Janne Huttunen jahut...@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
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:
Re: [Qemu-devel] Unknown command 0xffffff in SVGA command FIFO
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);
