Re: [Qemu-devel] [RESEND PATCH 2/6] memory: introduce AddressSpaceOps and IOMMUObject

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3990 bytes --]

On Fri, Jan 12, 2018 at 06:25:34PM +0800, Liu, Yi L wrote:
> On Wed, Dec 20, 2017 at 10:18:16PM +1100, David Gibson wrote:
> > On Wed, Dec 20, 2017 at 02:47:30PM +0800, Liu, Yi L wrote:
> > > On Mon, Dec 18, 2017 at 10:35:31PM +1100, David Gibson wrote:
> > > > On Wed, Nov 15, 2017 at 03:16:32PM +0800, Peter Xu wrote:
> > > > > On Tue, Nov 14, 2017 at 10:52:54PM +0100, Auger Eric wrote:
> > > > > 
> 
> [...]
> 
> Sorry for the delayed reply, spent some time on reconsidering your comments.
> 
> > 
> > I'm ok with calling it a "PASID context".
> > 
> > Thinking about this some more, here are some extra observations:
> > 
> >  * I think each device needs both a PASID context and an ordinary
> >    address space.  The PASID context would be used for bus
> >    transactions which include a process id, the address space for
> >    those that don't.
> > 
> >  * Theoretically, the PASID context could be modelled as an array/map
> >    of AddressSpace objects for each process ID.  However, creating all
> >    those AddressSpace objects in advance might be too expensive.  I
> >    can see a couple of options to avoid this:
> > 
> > 1) Have the PASID context class include a 'translate' method similar
> > to the one in IOMMUMemoryRegionClass, but taking a process ID as well
> > as an address.  This would avoid creating extra AddressSpace objects,
> > but might require duplicating a bunch of the translation code that
> > already exists for AddressSpace.
> > 
> > 2) "Lazily" create AddressSpace objects.  The generic part of the
> > PASID aware DMA helper functions would use a cache of AddressSpace's
> > for particular process IDs, using the AddressSpace (and MemoryRegion
> > within) to translate accesses for a particular process ID.  However,
> > these AddressSpace and MemoryRegion objects would only be created when
> > the device first accesses that address space.  In the common case,
> > where a single device is just being used by a single process or a
> > small number, this should keep the number of AddressSpace objects
> > relatively small.  Obviously the cache would need to be invalidated,
> > cleaning up the AddressSpace objects, when the PASID table is altered.
> 
> Sorry, a double check here. Does "AddressSpace objects" mean the existing
> AddressSpace definition in Qemu?

Yes.

> >  * I realize that the expected case here is with KVM, where the guest
> >    controls the first level translation, but the host controls the
> >    second level translation.  However, we should also be able to model
> >    the case where the guest controls both levels for the sake of full
> >    system emulation.  I think understanding this case will lead to a
> >    better design even for the simpler case.
> > 
> > Do you have a plan for what the virt-SVM aware DMA functions will look
> > like?
> 
> The behaviour is device specific.
> For a SVM capable physcial device, it would store the pasid value in a
> register locates in the deivce. e.g. a GPU context can be set to use SVM,
> after the pasid is set, any DMA from this context is DMAs target to a
> process virtual address space.

That doesn't sound any more device specific than any DMA operation,
and we have helpers for that.

> So for a virt-SVM aware DMA device, the device model needs to figure out
> the target address space. With the correct address space, then consume
> the translate() callback provided by iommu emulator. And then emulate the
> DMA operation for the emulated device.

Nearly all of that sounds like something that belongs in a helper
function.  Basically a varaint of dma_memory_rw() (and related
functions) that takes a PASID as well as an address.

> I'll try to get a new version with your suggestions.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]