All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] ioemu/qemu vga: save and restore vram buffer
@ 2007-12-10 18:14 ` Ian Jackson
  0 siblings, 0 replies; 18+ messages in thread
From: Ian Jackson @ 2007-12-10 18:14 UTC (permalink / raw)
  To: xen-devel, qemu-devel

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 1027 bytes --]

The existing stdvga driver from xen-unstable tools/ioemu/hw/vga* does
not save the emulated VGA memory contents.  The symptoms include video
malfunction after restore, including black screen (which can often be
fixed by asking the guest to redraw) but also missing font setup etc.
The attached patch fixes this by saving the entire VGA memory buffer,
just like the Xen ioemu Cirrus emulator does.

I have reinterpreted the `is_vbe' byte, which is related to
CONFIG_BOCHS_VBE, as a general flags word.  This enables my code to
allow old images to be restored (albeit with loss of VGA memory), by
using another bit in that word to indicate whether the VGA memory dump
is present.

I'm sending this patch to both xen-devel and qemu-devel, as
examination of a recent qemu cvs checkout shows the same bug to be
present there.  I don't know if the patch below (which is against
recent xen-unstable) will apply cleanly to qemu, but the code
generally looks very similar.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.


[-- Attachment #2: fix stdvga save/restore --]
[-- Type: text/plain, Size: 3119 bytes --]

diff -r 38a45b7c6cb5 tools/ioemu/hw/vga.c
--- a/tools/ioemu/hw/vga.c	Mon Dec 10 11:37:13 2007 +0000
+++ b/tools/ioemu/hw/vga.c	Mon Dec 10 17:55:22 2007 +0000
@@ -1742,6 +1742,8 @@ static void vga_save(QEMUFile *f, void *
 static void vga_save(QEMUFile *f, void *opaque)
 {
     VGAState *s = opaque;
+    unsigned save_format_flags;
+    uint32_t vram_size;
 #ifdef CONFIG_BOCHS_VBE
     int i;
 #endif
@@ -1772,8 +1774,9 @@ static void vga_save(QEMUFile *f, void *
     qemu_put_buffer(f, s->palette, 768);
 
     qemu_put_be32s(f, &s->bank_offset);
+    save_format_flags = VGA_SAVE_FORMAT_FLAG_VRAM_DATA;
 #ifdef CONFIG_BOCHS_VBE
-    qemu_put_byte(f, 1);
+    qemu_put_byte(f, save_format_flags | VGA_SAVE_FORMAT_FLAG_BOCHS_VBE);
     qemu_put_be16s(f, &s->vbe_index);
     for(i = 0; i < VBE_DISPI_INDEX_NB; i++)
         qemu_put_be16s(f, &s->vbe_regs[i]);
@@ -1781,17 +1784,19 @@ static void vga_save(QEMUFile *f, void *
     qemu_put_be32s(f, &s->vbe_line_offset);
     qemu_put_be32s(f, &s->vbe_bank_mask);
 #else
-    qemu_put_byte(f, 0);
-#endif
+    qemu_put_byte(f, save_format_flags);
+#endif
+    vram_size = s->vram_size;
+    qemu_put_be32s(f, &vram_size); 
+    qemu_put_buffer(f, s->vram_ptr, s->vram_size); 
 }
 
 static int vga_load(QEMUFile *f, void *opaque, int version_id)
 {
     VGAState *s = opaque;
-    int is_vbe, ret;
-#ifdef CONFIG_BOCHS_VBE
-    int i;
-#endif
+    int ret;
+    unsigned int save_format_flags;
+    uint32_t vram_size;
 
     if (version_id > 2)
         return -EINVAL;
@@ -1825,9 +1830,9 @@ static int vga_load(QEMUFile *f, void *o
     qemu_get_buffer(f, s->palette, 768);
 
     qemu_get_be32s(f, &s->bank_offset);
-    is_vbe = qemu_get_byte(f);
+    save_format_flags = qemu_get_byte(f);
 #ifdef CONFIG_BOCHS_VBE
-    if (!is_vbe)
+    if (!(save_format_flags & VGA_SAVE_FORMAT_FLAG_BOCHS_VBE))
         return -EINVAL;
     qemu_get_be16s(f, &s->vbe_index);
     for(i = 0; i < VBE_DISPI_INDEX_NB; i++)
@@ -1836,9 +1841,16 @@ static int vga_load(QEMUFile *f, void *o
     qemu_get_be32s(f, &s->vbe_line_offset);
     qemu_get_be32s(f, &s->vbe_bank_mask);
 #else
-    if (is_vbe)
+    if (save_format_flags & VGA_SAVE_FORMAT_FLAG_BOCHS_VBE)
         return -EINVAL;
 #endif
+    if (save_format_flags & VGA_SAVE_FORMAT_FLAG_VRAM_DATA) {
+	/* people who restore old images may be lucky ... */
+	qemu_get_be32s(f, &vram_size);
+	if (vram_size != s->vram_size)
+	    return -EINVAL;
+	qemu_get_buffer(f, s->vram_ptr, s->vram_size); 
+    }
 
     /* force refresh */
     s->graphic_mode = -1;
diff -r 38a45b7c6cb5 tools/ioemu/hw/vga_int.h
--- a/tools/ioemu/hw/vga_int.h	Mon Dec 10 11:37:13 2007 +0000
+++ b/tools/ioemu/hw/vga_int.h	Mon Dec 10 17:37:05 2007 +0000
@@ -157,6 +157,9 @@ static inline int c6_to_8(int v)
     return (v << 2) | (b << 1) | b;
 }
 
+#define VGA_SAVE_FORMAT_FLAG_BOCHS_VBE  0x01
+#define VGA_SAVE_FORMAT_FLAG_VRAM_DATA  0x02
+
 void vga_common_init(VGAState *s, DisplayState *ds, uint8_t *vga_ram_base, 
                      unsigned long vga_ram_offset, int vga_ram_size);
 uint32_t vga_mem_readb(void *opaque, target_phys_addr_t addr);

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH] ioemu/qemu vga: save and restore vram buffer
@ 2007-12-10 18:14 ` Ian Jackson
  0 siblings, 0 replies; 18+ messages in thread
From: Ian Jackson @ 2007-12-10 18:14 UTC (permalink / raw)
  To: xen-devel, qemu-devel

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 1027 bytes --]

The existing stdvga driver from xen-unstable tools/ioemu/hw/vga* does
not save the emulated VGA memory contents.  The symptoms include video
malfunction after restore, including black screen (which can often be
fixed by asking the guest to redraw) but also missing font setup etc.
The attached patch fixes this by saving the entire VGA memory buffer,
just like the Xen ioemu Cirrus emulator does.

I have reinterpreted the `is_vbe' byte, which is related to
CONFIG_BOCHS_VBE, as a general flags word.  This enables my code to
allow old images to be restored (albeit with loss of VGA memory), by
using another bit in that word to indicate whether the VGA memory dump
is present.

I'm sending this patch to both xen-devel and qemu-devel, as
examination of a recent qemu cvs checkout shows the same bug to be
present there.  I don't know if the patch below (which is against
recent xen-unstable) will apply cleanly to qemu, but the code
generally looks very similar.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.


[-- Attachment #2: fix stdvga save/restore --]
[-- Type: text/plain, Size: 3119 bytes --]

diff -r 38a45b7c6cb5 tools/ioemu/hw/vga.c
--- a/tools/ioemu/hw/vga.c	Mon Dec 10 11:37:13 2007 +0000
+++ b/tools/ioemu/hw/vga.c	Mon Dec 10 17:55:22 2007 +0000
@@ -1742,6 +1742,8 @@ static void vga_save(QEMUFile *f, void *
 static void vga_save(QEMUFile *f, void *opaque)
 {
     VGAState *s = opaque;
+    unsigned save_format_flags;
+    uint32_t vram_size;
 #ifdef CONFIG_BOCHS_VBE
     int i;
 #endif
@@ -1772,8 +1774,9 @@ static void vga_save(QEMUFile *f, void *
     qemu_put_buffer(f, s->palette, 768);
 
     qemu_put_be32s(f, &s->bank_offset);
+    save_format_flags = VGA_SAVE_FORMAT_FLAG_VRAM_DATA;
 #ifdef CONFIG_BOCHS_VBE
-    qemu_put_byte(f, 1);
+    qemu_put_byte(f, save_format_flags | VGA_SAVE_FORMAT_FLAG_BOCHS_VBE);
     qemu_put_be16s(f, &s->vbe_index);
     for(i = 0; i < VBE_DISPI_INDEX_NB; i++)
         qemu_put_be16s(f, &s->vbe_regs[i]);
@@ -1781,17 +1784,19 @@ static void vga_save(QEMUFile *f, void *
     qemu_put_be32s(f, &s->vbe_line_offset);
     qemu_put_be32s(f, &s->vbe_bank_mask);
 #else
-    qemu_put_byte(f, 0);
-#endif
+    qemu_put_byte(f, save_format_flags);
+#endif
+    vram_size = s->vram_size;
+    qemu_put_be32s(f, &vram_size); 
+    qemu_put_buffer(f, s->vram_ptr, s->vram_size); 
 }
 
 static int vga_load(QEMUFile *f, void *opaque, int version_id)
 {
     VGAState *s = opaque;
-    int is_vbe, ret;
-#ifdef CONFIG_BOCHS_VBE
-    int i;
-#endif
+    int ret;
+    unsigned int save_format_flags;
+    uint32_t vram_size;
 
     if (version_id > 2)
         return -EINVAL;
@@ -1825,9 +1830,9 @@ static int vga_load(QEMUFile *f, void *o
     qemu_get_buffer(f, s->palette, 768);
 
     qemu_get_be32s(f, &s->bank_offset);
-    is_vbe = qemu_get_byte(f);
+    save_format_flags = qemu_get_byte(f);
 #ifdef CONFIG_BOCHS_VBE
-    if (!is_vbe)
+    if (!(save_format_flags & VGA_SAVE_FORMAT_FLAG_BOCHS_VBE))
         return -EINVAL;
     qemu_get_be16s(f, &s->vbe_index);
     for(i = 0; i < VBE_DISPI_INDEX_NB; i++)
@@ -1836,9 +1841,16 @@ static int vga_load(QEMUFile *f, void *o
     qemu_get_be32s(f, &s->vbe_line_offset);
     qemu_get_be32s(f, &s->vbe_bank_mask);
 #else
-    if (is_vbe)
+    if (save_format_flags & VGA_SAVE_FORMAT_FLAG_BOCHS_VBE)
         return -EINVAL;
 #endif
+    if (save_format_flags & VGA_SAVE_FORMAT_FLAG_VRAM_DATA) {
+	/* people who restore old images may be lucky ... */
+	qemu_get_be32s(f, &vram_size);
+	if (vram_size != s->vram_size)
+	    return -EINVAL;
+	qemu_get_buffer(f, s->vram_ptr, s->vram_size); 
+    }
 
     /* force refresh */
     s->graphic_mode = -1;
diff -r 38a45b7c6cb5 tools/ioemu/hw/vga_int.h
--- a/tools/ioemu/hw/vga_int.h	Mon Dec 10 11:37:13 2007 +0000
+++ b/tools/ioemu/hw/vga_int.h	Mon Dec 10 17:37:05 2007 +0000
@@ -157,6 +157,9 @@ static inline int c6_to_8(int v)
     return (v << 2) | (b << 1) | b;
 }
 
+#define VGA_SAVE_FORMAT_FLAG_BOCHS_VBE  0x01
+#define VGA_SAVE_FORMAT_FLAG_VRAM_DATA  0x02
+
 void vga_common_init(VGAState *s, DisplayState *ds, uint8_t *vga_ram_base, 
                      unsigned long vga_ram_offset, int vga_ram_size);
 uint32_t vga_mem_readb(void *opaque, target_phys_addr_t addr);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH] ioemu/qemu vga: save and restore vram buffer
  2007-12-10 18:14 ` Ian Jackson
@ 2007-12-10 19:03   ` andrzej zaborowski
  -1 siblings, 0 replies; 18+ messages in thread
From: andrzej zaborowski @ 2007-12-10 19:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: xen-devel

On 10/12/2007, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> The existing stdvga driver from xen-unstable tools/ioemu/hw/vga* does
> not save the emulated VGA memory contents.  The symptoms include video
> malfunction after restore, including black screen (which can often be
> fixed by asking the guest to redraw) but also missing font setup etc.
> The attached patch fixes this by saving the entire VGA memory buffer,
> just like the Xen ioemu Cirrus emulator does.

Sounds reasonable.

>
> I have reinterpreted the `is_vbe' byte, which is related to
> CONFIG_BOCHS_VBE, as a general flags word.  This enables my code to
> allow old images to be restored (albeit with loss of VGA memory), by
> using another bit in that word to indicate whether the VGA memory dump
> is present.

You can use the version_id parameter for that. Increase the value
passed to register_savevm and load the vram only if version_id >= 2.

Regards

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] ioemu/qemu vga: save and restore vram buffer
@ 2007-12-10 19:03   ` andrzej zaborowski
  0 siblings, 0 replies; 18+ messages in thread
From: andrzej zaborowski @ 2007-12-10 19:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: xen-devel

On 10/12/2007, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> The existing stdvga driver from xen-unstable tools/ioemu/hw/vga* does
> not save the emulated VGA memory contents.  The symptoms include video
> malfunction after restore, including black screen (which can often be
> fixed by asking the guest to redraw) but also missing font setup etc.
> The attached patch fixes this by saving the entire VGA memory buffer,
> just like the Xen ioemu Cirrus emulator does.

Sounds reasonable.

>
> I have reinterpreted the `is_vbe' byte, which is related to
> CONFIG_BOCHS_VBE, as a general flags word.  This enables my code to
> allow old images to be restored (albeit with loss of VGA memory), by
> using another bit in that word to indicate whether the VGA memory dump
> is present.

You can use the version_id parameter for that. Increase the value
passed to register_savevm and load the vram only if version_id >= 2.

Regards

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Qemu-devel] [PATCH] ioemu/qemu vga: save and restore vram buffer (revised)
  2007-12-10 19:03   ` andrzej zaborowski
@ 2007-12-12 11:44     ` Ian Jackson
  -1 siblings, 0 replies; 18+ messages in thread
From: Ian Jackson @ 2007-12-12 11:44 UTC (permalink / raw)
  To: qemu-devel; +Cc: xen-devel

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 1033 bytes --]

andrzej zaborowski writes ("Re: [Qemu-devel] [PATCH] ioemu/qemu vga: save and restore vram buffer"):
> On 10/12/2007, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > I have reinterpreted the `is_vbe' byte, which is related to
> > CONFIG_BOCHS_VBE, as a general flags word.  This enables my code to
> > allow old images to be restored (albeit with loss of VGA memory), by
> > using another bit in that word to indicate whether the VGA memory dump
> > is present.
> 
> You can use the version_id parameter for that. Increase the value
> passed to register_savevm and load the vram only if version_id >= 2.

Oh!  That's much better.  Thanks.  Below are two patches:

The first one (stdvga-save-vram-update.patch) is against current
xen-unstable tip (which now includes my previous version) and should
be applied there.

The second (stdvga-save-vram-take2.patch) is a fresh diff against the
same qemu as before and should be regarded as replacing my previous
submission.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.


[-- Attachment #2: vga save vram in better way using version_id --]
[-- Type: text/plain, Size: 5104 bytes --]

diff -r f2f7c92bf1c1 tools/ioemu/hw/vga.c
--- a/tools/ioemu/hw/vga.c	Wed Dec 12 11:08:21 2007 +0000
+++ b/tools/ioemu/hw/vga.c	Wed Dec 12 11:27:24 2007 +0000
@@ -1742,7 +1742,6 @@ static void vga_save(QEMUFile *f, void *
 static void vga_save(QEMUFile *f, void *opaque)
 {
     VGAState *s = opaque;
-    unsigned save_format_flags;
     uint32_t vram_size;
 #ifdef CONFIG_BOCHS_VBE
     int i;
@@ -1774,9 +1773,8 @@ static void vga_save(QEMUFile *f, void *
     qemu_put_buffer(f, s->palette, 768);
 
     qemu_put_be32s(f, &s->bank_offset);
-    save_format_flags = VGA_SAVE_FORMAT_FLAG_VRAM_DATA;
 #ifdef CONFIG_BOCHS_VBE
-    qemu_put_byte(f, save_format_flags | VGA_SAVE_FORMAT_FLAG_BOCHS_VBE);
+    qemu_put_byte(f, 1);
     qemu_put_be16s(f, &s->vbe_index);
     for(i = 0; i < VBE_DISPI_INDEX_NB; i++)
         qemu_put_be16s(f, &s->vbe_regs[i]);
@@ -1784,7 +1782,7 @@ static void vga_save(QEMUFile *f, void *
     qemu_put_be32s(f, &s->vbe_line_offset);
     qemu_put_be32s(f, &s->vbe_bank_mask);
 #else
-    qemu_put_byte(f, save_format_flags);
+    qemu_put_byte(f, 0);
 #endif
     vram_size = s->vram_size;
     qemu_put_be32s(f, &vram_size); 
@@ -1794,11 +1792,13 @@ static int vga_load(QEMUFile *f, void *o
 static int vga_load(QEMUFile *f, void *opaque, int version_id)
 {
     VGAState *s = opaque;
-    int ret;
-    unsigned int save_format_flags;
+    int is_vbe, ret;
     uint32_t vram_size;
-
-    if (version_id > 2)
+#ifdef CONFIG_BOCHS_VBE
+    int i;
+#endif
+
+    if (version_id > 3)
         return -EINVAL;
 
     if (s->pci_dev && version_id >= 2) {
@@ -1830,9 +1830,9 @@ static int vga_load(QEMUFile *f, void *o
     qemu_get_buffer(f, s->palette, 768);
 
     qemu_get_be32s(f, &s->bank_offset);
-    save_format_flags = qemu_get_byte(f);
+    is_vbe = qemu_get_byte(f);
 #ifdef CONFIG_BOCHS_VBE
-    if (!(save_format_flags & VGA_SAVE_FORMAT_FLAG_BOCHS_VBE))
+    if (!is_vbe)
         return -EINVAL;
     qemu_get_be16s(f, &s->vbe_index);
     for(i = 0; i < VBE_DISPI_INDEX_NB; i++)
@@ -1841,10 +1841,10 @@ static int vga_load(QEMUFile *f, void *o
     qemu_get_be32s(f, &s->vbe_line_offset);
     qemu_get_be32s(f, &s->vbe_bank_mask);
 #else
-    if (save_format_flags & VGA_SAVE_FORMAT_FLAG_BOCHS_VBE)
+    if (is_vbe)
         return -EINVAL;
 #endif
-    if (save_format_flags & VGA_SAVE_FORMAT_FLAG_VRAM_DATA) {
+    if (version_id >= 3) {
 	/* people who restore old images may be lucky ... */
 	qemu_get_be32s(f, &vram_size);
 	if (vram_size != s->vram_size)
@@ -2064,7 +2064,7 @@ static void vga_init(VGAState *s)
 {
     int vga_io_memory;
 
-    register_savevm("vga", 0, 2, vga_save, vga_load, s);
+    register_savevm("vga", 0, 3, vga_save, vga_load, s);
 
     register_ioport_write(0x3c0, 16, 1, vga_ioport_write, s);
 
diff -r f2f7c92bf1c1 tools/ioemu/hw/vga_int.h
--- a/tools/ioemu/hw/vga_int.h	Wed Dec 12 11:08:21 2007 +0000
+++ b/tools/ioemu/hw/vga_int.h	Wed Dec 12 11:27:54 2007 +0000
@@ -157,9 +157,6 @@ static inline int c6_to_8(int v)
     return (v << 2) | (b << 1) | b;
 }
 
-#define VGA_SAVE_FORMAT_FLAG_BOCHS_VBE  0x01
-#define VGA_SAVE_FORMAT_FLAG_VRAM_DATA  0x02
-
 void vga_common_init(VGAState *s, DisplayState *ds, uint8_t *vga_ram_base, 
                      unsigned long vga_ram_offset, int vga_ram_size);
 uint32_t vga_mem_readb(void *opaque, target_phys_addr_t addr);

h--99eSOG+WoR
Content-Type: text/plain
Content-Description: vga save vram (revised)
Content-Disposition: inline;
	filename="stdvga-save-vram-take2.patch"
Content-Transfer-Encoding: 7bit

diff -r 4054cd60895b tools/ioemu/hw/vga.c
--- a/tools/ioemu/hw/vga.c	Mon Dec 10 13:49:22 2007 +0000
+++ b/tools/ioemu/hw/vga.c	Wed Dec 12 11:27:24 2007 +0000
@@ -1742,6 +1742,7 @@ static void vga_save(QEMUFile *f, void *
 static void vga_save(QEMUFile *f, void *opaque)
 {
     VGAState *s = opaque;
+    uint32_t vram_size;
 #ifdef CONFIG_BOCHS_VBE
     int i;
 #endif
@@ -1783,17 +1784,21 @@ static void vga_save(QEMUFile *f, void *
 #else
     qemu_put_byte(f, 0);
 #endif
+    vram_size = s->vram_size;
+    qemu_put_be32s(f, &vram_size); 
+    qemu_put_buffer(f, s->vram_ptr, s->vram_size); 
 }
 
 static int vga_load(QEMUFile *f, void *opaque, int version_id)
 {
     VGAState *s = opaque;
     int is_vbe, ret;
+    uint32_t vram_size;
 #ifdef CONFIG_BOCHS_VBE
     int i;
 #endif
 
-    if (version_id > 2)
+    if (version_id > 3)
         return -EINVAL;
 
     if (s->pci_dev && version_id >= 2) {
@@ -1839,6 +1844,13 @@ static int vga_load(QEMUFile *f, void *o
     if (is_vbe)
         return -EINVAL;
 #endif
+    if (version_id >= 3) {
+	/* people who restore old images may be lucky ... */
+	qemu_get_be32s(f, &vram_size);
+	if (vram_size != s->vram_size)
+	    return -EINVAL;
+	qemu_get_buffer(f, s->vram_ptr, s->vram_size); 
+    }
 
     /* force refresh */
     s->graphic_mode = -1;
@@ -2052,7 +2064,7 @@ static void vga_init(VGAState *s)
 {
     int vga_io_memory;
 
-    register_savevm("vga", 0, 2, vga_save, vga_load, s);
+    register_savevm("vga", 0, 3, vga_save, vga_load, s);
 
     register_ioport_write(0x3c0, 16, 1, vga_ioport_write, s);
 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH] ioemu/qemu vga: save and restore vram buffer (revised)
@ 2007-12-12 11:44     ` Ian Jackson
  0 siblings, 0 replies; 18+ messages in thread
From: Ian Jackson @ 2007-12-12 11:44 UTC (permalink / raw)
  To: qemu-devel; +Cc: xen-devel

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 1033 bytes --]

andrzej zaborowski writes ("Re: [Qemu-devel] [PATCH] ioemu/qemu vga: save and restore vram buffer"):
> On 10/12/2007, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > I have reinterpreted the `is_vbe' byte, which is related to
> > CONFIG_BOCHS_VBE, as a general flags word.  This enables my code to
> > allow old images to be restored (albeit with loss of VGA memory), by
> > using another bit in that word to indicate whether the VGA memory dump
> > is present.
> 
> You can use the version_id parameter for that. Increase the value
> passed to register_savevm and load the vram only if version_id >= 2.

Oh!  That's much better.  Thanks.  Below are two patches:

The first one (stdvga-save-vram-update.patch) is against current
xen-unstable tip (which now includes my previous version) and should
be applied there.

The second (stdvga-save-vram-take2.patch) is a fresh diff against the
same qemu as before and should be regarded as replacing my previous
submission.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.


[-- Attachment #2: vga save vram in better way using version_id --]
[-- Type: text/plain, Size: 5104 bytes --]

diff -r f2f7c92bf1c1 tools/ioemu/hw/vga.c
--- a/tools/ioemu/hw/vga.c	Wed Dec 12 11:08:21 2007 +0000
+++ b/tools/ioemu/hw/vga.c	Wed Dec 12 11:27:24 2007 +0000
@@ -1742,7 +1742,6 @@ static void vga_save(QEMUFile *f, void *
 static void vga_save(QEMUFile *f, void *opaque)
 {
     VGAState *s = opaque;
-    unsigned save_format_flags;
     uint32_t vram_size;
 #ifdef CONFIG_BOCHS_VBE
     int i;
@@ -1774,9 +1773,8 @@ static void vga_save(QEMUFile *f, void *
     qemu_put_buffer(f, s->palette, 768);
 
     qemu_put_be32s(f, &s->bank_offset);
-    save_format_flags = VGA_SAVE_FORMAT_FLAG_VRAM_DATA;
 #ifdef CONFIG_BOCHS_VBE
-    qemu_put_byte(f, save_format_flags | VGA_SAVE_FORMAT_FLAG_BOCHS_VBE);
+    qemu_put_byte(f, 1);
     qemu_put_be16s(f, &s->vbe_index);
     for(i = 0; i < VBE_DISPI_INDEX_NB; i++)
         qemu_put_be16s(f, &s->vbe_regs[i]);
@@ -1784,7 +1782,7 @@ static void vga_save(QEMUFile *f, void *
     qemu_put_be32s(f, &s->vbe_line_offset);
     qemu_put_be32s(f, &s->vbe_bank_mask);
 #else
-    qemu_put_byte(f, save_format_flags);
+    qemu_put_byte(f, 0);
 #endif
     vram_size = s->vram_size;
     qemu_put_be32s(f, &vram_size); 
@@ -1794,11 +1792,13 @@ static int vga_load(QEMUFile *f, void *o
 static int vga_load(QEMUFile *f, void *opaque, int version_id)
 {
     VGAState *s = opaque;
-    int ret;
-    unsigned int save_format_flags;
+    int is_vbe, ret;
     uint32_t vram_size;
-
-    if (version_id > 2)
+#ifdef CONFIG_BOCHS_VBE
+    int i;
+#endif
+
+    if (version_id > 3)
         return -EINVAL;
 
     if (s->pci_dev && version_id >= 2) {
@@ -1830,9 +1830,9 @@ static int vga_load(QEMUFile *f, void *o
     qemu_get_buffer(f, s->palette, 768);
 
     qemu_get_be32s(f, &s->bank_offset);
-    save_format_flags = qemu_get_byte(f);
+    is_vbe = qemu_get_byte(f);
 #ifdef CONFIG_BOCHS_VBE
-    if (!(save_format_flags & VGA_SAVE_FORMAT_FLAG_BOCHS_VBE))
+    if (!is_vbe)
         return -EINVAL;
     qemu_get_be16s(f, &s->vbe_index);
     for(i = 0; i < VBE_DISPI_INDEX_NB; i++)
@@ -1841,10 +1841,10 @@ static int vga_load(QEMUFile *f, void *o
     qemu_get_be32s(f, &s->vbe_line_offset);
     qemu_get_be32s(f, &s->vbe_bank_mask);
 #else
-    if (save_format_flags & VGA_SAVE_FORMAT_FLAG_BOCHS_VBE)
+    if (is_vbe)
         return -EINVAL;
 #endif
-    if (save_format_flags & VGA_SAVE_FORMAT_FLAG_VRAM_DATA) {
+    if (version_id >= 3) {
 	/* people who restore old images may be lucky ... */
 	qemu_get_be32s(f, &vram_size);
 	if (vram_size != s->vram_size)
@@ -2064,7 +2064,7 @@ static void vga_init(VGAState *s)
 {
     int vga_io_memory;
 
-    register_savevm("vga", 0, 2, vga_save, vga_load, s);
+    register_savevm("vga", 0, 3, vga_save, vga_load, s);
 
     register_ioport_write(0x3c0, 16, 1, vga_ioport_write, s);
 
diff -r f2f7c92bf1c1 tools/ioemu/hw/vga_int.h
--- a/tools/ioemu/hw/vga_int.h	Wed Dec 12 11:08:21 2007 +0000
+++ b/tools/ioemu/hw/vga_int.h	Wed Dec 12 11:27:54 2007 +0000
@@ -157,9 +157,6 @@ static inline int c6_to_8(int v)
     return (v << 2) | (b << 1) | b;
 }
 
-#define VGA_SAVE_FORMAT_FLAG_BOCHS_VBE  0x01
-#define VGA_SAVE_FORMAT_FLAG_VRAM_DATA  0x02
-
 void vga_common_init(VGAState *s, DisplayState *ds, uint8_t *vga_ram_base, 
                      unsigned long vga_ram_offset, int vga_ram_size);
 uint32_t vga_mem_readb(void *opaque, target_phys_addr_t addr);

h--99eSOG+WoR
Content-Type: text/plain
Content-Description: vga save vram (revised)
Content-Disposition: inline;
	filename="stdvga-save-vram-take2.patch"
Content-Transfer-Encoding: 7bit

diff -r 4054cd60895b tools/ioemu/hw/vga.c
--- a/tools/ioemu/hw/vga.c	Mon Dec 10 13:49:22 2007 +0000
+++ b/tools/ioemu/hw/vga.c	Wed Dec 12 11:27:24 2007 +0000
@@ -1742,6 +1742,7 @@ static void vga_save(QEMUFile *f, void *
 static void vga_save(QEMUFile *f, void *opaque)
 {
     VGAState *s = opaque;
+    uint32_t vram_size;
 #ifdef CONFIG_BOCHS_VBE
     int i;
 #endif
@@ -1783,17 +1784,21 @@ static void vga_save(QEMUFile *f, void *
 #else
     qemu_put_byte(f, 0);
 #endif
+    vram_size = s->vram_size;
+    qemu_put_be32s(f, &vram_size); 
+    qemu_put_buffer(f, s->vram_ptr, s->vram_size); 
 }
 
 static int vga_load(QEMUFile *f, void *opaque, int version_id)
 {
     VGAState *s = opaque;
     int is_vbe, ret;
+    uint32_t vram_size;
 #ifdef CONFIG_BOCHS_VBE
     int i;
 #endif
 
-    if (version_id > 2)
+    if (version_id > 3)
         return -EINVAL;
 
     if (s->pci_dev && version_id >= 2) {
@@ -1839,6 +1844,13 @@ static int vga_load(QEMUFile *f, void *o
     if (is_vbe)
         return -EINVAL;
 #endif
+    if (version_id >= 3) {
+	/* people who restore old images may be lucky ... */
+	qemu_get_be32s(f, &vram_size);
+	if (vram_size != s->vram_size)
+	    return -EINVAL;
+	qemu_get_buffer(f, s->vram_ptr, s->vram_size); 
+    }
 
     /* force refresh */
     s->graphic_mode = -1;
@@ -2052,7 +2064,7 @@ static void vga_init(VGAState *s)
 {
     int vga_io_memory;
 
-    register_savevm("vga", 0, 2, vga_save, vga_load, s);
+    register_savevm("vga", 0, 3, vga_save, vga_load, s);
 
     register_ioport_write(0x3c0, 16, 1, vga_ioport_write, s);
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH] ioemu/qemu vga: save and restore vram buffer (revised)
  2007-12-12 11:44     ` Ian Jackson
@ 2007-12-16 11:35       ` andrzej zaborowski
  -1 siblings, 0 replies; 18+ messages in thread
From: andrzej zaborowski @ 2007-12-16 11:35 UTC (permalink / raw)
  To: qemu-devel; +Cc: xen-devel

Hi,

On 12/12/2007, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> The first one (stdvga-save-vram-update.patch) is against current
> xen-unstable tip (which now includes my previous version) and should
> be applied there.
>
> The second (stdvga-save-vram-take2.patch) is a fresh diff against the
> same qemu as before and should be regarded as replacing my previous
> submission.

On a second look there's something else I don't understand. The vram
window is in RAM in stdvga, it's inside phys_ram_base, and the entire
chunk pointed to by phys_ram_base is saved in vl.c. It's also
saved/loaded as one of the first things that are loaded so at the time
stdvga resumes, the vram should already have the correct contents.
Regards

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] ioemu/qemu vga: save and restore vram buffer (revised)
@ 2007-12-16 11:35       ` andrzej zaborowski
  0 siblings, 0 replies; 18+ messages in thread
From: andrzej zaborowski @ 2007-12-16 11:35 UTC (permalink / raw)
  To: qemu-devel; +Cc: xen-devel

Hi,

On 12/12/2007, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> The first one (stdvga-save-vram-update.patch) is against current
> xen-unstable tip (which now includes my previous version) and should
> be applied there.
>
> The second (stdvga-save-vram-take2.patch) is a fresh diff against the
> same qemu as before and should be regarded as replacing my previous
> submission.

On a second look there's something else I don't understand. The vram
window is in RAM in stdvga, it's inside phys_ram_base, and the entire
chunk pointed to by phys_ram_base is saved in vl.c. It's also
saved/loaded as one of the first things that are loaded so at the time
stdvga resumes, the vram should already have the correct contents.
Regards

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH] ioemu/qemu vga: save and restore vram buffer (revised)
  2007-12-16 11:35       ` andrzej zaborowski
  (?)
@ 2007-12-17 11:18       ` Ian Jackson
       [not found]         ` <m2n.s.1JFvsr-002RPK@chiark.greenend.org.uk>
  2007-12-17 16:22           ` Paul Brook
  -1 siblings, 2 replies; 18+ messages in thread
From: Ian Jackson @ 2007-12-17 11:18 UTC (permalink / raw)
  To: qemu-devel; +Cc: xen-devel

andrzej zaborowski writes ("Re: [Qemu-devel] [PATCH] ioemu/qemu vga: save and restore vram buffer (revised)"):
> On a second look there's something else I don't understand. The vram
> window is in RAM in stdvga, it's inside phys_ram_base, and the entire
> chunk pointed to by phys_ram_base is saved in vl.c. It's also
> saved/loaded as one of the first things that are loaded so at the time
> stdvga resumes, the vram should already have the correct contents.

(You seem to be talking about the qemu case, rather than the Xen case,
but the principles are the same:)

phys_ram_base is used by qemu to store the guest's ordinary RAM.  It
is accessed by the guest when the guest does memory operations on
physical memory addresses referring to emulated RAM.

However, the emulated stdvga adaptor is not emulated RAM in that
sense.  It registers itself via cpu_register_io_memory, arranging that
references to its memory range of [0xa0000,0xc0000) are diverted to
vga_mem_{read,write}{b,w,l}.  Those functions provide access to the
VGA memory RAM buffer, which on a real video card is a separate set of
RAM chips and in qemu is a separate buffer (s->vram_*).

The access arrangements do not provide direct access to the whole of
the (emulated) VGA RAM.  The VGA RAM is VGA_RAM_SIZE long, ie
generally 8Mby.  That obviously doesn't fit in the available 128Kby of
address space.  Instead, depending on the vga mode a banking
arrangement is used.  See vga_mem_readb, for example.

In any case, vl.c's saving arrangements do save the buffer in
phys_ram_base - but that isn't what the guest sees in the VGA memory
area.  The guest sees the vga memory-mapped IO registers (whose
meaning _is_ generally saved by vga.c), plus it can use the VGA memory
area and those control registers to access the whole of s->vram_ptr in
a bank-switched way.  And it is that whole VGA memory buffer which is
`displayed' to (eg) vlc clients.

The area of phys_ram_base corresponding to the VGA memory area, which
is indeed saved and restored, is not used by anything.  But that's
only 128kby and in any case qemu compresses it.

For guests which make modest use of the vga facilities not saving the
buffer is at most a bit annoying - you just have to ask the guest to
repaint its screen, which is often easy and depending on circumstances
may happen anyway.  But guests which make fuller use of vga (for a
typical example, consider a graphical OS installer or bootloader) that
workaround isn't effective.

Ian.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH] ioemu/qemu vga: save and restore vram buffer (revised)
  2007-12-17 11:18       ` [Qemu-devel] " Ian Jackson
@ 2007-12-17 16:22           ` Paul Brook
  2007-12-17 16:22           ` Paul Brook
  1 sibling, 0 replies; 18+ messages in thread
From: Paul Brook @ 2007-12-17 16:22 UTC (permalink / raw)
  To: qemu-devel; +Cc: xen-devel, Ian Jackson

> In any case, vl.c's saving arrangements do save the buffer in
> phys_ram_base - but that isn't what the guest sees in the VGA memory

It doesn't matter what the guest physical mappings (if any) are.

> area.  The guest sees the vga memory-mapped IO registers (whose
> meaning _is_ generally saved by vga.c), plus it can use the VGA memory
> area and those control registers to access the whole of s->vram_ptr in
> a bank-switched way.  And it is that whole VGA memory buffer which is
> `displayed' to (eg) vlc clients.

If you look closer, you'll find that s->vram_ptr actually points to an offset 
from phys_ram_base. So the VGA framebuffer is already saved by ram_save.

If the xen patches have changed this then you may need your patch. It has no 
business in mainstream qemu though.

Paul

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] ioemu/qemu vga: save and restore vram buffer (revised)
@ 2007-12-17 16:22           ` Paul Brook
  0 siblings, 0 replies; 18+ messages in thread
From: Paul Brook @ 2007-12-17 16:22 UTC (permalink / raw)
  To: qemu-devel; +Cc: xen-devel, Ian Jackson

> In any case, vl.c's saving arrangements do save the buffer in
> phys_ram_base - but that isn't what the guest sees in the VGA memory

It doesn't matter what the guest physical mappings (if any) are.

> area.  The guest sees the vga memory-mapped IO registers (whose
> meaning _is_ generally saved by vga.c), plus it can use the VGA memory
> area and those control registers to access the whole of s->vram_ptr in
> a bank-switched way.  And it is that whole VGA memory buffer which is
> `displayed' to (eg) vlc clients.

If you look closer, you'll find that s->vram_ptr actually points to an offset 
from phys_ram_base. So the VGA framebuffer is already saved by ram_save.

If the xen patches have changed this then you may need your patch. It has no 
business in mainstream qemu though.

Paul

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] xen / qemu convergence ?
  2007-12-17 16:22           ` Paul Brook
  (?)
@ 2007-12-17 18:25           ` Ian Jackson
  2007-12-17 18:52               ` andrzej zaborowski
  2007-12-17 18:55               ` Paul Brook
  -1 siblings, 2 replies; 18+ messages in thread
From: Ian Jackson @ 2007-12-17 18:25 UTC (permalink / raw)
  To: qemu-devel, xen-devel

Paul Brook writes ("Re: [Qemu-devel] [PATCH] ioemu/qemu vga: save and
> restore vram buffer (revised)"): If you look closer, you'll find
> that s->vram_ptr actually points to an offset from phys_ram_base. So
> the VGA framebuffer is already saved by ram_save.

Oh yes.  That's not the case in the Xen version.  I'll explain a bit
more below.

> If the xen patches have changed this then you may need your patch. It has no 
> business in mainstream qemu though.

Yes, you appear to be right.


The Xen patches are far too big.  (To be honest, it looks rather like
they've been done with rather less care to avoid merge disruption than
I would have liked.)

I think it would be worthwhile trying to make them smaller.  Would
there be any interest in trying to converge somewhat ?  At the moment
the two trees are pretty badly diverged and cross-porting fixes of any
kind is very hard (and leads to these kind of mistakes).

I certainly wouldn't suggest importing the Xen patches wholesale (or
even piecemeal) into qemu.  They're quite unsuitable.  But I think
that there are potential improvements to qemu's flexibility which
would be valuable for the kinds of uses found in Xen.


To explain why there is a need for any divergence at all:

Xen obviously uses only some parts of qemu.  Other parts of qemu's
functionality are supplanted by hardware features and/or Xen
facilities.  The main part that is applicable to the Xen situation is
the library of emulated hardware in hw/*.

It might be nice to try to make it easier to untangle these from the
core of qemu's emulation ?  That might benefit other virtualisation
techniques (or indeed other weird uses for qemu's excellent library of
hardware emulations).  I don't think a fully plug-and-play approach is
viable in the foreseeable future but there would be things that qemu
could do that would reduce divergence.

I think we should be able to arrange that than wholesale and intrusive
changes, the Xen tree would entirely replace, or in omit, whole files
- diverging or disconnecting at reasonably small interfaces - and
perhaps use a few different build options.  Many of the existing Xen
patches would have to be reworked (or could be dropped) but I think
that would be of benefit for the Xen tree.

If there's any interest in this from the qemu side I think this would
be a worthwhile approach to try to aim for.


On to the technicalities of this particular case:

Xen's qemu has had qemu_ram_alloc completely removed.  This is because
in the Xen architecture, the qemu instance is not responsible for
allocating physical guest memory and is not capable of even accessing
it other than via cpu_physical_memory_rw (whose implementation now
arranges for the necessary memory mappings etc.)

So in the Xen case device emulators ought not to use qemu_ram_alloc.
The only device used by the pc system emulator in qemu which does so
at the moment is vga, by virtue of special handling in pc_init1.  (The
BIOS is done that way too but I don't regard that as a device and in
any case it's trivial.)

In the Xen version, this special handling for the emulated VGA memory
has been removed.  Instead, vga_init calls qemu_malloc to get vram_ptr
(with a bit of extra code to get correct alignment).  Obviously that
doesn't produce memory which will be saved so Xen's vga has to do that
itself.  Xen's Cirrus emulation is the same, but _does_ save the video
memory.

I don't really understand why the vga is handled in this way in qemu
but then I'm not an expert on PC graphics hardware.  Is it necessary
or desirable for the VGA RAM to take up virtual address space in this
way, or is there some other reason why VGA RAM in the ordinary vga
driver is regarded as a special use of system RAM rather than as a
special kind of hardware device ?


Regards,
Ian.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] xen / qemu convergence ?
  2007-12-17 18:25           ` [Qemu-devel] xen / qemu convergence ? Ian Jackson
@ 2007-12-17 18:52               ` andrzej zaborowski
  2007-12-17 18:55               ` Paul Brook
  1 sibling, 0 replies; 18+ messages in thread
From: andrzej zaborowski @ 2007-12-17 18:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: xen-devel

On 17/12/2007, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Paul Brook writes ("Re: [Qemu-devel] [PATCH] ioemu/qemu vga: save and
> > restore vram buffer (revised)"): If you look closer, you'll find
> > that s->vram_ptr actually points to an offset from phys_ram_base. So
> > the VGA framebuffer is already saved by ram_save.
>
> Oh yes.  That's not the case in the Xen version.  I'll explain a bit
> more below.
>
> > If the xen patches have changed this then you may need your patch. It has no
> > business in mainstream qemu though.
>
> Yes, you appear to be right.
>
>
> The Xen patches are far too big.  (To be honest, it looks rather like
> they've been done with rather less care to avoid merge disruption than
> I would have liked.)
>
> I think it would be worthwhile trying to make them smaller.  Would
> there be any interest in trying to converge somewhat ?  At the moment
> the two trees are pretty badly diverged and cross-porting fixes of any
> kind is very hard (and leads to these kind of mistakes).
>
> I certainly wouldn't suggest importing the Xen patches wholesale (or
> even piecemeal) into qemu.  They're quite unsuitable.  But I think
> that there are potential improvements to qemu's flexibility which
> would be valuable for the kinds of uses found in Xen.
>
>
> To explain why there is a need for any divergence at all:
>
> Xen obviously uses only some parts of qemu.  Other parts of qemu's
> functionality are supplanted by hardware features and/or Xen
> facilities.  The main part that is applicable to the Xen situation is
> the library of emulated hardware in hw/*.
>
> It might be nice to try to make it easier to untangle these from the
> core of qemu's emulation ?  That might benefit other virtualisation
> techniques (or indeed other weird uses for qemu's excellent library of
> hardware emulations).  I don't think a fully plug-and-play approach is
> viable in the foreseeable future but there would be things that qemu
> could do that would reduce divergence.
>
> I think we should be able to arrange that than wholesale and intrusive
> changes, the Xen tree would entirely replace, or in omit, whole files
> - diverging or disconnecting at reasonably small interfaces - and
> perhaps use a few different build options.  Many of the existing Xen
> patches would have to be reworked (or could be dropped) but I think
> that would be of benefit for the Xen tree.
>
> If there's any interest in this from the qemu side I think this would
> be a worthwhile approach to try to aim for.
>
>
> On to the technicalities of this particular case:
>
> Xen's qemu has had qemu_ram_alloc completely removed.  This is because
> in the Xen architecture, the qemu instance is not responsible for
> allocating physical guest memory and is not capable of even accessing
> it other than via cpu_physical_memory_rw (whose implementation now
> arranges for the necessary memory mappings etc.)
>
> So in the Xen case device emulators ought not to use qemu_ram_alloc.
> The only device used by the pc system emulator in qemu which does so
> at the moment is vga, by virtue of special handling in pc_init1.  (The
> BIOS is done that way too but I don't regard that as a device and in
> any case it's trivial.)
>
> In the Xen version, this special handling for the emulated VGA memory
> has been removed.  Instead, vga_init calls qemu_malloc to get vram_ptr
> (with a bit of extra code to get correct alignment).  Obviously that
> doesn't produce memory which will be saved so Xen's vga has to do that
> itself.  Xen's Cirrus emulation is the same, but _does_ save the video
> memory.
>
> I don't really understand why the vga is handled in this way in qemu
> but then I'm not an expert on PC graphics hardware.  Is it necessary
> or desirable for the VGA RAM to take up virtual address space in this
> way, or is there some other reason why VGA RAM in the ordinary vga
> driver is regarded as a special use of system RAM rather than as a
> special kind of hardware device ?

There's currently no way in qemu to map a chunk of host memory to
guest memory 1:1 if it's not in phys_ram_base, so all video adapters
in qemu do that. Mapped memory that's part of phys_ram_base also gets
dirty pages tracking for free. A way to map any arbitrary host address
to guest RAM would be useful for cases like the vmware-svga where no
dirty tracking is needed if the SDL framebuffer is made directly
accessible to guest.

Cirrus and stdvga code in qemu appears to do exactly the same thing,
I'm not sure why there is a difference between the two in Xen.

Regards

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: xen / qemu convergence ?
@ 2007-12-17 18:52               ` andrzej zaborowski
  0 siblings, 0 replies; 18+ messages in thread
From: andrzej zaborowski @ 2007-12-17 18:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: xen-devel

On 17/12/2007, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Paul Brook writes ("Re: [Qemu-devel] [PATCH] ioemu/qemu vga: save and
> > restore vram buffer (revised)"): If you look closer, you'll find
> > that s->vram_ptr actually points to an offset from phys_ram_base. So
> > the VGA framebuffer is already saved by ram_save.
>
> Oh yes.  That's not the case in the Xen version.  I'll explain a bit
> more below.
>
> > If the xen patches have changed this then you may need your patch. It has no
> > business in mainstream qemu though.
>
> Yes, you appear to be right.
>
>
> The Xen patches are far too big.  (To be honest, it looks rather like
> they've been done with rather less care to avoid merge disruption than
> I would have liked.)
>
> I think it would be worthwhile trying to make them smaller.  Would
> there be any interest in trying to converge somewhat ?  At the moment
> the two trees are pretty badly diverged and cross-porting fixes of any
> kind is very hard (and leads to these kind of mistakes).
>
> I certainly wouldn't suggest importing the Xen patches wholesale (or
> even piecemeal) into qemu.  They're quite unsuitable.  But I think
> that there are potential improvements to qemu's flexibility which
> would be valuable for the kinds of uses found in Xen.
>
>
> To explain why there is a need for any divergence at all:
>
> Xen obviously uses only some parts of qemu.  Other parts of qemu's
> functionality are supplanted by hardware features and/or Xen
> facilities.  The main part that is applicable to the Xen situation is
> the library of emulated hardware in hw/*.
>
> It might be nice to try to make it easier to untangle these from the
> core of qemu's emulation ?  That might benefit other virtualisation
> techniques (or indeed other weird uses for qemu's excellent library of
> hardware emulations).  I don't think a fully plug-and-play approach is
> viable in the foreseeable future but there would be things that qemu
> could do that would reduce divergence.
>
> I think we should be able to arrange that than wholesale and intrusive
> changes, the Xen tree would entirely replace, or in omit, whole files
> - diverging or disconnecting at reasonably small interfaces - and
> perhaps use a few different build options.  Many of the existing Xen
> patches would have to be reworked (or could be dropped) but I think
> that would be of benefit for the Xen tree.
>
> If there's any interest in this from the qemu side I think this would
> be a worthwhile approach to try to aim for.
>
>
> On to the technicalities of this particular case:
>
> Xen's qemu has had qemu_ram_alloc completely removed.  This is because
> in the Xen architecture, the qemu instance is not responsible for
> allocating physical guest memory and is not capable of even accessing
> it other than via cpu_physical_memory_rw (whose implementation now
> arranges for the necessary memory mappings etc.)
>
> So in the Xen case device emulators ought not to use qemu_ram_alloc.
> The only device used by the pc system emulator in qemu which does so
> at the moment is vga, by virtue of special handling in pc_init1.  (The
> BIOS is done that way too but I don't regard that as a device and in
> any case it's trivial.)
>
> In the Xen version, this special handling for the emulated VGA memory
> has been removed.  Instead, vga_init calls qemu_malloc to get vram_ptr
> (with a bit of extra code to get correct alignment).  Obviously that
> doesn't produce memory which will be saved so Xen's vga has to do that
> itself.  Xen's Cirrus emulation is the same, but _does_ save the video
> memory.
>
> I don't really understand why the vga is handled in this way in qemu
> but then I'm not an expert on PC graphics hardware.  Is it necessary
> or desirable for the VGA RAM to take up virtual address space in this
> way, or is there some other reason why VGA RAM in the ordinary vga
> driver is regarded as a special use of system RAM rather than as a
> special kind of hardware device ?

There's currently no way in qemu to map a chunk of host memory to
guest memory 1:1 if it's not in phys_ram_base, so all video adapters
in qemu do that. Mapped memory that's part of phys_ram_base also gets
dirty pages tracking for free. A way to map any arbitrary host address
to guest RAM would be useful for cases like the vmware-svga where no
dirty tracking is needed if the SDL framebuffer is made directly
accessible to guest.

Cirrus and stdvga code in qemu appears to do exactly the same thing,
I'm not sure why there is a difference between the two in Xen.

Regards

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] xen / qemu convergence ?
  2007-12-17 18:25           ` [Qemu-devel] xen / qemu convergence ? Ian Jackson
@ 2007-12-17 18:55               ` Paul Brook
  2007-12-17 18:55               ` Paul Brook
  1 sibling, 0 replies; 18+ messages in thread
From: Paul Brook @ 2007-12-17 18:55 UTC (permalink / raw)
  To: qemu-devel; +Cc: xen-devel, Ian Jackson

> I don't really understand why the vga is handled in this way in qemu
> but then I'm not an expert on PC graphics hardware.  Is it necessary
> or desirable for the VGA RAM to take up virtual address space in this
> way, or is there some other reason why VGA RAM in the ordinary vga
> driver is regarded as a special use of system RAM rather than as a
> special kind of hardware device ?

RAM is RAM. We don't care whether it's nominally owned by the vga controller 
or the "system". If you don't do this then all accesses have to go via 
horribly slow IO callbacks, which is just silly.

I've no idea what you're talking about when you say it's "taking up virtual 
address space".

Paul

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: xen / qemu convergence ?
@ 2007-12-17 18:55               ` Paul Brook
  0 siblings, 0 replies; 18+ messages in thread
From: Paul Brook @ 2007-12-17 18:55 UTC (permalink / raw)
  To: qemu-devel; +Cc: xen-devel, Ian Jackson

> I don't really understand why the vga is handled in this way in qemu
> but then I'm not an expert on PC graphics hardware.  Is it necessary
> or desirable for the VGA RAM to take up virtual address space in this
> way, or is there some other reason why VGA RAM in the ordinary vga
> driver is regarded as a special use of system RAM rather than as a
> special kind of hardware device ?

RAM is RAM. We don't care whether it's nominally owned by the vga controller 
or the "system". If you don't do this then all accesses have to go via 
horribly slow IO callbacks, which is just silly.

I've no idea what you're talking about when you say it's "taking up virtual 
address space".

Paul

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] xen / qemu convergence ?
  2007-12-17 18:52               ` andrzej zaborowski
  (?)
@ 2007-12-18 10:41               ` Ian Jackson
  -1 siblings, 0 replies; 18+ messages in thread
From: Ian Jackson @ 2007-12-18 10:41 UTC (permalink / raw)
  To: qemu-devel; +Cc: xen-devel

andrzej zaborowski writes ("Re: [Qemu-devel] xen / qemu convergence ?"):
> There's currently no way in qemu to map a chunk of host memory to
> guest memory 1:1 if it's not in phys_ram_base, so all video adapters
> in qemu do that. Mapped memory that's part of phys_ram_base also gets
> dirty pages tracking for free. A way to map any arbitrary host address
> to guest RAM would be useful for cases like the vmware-svga where no
> dirty tracking is needed if the SDL framebuffer is made directly
> accessible to guest.

Right.  The Xen versions of the vga emulators don't map the vga ram
into the guest's address space wholesale.  Accesses to the vga ram all
use the I/O callbacks through the traditional vga memory window.  To
improve the performance there's (in as-yet-unreleased versions of Xen)
a Xen-specific shadow memory arrangement which tracks the guest's
writes.  (I haven't looked into that in detail.)

(And of course in the Xen case, there is no host memory corresponding
to ordinary guest ram.)

> Cirrus and stdvga code in qemu appears to do exactly the same thing,
> I'm not sure why there is a difference between the two in Xen.

It's just a mistake: the Cirrus emulator had been changed to save its
private memory, but the corresponding required change to stdvga had
been overlooked.

Paul Brook writes ("Re: [Qemu-devel] xen / qemu convergence ?"):
> RAM is RAM. We don't care whether it's nominally owned by the vga controller 
> or the "system". If you don't do this then all accesses have to go via 
> horribly slow IO callbacks, which is just silly.

I see, yes.

> I've no idea what you're talking about when you say it's "taking up virtual 
> address space".

I meant that it allocates part of the space indexed by ram_addr_t but
on rereading the code I think that's not visible to the guest and is
just used for qemu's own bookkeeping so I was mistaken.

Thanks,
Ian.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Re: [Qemu-devel] [PATCH] ioemu/qemu vga: save and restore vram buffer (revised)
       [not found]     ` <m2n.s.1JFvre-002RW5@chiark.greenend.org.uk>
@ 2008-01-21 11:41       ` Ian Jackson
  0 siblings, 0 replies; 18+ messages in thread
From: Ian Jackson @ 2008-01-21 11:41 UTC (permalink / raw)
  To: xen-devel

andrzej zaborowski writes:
> ...
Paul Brook writes:
> ...
andrzej zaborowski writes:
> ...
Paul Brook writes:
> ...

These messages seem to have been stuck in the xen-devel moderation
queue since before Christmas.  The thread carried on on qemu-devel, so
I don't think we need to continue it again here.

Ian.

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2008-01-21 11:41 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-12-10 18:14 [Qemu-devel] [PATCH] ioemu/qemu vga: save and restore vram buffer Ian Jackson
2007-12-10 18:14 ` Ian Jackson
2007-12-10 19:03 ` [Qemu-devel] " andrzej zaborowski
2007-12-10 19:03   ` andrzej zaborowski
2007-12-12 11:44   ` [Qemu-devel] [PATCH] ioemu/qemu vga: save and restore vram buffer (revised) Ian Jackson
2007-12-12 11:44     ` Ian Jackson
2007-12-16 11:35     ` [Qemu-devel] " andrzej zaborowski
2007-12-16 11:35       ` andrzej zaborowski
2007-12-17 11:18       ` [Qemu-devel] " Ian Jackson
     [not found]         ` <m2n.s.1JFvsr-002RPK@chiark.greenend.org.uk>
2007-12-17 16:22         ` Paul Brook
2007-12-17 16:22           ` Paul Brook
2007-12-17 18:25           ` [Qemu-devel] xen / qemu convergence ? Ian Jackson
2007-12-17 18:52             ` andrzej zaborowski
2007-12-17 18:52               ` andrzej zaborowski
2007-12-18 10:41               ` [Qemu-devel] " Ian Jackson
2007-12-17 18:55             ` Paul Brook
2007-12-17 18:55               ` Paul Brook
     [not found]     ` <m2n.s.1JFvre-002RW5@chiark.greenend.org.uk>
2008-01-21 11:41       ` Re: [Qemu-devel] [PATCH] ioemu/qemu vga: save and restore vram buffer (revised) Ian Jackson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.