[RFC][PATCH] qemu-kvm: Drop vga dirty logging workarounds

[RFC][PATCH] qemu-kvm: Drop vga dirty logging workarounds

[Jan Kiszka]

These diffs to upstream should all date back to the days qemu-kvm
supported vga dirty logging with restricted/broken kvm kernel modules.
We no longer do, so there is no need for those workarounds. Even worse
they can trigger internal bug checks these days:

BUG: kvm_dirty_pages_log_change: invalid parameters 00000000000a8000-00000000000affff

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/cirrus_vga.c |   16 ----------------
 hw/vga-pci.c    |    2 --
 hw/vga.c        |   44 ++++++++------------------------------------
 3 files changed, 8 insertions(+), 54 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index a580b57..35b8b0e 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -32,7 +32,6 @@
 #include "console.h"
 #include "vga_int.h"
 #include "kvm.h"
-#include "qemu-kvm.h"
 #include "loader.h"
 
 /*
@@ -2553,7 +2552,6 @@ static CPUWriteMemoryFunc * const cirrus_linear_bitblt_write[3] = {
 
 static void map_linear_vram(CirrusVGAState *s)
 {
-    vga_dirty_log_stop(&s->vga);
     if (!s->vga.map_addr && s->vga.lfb_addr && s->vga.lfb_end) {
         s->vga.map_addr = s->vga.lfb_addr;
         s->vga.map_end = s->vga.lfb_end;
@@ -2566,16 +2564,11 @@ static void map_linear_vram(CirrusVGAState *s)
 #ifndef TARGET_IA64
     s->vga.lfb_vram_mapped = 0;
 
-    cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x8000,
-                                (s->vga.vram_offset + s->cirrus_bank_base[0]) | IO_MEM_UNASSIGNED);
-    cpu_register_physical_memory(isa_mem_base + 0xa8000, 0x8000,
-                                (s->vga.vram_offset + s->cirrus_bank_base[1]) | IO_MEM_UNASSIGNED);
     if (!(s->cirrus_srcptr != s->cirrus_srcptr_end)
         && !((s->vga.sr[0x07] & 0x01) == 0)
         && !((s->vga.gr[0x0B] & 0x14) == 0x14)
         && !(s->vga.gr[0x0B] & 0x02)) {
 
-        vga_dirty_log_stop(&s->vga);
         cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x8000,
                                     (s->vga.vram_offset + s->cirrus_bank_base[0]) | IO_MEM_RAM);
         cpu_register_physical_memory(isa_mem_base + 0xa8000, 0x8000,
@@ -2594,7 +2587,6 @@ static void map_linear_vram(CirrusVGAState *s)
 
 static void unmap_linear_vram(CirrusVGAState *s)
 {
-    vga_dirty_log_stop(&s->vga);
     if (s->vga.map_addr && s->vga.lfb_addr && s->vga.lfb_end) {
         s->vga.map_addr = s->vga.map_end = 0;
          cpu_register_physical_memory(s->vga.lfb_addr, s->vga.vram_size,
@@ -2602,8 +2594,6 @@ static void unmap_linear_vram(CirrusVGAState *s)
     }
     cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x20000,
                                  s->vga.vga_io_memory);
-
-    vga_dirty_log_start(&s->vga);
 }
 
 /* Compute the memory access functions */
