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

[Anthony Liguori <anthony@codemonkey.ws>]

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

That's why I sent a note first instead of just committing :-)

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

Okay, then let's ignore the last two for now.  A DMA API is really 
important for performance and I'd like to get something in the tree we 
can start working with.

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

I have a hard time disagree that it should take a transfer length, if 
for nothing else, to use to update the right amount of the dirty 
bitmap.  Avi, do you object to having unmap take a transfer length?

FWIW, I don't think we need to support RMW operations right now, but I 
think the transfer length is still required since you may get a partial 
IO result.  It may not be an important issue of correctness but it's 
still an issue of correctness.


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

map() can, and will, fail under certain conditions and the code has to 
accommodate that.

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

Are you arguing for "callback" based mapping?  The vast majority of 
devices will end up using a callback based approach so I'm not sure what 
you're unhappy about.

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

I understand the point you make here and I think we'll end up having a 
callback API.  But for reasons I've already mentioned, I don't think it 
makes sense for it to be the core API since it introduces very difficult 
to eliminate deadlocks.

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

It is.  Avi has promised to fix that :-)

Regards,

Anthony Liguori

> Ian.
>
>
>