Re: [Qemu-devel] [PATCH 1/5] Add target memory mapping API

[Ian Jackson]

Avi Kivity writes ("[Qemu-devel] [PATCH 1/5] Add target memory mapping API"):
> Devices accessing large amounts of memory (as with DMA) will wish to obtain
> a pointer to guest memory rather than access it indirectly via
> cpu_physical_memory_rw().  Add a new API to convert target addresses to
> host pointers.

In the Xen qemu tree we have alternative implementations of
cpu_physical_memory_{read,write}.  So we will need our own
implementation of this too.

So it is important to us that this interface is sufficient for its
callers (so that it doesn't need to change in the future), sufficient
for various different implementations (so that we can implement our
version), and correct in the sense that it is possible to write
correct code which uses it.

So as a result I have some comments about this API.  In summary I'm
afraid I think it needs a bit more work.  Perhaps we could have a
discussion about what this API should look like, by waving function
prototypes about (rather than complete patchsets) ?

Also there are some questions which your interface specification
leaves unanswered; these questions about the API semantics should
really be addressed in comments by the declaration.

> +void *cpu_physical_memory_map(target_phys_addr_t addr,
> +                              target_phys_addr_t *plen,
> +                              int is_write);

If the caller specifies is_write==1, are they entitled to read from
the buffer and expect it to contain something reasonable ?  In your
implementation, the answer appears to be `no'.

Is the caller allowed to assume that the accesses to the underlying
memory happen in any particular order - in particular, that they
happen in the same order as the caller's accesses to the buffer ?  Is
the caller allowed to assume that the same number of accesses to the
underlying memory happen as are made by the caller to the buffer ?
No, because if there is a bounce buffer the caller's accesses are
irrelevant.

What happens if the caller maps with is_write==1, writes part of the
buffer, and then unmaps ?  In your implementation undefined data is
written to guest memory.  I think that since not all callers can
guarantee to wish to write to the whole region, this means that the
interface is defective.  Callers need to be required to specify which
areas they have written.

The interface when cpu_physical_memory_map returns 0 is strange.
Normally everything in qemu is done with completion callbacks, but
here we have a kind of repeated polling.

This function should return a separate handle as well as the physical
memory pointer.  That will make it much easier to provide an
implementation which permits multiple bounce buffers or multiple
mappings simultaneously.


So I would prefer something like

  #define CPUPHYSMEMMAPFLAG_READABLE    001
  #define CPUPHYSMEMMAPFLAG_WRITABLE    002

  #define CPUPHYSMEMMAPFLAG_ALLOWFAIL   010
     /* If _ALLOWFAIL set then on callback, buffer may be NULL, in
      * which case caller should use cpu_physical_memory_{read,write} */

  typedef struct {
    /* to be filled in by caller before calling _map: */
    unsigned flags;
    target_phys_addr_t addr, len; /* len is updated by _map */
    /* filled in by _map: */
    void *buffer;
    /* private to _map et al: */
  } CpuPhysicalMemoryMapping;

  void cpu_physical_memory_map(CpuPhysicalMemoryMapping*;
      CpuPhysicalMemoryMapCallback *cb, void *cb_arg);
    /* There may be a limit on the number of concurrent maps
     * and the limit may be as low as one. */

  typedef void CpuPhysicalMemoryMapCallback(CpuPhysicalMemoryMapping*,
      void *cb_arg);

  void cpu_physical_memory_map_notify_read(CpuPhysicalMemoryMapping*,
      target_phys_addr_t addr, target_phys_addr_t *plen);
    /* must be called before read accesses to any buffer region */

  void cpu_physical_memory_map_notify_write(CpuPhysicalMemoryMapping*,
      target_phys_addr_t addr, target_phys_addr_t *plen);
    /* should be called after write accesses to any buffer region
     * unless it is OK for it to be indeterminate whether the
     * guest's memory actually gets written */

  void cpu_physical_memory_unmap(CpuPhysicalMemoryMapping*);
    /* Guest memory may be read and written out of order or even a
     * different number of times. */

Ian.