From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:59576) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QMioV-0007Ff-7m for qemu-devel@nongnu.org; Wed, 18 May 2011 11:37:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QMioU-0000Xt-2V for qemu-devel@nongnu.org; Wed, 18 May 2011 11:37:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:27515) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QMioT-0000Xn-Nf for qemu-devel@nongnu.org; Wed, 18 May 2011 11:37:29 -0400 Message-ID: <4DD3E7B4.9020802@redhat.com> Date: Wed, 18 May 2011 18:37:24 +0300 From: Avi Kivity MIME-Version: 1.0 References: <4DD3C5B9.1080908@redhat.com> <4DD3E0E5.2030700@codemonkey.ws> In-Reply-To: <4DD3E0E5.2030700@codemonkey.ws> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC] Memory API List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel On 05/18/2011 06:08 PM, Anthony Liguori wrote: > On 05/18/2011 08:12 AM, Avi Kivity wrote: >> The current memory APIs (cpu_register_io_memory, >> cpu_register_physical_memory) suffer from a number of drawbacks: >> >> - lack of centralized bookkeeping >> - a cpu_register_physical_memory() that overlaps an existing region will >> overwrite the preexisting region; but a following >> cpu_unregister_physical_memory() will not restore things to status quo >> ante. >> - coalescing and dirty logging are all cleared on unregistering, so the >> client has to re-issue them when re-registering >> - lots of opaques >> - no nesting >> - if a region has multiple subregions that need different handling >> (different callbacks, RAM interspersed with MMIO) then client code has >> to deal with that manually >> - we can't interpose code between a device and global memory handling >> >> To fix that, I propose an new API to replace the existing one: >> >> >> #ifndef MEMORY_H >> #define MEMORY_H >> >> typedef struct MemoryRegionOps MemoryRegionOps; >> typedef struct MemoryRegion MemoryRegion; >> >> typedef uint32_t (*MemoryReadFunc)(MemoryRegion *mr, target_phys_addr_t >> addr); >> typedef void (*MemoryWriteFunc)(MemoryRegion *mr, target_phys_addr_t >> addr, >> uint32_t data); > > > The API should be 64-bit and needs to have a size associated with it. That will make conversion hard. Maybe we can have a native with-size callback, and a helper that converts to the traditional form. Otherwise the effort to fully covert the tree increases dramatically. >> >> void memory_region_init(MemoryRegion *mr, >> target_phys_addr_t size); > > What is this used for? It's not clear to me. Empty container, fill with memory_region_add_subregion() to taste. > >> void memory_region_init_io(MemoryRegion *mr, >> const MemoryRegionOps *ops, >> target_phys_addr_t size); > > How does one test a MemoryRegion to determine it's type? One doesn't. Of course the low-level will need to do this (via a private API that isn't exposed to devices). >> void memory_region_init_ram(MemoryRegion *mr, >> target_phys_addr_t size); >> void memory_region_init_ram_ptr(MemoryRegion *mr, >> target_phys_addr_t size, >> void *ptr); >> void memory_region_destroy(MemoryRegion *mr); >> void memory_region_set_offset(MemoryRegion *mr, target_phys_addr_t >> offset); > > What's "offset" mean? It's the equivalent of cpu_register_physical_memory_offset(). Note the intent is to have addresses always be relative to the innermost container. Perhaps we can get away without offset. > >> void memory_region_set_log(MemoryRegion *mr, bool log); >> void memory_region_clear_coalescing(MemoryRegion *mr); >> void memory_region_add_coalescing(MemoryRegion *mr, >> target_phys_addr_t offset, >> target_phys_addr_t size); > > I don't think it's worth while to try to fit coalescing into this API. > It's a KVM specific hack. I think it's fine to be a hacked on API. The problem is that only the device knows about coalescing, while the region can be mapped, unmapped, or moved without device knowledge. So if a PCI is unmapped and then remapped (possibly at a different address) we need to tell kvm about it, but there is no device callback involved. > I think it pretty much makes sense to me. It might be worth while to > allow memory region definitions to be static. For instance: > > MemoryRegion e1000_bar = { ... }; > It doesn't work in general, since multiple instances of e1000 will each need its own e1000_bar. So e1000_bar will be a member of the e1000 device state structure. -- error compiling committee.c: too many arguments to function