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

[Jan Kiszka]

On 2011-05-18 15:12, 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.

Restoring is not the problem. The problem is that the current API
deletes or truncates regions implicitly by overwriting. That makes
tracking the layout hard, and it is also error-prone as the device that
accidentally overlaps with some other device won't receive a
notification of this potential conflict.

Such implicite truncation or deletion must be avoided in a new API,
forcing the users to explicitly reference an existing region when
dropping or modifying it. But your API goes in the right direction.

>    - 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

I would add another drawback:

 - Inability to identify the origin of a region accesses and handle them
   differently based on the source.

   That is at least problematic for the x86 APIC which is CPU local. Our
   current way do deal with it is, well, very creative and falls to
   dust if a guest actually tries to remap the APIC.

However, I'm unsure if that can easily be addressed. As long as only x86
is affected, it's tricky to ask for a big infrastructure to handle this
special case. Maybe there some other use cases, don't know.

> 
> 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);
> 
> struct MemoryRegionOps {
>     MemoryReadFunc readb, readw, readl;
>     MemoryWriteFunc writeb, writew, writel;
> };
> 
> struct MemoryRegion {
>     const MemoryRegionOps *ops;
>     target_phys_addr_t size;
>     target_phys_addr_t addr;
> };
> 
> void memory_region_init(MemoryRegion *mr,
>                         target_phys_addr_t size);

What use case would this abstract region cover?

> void memory_region_init_io(MemoryRegion *mr,
>                            const MemoryRegionOps *ops,
>                            target_phys_addr_t size);
> 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);
> 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);
> 
> void memory_region_add_subregion(MemoryRegion *mr,
>                                  target_phys_addr_t offset,
>                                  MemoryRegion *subregion);
> void memory_region_del_subregion(MemoryRegion *mr,
>                                  target_phys_addr_t offset,
>                                  MemoryRegion *subregion);
> 
> void cpu_register_memory_region(MemoryRegion *mr, target_phys_addr_t addr);

This could create overlaps. I would suggest to reject them, so we need a
return code.

> void cpu_unregister_memory_region(MemoryRegion *mr);
> 
> #endif
> 
> The API is nested: you can define, say, a PCI BAR containing RAM and
> MMIO, and give it to the PCI subsystem.  PCI can then enable/disable the
> BAR and move it to different addresses without calling any callbacks;
> the client code can enable or disable logging or coalescing without
> caring if the BAR is mapped or not.  For example:

Interesting feature.

> 
>   MemoryRegion mr, mr_mmio, mr_ram;
> 
>   memory_region_init(&mr);
>   memory_region_init_io(&mr_mmio, &mmio_ops, 0x1000);
>   memory_region_init_ram(&mr_ram, 0x100000);
>   memory_region_add_subregion(&mr, 0, &mr_ram);
>   memory_region_add_subregion(&mr, 0x10000, &mr_io);
>   memory_region_add_coalescing(&mr_ram, 0, 0x100000);
>   pci_register_bar(&pci_dev, 0, &mr);
> 
> at this point the PCI subsystem knows everything about the BAR and can
> enable or disable it, or move it around, without further help from the
> device code.  On the other hand, the device code can change logging or
> coalescing, or even change the structure of the region, without caring
> about whether the region is currently registered or not.
> 
> If we can agree on the API, then I think the way forward is to implement
> it in terms of the old API, change over all devices, then fold the old
> API into the new one.

There are more aspects that should be clarified before moving forward:
 - How to maintain memory regions internally?
 - Get rid of wasteful PhysPageDesc at this chance?
 - How to hook into the region maintenance (CPUPhysMemoryClient,
   listening vs. filtering or modifying changes)? How to simplify
   memory clients this way?

BTW, any old API should be removed ASAP once the new one demonstrated
its feasibility. IMHO, we can afford carrying yet another set of legacy
interface around.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux