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

[Blue Swirl <blauwirbel@gmail.com>]

On Sun, May 22, 2011 at 2:36 PM, Avi Kivity <avi@redhat.com> 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?

> (motivation - hw/e1000.c).
>
>> >>  >    + * conflicts are resolved by having a higher @priority hide a
>> >> lower
>> >>  >  @priority.
>> >>  >    + * Subregions without priority are taken as @priority 0.
>> >>  >    + */
>> >>  >    +void memory_region_add_subregion_overlap(MemoryRegion *mr,
>> >>  >    +                                         target_phys_addr_t
>> >> offset,
>> >>  >    +                                         MemoryRegion
>> >> *subregion,
>> >>  >    +                                         unsigned priority);
>> >>  >    +/* Remove a subregion. */
>> >>  >    +void memory_region_del_subregion(MemoryRegion *mr,
>> >>  >    +                                 MemoryRegion *subregion);
>> >>
>> >>  What would the subregions be used for?
>> >
>> >  Subregions describe the flow of data through the memory bus.  We'd have
>> > a
>> >  subregion for the PCI bus, with its own subregions for various BARs,
>> > with
>> >  some having subregions for dispatching different MMIO types within the
>> > BAR.
>> >
>> >  This allows, for example, the PCI layer to move a BAR without the PCI
>> > device
>> >  knowing anything about it.
>>
>> But why can't a first class region be used for that?
>
> 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'?

> It's a tree of regions, each level adding an offset, clipping, and perhaps
> other attributes, with the leaves providing actual memory (mmio or RAM).

I thought that there are two classes of regions, like PCI device vs. a
single BAR.