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

[Avi Kivity <avi@redhat.com>]

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