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

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

[Sethi Varun-B16395]

> -----Original Message-----
> From: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org [mailto:iommu-
> bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org] On Behalf Of Joerg Roedel
> Sent: Thursday, January 19, 2012 8:00 PM
> To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Cc: Ohad Ben-Cohen; Tony Lindgren; Hiroshi DOYU; linux-
> kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Laurent Pinchart; Wood Scott-B07421; David Brown;
> David Woodhouse
> Subject: [PATCH 2/5] iommu/amd: Implement DOMAIN_ATTR_GEOMETRY attribute
> 
> Implement the attribute itself and add the code for the AMD IOMMU driver.
> 
> Signed-off-by: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
> ---
>  drivers/iommu/amd_iommu.c |    4 ++++
>  drivers/iommu/iommu.c     |   19 ++++++++++++++++---
>  include/linux/iommu.h     |   11 ++++++++++-
>  3 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c index
> cce1f03..421c3e6 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -3030,6 +3030,10 @@ static int amd_iommu_domain_init(struct
> iommu_domain *dom)
> 
>  	dom->priv = domain;
> 
> +	dom->geometry.aperture_start = 0;
> +	dom->geometry.aperture_end   = ~0ULL;
> +	dom->geometry.force_aperture = true;
> +
>  	return 0;
> 
>  out_free:
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index
> ef54718..3d0b0bf 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -345,10 +345,23 @@ EXPORT_SYMBOL_GPL(iommu_device_group);
>  int iommu_domain_get_attr(struct iommu_domain *domain,
>  			  enum iommu_attr attr, void *data)
>  {
> -	if (!domain->ops->domain_get_attr)
> -		return -EINVAL;
> +	struct iommu_domain_geometry *geometry;
> +	int ret = 0;
> +
> +	switch (attr) {
> +	case DOMAIN_ATTR_GEOMETRY:
> +		geometry  = data;
> +		*geometry = domain->geometry;
> +
> +		break;
> +	default:
> +		if (!domain->ops->domain_get_attr)
> +			return -EINVAL;
> 
> -	return domain->ops->domain_get_attr(domain, attr, data);
> +		ret = domain->ops->domain_get_attr(domain, attr, data);
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(iommu_domain_get_attr);
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h index
> 32d0de1..3f971b3 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -37,16 +37,24 @@ struct iommu_domain;  typedef int
> (*iommu_fault_handler_t)(struct iommu_domain *,
>  				struct device *, unsigned long, int);
> 
> +struct iommu_domain_geometry {
> +	u64 aperture_start;	/* First address that can be mapped    */
> +	u64 aperture_end;	/* Last address that can be mapped     */
> +	bool force_aperture;	/* DMA only allowed in mappable range? */
> +};
> +
>  struct iommu_domain {
>  	struct iommu_ops *ops;
>  	void *priv;
>  	iommu_fault_handler_t handler;
> +	struct iommu_domain_geometry geometry;
>  };
In case of our iommu implementation the iommu_domain_geometry would include additional attributes. Why can't we let the geometry be
Implementation dependent?

-Varun

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

[Joerg Roedel]

On Thu, Jan 19, 2012 at 05:16:48PM +0000, Sethi Varun-B16395 wrote:
> >  struct iommu_domain {
> >  	struct iommu_ops *ops;
> >  	void *priv;
> >  	iommu_fault_handler_t handler;
> > +	struct iommu_domain_geometry geometry;
> >  };
> In case of our iommu implementation the iommu_domain_geometry would
> include additional attributes. Why can't we let the geometry be
> Implementation dependent?

Because the values in this generic geometry-struct make sense for more
than one IOMMU (short-term for Tegra GART and Freescale PAMU). If you
have additional vendor-specific ones you could add them via a
vendor-specific attribute.

Regards,

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

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

[Scott Wood]

On 01/20/2012 10:03 AM, Joerg Roedel wrote:
> On Thu, Jan 19, 2012 at 05:16:48PM +0000, Sethi Varun-B16395 wrote:
>>>  struct iommu_domain {
>>>  	struct iommu_ops *ops;
>>>  	void *priv;
>>>  	iommu_fault_handler_t handler;
>>> +	struct iommu_domain_geometry geometry;
>>>  };
>> In case of our iommu implementation the iommu_domain_geometry would
>> include additional attributes. Why can't we let the geometry be
>> Implementation dependent?
> 
> Because the values in this generic geometry-struct make sense for more
> than one IOMMU (short-term for Tegra GART and Freescale PAMU). If you
> have additional vendor-specific ones you could add them via a
> vendor-specific attribute.

Freescale PAMU is the IOMMU that Varun is talking about, that needs
additional geometry attributes (in particular, subwindow count).

How should a PAMU driver interpret "force_aperture"?  When would DMA
ever be allowed outside the specified range?  What does the range mean
in that case?

-Scott

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

[Joerg Roedel]

On Thu, Jan 26, 2012 at 12:25:32PM -0600, Scott Wood wrote:
> On 01/20/2012 10:03 AM, Joerg Roedel wrote:
> > Because the values in this generic geometry-struct make sense for more
> > than one IOMMU (short-term for Tegra GART and Freescale PAMU). If you
> > have additional vendor-specific ones you could add them via a
> > vendor-specific attribute.
> 
> Freescale PAMU is the IOMMU that Varun is talking about, that needs
> additional geometry attributes (in particular, subwindow count).

This can be solved by a PAMU specific attribute.

> How should a PAMU driver interpret "force_aperture"?  When would DMA
> ever be allowed outside the specified range?  What does the range mean
> in that case?

The force_aperture flag indicated whether DMA is only allowed between
aperture_start and apertuer_end or if DMA is allowed outside of this
range too (unmapped in this case).

The AMD GART for example would set this flag to false because it does
not enforce DMA to be in the aperture-range.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

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

[Scott Wood]

On 01/26/2012 12:31 PM, Joerg Roedel wrote:
> On Thu, Jan 26, 2012 at 12:25:32PM -0600, Scott Wood wrote:
>> How should a PAMU driver interpret "force_aperture"?  When would DMA
>> ever be allowed outside the specified range?  What does the range mean
>> in that case?
> 
> The force_aperture flag indicated whether DMA is only allowed between
> aperture_start and apertuer_end or if DMA is allowed outside of this
> range too (unmapped in this case).
> 
> The AMD GART for example would set this flag to false because it does
> not enforce DMA to be in the aperture-range.

Why is this not an AMD GART specific attribute?  Is there any feature
reporting mechanism by which a user would know if that flag is supported?

If it must be in the generic struct, it would be nice to invert the
polarity so that the default (after zeroing) is something that should be
more widely supportable, and less likely to create unintended identity
mappings.

How in general are available attributes and restrictions on possible
values to be communicated to users of the API?

-Scott

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

[Joerg Roedel]

On Thu, Jan 26, 2012 at 12:42:10PM -0600, Scott Wood wrote:
> On 01/26/2012 12:31 PM, Joerg Roedel wrote:
> > The force_aperture flag indicated whether DMA is only allowed between
> > aperture_start and apertuer_end or if DMA is allowed outside of this
> > range too (unmapped in this case).
> > 
> > The AMD GART for example would set this flag to false because it does
> > not enforce DMA to be in the aperture-range.
> 
> Why is this not an AMD GART specific attribute?  Is there any feature
> reporting mechanism by which a user would know if that flag is supported?

Because this is a flag that makes sense for all IOMMU. Every IOMMU
either allows DMA outside its aperture or it doesn't.

Another reason why it must be in the generic struct is the intended
generic dma-ops layer on-top. This code can decide on this flag wheter a
address needs to be remapped at all.

> If it must be in the generic struct, it would be nice to invert the
> polarity so that the default (after zeroing) is something that should be
> more widely supportable, and less likely to create unintended identity
> mappings.

Setting this flag wrong does not create unintended identity mappings.

> How in general are available attributes and restrictions on possible
> values to be communicated to users of the API?

The possible attributes can be found in inlude/linux/iommu.h. But I
don't understand what you mean by 'restrictions on possible values'. The
geometry attribute is filled by the IOMMU driver dependent on the
hardware capabilities. There are no limits from the iommu-code side.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

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

[Scott Wood]

On 01/26/2012 12:51 PM, Joerg Roedel wrote:
> On Thu, Jan 26, 2012 at 12:42:10PM -0600, Scott Wood wrote:
>> On 01/26/2012 12:31 PM, Joerg Roedel wrote:
>>> The force_aperture flag indicated whether DMA is only allowed between
>>> aperture_start and apertuer_end or if DMA is allowed outside of this
>>> range too (unmapped in this case).
>>>
>>> The AMD GART for example would set this flag to false because it does
>>> not enforce DMA to be in the aperture-range.
>>
>> Why is this not an AMD GART specific attribute?  Is there any feature
>> reporting mechanism by which a user would know if that flag is supported?
> 
> Because this is a flag that makes sense for all IOMMU. Every IOMMU
> either allows DMA outside its aperture or it doesn't.
> 
> Another reason why it must be in the generic struct is the intended
> generic dma-ops layer on-top. This code can decide on this flag wheter a
> address needs to be remapped at all.

So the DMA API would just read this, not write it?

Still no reason why it couldn't be a separate attribute.  Then if you
get a failure trying to write it, it's more obvious why.

>> If it must be in the generic struct, it would be nice to invert the
>> polarity so that the default (after zeroing) is something that should be
>> more widely supportable, and less likely to create unintended identity
>> mappings.
> 
> Setting this flag wrong does not create unintended identity mappings.

Failing to set it means that DMA can go through that is not limited to
explicitly created mappings.  In some contexts (e.g. vfio) this is a
security hole.

>> How in general are available attributes and restrictions on possible
>> values to be communicated to users of the API?
> 
> The possible attributes can be found in inlude/linux/iommu.h.

I meant possible for the currently running hardware.

> But I don't understand what you mean by 'restrictions on possible values'. The
> geometry attribute is filled by the IOMMU driver dependent on the
> hardware capabilities. There are no limits from the iommu-code side.

How does the user of the iommu API discover the hardware capabilities?

-Scott

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

[Joerg Roedel]

On Thu, Jan 26, 2012 at 01:00:37PM -0600, Scott Wood wrote:
> On 01/26/2012 12:51 PM, Joerg Roedel wrote:
> > Because this is a flag that makes sense for all IOMMU. Every IOMMU
> > either allows DMA outside its aperture or it doesn't.
> > 
> > Another reason why it must be in the generic struct is the intended
> > generic dma-ops layer on-top. This code can decide on this flag wheter a
> > address needs to be remapped at all.
> 
> So the DMA API would just read this, not write it?

The whole geometry thing is only implemented on the read side. There is
no implementation in domain_set_attr for it. So the geometry
information is read-only by default.

> Still no reason why it couldn't be a separate attribute.  Then if you
> get a failure trying to write it, it's more obvious why.

This would mean iommu specific hacks, which are not necessary in this
case.

> > Setting this flag wrong does not create unintended identity mappings.
> 
> Failing to set it means that DMA can go through that is not limited to
> explicitly created mappings.  In some contexts (e.g. vfio) this is a
> security hole.

No, when the hardware does not allow this, then software can't enforce
it. Again, the whole geometry attribute is only for iommu drivers to
export what the hardware can do. It is not for software to configure the
iommu driver.

> > But I don't understand what you mean by 'restrictions on possible values'. The
> > geometry attribute is filled by the IOMMU driver dependent on the
> > hardware capabilities. There are no limits from the iommu-code side.
> 
> How does the user of the iommu API discover the hardware capabilities?

Which hardware capabilities besides the geometry do you mean?


	Joerg

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

[Scott Wood]

On 01/26/2012 01:44 PM, Joerg Roedel wrote:
> On Thu, Jan 26, 2012 at 01:00:37PM -0600, Scott Wood wrote:
>> On 01/26/2012 12:51 PM, Joerg Roedel wrote:
>>> Because this is a flag that makes sense for all IOMMU. Every IOMMU
>>> either allows DMA outside its aperture or it doesn't.
>>>
>>> Another reason why it must be in the generic struct is the intended
>>> generic dma-ops layer on-top. This code can decide on this flag wheter a
>>> address needs to be remapped at all.
>>
>> So the DMA API would just read this, not write it?
> 
> The whole geometry thing is only implemented on the read side. There is
> no implementation in domain_set_attr for it. So the geometry
> information is read-only by default.

We will need to be able to set this, for some vfio+kvm uses.

>> Still no reason why it couldn't be a separate attribute.  Then if you
>> get a failure trying to write it, it's more obvious why.
> 
> This would mean iommu specific hacks, which are not necessary in this
> case.

Why would making it a separate generic attribute require iommu specific
hacks?

>>> Setting this flag wrong does not create unintended identity mappings.
>>
>> Failing to set it means that DMA can go through that is not limited to
>> explicitly created mappings.  In some contexts (e.g. vfio) this is a
>> security hole.
> 
> No, when the hardware does not allow this, then software can't enforce
> it.

OK, it looked like an option that could be enabled or disabled --
because I didn't realize you were considering geometry as read-only.

> Again, the whole geometry attribute is only for iommu drivers to
> export what the hardware can do. It is not for software to configure the
> iommu driver.

So how would a vfio user set up the iommu so a kvm guest sees iova ==
guest physical, for at least its main memory, if the default aperture
doesn't cover it?

We also will gain performance by being able to set custom geometry,
because we will not be as hard on the IOMMU cache if we can use fewer,
larger subwindows.

It's not configuring the iommu driver, just this particular domain.

>>> But I don't understand what you mean by 'restrictions on possible values'. The
>>> geometry attribute is filled by the IOMMU driver dependent on the
>>> hardware capabilities. There are no limits from the iommu-code side.
>>
>> How does the user of the iommu API discover the hardware capabilities?
> 
> Which hardware capabilities besides the geometry do you mean?

Well, we have things like stash target (automatic cache prefetch after
DMA) configuration, but in this case I was thinking about restrictions
on what kind of aperture you can set, and what sort of mappings you can
create with the result.

On current Freescale PAMUs, the aperture must be a size-aligned
power-of-two region, which is carved into up to 256 equal-size
subwindows.  A mapping can be any power of 2 up to the subwindow size,
but its iova must be the subwindow start address.

We cannot just say "oh look, an aperture from here to there, we can
create all the mappings we want within that space", at least not with an
aperture over 1 MiB (256 contiguous 4k subwindows).

-Scott

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

[Joerg Roedel]

On Thu, Jan 26, 2012 at 02:02:26PM -0600, Scott Wood wrote:
> On 01/26/2012 01:44 PM, Joerg Roedel wrote:

> >>> Another reason why it must be in the generic struct is the intended
> >>> generic dma-ops layer on-top. This code can decide on this flag wheter a
> >>> address needs to be remapped at all.
> >>
> >> So the DMA API would just read this, not write it?

Yes, the dma-ops code needs this information to decide whether remapping
is required at all and where the remap window is.

> > The whole geometry thing is only implemented on the read side. There is
> > no implementation in domain_set_attr for it. So the geometry
> > information is read-only by default.
> 
> We will need to be able to set this, for some vfio+kvm uses.

That's fine. Just implement a handler for it in the driver-specific
set_attr callback then. 

> >> Still no reason why it couldn't be a separate attribute.  Then if you
> >> get a failure trying to write it, it's more obvious why.
> > 
> > This would mean iommu specific hacks, which are not necessary in this
> > case.
> 
> Why would making it a separate generic attribute require iommu specific
> hacks?

Because the dma-ops code needs something like

	iommu_domain_get_attr(domain, GART_ATTR_FORCE_APERTURE, data);

to check it. I call this a hack because the dma-ops code asks if it runs
on a specific hardware. This is not necessary here.

> 
> >>> Setting this flag wrong does not create unintended identity mappings.
> >>
> >> Failing to set it means that DMA can go through that is not limited to
> >> explicitly created mappings.  In some contexts (e.g. vfio) this is a
> >> security hole.
> > 
> > No, when the hardware does not allow this, then software can't enforce
> > it.
> 
> OK, it looked like an option that could be enabled or disabled --
> because I didn't realize you were considering geometry as read-only.

It is indeed read-only for most iommus and the dma-ops code only needs
to read this information. And it is necessary for the iommu-api user to
get this information when we want to support iommus that have a limited
remapping-window for each domain.

> So how would a vfio user set up the iommu so a kvm guest sees iova ==
> guest physical, for at least its main memory, if the default aperture
> doesn't cover it?
> 
> We also will gain performance by being able to set custom geometry,
> because we will not be as hard on the IOMMU cache if we can use fewer,
> larger subwindows.

No problem if you also implement the write-side for your implementation.

> > Which hardware capabilities besides the geometry do you mean?
> 
> Well, we have things like stash target (automatic cache prefetch after
> DMA) configuration, but in this case I was thinking about restrictions
> on what kind of aperture you can set, and what sort of mappings you can
> create with the result.

The stash target is a perfect fit for a PAMU specific domain attribute.

> On current Freescale PAMUs, the aperture must be a size-aligned
> power-of-two region, which is carved into up to 256 equal-size
> subwindows.  A mapping can be any power of 2 up to the subwindow size,
> but its iova must be the subwindow start address.
> 
> We cannot just say "oh look, an aperture from here to there, we can
> create all the mappings we want within that space", at least not with an
> aperture over 1 MiB (256 contiguous 4k subwindows).

Yes, we talked about that already. Probably we should talk about code to
make progress here. Do you have anything ready to post?


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

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

[Scott Wood]

On 01/27/2012 05:01 AM, Joerg Roedel wrote:
> On Thu, Jan 26, 2012 at 02:02:26PM -0600, Scott Wood wrote:
>> On 01/26/2012 01:44 PM, Joerg Roedel wrote:
> 
>>>>> Another reason why it must be in the generic struct is the intended
>>>>> generic dma-ops layer on-top. This code can decide on this flag wheter a
>>>>> address needs to be remapped at all.
>>>>
>>>> So the DMA API would just read this, not write it?
> 
> Yes, the dma-ops code needs this information to decide whether remapping
> is required at all and where the remap window is.
> 
>>> The whole geometry thing is only implemented on the read side. There is
>>> no implementation in domain_set_attr for it. So the geometry
>>> information is read-only by default.
>>
>> We will need to be able to set this, for some vfio+kvm uses.
> 
> That's fine. Just implement a handler for it in the driver-specific
> set_attr callback then. 

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.

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?

>>>> Still no reason why it couldn't be a separate attribute.  Then if you
>>>> get a failure trying to write it, it's more obvious why.
>>>
>>> This would mean iommu specific hacks, which are not necessary in this
>>> case.
>>
>> Why would making it a separate generic attribute require iommu specific
>> hacks?
> 
> Because the dma-ops code needs something like
> 
> 	iommu_domain_get_attr(domain, GART_ATTR_FORCE_APERTURE, data);
> 
> to check it. I call this a hack because the dma-ops code asks if it runs
> on a specific hardware. This is not necessary here.

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.

>>> Which hardware capabilities besides the geometry do you mean?
>>
>> Well, we have things like stash target (automatic cache prefetch after
>> DMA) configuration, but in this case I was thinking about restrictions
>> on what kind of aperture you can set, and what sort of mappings you can
>> create with the result.
> 
> The stash target is a perfect fit for a PAMU specific domain attribute.

Yes.

> Yes, we talked about that already. Probably we should talk about code to
> make progress here. Do you have anything ready to post?

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.

-Scott

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

[Joerg Roedel]

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.

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

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

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


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

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

[Scott Wood]

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

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

[Sethi Varun-B16395]

> -----Original Message-----
> From: Joerg Roedel [mailto:joerg.roedel-5C7GfCeVMHo@public.gmane.org]
> Sent: Friday, January 27, 2012 12:21 AM
> To: Wood Scott-B07421
> Cc: Sethi Varun-B16395; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Ohad Ben-Cohen;
> Tony Lindgren; Hiroshi DOYU; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Laurent
> Pinchart; Wood Scott-B07421; David Brown; David Woodhouse
> Subject: Re: [PATCH 2/5] iommu/amd: Implement DOMAIN_ATTR_GEOMETRY
> attribute
> 
> On Thu, Jan 26, 2012 at 12:42:10PM -0600, Scott Wood wrote:
> > On 01/26/2012 12:31 PM, Joerg Roedel wrote:
> > > The force_aperture flag indicated whether DMA is only allowed
> > > between aperture_start and apertuer_end or if DMA is allowed outside
> > > of this range too (unmapped in this case).
> > >
> > > The AMD GART for example would set this flag to false because it
> > > does not enforce DMA to be in the aperture-range.
> >
> > Why is this not an AMD GART specific attribute?  Is there any feature
> > reporting mechanism by which a user would know if that flag is
> supported?
> 
> Because this is a flag that makes sense for all IOMMU. Every IOMMU either
> allows DMA outside its aperture or it doesn't.
> 
> Another reason why it must be in the generic struct is the intended
> generic dma-ops layer on-top. This code can decide on this flag wheter a
> address needs to be remapped at all.


Can you please explain how would IOMMU remapping be used, based on the force_aperture flag.

-Varun 

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

[Joerg Roedel]

On Mon, Jan 30, 2012 at 06:27:30AM +0000, Sethi Varun-B16395 wrote:

> Can you please explain how would IOMMU remapping be used, based on the
> force_aperture flag.

If force_aperture is flase the dma-api code needs to check if remapping
is required at all. It performs the following checks:

	* Does the physical address of the buffer not overlap with the
	  aperture?
	* Fits the buffer into the device's dma address mask?

If both questions are answered with yes no remapping is required and
dma-api just returns the physical address. Otherwise it calls into the
dma-api to create a mapping.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 0/5] IOMMU: Make IOMMU-API ready for GART-like hardware

[Hiroshi Doyu]

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

Hi Joerg,

From: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
Subject: [PATCH 0/5] IOMMU: Make IOMMU-API ready for GART-like hardware
Date: Thu, 19 Jan 2012 15:30:00 +0100
Message-ID: <1326983405-319-1-git-send-email-joerg.roedel-5C7GfCeVMHo@public.gmane.org>

> Hi,
> 
> this patch-set adds two new functions the the IOMMU-API:
> 
> 	* iommu_domain_set_attr()
> 	* iommu_domain_get_attr()
> 
> These are generic functions to set and change attributes of a domain.
> The plan is to have a set of generic attributes implemented by more than
> one IOMMU driver as well as driver specific ones. As an example for the
> first type this patch-set also implements the DOMAIN_ATTR_GEOMETRY
> attribute. It can be used to query information about the address space
> window that can be remapped by the iommu hardware. With this attribute
> is is very easy to support IOMMUs like GART and friends.

For Tegra GART & SMMU, this info is passed from board files as
resource data via "platform_get_resource(,IORESOURCE_MEM,)". I think
that this could be the attachement patch in Tegra case. The attached
patches are based on the following post.

https://lkml.org/lkml/2012/1/5/26
https://lkml.org/lkml/2012/1/5/27

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-iommu-tegra-gart-Implement-DOMAIN_ATTR_GEOMETRY-attr.patch --]
[-- Type: text/x-patch; name="0001-iommu-tegra-gart-Implement-DOMAIN_ATTR_GEOMETRY-attr.patch", Size: 1059 bytes --]

