Re: [Qemu-devel] [patch v4 10/16] memory: introduce lock ops for MemoryRegionOps

[Jan Kiszka]

On 2012-10-23 07:53, liu ping fan wrote:
> On Mon, Oct 22, 2012 at 6:30 PM, Avi Kivity <address@hidden> 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