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

[Anthony Liguori <anthony@codemonkey.ws>]

Glauber Costa wrote:
>>> +    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?
>>     
>
> ENOFOLLOW. This is exactly what I'm doing.
>   

I was confused about isa_mem_base.  Just ignore me :-).

Regards,

Anthony Liguori

>>> +        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.
>>     
>
> Again, I don't follow. We map 0xa0000 and 0xa8000 to some ram_addr_t,
> and then enable logging in the very 0xa0000 and 0xa8000. What's the problem
> with that? One late nitpick, it is that for consistency, I registered
> 0xa0000 + isa_mem_base,
> (usually 0), and should use it in kvm_log_start.
>
>   
>>> +    }
>>> +    else {
>>>
>>>       
>> This is formatted incorrectly.
>>
>>     
>>>  +
>>>  /*
>>>  * graphic modes
>>>  */
>>>
>>>       
>> More extra whitespace.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>
>>
>>     
>
>
>
>