From 82d0f808a6ab0b8ce3837945f53d9ef042b5dacd Mon Sep 17 00:00:00 2001
From: Hiroshi DOYU <hdoyu@nvidia.com>
Date: Fri, 20 Jan 2012 07:56:07 +0200
Subject: [PATCH 1/2] iommu/tegra/gart: Implement DOMAIN_ATTR_GEOMETRY
 attribute

Implement the attribute for the Tegra GART driver.

Signed-off-by: Hiroshi DOYU <hdoyu@nvidia.com>
---
 drivers/iommu/tegra-gart.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index feaccd4..e237b08 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -181,6 +181,12 @@ static int gart_iommu_attach_dev(struct iommu_domain *domain,
 	if (!gart)
 		return -EINVAL;
 	domain->priv = gart;
+
+	domain->geometry.aperture_start = gart->iovmm_base;
+	domain->geometry.aperture_end   = gart->iovmm_base +
+		gart->page_count * GART_PAGE_SIZE - 1;
+	domain->geometry.force_aperture = true;
+
 	client = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
 	if (!client)
 		return -ENOMEM;
-- 
1.7.5.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-iommu-tegra-smmu-Implement-DOMAIN_ATTR_GEOMETRY-attr.patch --]
[-- Type: text/x-patch; name="0002-iommu-tegra-smmu-Implement-DOMAIN_ATTR_GEOMETRY-attr.patch", Size: 984 bytes --]