@@ -3156,8 +3146,6 @@ static void cirrus_pci_lfb_map(PCIDevice *d, int region_num,
 {
     CirrusVGAState *s = &DO_UPCAST(PCICirrusVGAState, dev, d)->cirrus_vga;
 
-    vga_dirty_log_stop(&s->vga);
-
     /* XXX: add byte swapping apertures */
     cpu_register_physical_memory(addr, s->vga.vram_size,
 				 s->cirrus_linear_io_addr);
@@ -3189,14 +3177,10 @@ static void pci_cirrus_write_config(PCIDevice *d,
     PCICirrusVGAState *pvs = DO_UPCAST(PCICirrusVGAState, dev, d);
     CirrusVGAState *s = &pvs->cirrus_vga;
 
-    vga_dirty_log_stop(&s->vga);
-
     pci_default_write_config(d, address, val, len);
     if (s->vga.map_addr && d->io_regions[0].addr == PCI_BAR_UNMAPPED)
         s->vga.map_addr = 0;
     cirrus_update_memory_access(s);
-
-    vga_dirty_log_start(&s->vga);
 }
 
 static int pci_cirrus_vga_initfn(PCIDevice *dev)
diff --git a/hw/vga-pci.c b/hw/vga-pci.c
index 3907871..2315f70 100644
--- a/hw/vga-pci.c
+++ b/hw/vga-pci.c
@@ -68,11 +68,9 @@ static void pci_vga_write_config(PCIDevice *d,
     PCIVGAState *pvs = container_of(d, PCIVGAState, dev);
     VGACommonState *s = &pvs->vga;
 
-    vga_dirty_log_stop(s);
     pci_default_write_config(d, address, val, len);
     if (s->map_addr && pvs->dev.io_regions[0].addr == -1)
         s->map_addr = 0;
-    vga_dirty_log_start(s);
 }
 
 static int pci_vga_initfn(PCIDevice *dev)
diff --git a/hw/vga.c b/hw/vga.c
index c316f72..36763df 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -1282,8 +1282,6 @@ static void vga_draw_text(VGACommonState *s, int full_update)
     vga_draw_glyph8_func *vga_draw_glyph8;
     vga_draw_glyph9_func *vga_draw_glyph9;
 
-    vga_dirty_log_stop(s);
-
     /* compute font data address (in plane 2) */
     v = s->sr[3];
     offset = (((v >> 4) & 1) | ((v << 1) & 6)) * 8192 * 4 + 2;
@@ -1595,65 +1593,40 @@ static void vga_sync_dirty_bitmap(VGACommonState *s)
     }
 #endif
 
-    vga_dirty_log_start(s);
-}
-
-static int s1, s2, s3;
-
-static void mark_dirty(target_phys_addr_t start, target_phys_addr_t len)
-{
-    target_phys_addr_t end = start + len;
-
-    while (start < end) {
-        cpu_physical_memory_set_dirty(cpu_get_physical_page_desc(start));
-        start += TARGET_PAGE_SIZE;
-    }
 }
 
 void vga_dirty_log_start(VGACommonState *s)
 {
     if (kvm_enabled() && s->map_addr)
-        if (!s1) {
-            kvm_log_start(s->map_addr, s->map_end - s->map_addr);
-            mark_dirty(s->map_addr, s->map_end - s->map_addr);
-            s1 = 1;
-        }
+        kvm_log_start(s->map_addr, s->map_end - s->map_addr);
+
     if (kvm_enabled() && s->lfb_vram_mapped) {
-        if (!s2) {
-            kvm_log_start(isa_mem_base + 0xa0000, 0x8000);
-            kvm_log_start(isa_mem_base + 0xa8000, 0x8000);
-            mark_dirty(isa_mem_base + 0xa0000, 0x10000);
-        }
-        s2 = 1;
+        kvm_log_start(isa_mem_base + 0xa0000, 0x8000);
+        kvm_log_start(isa_mem_base + 0xa8000, 0x8000);
     }
 
 #ifdef CONFIG_BOCHS_VBE
     if (kvm_enabled() && s->vbe_mapped) {
-        if (!s3) {
-            kvm_log_start(VBE_DISPI_LFB_PHYSICAL_ADDRESS, s->vram_size);
-        }
-        s3 = 1;
+        kvm_log_start(VBE_DISPI_LFB_PHYSICAL_ADDRESS, s->vram_size);
     }
 #endif
 }
 
 void vga_dirty_log_stop(VGACommonState *s)
 {
-    if (kvm_enabled() && s->map_addr && s1)
+    if (kvm_enabled() && s->map_addr)
 	kvm_log_stop(s->map_addr, s->map_end - s->map_addr);
 
-    if (kvm_enabled() && s->lfb_vram_mapped && s1) {
+    if (kvm_enabled() && s->lfb_vram_mapped) {
 	kvm_log_stop(isa_mem_base + 0xa0000, 0x8000);
 	kvm_log_stop(isa_mem_base + 0xa8000, 0x8000);
     }
 
 #ifdef CONFIG_BOCHS_VBE
-    if (kvm_enabled() && s->vbe_mapped && s3) {
+    if (kvm_enabled() && s->vbe_mapped) {
 	kvm_log_stop(VBE_DISPI_LFB_PHYSICAL_ADDRESS, s->vram_size);
     }
 #endif
-
-    s1 = s2 = s3 = 0;
 }
 
 void vga_dirty_log_restart(VGACommonState *s)
@@ -1891,7 +1864,6 @@ static void vga_draw_blank(VGACommonState *s, int full_update)
         return;
     if (s->last_scr_width <= 0 || s->last_scr_height <= 0)
         return;
-    vga_dirty_log_stop(s);
 
     s->rgb_to_pixel =
         rgb_to_pixel_dup_table[get_depth_index(s->ds)];
-- 
1.7.1

Re: [RFC][PATCH] qemu-kvm: Drop vga dirty logging workarounds

[Avi Kivity]

On 11/15/2010 12:32 PM, Jan Kiszka wrote:
> These diffs to upstream should all date back to the days qemu-kvm
> supported vga dirty logging with restricted/broken kvm kernel modules.
> We no longer do, so there is no need for those workarounds. Even worse
> they can trigger internal bug checks these days:
>
> BUG: kvm_dirty_pages_log_change: invalid parameters 00000000000a8000-00000000000affff

I'd like to apply this.  What kind of testing has this seen?  autotest 
likely isn't a good enough test.

-- 
error compiling committee.c: too many arguments to function

Re: [RFC][PATCH] qemu-kvm: Drop vga dirty logging workarounds

[Jan Kiszka]

Am 15.11.2010 14:16, Avi Kivity wrote:
> On 11/15/2010 12:32 PM, Jan Kiszka wrote:
>> These diffs to upstream should all date back to the days qemu-kvm
>> supported vga dirty logging with restricted/broken kvm kernel modules.
>> We no longer do, so there is no need for those workarounds. Even worse
>> they can trigger internal bug checks these days:
>>
>> BUG: kvm_dirty_pages_log_change: invalid parameters 00000000000a8000-00000000000affff
> 
> I'd like to apply this.  What kind of testing has this seen?  autotest 
> likely isn't a good enough test.

No systematic testing.

It's based on the fact that I'm not aware of any VGA issues in upstream
when KVM is enabled, on the fact that explicit log enable/disable became
obsolete when switching to upstream's logging backend, and the annoying
BUG messages only issued under qemu-kvm. There just remains a slight
uncertainty about what mark_dirty and s[123] were once doing - or may
still be useful for.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [RFC][PATCH] qemu-kvm: Drop vga dirty logging workarounds

[Avi Kivity]

On 11/15/2010 03:26 PM, Jan Kiszka wrote:
> Am 15.11.2010 14:16, Avi Kivity wrote:
> >  On 11/15/2010 12:32 PM, Jan Kiszka wrote:
> >>  These diffs to upstream should all date back to the days qemu-kvm
> >>  supported vga dirty logging with restricted/broken kvm kernel modules.
> >>  We no longer do, so there is no need for those workarounds. Even worse
> >>  they can trigger internal bug checks these days:
> >>
> >>  BUG: kvm_dirty_pages_log_change: invalid parameters 00000000000a8000-00000000000affff
> >
> >  I'd like to apply this.  What kind of testing has this seen?  autotest
> >  likely isn't a good enough test.
>
> No systematic testing.
>
> It's based on the fact that I'm not aware of any VGA issues in upstream
> when KVM is enabled,

I don't think upstream+kvm sees a lot of testing.

>   on the fact that explicit log enable/disable became
> obsolete when switching to upstream's logging backend, and the annoying
> BUG messages only issued under qemu-kvm. There just remains a slight
> uncertainty about what mark_dirty and s[123] were once doing - or may
> still be useful for.

I remember re-adding some kvm-specific dirty logging code after 
regressions were uncovered, so I'm a little worried.  I guess we can 
risk it, we can always revert it (or fix) if it turns out badly.

-- 
error compiling committee.c: too many arguments to function