Re: [Qemu-devel] [RFC v1] Add declarations for hierarchical memory region API

[Blue Swirl]

On Sun, May 22, 2011 at 3:18 PM, Avi Kivity <address@hidden> wrote:
> On 05/22/2011 03:06 PM, Blue Swirl wrote:
>>
>> On Sun, May 22, 2011 at 2:36 PM, Avi Kivity<address@hidden>  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()