From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:35704) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QMhrI-0003pn-O9 for qemu-devel@nongnu.org; Wed, 18 May 2011 10:36:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QMhrH-0005KZ-7I for qemu-devel@nongnu.org; Wed, 18 May 2011 10:36:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:25142) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QMhrG-0005KU-PF for qemu-devel@nongnu.org; Wed, 18 May 2011 10:36:19 -0400 Message-ID: <4DD3D95E.2060105@redhat.com> Date: Wed, 18 May 2011 17:36:14 +0300 From: Avi Kivity MIME-Version: 1.0 References: <4DD3C5B9.1080908@redhat.com> <4DD3D236.90708@siemens.com> In-Reply-To: <4DD3D236.90708@siemens.com> 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: Jan Kiszka Cc: qemu-devel On 05/18/2011 05:05 PM, Jan Kiszka wrote: > On 2011-05-18 15:12, 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. > > Restoring is not the problem. The problem is that the current API > deletes or truncates regions implicitly by overwriting. That makes > tracking the layout hard, and it is also error-prone as the device that > accidentally overlaps with some other device won't receive a > notification of this potential conflict. > > Such implicite truncation or deletion must be avoided in a new API, > forcing the users to explicitly reference an existing region when > dropping or modifying it. But your API goes in the right direction. It is avoided. The unregistering/deleting APIs do not take a range, just an object. This implies that the range is stored in the object. The initial implementation will probably have the same problem as it will still be backed by the phys_desc array, but we'll have all of the information in MemoryRegions (we can have a single MemoryRegion that is all of memory) so we can avoid it with a better information. > > > - 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 > > I would add another drawback: > > - Inability to identify the origin of a region accesses and handle them > differently based on the source. > > That is at least problematic for the x86 APIC which is CPU local. Our > current way do deal with it is, well, very creative and falls to > dust if a guest actually tries to remap the APIC. > > However, I'm unsure if that can easily be addressed. As long as only x86 > is affected, it's tricky to ask for a big infrastructure to handle this > special case. Maybe there some other use cases, don't know. We could implement it with a per-cpu MemoryRegion, with each cpu's MemoryRegion populated by a different APIC sub-region. > > > > > 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); > > > > struct MemoryRegionOps { > > MemoryReadFunc readb, readw, readl; > > MemoryWriteFunc writeb, writew, writel; > > }; > > > > struct MemoryRegion { > > const MemoryRegionOps *ops; > > target_phys_addr_t size; > > target_phys_addr_t addr; > > }; > > > > void memory_region_init(MemoryRegion *mr, > > target_phys_addr_t size); > > What use case would this abstract region cover? An empty container, fill it with memory_region_add_subregion(). > > > void memory_region_init_io(MemoryRegion *mr, > > const MemoryRegionOps *ops, > > target_phys_addr_t size); > > 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); > > 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); > > > > void memory_region_add_subregion(MemoryRegion *mr, > > target_phys_addr_t offset, > > MemoryRegion *subregion); > > void memory_region_del_subregion(MemoryRegion *mr, > > target_phys_addr_t offset, > > MemoryRegion *subregion); > > > > void cpu_register_memory_region(MemoryRegion *mr, target_phys_addr_t addr); > > This could create overlaps. I would suggest to reject them, so we need a > return code. There is nothing we can do with a return code. You can't fail an mmio that causes overlapping physical memory map. > > > void cpu_unregister_memory_region(MemoryRegion *mr); Instead, we need cpu_unregister_memory_region() to restore any previously hidden ranges. > > > > #endif > > > > The API is nested: you can define, say, a PCI BAR containing RAM and > > MMIO, and give it to the PCI subsystem. PCI can then enable/disable the > > BAR and move it to different addresses without calling any callbacks; > > the client code can enable or disable logging or coalescing without > > caring if the BAR is mapped or not. For example: > > Interesting feature. > > > > > MemoryRegion mr, mr_mmio, mr_ram; > > > > memory_region_init(&mr); > > memory_region_init_io(&mr_mmio,&mmio_ops, 0x1000); > > memory_region_init_ram(&mr_ram, 0x100000); > > memory_region_add_subregion(&mr, 0,&mr_ram); > > memory_region_add_subregion(&mr, 0x10000,&mr_io); > > memory_region_add_coalescing(&mr_ram, 0, 0x100000); > > pci_register_bar(&pci_dev, 0,&mr); > > > > at this point the PCI subsystem knows everything about the BAR and can > > enable or disable it, or move it around, without further help from the > > device code. On the other hand, the device code can change logging or > > coalescing, or even change the structure of the region, without caring > > about whether the region is currently registered or not. > > > > If we can agree on the API, then I think the way forward is to implement > > it in terms of the old API, change over all devices, then fold the old > > API into the new one. > > There are more aspects that should be clarified before moving forward: > - How to maintain memory regions internally? Not sure what you mean by the question, but my plan was to have the client responsible for allocating the objects (and later use container_of() in the callbacks - note there are no void *s any longer). > - Get rid of wasteful PhysPageDesc at this chance? That's the intent, but not at this chance, rather later on. But I want the API to be compatible with the goal so we don't have to touch all devices again. > - How to hook into the region maintenance (CPUPhysMemoryClient, > listening vs. filtering or modifying changes)? How to simplify > memory clients this way? I'd leave things as is, at least for the beginning. CPUPhysMemoryClient is global in nature, whereas MemoryRegion is local (offsets are relative to the containing region). > > BTW, any old API should be removed ASAP once the new one demonstrated > its feasibility. IMHO, we can afford carrying yet another set of legacy > interface around. That's the plan. New API implemented on top of old API, convert all devices, fold old API into new API. -- error compiling committee.c: too many arguments to function