Re: [PATCH 3/5] memory: Flush coalesced MMIO on mapping and state changes

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2012-06-25 10:57, Avi Kivity wrote:
> On 06/25/2012 10:01 AM, Jan Kiszka wrote:
>> Flush pending coalesced MMIO before performing mapping or state changes
>> that could affect the event orderings or route the buffered requests to
>> a wrong region.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> In addition, we also have to
> 
> Yes, we do.
> 
>>  
>>  void memory_region_transaction_begin(void)
>>  {
>> +    qemu_flush_coalesced_mmio_buffer();
>>      ++memory_region_transaction_depth;
>>  }
> 
> Why is this needed?
> 
>>  
>> @@ -1109,6 +1110,9 @@ void memory_region_sync_dirty_bitmap(MemoryRegion *mr)
>>  
>>  void memory_region_set_readonly(MemoryRegion *mr, bool readonly)
>>  {
>> +    if (!QTAILQ_EMPTY(&mr->coalesced)) {
>> +        qemu_flush_coalesced_mmio_buffer();
>> +    }
> 
> The readonly attribute is inherited by subregions and alias targets, so
> this check is insufficient.  See render_memory_region().  Need to flush
> unconditionally.
> 
>>      if (mr->readonly != readonly) {
>>          mr->readonly = readonly;
>>          memory_region_update_topology(mr);
>> @@ -1117,6 +1121,9 @@ void memory_region_set_readonly(MemoryRegion *mr, bool readonly)
>>  
>>  void memory_region_rom_device_set_readable(MemoryRegion *mr, bool readable)
>>  {
>> +    if (!QTAILQ_EMPTY(&mr->coalesced)) {
>> +        qemu_flush_coalesced_mmio_buffer();
>> +    }
> 
> This property is not inherited, but let's flush unconditionally just the
> same, to reduce fragility.
> 
>> @@ -1219,6 +1228,9 @@ void memory_region_add_eventfd(MemoryRegion *mr,
>>      };
>>      unsigned i;
>>  
>> +    if (!QTAILQ_EMPTY(&mr->coalesced)) {
>> +        qemu_flush_coalesced_mmio_buffer();
>> +    }
> 
> Ditto.  It's possible that an eventfd overlays a subregion which has
> coalescing enabled.  It's not really defined what happens in this case,
> and such code and its submitter should be perma-nacked, but let's play
> it safe here since there isn't much to be gained by avoiding the flush.
>  This code is a very slow path anyway, including and rcu and/or srcu
> synchronization, and a rebuild of the dispatch radix tree (trees when we
> dma-enable this code).
> 
>>      for (i = 0; i < mr->ioeventfd_nb; ++i) {
>>          if (memory_region_ioeventfd_before(mrfd, mr->ioeventfds[i])) {
>>              break;
>> @@ -1249,6 +1261,9 @@ void memory_region_del_eventfd(MemoryRegion *mr,
>>      };
>>      unsigned i;
>>  
>> +    if (!QTAILQ_EMPTY(&mr->coalesced)) {
>> +        qemu_flush_coalesced_mmio_buffer();
>> +    }
> 
> Same.

OK.

> 
> The repetitiveness of this code suggests a different way of doing this:
> make every API call be its own subtransaction and perform the flush in
> memory_region_begin_transaction() (maybe that's the answer to my
> question above).

So you want me to wrap the core of those services in
begin/commit_transaction instead? Just to be sure I got the idea.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux