Re: [Qemu-devel] [PATCH for-2.4 2/2] framebuffer: set DIRTY_MEMORY_VGA on RAM that is used for the framebuffer

[Paolo Bonzini]

On 23/07/2015 12:22, Peter Maydell wrote:
> I have a few minor nits below, but the patch works and looks
> good overall.
> 
> At the moment the pattern in all the callsites is
>  if (s->invalidate) {
>      framebuffer_update_memory_section(...);
>  }
>  framebuffer_update_display(...);
> 
> What's the rationale for not just having framebuffer_update_display()
> do this -- that we might in future want to be cleverer about how
> often we call framebuffer_update_memory_section() ?

Yes.  I initially set out adding framebuffer_update_memory_section calls
after the register writes, but using s->invalidate is simpler albeit
marginally less efficient.

>> +    memory_region_reset_dirty(mem, mem_section->offset_within_region, 
>> src_len,
>>                                DIRTY_MEMORY_VGA);
> 
> Not new in this patch, but isn't there technically a race
> condition if the guest writes to the framebuffer memory
> after we've done the memory_region_get_dirty() for that
> row but before we clear all the dirty bits again here?

Yes, I think you're right.

>>
>> +void framebuffer_update_memory_section(
>> +    MemoryRegionSection *mem_section,
>> +    MemoryRegion *root,
>> +    hwaddr base,
>> +    unsigned rows,
>> +    unsigned src_width);
> 
> A doc-comment header would be nice.

Ok.

>>
>> +    if (s->invalidated) {
>> +        framebuffer_update_memory_section(&s->fbsection, s->sysmem,
>> +                                          addr, src_width, s->yres);
> 
> Aren't src_width and s->yres in the wrong order here?
> They should be in the same order as they are in the
> (ditto in the other hunks in the patch for this file).

Ouch, indeed.

Paolo