Re: [PATCH RFC] dma_rw.h (was Re: [PATCH 0/7] AMD IOMMU emulation patchset v4)

[Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro>]

On Thu, Sep 16, 2010 at 11:20:43AM +0200, Michael S. Tsirkin wrote:
> On Thu, Sep 16, 2010 at 10:06:16AM +0300, Eduard - Gabriel Munteanu wrote:

[snip]

> 
> No, DMADevice is a device that does DMA.
> So e.g. a PCI device would embed one.
> Remember, traslations are per device, right?
> DMAMmu is part of the iommu object.
> 

I agree, all I'm saying is a DMADevice is insufficient to invalidate one
of the many maps a given device holds, at least without resorting to
horrible tricks or destroying them all.

> > so doing this makes it really
> > hard to invalidate a specific map when there are more of them. It forces
> > device code to act as a bus, provide fake 'DMADevice's for each map and
> > dispatch translation to the real DMATranslateFunc. I see no other way.
> > 
> > If you really want more type-safety (although I think this is a case of
> > a true opaque identifying something only device code understands), I
> > have another proposal: have a DMAMap embedded in the opaque. Example
> > from dma-helpers.c:
> > 
> > typedef struct {
> > 	DMADevice *owner;
> > 	[...]
> > } DMAMap;
> > 
> > typedef struct {
> > 	[...]
> > 	DMAMap map;
> > 	[...]
> > } DMAAIOCB;
> > 
> > /* The callback. */
> > static void dma_bdrv_cancel(DMAMap *map)
> > {
> > 	DMAAIOCB *dbs = container_of(map, DMAAIOCB, map);
> > 
> > 	[...]
> > }
> > 
> > The upside is we only need to pass the DMAMap. That can also contain
> > details of the actual map in case the device wants to release only the
> > relevant range and remap the rest.
> 
> Fine.
> Or maybe DMAAIOCB (just make some letters lower case: DMAIocb?).
> Everyone will use it anyway, right?

No, you misunderstood me. DMAAIOCB is already there in dma-helpers.c,
it's the opaque I used and it was already there before my patches.

The idea was to define DMAMap and embed it in DMAAIOCB, so we can
upcast from the former to the latter (which is what we actually need).

IDE DMA uses that, but other devices would use whatever they want, even
nothing except the DMAMap. The only requirement is that the container
struct allows device code to acknowledge the invalidation (in case of
AIO, we simply kill that thread and release resources).

Well, perhaps DMAMap isn't the best name, but you get the idea.

> > > I saw devices do stl_le_phys and such, these
> > > might need to be wrapped as well.
> > 
> > stl_le_phys() is defined and used only by hw/eepro100.c. That's already
> > dealt with by converting the device.
> > 
> 
> I see. Need to get around to adding some prefix to it to make this clear.
> 
> > 	Thanks,
> > 	Eduard
> > 

[snip]


	Eduard