Re: [Qemu-devel] [PATCH 6/6] kvm: vga optimization.

[Anthony Liguori]

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