From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1L2ouM-0001Ld-Et for qemu-devel@nongnu.org; Wed, 19 Nov 2008 10:23:58 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1L2ouJ-0001K8-Fc for qemu-devel@nongnu.org; Wed, 19 Nov 2008 10:23:57 -0500 Received: from [199.232.76.173] (port=54446 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1L2ouJ-0001Jw-9Q for qemu-devel@nongnu.org; Wed, 19 Nov 2008 10:23:55 -0500 Received: from qb-out-1314.google.com ([72.14.204.175]:31944) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1L2ouI-0007lY-C4 for qemu-devel@nongnu.org; Wed, 19 Nov 2008 10:23:54 -0500 Received: by qb-out-1314.google.com with SMTP id e19so1059qba.8 for ; Wed, 19 Nov 2008 07:23:53 -0800 (PST) Message-ID: <49242F84.2010809@codemonkey.ws> Date: Wed, 19 Nov 2008 09:23:48 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 6/6] kvm: vga optimization. References: <1227108377-8442-1-git-send-email-glommer@redhat.com> <1227108377-8442-2-git-send-email-glommer@redhat.com> <1227108377-8442-3-git-send-email-glommer@redhat.com> <1227108377-8442-4-git-send-email-glommer@redhat.com> <1227108377-8442-5-git-send-email-glommer@redhat.com> <1227108377-8442-6-git-send-email-glommer@redhat.com> <1227108377-8442-7-git-send-email-glommer@redhat.com> In-Reply-To: <1227108377-8442-7-git-send-email-glommer@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Glauber Costa wrote: > @@ -1383,6 +1400,7 @@ cirrus_hook_write_sr(CirrusVGAState * s, unsigned reg_index, int reg_value) > reg_index, reg_value); > #endif > break; > + > case 0x17: // Configuration Readback and Extended Control > Please don't introduce stray whitespace. > s->sr[reg_index] = (s->sr[reg_index] & 0x38) | (reg_value & 0xc7); > cirrus_update_memory_access(s); > @@ -1528,12 +1546,13 @@ cirrus_hook_write_gr(CirrusVGAState * s, unsigned reg_index, int reg_value) > s->gr[reg_index] = reg_value; > cirrus_update_bank_ptr(s, 0); > cirrus_update_bank_ptr(s, 1); > - break; > + cirrus_update_memory_access(s); > + break; > This formatting seems way off? > case 0x0B: > s->gr[reg_index] = reg_value; > cirrus_update_bank_ptr(s, 0); > cirrus_update_bank_ptr(s, 1); > - cirrus_update_memory_access(s); > + cirrus_update_memory_access(s); > break; > And this is just a reformatting change. Please avoid these. > case 0x10: // BGCOLOR 0x0000ff00 > case 0x11: // FGCOLOR 0x0000ff00 > @@ -2618,6 +2637,48 @@ static CPUWriteMemoryFunc *cirrus_linear_bitblt_write[3] = { > cirrus_linear_bitblt_writel, > }; > > +static void map_linear_vram(CirrusVGAState *s) > +{ > + > + if (!s->map_addr && s->lfb_addr && s->lfb_end) { > + s->map_addr = s->lfb_addr; > + s->map_end = s->lfb_end; > + cpu_register_physical_memory(s->map_addr, s->map_end - s->map_addr, s->vram_offset); > + vga_dirty_log_start((VGAState *)s); > + } > + > + if (!s->map_addr) > + return; > + > + if (cirrus_lfb_is_mapped(s)) { > + cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x8000, > + (s->vram_offset + s->cirrus_bank_base[0]) | IO_MEM_RAM); > + cpu_register_physical_memory(isa_mem_base + 0xa8000, 0x8000, > + (s->vram_offset + s->cirrus_bank_base[1]) | IO_MEM_RAM); > Isn't necessary to reregister 0xa0000 too? > + if (kvm_enabled()) { > + kvm_log_start(0xa0000, 0x8000); > + kvm_log_start(0xa8000, 0x8000); > + } > Why would you enable logging on a different region from what you've registered? Shouldn't you enable logging on both regions? If we're going to enable logging based on target_phys_addr_t instead of ram_addr_t (and I think we should), then we should enable it on all target_phys_addr_ts. > + } > + else { > This is formatted incorrectly. > > + > /* > * graphic modes > */ > More extra whitespace. Regards, Anthony Liguori