Re: [Qemu-devel] [PATCH 4/9] vga: do not dynamically allocate chain4_alias

[Paolo Bonzini <pbonzini@redhat.com>]

Il 31/07/2014 14:01, Peter Crosthwaite ha scritto:
> On Wed, Jul 30, 2014 at 8:27 PM, Paolo Bonzini <pbonzini@redhat.com> 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 <pbonzini@redhat.com>
> 
> Otherwise,
> 
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
>> ---
>>  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
>>
>>
>>