[v4,10/16] memory: introduce lock ops for MemoryRegionOps

This can help memory core to use mr's fine lock to mmio dispatch.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 memory.c |   16 +++++++++++++++-
 memory.h |    2 ++
 2 files changed, 17 insertions(+), 1 deletions(-)

On Mon, Oct 22, 2012 at 6:30 PM, Avi Kivity <avi@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?

> --
> error compiling committee.c: too many arguments to function
>

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