On Tue, Oct 23, 2012 at 8:36 PM, Avi Kivity <a...@redhat.com> wrote:
> On 10/23/2012 02:12 PM, Jan Kiszka wrote:
>> On 2012-10-22 11:23, Liu Ping Fan wrote:
>>> Without biglock, we try to protect the mr by increase refcnt.
>>> If we can inc refcnt, go backward and resort to biglock.
>>>
>>> Another point is memory radix-tree can be flushed by another
>>> thread, so we should get the copy of terminal mr to survive
>>> from such issue.
>>>
>>> +
>>> void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>>> int len, int is_write)
>>> {
>>> @@ -3413,14 +3489,28 @@ void cpu_physical_memory_rw(target_phys_addr_t
>>> addr, uint8_t *buf,
>>> uint8_t *ptr;
>>> uint32_t val;
>>> target_phys_addr_t page;
>>> - MemoryRegionSection *section;
>>> + MemoryRegionSection *section, obj_mrs;
>>> + int safe_ref;
>>>
>>> while (len > 0) {
>>> page = addr & TARGET_PAGE_MASK;
>>> l = (page + TARGET_PAGE_SIZE) - addr;
>>> if (l > len)
>>> l = len;
>>> - section = phys_page_find(page >> TARGET_PAGE_BITS);
>>> + qemu_mutex_lock(&mem_map_lock);
>>> + safe_ref = phys_page_lookup(page, &obj_mrs);
>>> + qemu_mutex_unlock(&mem_map_lock);
>>> + if (safe_ref == 0) {
>>> + qemu_mutex_lock_iothread();
>>> + qemu_mutex_lock(&mem_map_lock);
>>> + /* At the 2nd try, mem map can change, so need to judge it
>>> again */
>>> + safe_ref = phys_page_lookup(page, &obj_mrs);
>>> + qemu_mutex_unlock(&mem_map_lock);
>>> + if (safe_ref > 0) {
>>> + qemu_mutex_unlock_iothread();
>>> + }
>>> + }
>>> + section = &obj_mrs;
>>>
>>> if (is_write) {
>>> if (!memory_region_is_ram(section->mr)) {
>>> @@ -3491,10 +3581,16 @@ void cpu_physical_memory_rw(target_phys_addr_t
>>> addr, uint8_t *buf,
>>> qemu_put_ram_ptr(ptr);
>>> }
>>> }
>>> +
>>> + memory_region_section_unref(&obj_mrs);
>>
>> The mapping cannot change from not-referenced to reference-counted while
>> we were dispatching? I mean the case where we found not ref callback on
>> entry and took the big lock, but now there is an unref callback.
>
> We drop the big lock in that case, so we end up in the same situation.
>
>>
>>> len -= l;
>>> buf += l;
>>> addr += l;
>>> + if (safe_ref == 0) {
>>> + qemu_mutex_unlock_iothread();
>>> + }
>>> }
>>> +
>>> }
>>>
>>> /* used for ROM loading : can write in RAM and ROM */
>>> @@ -3504,14 +3600,18 @@ void
>>> cpu_physical_memory_write_rom(target_phys_addr_t addr,
>>> int l;
>>> uint8_t *ptr;
>>> target_phys_addr_t page;
>>> - MemoryRegionSection *section;
>>> + MemoryRegionSection *section, mr_obj;
>>>
>>> while (len > 0) {
>>> page = addr & TARGET_PAGE_MASK;
>>> l = (page + TARGET_PAGE_SIZE) - addr;
>>> if (l > len)
>>> l = len;
>>> - section = phys_page_find(page >> TARGET_PAGE_BITS);
>>> +
>>> + qemu_mutex_lock(&mem_map_lock);
>>> + phys_page_lookup(page, &mr_obj);
>>> + qemu_mutex_unlock(&mem_map_lock);
>>> + section = &mr_obj;
>>
>> But here we don't care about the return code of phys_page_lookup and all
>> related topics? Because we assume the BQL is held? Reminds me that we
>> will need some support for assert(qemu_mutex_is_locked(&lock)).
>
> I guess it's better to drop that assumption than to have asymmetric APIs.
>
Yes, now the updater of physmap based on mem_map_lock, and the same it
will be for readers.
>>>
>>> @@ -4239,9 +4345,12 @@ bool virtio_is_big_endian(void)
>>> #ifndef CONFIG_USER_ONLY
>>> bool cpu_physical_memory_is_io(target_phys_addr_t phys_addr)
>>> {
>>> - MemoryRegionSection *section;
>>> + MemoryRegionSection *section, mr_obj;
>>>
>>> - section = phys_page_find(phys_addr >> TARGET_PAGE_BITS);
>>> + qemu_mutex_lock(&mem_map_lock);
>>> + phys_page_lookup(phys_addr, &mr_obj);
>>> + qemu_mutex_unlock(&mem_map_lock);
>>> + section = &mr_obj;
>>
>> Err, no unref needed here?
>
> Need _ref in the name to remind reviewers that it leaves the refcount
> unbalanced.
>
Oh, here is a bug, need unref. As to unbalanced refcount, it will be
adopted for virtio-blk listener (not implement in this patchset)
Regards,
pingfan
> --
> error compiling committee.c: too many arguments to function
>
