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

[Avi Kivity <avi@redhat.com>]

Ian Jackson wrote:
> 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.
>   

Sure.

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

Correct.  If you need to perform read-modify-write, you need to use 
cpu_physical_memory_rw(), twice.  If we ever want to support RMW, we'll 
need to add another value for is_write.  I don't think we have 
interesting devices at this point which require efficient RMW.

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

Right.  Moreover, the access sizes will be swallowed by the API; devices 
are allowed to provide different implementations for different access sizes.

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

Alternatively, only use this interface with devices where this doesn't 
matter.  Given that bouncing happens for mmio only, this would be all 
devices which you'd want to use this interface with anyway.

(I'm assuming that you'll implement the fastpath by directly mapping 
guest memory, not bouncing).

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

I agree.  This was done at Anthony's request so I'll defer the response 
to him.

A variant of this API (posted by Andrea) hid all of the scheduling away 
within the implementation.

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

The downside to a separate handle is that device emulation code will now 
need to maintain the handle in addition to the the virtual address.  
Since the addresses will typically be maintained in an iovec, this means 
another array to be allocated and resized.

The design goals here were to keep things as simple as possible for the 
fast path.  Since the API fits all high-bandwidth devices that I know 
of, I don't think it's a good tradeoff to make the API more complex in 
order to be applicable to some corner cases.

Provided you implement the RAM case by mmap()ing, not bouncing, I think 
it will fit Xen quite well.  You might need to return NULL when the map 
cache is exhausted, but callers will not need to be modified.

>
> 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. */
>   

If we go in this direction the API should manage scatter/gather vectors, 
not single mappings.  This will remove the need for clients to store 
handles.

I suggest you look at Andrea's patchset for a very different approach.

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