Re: [Qemu-devel] [PATCH 07/13] usb: Convert usb_packet_{map, unmap} to universal DMA helpers

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Wed, 2012-06-20 at 08:25 +0200, Gerd Hoffmann wrote:
> Hi,
> 
> >> Like with any other device, it's hard ... what would happen on real
> >> hardware is that the USB controller will get a target abort, which will
> >> result in the controller reporting an error (typically in the PCI status
> >> register) and stopping.
> 
> Not that hard, code to cancel in-flight transactions is in place already
> as this can happen for other reasons too.

Ok, good,.

> > handle this.  However, the USB case should be ok - it may not be
> > theoretically guaranteed that the calls are close, but it's certainly
> > the case at the moment.
> 
> Depends on the device.  For the usb hid devices (which is the most
> important use case for power I think) packets will be processed
> synchronously, so there is no problem here.
> 
> usb-storage can keep packets in flight without holding the qemu lock
> (waiting for async block I/O finish).  Shouldn't be too long though.
> 
> usb-host keeps pretty much every packet in flight without holding the
> qemu lock as it passes on the requests to the hosts usbfs, then waits
> asynchronously for the request finish before returning the result to the
> guest.  Depending on the kind of device you are passing though this can
> be *very* long (minutes).

Right so with the initial patch series I sent, nothing will happen in
that we don't actually keep track of mappings, don't call the cancel
callback and anyways, OHCI/EHCI don't register a cancel callback.

As I wrote earlier, this is not very harmful so it's good to get merged,
and we can look into improving it and add the cancellation mechanism on
top. There was some original invalidation code from David that was
trying to wait on all pending maps but that had issues, Anthony wasn't
too happy about it, so I decided to attempt to submit/merge the patch
series without solving that issue.

To properly implement cancel without too much overhead, we need some
tracking of qemu maps and we need a quick way to know when the guest
invalidates a translation, if that translation has maps associated with
it.

The best way to do that, from my little experience messing around with
it, is going to essentially be implementation specific (ie depends on
the actual iommu backend).

For example, on TCEs, I could keep a parallel bitmap indicating when a
map is present for a given entry. That could be very efficient if I know
that there won't be more than one qemu map at a time for a given entry
though, so we should discuss whether that's an acceptable limitation.

There's also some "interesting" issues due to the fact that we populate
the TCE tables directly from KVM in "real mode" for speed, so that
bitmap would need to be some kind of shared memory with the kernel
(without locks !) and the kernel would have to be updated to fallback to
sending the invalidation hypercalls to qemu when it collides with a
populated map entry.

It's all doable, it's also a bit tricky, potentially quite a bit of
code, new KVM/qemu interfaces, etc... for a problem that's going to be a
non-issue pretty much 99.9% of the time :-) We still need to address it,
but I haven't convinced myself yet that I have come up with the best
solution :-)

Cheers,
Ben.