Re: [RFC v2 01/20] Hierarchical memory region API

Avi Kivity
2011-06-28T11:51:27Z

On 06/28/2011 01:03 PM, Michael S. Tsirkin wrote:
> On Mon, Jun 27, 2011 at 04:21:48PM +0300, Avi Kivity wrote:
>
> ...
>
> > +static bool memory_region_access_valid(MemoryRegion *mr,
> > + target_phys_addr_t addr,
> > + unsigned size)
> > +{
> > + if (!mr->ops->valid.unaligned&& (addr& (size - 1))) {
> > + return false;
> > + }
> > +
> > + /* Treat zero as compatibility all valid */
> > + if (!mr->ops->valid.max_access_size) {
> > + return true;
> > + }
> > +
> > + if (size> mr->ops->valid.max_access_size
> > + || size< mr->ops->valid.min_access_size) {
> > + return false;
> > + }
> > + return true;
> > +}
> > +
> > +static uint32_t memory_region_read_thunk_n(void *_mr,
> > + target_phys_addr_t addr,
> > + unsigned size)
> > +{
> > + MemoryRegion *mr = _mr;
> > + unsigned access_size, access_size_min, access_size_max;
> > + uint64_t access_mask;
> > + uint32_t data = 0, tmp;
> > + unsigned i;
> > +
> > + if (!memory_region_access_valid(mr, addr, size)) {
> > + return -1U; /* FIXME: better signalling */
> > + }
> > +
> > + /* FIXME: support unaligned access */
> > +
> > + access_size_min = mr->ops->impl.max_access_size;
>
> min = max: Intentional? Cut&paste error?

Bug; thanks.

> > diff --git a/memory.h b/memory.h
> > new file mode 100644
> > index 0000000..a67ff94
> > --- /dev/null
> > +++ b/memory.h
> > @@ -0,0 +1,201 @@
> > +#ifndef MEMORY_H
> > +#define MEMORY_H
> > +
> > +#ifndef CONFIG_USER_ONLY
>
> What's the story with this ifdef?
> There are no stubs provided ...

No callers either - I build with a full configuration. I prefer the
#ifdef here rather than all call sites.

> > +
> > +struct MemoryRegion {
> > + /* All fields are private - violators will be prosecuted */
> > + const MemoryRegionOps *ops;
> > + MemoryRegion *parent;
> > + uint64_t size;
> > + target_phys_addr_t addr;
> > + target_phys_addr_t offset;
> > + ram_addr_t ram_addr;
> > + bool has_ram_addr;
> > + MemoryRegion *alias;
> > + target_phys_addr_t alias_offset;
> > + unsigned priority;
> > + bool may_overlap;
> > + QTAILQ_HEAD(subregions, MemoryRegion) subregions;
> > + QTAILQ_ENTRY(MemoryRegion) subregions_link;
> > + QTAILQ_HEAD(coalesced_ranges, CoalescedMemoryRange) coalesced;
> > + const char *name;
>
> I'm never completely sure whether these should be target addresses
> or bus addresses or just uint64_t.
> With pci on a 32 bit system you can stick a 64 bit address
> in a BAR and the result will be that it is never accessed
> from the CPU.
>

I agree. Anyone objects to making the memory API 64-bit?

It will reduce performance slightly for 32-on-32, but these
configurations are getting rarer, and the performance loss is quite small.

Maybe we should make t_p_a_t 64-bit unconditionally. Note that sizes
have to be 64-bit in any case, otherwise you can't express a 4G range
without tricks.

--
error compiling committee.c: too many arguments to function