Re: [PATCH 2/5] iommu/amd: Implement DOMAIN_ATTR_GEOMETRY attribute

[Scott Wood <scottwood@freescale.com>]

On 01/30/2012 08:24 AM, Joerg Roedel wrote:
> On Fri, Jan 27, 2012 at 03:22:43PM -0600, Scott Wood wrote:
> 
>> OK, so there's a geometry that is read-only, and potentially a
>> driver-specific geometry that is read/write.  The default config for
>> PAMU would likely be a 1 MiB aperture in which the dma api can do
>> arbitrary 4k mappings -- this fits within the get generic geometry
>> operation.
> 
> Better: There is a read-only geometry for _all_ IOMMUs. Some IOMMUs may
> also allow to write the geometry, like PAMU.

But we do not want to use this struct for writing.  We do not want
force_aperture to be settable (we could error if it's set incorrectly,
but that's unpleasant -- and what happens if another attribute is added
in the future?  There would otherwise be no reason to do a get-geometry
first).  We do want to be able to set subwindows and would prefer (but
don't insist) that it be one atomic operation to set
start/end/subwindows.  For PAMU you really shouldn't be setting
start/end if you aren't going to set the subwindow count.  I don't see
what real benefit we get by reusing the generic geometry struct here --
we're not giving up any opportunities for common code.

>> Should generic get geometry return an error if the driver-specific
>> geometry has been set to something that doesn't fit within the generic
>> geometry model?
> 
> A domain can only have one geometry. So if you set a new geometry
> subsequent calls to get_attr will return the new geometry.

But the implementation may support geometries that are not expressable
with the generic struct.  As soon as you set an aperture larger than 1
MiB we cannot support arbitrary mappings within the aperture.  If we
return a generic geometry showing the larger aperture, that would
mislead generic users.

The geometry should be reset when the iommu is taken away from one user
(e.g. vfio) and given to another (e.g. dma api), so the dma api should
never see the error unless something has gone wrong (in which case it's
better to flag the error early).

>> I said a generic attribute (not GART specific) -- but if we're never
>> going to use the generic geometry struct for a set operation, bundling
>> it should be OK.
> 
> The generic struct should be used to set the geometry. But you can read
> out the old geometry and set force_aperture to the same value in the new
> geometry. Drivers should actually return -EINVAL when the user tries to
> set an unsupported value for force_aperture.

Wouldn't errors be less likely, and easier to diagnose, if
force_aperture weren't part of the struct in the first place?

If the idea is to keep attributes as granular as practical, it seems the
bundling goes against that.  If that isn't a goal, then I don't see the
problem with the PAMU geometry bundling start/end with subwindows.

>> No, at this point I'm just trying to follow the API development while
>> tending to other tasks.  I think Varun is working on the code for now.
> 
> Okay, maybe it is better to follow a 'release early, release often'
> model here. So we can work out the issues together.

I agree, and will keep prodding coworkers in this direction.

-Scott