On Sun, May 22, 2011 at 3:18 PM, Avi Kivity <avi@xxxxxxxxxx> wrote: > On 05/22/2011 03:06 PM, Blue Swirl wrote: >> >> On Sun, May 22, 2011 at 2:36 PM, Avi Kivity<avi@xxxxxxxxxx> Âwrote: >> > ÂOn 05/22/2011 12:32 PM, Blue Swirl wrote: >> >> >> >> Â>> Â Â> Â Â Â+void memory_region_add_coalescing(MemoryRegion *mr, >> >> Â>> Â Â> Â Â Â+ Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âtarget_phys_addr_t >> >> offset, >> >> Â>> Â Â> Â Â Â+ Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âtarget_phys_addr_t >> >> size); >> >> Â>> Â Â> Â Â Â+/* Disable MMIO coalescing for the region. */ >> >> Â>> Â Â> Â Â Â+void memory_region_clear_coalescing(MemoryRegion *mr); >> >> Â>> >> >> Â>> Â ÂPerhaps the interface could be more generic, like >> >> Â>> Â Â+void memory_region_set_property(MemoryRegion *mr, unsigned >> >> flags); >> >> Â>> Â Â+void memory_region_clear_property(MemoryRegion *mr, unsigned >> >> flags); >> >> Â>> >> >> Â> >> >> Â> Â ÂCoalescing is a complex property, not just a boolean attribute. >> >> ÂWe >> >> Â> Âprobably >> >> Â> Â Âwill have a number of boolean attributes later, though. >> >> >> >> ÂBut what is the difference between adding coalescing to an area and >> >> Âsetting the bit property 'coalescing' to an area? At least what you >> >> Âpropose now is not so complex that it couldn't be handled as a single >> >> Âbit. >> > >> > ÂLook at the API - add_coalescing() sets the coalescing property on a >> > Âsubrange of the memory region, not the entire region. >> >> Right, but doesn't the same apply to any other properties, they may >> apply to a full range or just a subrange? > > We'll know when we have more properties. ÂI expect most will be region-wide. Since we don't know about those yet, coalescing API could be like you propose. Later it can be changed to the property API, or leave for convenience. >> > >> > ÂSubregions are first-class regions. ÂIn fact all regions are subregions >> > Âexcept the root. >> >> Oh, I see now. Maybe the comments should describe this. Or perhaps the >> terms should be something like 'bus/bridge/root' and 'region' instead >> of 'region' and 'subregion'? > > Problem is, memory_region_add_subregion() adds both sub-bridges and leaf > regions. > > It's quite possible that BAR 0 will be a leaf region, and BAR 1 will be a > sub-bridge. > > Can you suggest an alternative naming for the API? How about memory_region_container_init() memory_region_add() -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
