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

[Ian Jackson]

Anthony Liguori writes ("[Qemu-devel] Re: [PATCH 1/5] Add target memory mapping 
API"):
> I've gone though the thread again and I think this patch series is a 
> pretty good start.  I think the API can be refined a bit more down the 
> road to better support Xen but I think this is a good start.

I'm afraid I disagree.

> Does anyone have major blockers with this API?  If not, I'd like to 
> commit these patches.  The consumers of it should be small provided that 
> we make sure to write helpers for the various types of IO pipelines.

I have three criticisms, the first of which is in my view a major
blocker:

 * The unmap call should take a transfer length, for the reasons
   I have explained.

   This is absolutely critical for correctness if bounce buffers are
   used.

   I think it is important that the API permits implementations where
   the memory cannot be just mapped.  At the moment the APIs used by
   qemu device emulations do not assume that they can access RAM
   willy-nilly; they are all expected to go through
   cpu_physical_memory_rw.  I think it is important to preserve this
   for the future flexibility of the qemu codebase.

   Note that Xen is not such an implementation, although in the Xen
   case the additional performance of mapping is not so important so
   we might not choose to implement the mapping right away.  Obviously
   it would be much easier for us just to implement this mapping than
   to argue at length on this list.  It's not hard for Xen.  So I'm
   not trying to special-case Xen here.  You shouldn't read my
   criticisms as some kind of Xen fanatic trying to defend some crazy
   Xen behaviour.

   I'm trying to preserve an important and useful property of the
   internal qemu API.  My suggestion will mean the device emulation
   parts of qemu continue to be reasonably easily useable separately
   from the rest of qemu, and possibly very separately from any
   emulation of CPU or memory.  The benefit is difficult to evaluate
   exactly but the cost is very small.

Of my criticisms, I think this is by far the most important.  It
relates to the correctness of the API and in my view it is essential
that this be properly addressed.

Avi will say again (as if repetition made things true) that `bounce
buffers are never used' but of course they will be sometimes.  If they
were never used his patch wouldn't need to have that code in them.
It's true that they will be _rarely_ used but a lack of correctness in
a rare case is not acceptable either.

The second criticism is a matter of taste, perhaps, and the third can
be addressed later:

 * The mixed call/callback API: DMAs which can be mappped immediately
   are treated as synchronous calls; ones which can't involve a
   separate callback.

   This is no different semantically from a pure-callback API.
   However, it makes life more clumsy for the caller - the caller now
   needs two code paths: one for immediate success, and one for
   deferred allocation.

   Callbacks are increasingly dominating the innards of qemu for good
   reasons.  I think I have explained how a callback style can provide
   the necessary functionality.

 * The documentation, in the form of comments describing the
   semantics of and restrictions on the use of the DMA mapping,
   is inadequate.

Ian.