From 04388853213b89eae29c9138eb53359aba6de18e Mon Sep 17 00:00:00 2001
From: Hiroshi DOYU <hdoyu@nvidia.com>
Date: Fri, 20 Jan 2012 08:02:54 +0200
Subject: [PATCH 2/2] iommu/tegra/smmu: Implement DOMAIN_ATTR_GEOMETRY
 attribute

Implement the attribute for the Tegra SMMU driver.

Signed-off-by: Hiroshi DOYU <hdoyu@nvidia.com>
---
 drivers/iommu/tegra-smmu.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 7deea32..84504ff 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -723,6 +723,12 @@ found:
 
 	spin_unlock_irqrestore(&as->lock, flags);
 	domain->priv = as;
+
+	domain->geometry.aperture_start = smmu->iovmm_base;
+	domain->geometry.aperture_end   = smmu->iovmm_base +
+		smmu->page_count * SMMU_PAGE_SIZE - 1;
+	domain->geometry.force_aperture = true;
+
 	dev_dbg(smmu->dev, "smmu_as@%p\n", as);
 	return 0;
 
-- 
1.7.5.4


[-- Attachment #4: Type: text/plain, Size: 190 bytes --]

_______________________________________________
iommu mailing list
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/5] IOMMU: Make IOMMU-API ready for GART-like hardware

