Re: [Qemu-devel] [RFC] Memory API

[Avi Kivity]

On 05/18/2011 06:08 PM, Anthony Liguori wrote:
> On 05/18/2011 08:12 AM, 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.
> > - 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
> >
> > 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);
>
>
> The API should be 64-bit and needs to have a size associated with it.

That will make conversion hard.  Maybe we can have a native with-size 
callback, and a helper that converts to the traditional form.  Otherwise 
the effort to fully covert the tree increases dramatically.

> > void memory_region_init(MemoryRegion *mr,
> > target_phys_addr_t size);
>
> What is this used for?  It's not clear to me.

Empty container, fill with memory_region_add_subregion() to taste.

> > void memory_region_init_io(MemoryRegion *mr,
> > const MemoryRegionOps *ops,
> > target_phys_addr_t size);
>
> How does one test a MemoryRegion to determine it's type?

One doesn't.  Of course the low-level will need to do this (via a 
private API that isn't exposed to devices).

> > 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);
>
> What's "offset" mean?

It's the equivalent of cpu_register_physical_memory_offset().

Note the intent is to have addresses always be relative to the innermost 
container.  Perhaps we can get away without 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);
>
> I don't think it's worth while to try to fit coalescing into this API. 
> It's a KVM specific hack.  I think it's fine to be a hacked on API.

The problem is that only the device knows about coalescing, while the 
region can be mapped, unmapped, or moved without device knowledge.  So 
if a PCI is unmapped and then remapped (possibly at a different address) 
we need to tell kvm about it, but there is no device callback involved.

> I think it pretty much makes sense to me.  It might be worth while to 
> allow memory region definitions to be static.  For instance:
>
> MemoryRegion e1000_bar = { ... };

It doesn't work in general, since multiple instances of e1000 will each 
need its own e1000_bar.  So e1000_bar will be a member of the e1000 
device state structure.

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