On 2012-10-23 07:53, liu ping fan wrote:
> On Mon, Oct 22, 2012 at 6:30 PM, Avi Kivity <a...@redhat.com> wrote:
>> On 10/22/2012 11:23 AM, Liu Ping Fan wrote:
>>> This can help memory core to use mr's fine lock to mmio dispatch.
>>>
>>> diff --git a/memory.c b/memory.c
>>> index d528d1f..86d5623 100644
>>> --- a/memory.c
>>> +++ b/memory.c
>>> @@ -1505,13 +1505,27 @@ void set_system_io_map(MemoryRegion *mr)
>>>
>>> uint64_t io_mem_read(MemoryRegion *mr, target_phys_addr_t addr, unsigned
>>> size)
>>> {
>>> - return memory_region_dispatch_read(mr, addr, size);
>>> + uint64_t ret;
>>> + if (mr->ops->lock) {
>>> + mr->ops->lock(mr);
>>> + }
>>> + ret = memory_region_dispatch_read(mr, addr, size);
>>> + if (mr->ops->lock) {
>>> + mr->ops->unlock(mr);
>>> + }
>>> + return ret;
>>> }
>>>
>>> void io_mem_write(MemoryRegion *mr, target_phys_addr_t addr,
>>> uint64_t val, unsigned size)
>>> {
>>> + if (mr->ops->lock) {
>>> + mr->ops->lock(mr);
>>> + }
>>> memory_region_dispatch_write(mr, addr, val, size);
>>> + if (mr->ops->lock) {
>>> + mr->ops->unlock(mr);
>>> + }
>>> }
>>>
>>> typedef struct MemoryRegionList MemoryRegionList;
>>> diff --git a/memory.h b/memory.h
>>> index 9039411..5d00066 100644
>>> --- a/memory.h
>>> +++ b/memory.h
>>> @@ -69,6 +69,8 @@ struct MemoryRegionOps {
>>> unsigned size);
>>> int (*ref)(MemoryRegion *mr);
>>> void (*unref)(MemoryRegion *mr);
>>> + void (*lock)(MemoryRegion *mr);
>>> + void (*unlock)(MemoryRegion *mr);
>>>
>>> enum device_endian endianness;
>>> /* Guest-visible constraints: */
>>>
>>
>> Is this really needed? Can't read/write callbacks lock and unlock
>> themselves?
>>
> We can. But then, we need to expand the logic there, and use addr to
> tell which fine lock to snatch and release. Which one do you prefer,
> fold the logic into memory core or spread it into devices?
I also don't see why having to provide additional callbacks is simpler
than straight calls to qemu_mutex_lock/unlock from within the access
handlers.
Moreover, the second model will allow to take or not take the lock
depending on the access address / width / device state / whatever in a
more comprehensible way as you can follow the control flow better when
there are less callbacks. Many plain "read register X" accesses will not
require any device-side locking at all.
Granted, a downside is that the risk of leaking locks in case of early
returns etc. increases.
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