[joerg.roedel-5C7GfCeVMHo]

Hi,

On Fri, Jan 20, 2012 at 07:14:03AM +0100, Hiroshi Doyu wrote:
> From: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
> Subject: [PATCH 0/5] IOMMU: Make IOMMU-API ready for GART-like hardware
> Date: Thu, 19 Jan 2012 15:30:00 +0100
> Message-ID: <1326983405-319-1-git-send-email-joerg.roedel-5C7GfCeVMHo@public.gmane.org>
> 
> > Hi,
> > 
> > this patch-set adds two new functions the the IOMMU-API:
> > 
> > 	* iommu_domain_set_attr()
> > 	* iommu_domain_get_attr()
> > 
> > These are generic functions to set and change attributes of a domain.
> > The plan is to have a set of generic attributes implemented by more than
> > one IOMMU driver as well as driver specific ones. As an example for the
> > first type this patch-set also implements the DOMAIN_ATTR_GEOMETRY
> > attribute. It can be used to query information about the address space
> > window that can be remapped by the iommu hardware. With this attribute
> > is is very easy to support IOMMUs like GART and friends.
> 
> For Tegra GART & SMMU, this info is passed from board files as
> resource data via "platform_get_resource(,IORESOURCE_MEM,)". I think
> that this could be the attachement patch in Tegra case. The attached
> patches are based on the following post.
> 
> https://lkml.org/lkml/2012/1/5/26
> https://lkml.org/lkml/2012/1/5/27

