From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42800) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQaF5-0002tZ-7k for qemu-devel@nongnu.org; Tue, 23 Oct 2012 04:53:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TQaEj-00034N-4A for qemu-devel@nongnu.org; Tue, 23 Oct 2012 04:53:43 -0400 Received: from david.siemens.de ([192.35.17.14]:15105) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQaEi-000347-R8 for qemu-devel@nongnu.org; Tue, 23 Oct 2012 04:53:21 -0400 Message-ID: <50865AFC.2060503@siemens.com> Date: Tue, 23 Oct 2012 10:53:16 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1350897839-29593-1-git-send-email-pingfank@linux.vnet.ibm.com> <1350897839-29593-11-git-send-email-pingfank@linux.vnet.ibm.com> <5085202D.7090601@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [patch v4 10/16] memory: introduce lock ops for MemoryRegionOps List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: liu ping fan Cc: Stefan Hajnoczi , Marcelo Tosatti , "qemu-devel@nongnu.org" , Avi Kivity , Anthony Liguori , Paolo Bonzini On 2012-10-23 07:53, liu ping fan wrote: > On Mon, Oct 22, 2012 at 6:30 PM, Avi Kivity 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