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

[Ian Jackson]

Anthony Liguori writes ("Re: [Qemu-devel] Re: [PATCH 1/5] Add target memory 
mapping API"):
> Ian Jackson wrote:
> > I'm afraid I disagree.
> 
> That's why I sent a note first instead of just committing :-)

Thanks.

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

Right.  I agree that this is a good and useful thing to have.

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

I hadn't thought about the dirty bitmap but of course you are right
there too.

> 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 r-m-w operations are much less practical, really.  If map
isn't available for some reason then a direct read-modify-write
interface will be almost impossible to emulate with
cpu_physical_memory_rw.  So I think we shouldn't provide one.

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

Yes.

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

No, at this point I'm just arguing in favour of a transfer length at
unmap.  That lack is the only thing about Avi's patches that I think
is a major blocker.


The question of callbacks is my next point:

> > The second criticism is a matter of taste, perhaps, and the third can
> > be addressed later:
> >
> >  * The mixed call/callback API: [...]
> >
> >    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.

I think my most recently sketched-out proposal (where the caller
passes a DMA sg list to the mapping request function) doesn't have
these deadlocks ?

It seems to me that the way to avoid the deadlock is to allow the
caller to make their whole request at once rather than having two
callers both racing to grab resources.

Regards,
Ian.