I will start merging new patches next week. These patches are on my
list. Once we have settled on a version of the patches in this thread I
also apply your posted addtion.


	Joerg



-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

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

[Hiroshi Doyu]

From: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
Subject: Re: [PATCH 2/5] iommu/amd: Implement DOMAIN_ATTR_GEOMETRY attribute
Date: Thu, 19 Jan 2012 17:27:09 +0100
Message-ID: <201201191727.10176.laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

> Hi Joerg,
> 
> On Thursday 19 January 2012 17:07:39 Joerg Roedel wrote:
> > On Thu, Jan 19, 2012 at 04:46:13PM +0100, Laurent Pinchart wrote:
> > > > +struct iommu_domain_geometry {
> > > > +	u64 aperture_start;	/* First address that can be mapped    */
> > > > +	u64 aperture_end;	/* Last address that can be mapped     */
> > > 
> > > Would it make sense to use a platform-dependent type that represents
> > > physical addresses instead of u64 ?
> > 
> > Well, u64 is a catch-all datatype which should suit all drivers. Is this
> > a problem at ARM?
> 
> No, it's not a problem for ARM, at least to my knowledge. It just struck me as 
> weird, as we have specific data types for other kinds of addresses (such as 
> dma_addr_t).
> 
> > We can also go the more complicated way of introducing an iova_addr_t or
> > something. But if possible I would like to avoid that.

"dma_addr_t" sounds nicer(and makes still sense) to me than a new "iova_addr_t".

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

[Joerg Roedel]

On Thu, Jan 19, 2012 at 05:27:09PM +0100, Laurent Pinchart wrote:
> > Well, u64 is a catch-all datatype which should suit all drivers. Is this
> > a problem at ARM?
> 
> No, it's not a problem for ARM, at least to my knowledge. It just struck me as 
> weird, as we have specific data types for other kinds of addresses (such as 
> dma_addr_t).

Okay, I looked at the definition and dma_addr_t seems like a good fit.
To be consistent we should change the iova type of the map/unmap
functions to dma_addr_t too.

Regards,

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

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

[Sethi Varun-B16395]

> -----Original Message-----
> From: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org [mailto:iommu-
> bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org] On Behalf Of Joerg Roedel
> Sent: Friday, January 20, 2012 9:31 PM
> To: Laurent Pinchart
> Cc: Ohad Ben-Cohen; Tony Lindgren; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; Wood Scott-B07421; David Brown; David
> Woodhouse
> Subject: Re: [PATCH 2/5] iommu/amd: Implement DOMAIN_ATTR_GEOMETRY
> attribute
> 
> On Thu, Jan 19, 2012 at 05:27:09PM +0100, Laurent Pinchart wrote:
> > > Well, u64 is a catch-all datatype which should suit all drivers. Is
> > > this a problem at ARM?
> >
> > No, it's not a problem for ARM, at least to my knowledge. It just
> > struck me as weird, as we have specific data types for other kinds of
> > addresses (such as dma_addr_t).
> 
> Okay, I looked at the definition and dma_addr_t seems like a good fit.
> To be consistent we should change the iova type of the map/unmap
> functions to dma_addr_t too.
This change is certainly required in the iommu API map/unmap calls.

-Varun

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

[Scott Wood]

On 01/19/2012 10:27 AM, Laurent Pinchart wrote:
> Hi Joerg,
> 
> On Thursday 19 January 2012 17:07:39 Joerg Roedel wrote:
>> On Thu, Jan 19, 2012 at 04:46:13PM +0100, Laurent Pinchart wrote:
>>>> +struct iommu_domain_geometry {
>>>> +	u64 aperture_start;	/* First address that can be mapped    */
>>>> +	u64 aperture_end;	/* Last address that can be mapped     */
>>>
>>> Would it make sense to use a platform-dependent type that represents
>>> physical addresses instead of u64 ?
>>
>> Well, u64 is a catch-all datatype which should suit all drivers. Is this
>> a problem at ARM?
> 
> No, it's not a problem for ARM, at least to my knowledge. It just struck me as 
> weird, as we have specific data types for other kinds of addresses (such as 
> dma_addr_t).

If we want to be able to expose these attribute structs to userspace via
vfio, we'll want to stick with fixed size types.

-Scott