Patchwork Another fix to VMware depth computation

login
register
mail settings
Submitter Tian, Kevin
Date June 28, 2010, 7:59 a.m.
Message ID <625BA99ED14B2D499DC4E29D8138F150308467AEE2@shsmsx502.ccr.corp.intel.com>
Download mbox | patch
Permalink /patch/57118/
State New
Headers show

Comments

Tian, Kevin - June 28, 2010, 7:59 a.m.
(patch made upon 0.12.4 release; tested on 0.12 branch; build test on master)

 console.h       |   13 -------------
 hw/vmware_vga.c |   10 ++--------
 qemu-common.h   |    1 -
 vl.c            |    8 --------
 4 files changed, 2 insertions(+), 30 deletions(-)


Thanks,
Kevin
Tian, Kevin - June 30, 2010, 12:47 a.m.
Looks no response here. Could anyone let me know anything else I need do to make
this patch ready for acceptance? I'd like to follow any guideline if there is.

I also CC Anthony here since he worked on a similar fix before. If any other stakeholder
I should CC, please let me know.

Thanks a lot,
Kevin

>From: Tian, Kevin
>Sent: Monday, June 28, 2010 3:59 PM
>
>(patch made upon 0.12.4 release; tested on 0.12 branch; build test on master)
>
> console.h       |   13 -------------
> hw/vmware_vga.c |   10 ++--------
> qemu-common.h   |    1 -
> vl.c            |    8 --------
> 4 files changed, 2 insertions(+), 30 deletions(-)
>
>=====
>    Another fix to VMware depth computation
>
>    VMware SVGA presents to the guest with the depth of the host surface it renders
>    to, and rejects to work if the two sides are mismatched. One problem is that
>    current VMware VGA may calculate a wrong host depth, and then memcpy from virtual
>    framebuffer to host surface may trigger segmentation fault. For example, when
>    launching Qemu in a VNC connection, VMware SVGA thinks depth as '32', however the
>    actual depth of VNC is '16'. The fault also happens when the host depth is not
>    32 bit.
>
>    4b5db3749c5fdba93e1ac0e8748c9a9a1064319f tempts to fix a similar issue, by
>    changing from hard-coded 24bit depth to instead query the surface allocator
>    (e.g. sdl). However it doesn't really work, because the point where query
>    is invoked is earlier than the point where sdl is initialized. At query time,
>    qemu uses a default surface allocator which, again, provides another hard-coded
>    depth value - 32bit. So it happens to make VMware SVGA working on some hosts,
>    but still fails in others.
>
>    To solve this issue, this commit introduces a postcall interface to display
>    surface, which is walked after surface allocators are actually initialized.
>    At that point it's then safe to query host depth and present to the guest.
>
>    Signed-off-by Kevin Tian <kevin.tian@intel.com>
>
>diff --git a/console.h b/console.h
>index dfc8ae4..05fbf17 100644
>--- a/console.h
>+++ b/console.h
>@@ -122,6 +122,12 @@ struct DisplayAllocator {
>     void (*free_displaysurface)(DisplaySurface *surface);
> };
>
>+struct DisplayPostCallback {
>+    void (*postcall) (void *);
>+    void *parm;
>+    struct DisplayPostCallback *next;
>+};
>+
> struct DisplayState {
>     struct DisplaySurface *surface;
>     void *opaque;
>@@ -129,6 +135,7 @@ struct DisplayState {
>
>     struct DisplayAllocator* allocator;
>     struct DisplayChangeListener* listeners;
>+    struct DisplayPostCallback* postcalls;
>
>     void (*mouse_set)(int x, int y, int on);
>     void (*cursor_define)(int width, int height, int bpp, int hot_x, int hot_y,
>@@ -185,6 +192,12 @@ static inline void register_displaychangelistener(DisplayState *ds,
>DisplayChang
>     ds->listeners = dcl;
> }
>
>+static inline void register_displaypostcallback(DisplayState *ds, DisplayPostCallback *dpc)
>+{
>+    dpc->next = ds->postcalls;
>+    ds->postcalls = dpc;
>+}
>+
> static inline void dpy_update(DisplayState *s, int x, int y, int w, int h)
> {
>     struct DisplayChangeListener *dcl = s->listeners;
>diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
>index 01bb85b..d73cca6 100644
>--- a/hw/vmware_vga.c
>+++ b/hw/vmware_vga.c
>@@ -927,8 +927,9 @@ static void vmsvga_update_display(void *opaque)
>     }
> }
>
>-static void vmsvga_reset(struct vmsvga_state_s *s)
>+static void vmsvga_reset(void *parm)
> {
>+    struct vmsvga_state_s *s = (struct vmsvga_state_s *)parm;
>     s->index = 0;
>     s->enable = 0;
>     s->config = 0;
>@@ -1133,6 +1134,8 @@ static const VMStateDescription vmstate_vmware_vga = {
>
> static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size)
> {
>+    DisplayPostCallback *dpc;
>+
>     s->scratch_size = SVGA_SCRATCH_SIZE;
>     s->scratch = qemu_malloc(s->scratch_size * 4);
>
>@@ -1160,7 +1163,10 @@ static void vmsvga_init(struct vmsvga_state_s *s, int
>vga_ram_size)
>
>     rom_add_vga(VGABIOS_FILENAME);
>
>-    vmsvga_reset(s);
>+    dpc = qemu_mallocz(sizeof(DisplayPostCallback));
>+    dpc->postcall = vmsvga_reset;
>+    dpc->parm = s;
>+    register_displaypostcallback(s->vga.ds, dpc);
> }
>
> static void pci_vmsvga_map_ioport(PCIDevice *pci_dev, int region_num,
>diff --git a/qemu-common.h b/qemu-common.h
>index a23afbc..19f107a 100644
>--- a/qemu-common.h
>+++ b/qemu-common.h
>@@ -198,6 +198,7 @@ typedef struct DisplayState DisplayState;
> typedef struct DisplayChangeListener DisplayChangeListener;
> typedef struct DisplaySurface DisplaySurface;
> typedef struct DisplayAllocator DisplayAllocator;
>+typedef struct DisplayPostCallback DisplayPostCallback;
> typedef struct PixelFormat PixelFormat;
> typedef struct TextConsole TextConsole;
> typedef TextConsole QEMUConsole;
>diff --git a/vl.c b/vl.c
>index 39182ea..9a3e9fd 100644
>--- a/vl.c
>+++ b/vl.c
>@@ -4863,6 +4863,7 @@ int main(int argc, char **argv, char **envp)
>     char boot_devices[33] = "cad"; /* default to HD->floppy->CD-ROM */
>     DisplayState *ds;
>     DisplayChangeListener *dcl;
>+    DisplayPostCallback *dpc;
>     int cyls, heads, secs, translation;
>     QemuOpts *hda_opts = NULL, *opts;
>     int optind;
>@@ -6053,6 +6053,13 @@ int main(int argc, char **argv, char **envp)
>     }
>     dpy_resize(ds);
>
>+    dpc = ds->postcalls;
>+    while (dpc != NULL) {
>+        if (dpc->postcall != NULL)
>+            dpc->postcall(dpc->parm);
>+        dpc = dpc->next;
>+    }
>+
>     dcl = ds->listeners;
>     while (dcl != NULL) {
>         if (dcl->dpy_refresh != NULL) {
>
>Thanks,
>Kevin

Patch

=====
    Another fix to VMware depth computation
    
    VMware SVGA presents to the guest with the depth of the host surface it renders
    to, and rejects to work if the two sides are mismatched. One problem is that
    current VMware VGA may calculate a wrong host depth, and then memcpy from virtual
    framebuffer to host surface may trigger segmentation fault. For example, when
    launching Qemu in a VNC connection, VMware SVGA thinks depth as '32', however the
    actual depth of VNC is '16'. The fault also happens when the host depth is not
    32 bit.
    
    4b5db3749c5fdba93e1ac0e8748c9a9a1064319f tempts to fix a similar issue, by
    changing from hard-coded 24bit depth to instead query the surface allocator
    (e.g. sdl). However it doesn't really work, because the point where query
    is invoked is earlier than the point where sdl is initialized. At query time,
    qemu uses a default surface allocator which, again, provides another hard-coded
    depth value - 32bit. So it happens to make VMware SVGA working on some hosts,
    but still fails in others.
    
    To solve this issue, this commit introduces a postcall interface to display
    surface, which is walked after surface allocators are actually initialized.
    At that point it's then safe to query host depth and present to the guest.
    
    Signed-off-by Kevin Tian <kevin.tian@intel.com>

diff --git a/console.h b/console.h
index dfc8ae4..05fbf17 100644
--- a/console.h
+++ b/console.h
@@ -122,6 +122,12 @@  struct DisplayAllocator {
     void (*free_displaysurface)(DisplaySurface *surface);
 };
 
+struct DisplayPostCallback {
+    void (*postcall) (void *);
+    void *parm;
+    struct DisplayPostCallback *next;
+};
+
 struct DisplayState {
     struct DisplaySurface *surface;
     void *opaque;
@@ -129,6 +135,7 @@  struct DisplayState {
 
     struct DisplayAllocator* allocator;
     struct DisplayChangeListener* listeners;
+    struct DisplayPostCallback* postcalls;
 
     void (*mouse_set)(int x, int y, int on);
     void (*cursor_define)(int width, int height, int bpp, int hot_x, int hot_y,
@@ -185,6 +192,12 @@  static inline void register_displaychangelistener(DisplayState *ds, DisplayChang
     ds->listeners = dcl;
 }
 
+static inline void register_displaypostcallback(DisplayState *ds, DisplayPostCallback *dpc)
+{
+    dpc->next = ds->postcalls;
+    ds->postcalls = dpc;
+}
+
 static inline void dpy_update(DisplayState *s, int x, int y, int w, int h)
 {
     struct DisplayChangeListener *dcl = s->listeners;
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 01bb85b..d73cca6 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -927,8 +927,9 @@  static void vmsvga_update_display(void *opaque)
     }
 }
 
-static void vmsvga_reset(struct vmsvga_state_s *s)
+static void vmsvga_reset(void *parm)
 {
+    struct vmsvga_state_s *s = (struct vmsvga_state_s *)parm;
     s->index = 0;
     s->enable = 0;
     s->config = 0;
@@ -1133,6 +1134,8 @@  static const VMStateDescription vmstate_vmware_vga = {
 
 static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size)
 {
+    DisplayPostCallback *dpc;
+
     s->scratch_size = SVGA_SCRATCH_SIZE;
     s->scratch = qemu_malloc(s->scratch_size * 4);
 
@@ -1160,7 +1163,10 @@  static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size)
 
     rom_add_vga(VGABIOS_FILENAME);
 
-    vmsvga_reset(s);
+    dpc = qemu_mallocz(sizeof(DisplayPostCallback));
+    dpc->postcall = vmsvga_reset;
+    dpc->parm = s;
+    register_displaypostcallback(s->vga.ds, dpc);
 }
 
 static void pci_vmsvga_map_ioport(PCIDevice *pci_dev, int region_num,
diff --git a/qemu-common.h b/qemu-common.h
index a23afbc..19f107a 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -198,6 +198,7 @@  typedef struct DisplayState DisplayState;
 typedef struct DisplayChangeListener DisplayChangeListener;
 typedef struct DisplaySurface DisplaySurface;
 typedef struct DisplayAllocator DisplayAllocator;
+typedef struct DisplayPostCallback DisplayPostCallback;
 typedef struct PixelFormat PixelFormat;
 typedef struct TextConsole TextConsole;
 typedef TextConsole QEMUConsole;
diff --git a/vl.c b/vl.c
index 39182ea..9a3e9fd 100644
--- a/vl.c
+++ b/vl.c
@@ -4863,6 +4863,7 @@  int main(int argc, char **argv, char **envp)
     char boot_devices[33] = "cad"; /* default to HD->floppy->CD-ROM */
     DisplayState *ds;
     DisplayChangeListener *dcl;
+    DisplayPostCallback *dpc;
     int cyls, heads, secs, translation;
     QemuOpts *hda_opts = NULL, *opts;
     int optind;
@@ -6053,6 +6053,13 @@  int main(int argc, char **argv, char **envp)
     }
     dpy_resize(ds);
 
+    dpc = ds->postcalls;
+    while (dpc != NULL) {
+        if (dpc->postcall != NULL)
+            dpc->postcall(dpc->parm);
+        dpc = dpc->next;
+    }
+
     dcl = ds->listeners;
     while (dcl != NULL) {
         if (dcl->dpy_refresh != NULL) {