Re: [Qemu-devel] [PATCH v7 5/7] memory: make mmio dispatch able to be out of biglock

[Paolo Bonzini]

Il 06/05/2013 13:25, Jan Kiszka ha scritto:
> On 2013-05-06 13:21, Paolo Bonzini wrote:
>> Il 25/11/2012 03:03, Liu Ping Fan ha scritto:
>>> @@ -3550,12 +3668,11 @@ void *address_space_map(AddressSpace *as,
>>>                          target_phys_addr_t *plen,
>>>                          bool is_write)
>>>  {
>>> -    AddressSpaceDispatch *d = as->dispatch;
>>>      target_phys_addr_t len = *plen;
>>>      target_phys_addr_t todo = 0;
>>>      int l;
>>>      target_phys_addr_t page;
>>> -    MemoryRegionSection *section;
>>> +    MemoryRegionSection *section, mr_obj;
>>>      ram_addr_t raddr = RAM_ADDR_MAX;
>>>      ram_addr_t rlen;
>>>      void *ret;
>>> @@ -3565,7 +3682,8 @@ void *address_space_map(AddressSpace *as,
>>>          l = (page + TARGET_PAGE_SIZE) - addr;
>>>          if (l > len)
>>>              l = len;
>>> -        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
>>> +        address_space_section_lookup_ref(as, page >> TARGET_PAGE_BITS, 
>>> &mr_obj);
>>> +        section = &mr_obj;
>>>  
>>>          if (!(memory_region_is_ram(section->mr) && !section->readonly)) {
>>>              if (todo || bounce.buffer) {
>>> @@ -3579,6 +3697,7 @@ void *address_space_map(AddressSpace *as,
>>>              }
>>>  
>>>              *plen = l;
>>> +            memory_region_section_unref(&mr_obj);
>>>              return bounce.buffer;
>>>          }
>>>          if (!todo) {
>>> @@ -3589,6 +3708,7 @@ void *address_space_map(AddressSpace *as,
>>>          len -= l;
>>>          addr += l;
>>>          todo += l;
>>> +        memory_region_section_unref(&mr_obj);
>>>      }
>>>      rlen = todo;
>>>      ret = qemu_ram_ptr_length(raddr, &rlen);
>>
>> I think this unref is wrong.  You need to delay it to the
>> address_space_unmap, and this in turns requires changing the signature
>> of address_space_map.
> 
> Can't RAMBlock hold a reference to the associated region? Then this
> could be retrieved on unmap without bothering the caller.

Right you are. :)  In fact, RAMBlock already does have block->mr.

Paolo