From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38313) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCp7c-00027t-Vp for qemu-devel@nongnu.org; Thu, 31 Jul 2014 08:06:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XCp7X-0004vJ-Of for qemu-devel@nongnu.org; Thu, 31 Jul 2014 08:06:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59360) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCp7X-0004vC-GT for qemu-devel@nongnu.org; Thu, 31 Jul 2014 08:06:07 -0400 Message-ID: <53DA312B.4060605@redhat.com> Date: Thu, 31 Jul 2014 14:06:03 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1406716032-21795-1-git-send-email-pbonzini@redhat.com> <1406716032-21795-5-git-send-email-pbonzini@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/9] vga: do not dynamically allocate chain4_alias List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: "qemu-devel@nongnu.org Developers" Il 31/07/2014 14:01, Peter Crosthwaite ha scritto: > On Wed, Jul 30, 2014 at 8:27 PM, Paolo Bonzini wrote: >> Instead, add a boolean variable to indicate the presence of the region. >> > > Patch is good. Can you add a sentence on why you are doing this > though? Is it just the save on the repeated malloc and free (which is > sane in its own right) or something bigger in the context of your > series? Even more than the repeated malloc and free, it's the repeated add_child/unparent. Paolo >> Signed-off-by: Paolo Bonzini > > Otherwise, > > Reviewed-by: Peter Crosthwaite > >> --- >> hw/display/vga.c | 24 ++++++++++-------------- >> hw/display/vga_int.h | 3 ++- >> 2 files changed, 12 insertions(+), 15 deletions(-) >> >> diff --git a/hw/display/vga.c b/hw/display/vga.c >> index 4b089a3..68cfee2 100644 >> --- a/hw/display/vga.c >> +++ b/hw/display/vga.c >> @@ -168,15 +168,18 @@ static uint8_t expand4to8[16]; >> >> static void vga_update_memory_access(VGACommonState *s) >> { >> - MemoryRegion *region, *old_region = s->chain4_alias; >> hwaddr base, offset, size; >> >> if (s->legacy_address_space == NULL) { >> return; >> } >> >> - s->chain4_alias = NULL; >> - >> + if (s->has_chain4_alias) { >> + memory_region_del_subregion(s->legacy_address_space, &s->chain4_alias); >> + memory_region_destroy(&s->chain4_alias); >> + s->has_chain4_alias = false; >> + s->plane_updated = 0xf; >> + } >> if ((s->sr[VGA_SEQ_PLANE_WRITE] & VGA_SR02_ALL_PLANES) == >> VGA_SR02_ALL_PLANES && s->sr[VGA_SEQ_MEMORY_MODE] & VGA_SR04_CHN_4M) { >> offset = 0; >> @@ -201,18 +204,11 @@ static void vga_update_memory_access(VGACommonState *s) >> break; >> } >> base += isa_mem_base; >> - region = g_malloc(sizeof(*region)); >> - memory_region_init_alias(region, memory_region_owner(&s->vram), >> + memory_region_init_alias(&s->chain4_alias, memory_region_owner(&s->vram), >> "vga.chain4", &s->vram, offset, size); >> memory_region_add_subregion_overlap(s->legacy_address_space, base, >> - region, 2); >> - s->chain4_alias = region; >> - } >> - if (old_region) { >> - memory_region_del_subregion(s->legacy_address_space, old_region); >> - memory_region_destroy(old_region); >> - g_free(old_region); >> - s->plane_updated = 0xf; >> + &s->chain4_alias, 2); >> + s->has_chain4_alias = true; >> } >> } >> >> @@ -1321,7 +1317,7 @@ static void vga_draw_text(VGACommonState *s, int full_update) >> s->font_offsets[1] = offset; >> full_update = 1; >> } >> - if (s->plane_updated & (1 << 2) || s->chain4_alias) { >> + if (s->plane_updated & (1 << 2) || s->has_chain4_alias) { >> /* if the plane 2 was modified since the last display, it >> indicates the font may have been modified */ >> s->plane_updated = 0; >> diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h >> index 5320abd..641f8f4 100644 >> --- a/hw/display/vga_int.h >> +++ b/hw/display/vga_int.h >> @@ -94,7 +94,8 @@ typedef struct VGACommonState { >> uint32_t vram_size; >> uint32_t vram_size_mb; /* property */ >> uint32_t latch; >> - MemoryRegion *chain4_alias; >> + bool has_chain4_alias; >> + MemoryRegion chain4_alias; >> uint8_t sr_index; >> uint8_t sr[256]; >> uint8_t gr_index; >> -- >> 1.8.3.1 >> >> >>