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

["Glauber Costa" <glommer@gmail.com>]

>> +    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.
>
>> +        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
>
>
>



-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."