[v8,6/8] iommu/msi-iommu: iommu_msi_domain

This function checks whether
- the device belongs to a non default iommu domain
- this iommu domain requires the MSI address to be mapped.

If those conditions are met, the function returns the iommu domain
to be used for mapping the MSI doorbell; else it returns NULL.

Signed-off-by: Eric Auger <eric.auger@linaro.org>


---

v7 -> v8:
- renamed  iommu_msi_mapping_desc_to_domain to iommu_msi_domain
- the function now takes a struct device *
- use DOMAIN_ATTR_MSI_GEOMETRY attribute
---
 drivers/iommu/msi-iommu.c | 17 +++++++++++++++++
 include/linux/msi-iommu.h | 14 ++++++++++++++
 2 files changed, 31 insertions(+)

-- 
1.9.1

Hi Alex,
On 04/29/2016 12:27 AM, Alex Williamson wrote:
> On Thu, 28 Apr 2016 08:15:21 +0000

> Eric Auger <eric.auger@linaro.org> wrote:

> 

>> This function checks whether

>> - the device belongs to a non default iommu domain

>> - this iommu domain requires the MSI address to be mapped.

>>

>> If those conditions are met, the function returns the iommu domain

>> to be used for mapping the MSI doorbell; else it returns NULL.

>>

>> Signed-off-by: Eric Auger <eric.auger@linaro.org>

>>

>> ---

>>

>> v7 -> v8:

>> - renamed  iommu_msi_mapping_desc_to_domain to iommu_msi_domain

>> - the function now takes a struct device *

>> - use DOMAIN_ATTR_MSI_GEOMETRY attribute

>> ---

>>  drivers/iommu/msi-iommu.c | 17 +++++++++++++++++

>>  include/linux/msi-iommu.h | 14 ++++++++++++++

>>  2 files changed, 31 insertions(+)

>>

>> diff --git a/drivers/iommu/msi-iommu.c b/drivers/iommu/msi-iommu.c

>> index 203e86e..023ff17 100644

>> --- a/drivers/iommu/msi-iommu.c

>> +++ b/drivers/iommu/msi-iommu.c

>> @@ -243,3 +243,20 @@ unlock:

>>  	}

>>  }

>>  EXPORT_SYMBOL_GPL(iommu_msi_put_doorbell_iova);

>> +

>> +struct iommu_domain *iommu_msi_domain(struct device *dev)

>> +{

>> +	struct iommu_domain *d = iommu_get_domain_for_dev(dev);

>> +	struct iommu_domain_msi_geometry msi_geometry;

>> +

>> +	if (!d || (d->type == IOMMU_DOMAIN_DMA))

>> +		return NULL;

>> +

>> +	iommu_domain_get_attr(d, DOMAIN_ATTR_MSI_GEOMETRY, &msi_geometry);

>> +	if (!msi_geometry.programmable)

> 

> It feels like we're conflating ideas with using the "programmable" flag

> in this way.  AIUI the IOMMU API consumer is supposed to see the

> invalid MSI geometry with the programmable feature set and know to call

> iommu_msi_set_aperture().  Looking for the msi_cookie would tell us if

> that had been done, but that doesn't tell us if it should have been

> done.  iommu_msi_msg_pa_to_va() handles this later, if we return

> NULL here that function returns success otherwise it goes on to fail if

> the iova or msi cookie is not set.  So really what we're trying to flag

> is that the MSI geometry participates in the IOMMU-MSI API you've

> created and we should pick a feature name that says that rather than

> something as vague a "programmable".  Perhaps simply iommu_msi_api

> rather than programmable.

Yes I had the same questioning too when I wrote this code. So if my
understanding is correct we would have
- programmable: tells whether the MSI range is fixed or pogrammable and,
- mapping_required (new boolean field): indicating whether MSIs need to
be mapped in the IOMMU
> 

> BTW, I don't see that you ever set aperture_start/end once

> iommu_msi_set_aperture() has been called.  It seems like doing so would

> add some consistency to that MSI geometry attribute.

Indeed currently I mentionned:
/* In case the aperture is programmable, start/end are set to 0 */

If I set start/end in iommu_msi_set_aperture then I think I should also
return the actual values in iommu_domain_get_attr. I thought the extra
care to handle the possible race between the set_aperture (msi_iommu)
and the get_attr (iommu) was maybe not worth the benefits:
is_aperture_set is not visible to get_attr so I would be forced to
introduce a mutex at iommu_domain level or call an msi-iommu function
from iommu implementation. So I preferred to keep start/end as read-only
info initialized by the iommu driver.

> 

> Nice series overall.  Thanks,


Thanks

Eric
> 

> Alex

> 

>> +		return NULL;

>> +

>> +	return d;

>> +}

>> +EXPORT_SYMBOL_GPL(iommu_msi_domain);

>> +

>> diff --git a/include/linux/msi-iommu.h b/include/linux/msi-iommu.h

>> index 1cd115f..114bd69 100644

>> --- a/include/linux/msi-iommu.h

>> +++ b/include/linux/msi-iommu.h

>> @@ -81,6 +81,15 @@ int iommu_msi_get_doorbell_iova(struct iommu_domain *domain,

>>   */

>>  void iommu_msi_put_doorbell_iova(struct iommu_domain *domain, phys_addr_t addr);

>>  

>> +/**

>> + * iommu_msi_domain: in case the device is upstream to an IOMMU and this IOMMU

>> + * translates the MSI transaction, returns the iommu domain the MSI doorbell

>> + * address must be mapped in; else returns NULL.

>> + *

>> + * @dev: device handle

>> + */

>> +struct iommu_domain *iommu_msi_domain(struct device *dev);

>> +

>>  #else

>>  

>>  static inline int

>> @@ -100,5 +109,10 @@ static inline int iommu_msi_get_doorbell_iova(struct iommu_domain *domain,

>>  static inline void iommu_msi_put_doorbell_iova(struct iommu_domain *domain,

>>  					       phys_addr_t addr) {}

>>  

>> +static inline struct iommu_domain *iommu_msi_domain(struct device *dev)

>> +{

>> +	return NULL;

>> +}

>> +

>>  #endif	/* CONFIG_IOMMU_MSI */

>>  #endif	/* __MSI_IOMMU_H */

>

Hi Alex,
On 05/02/2016 10:23 PM, Alex Williamson wrote:
> Hi Eric,

> 

> On Mon, 2 May 2016 17:48:13 +0200

> Eric Auger <eric.auger@linaro.org> wrote:

> 

>> Hi Alex,

>> On 04/29/2016 12:27 AM, Alex Williamson wrote:

>>> On Thu, 28 Apr 2016 08:15:21 +0000

>>> Eric Auger <eric.auger@linaro.org> wrote:

>>>   

>>>> This function checks whether

>>>> - the device belongs to a non default iommu domain

>>>> - this iommu domain requires the MSI address to be mapped.

>>>>

>>>> If those conditions are met, the function returns the iommu domain

>>>> to be used for mapping the MSI doorbell; else it returns NULL.

>>>>

>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>

>>>>

>>>> ---

>>>>

>>>> v7 -> v8:

>>>> - renamed  iommu_msi_mapping_desc_to_domain to iommu_msi_domain

>>>> - the function now takes a struct device *

>>>> - use DOMAIN_ATTR_MSI_GEOMETRY attribute

>>>> ---

>>>>  drivers/iommu/msi-iommu.c | 17 +++++++++++++++++

>>>>  include/linux/msi-iommu.h | 14 ++++++++++++++

>>>>  2 files changed, 31 insertions(+)

>>>>

>>>> diff --git a/drivers/iommu/msi-iommu.c b/drivers/iommu/msi-iommu.c

>>>> index 203e86e..023ff17 100644

>>>> --- a/drivers/iommu/msi-iommu.c

>>>> +++ b/drivers/iommu/msi-iommu.c

>>>> @@ -243,3 +243,20 @@ unlock:

>>>>  	}

>>>>  }

>>>>  EXPORT_SYMBOL_GPL(iommu_msi_put_doorbell_iova);

>>>> +

>>>> +struct iommu_domain *iommu_msi_domain(struct device *dev)

>>>> +{

>>>> +	struct iommu_domain *d = iommu_get_domain_for_dev(dev);

>>>> +	struct iommu_domain_msi_geometry msi_geometry;

>>>> +

>>>> +	if (!d || (d->type == IOMMU_DOMAIN_DMA))

>>>> +		return NULL;

>>>> +

>>>> +	iommu_domain_get_attr(d, DOMAIN_ATTR_MSI_GEOMETRY, &msi_geometry);

>>>> +	if (!msi_geometry.programmable)  

>>>

>>> It feels like we're conflating ideas with using the "programmable" flag

>>> in this way.  AIUI the IOMMU API consumer is supposed to see the

>>> invalid MSI geometry with the programmable feature set and know to call

>>> iommu_msi_set_aperture().  Looking for the msi_cookie would tell us if

>>> that had been done, but that doesn't tell us if it should have been

>>> done.  iommu_msi_msg_pa_to_va() handles this later, if we return

>>> NULL here that function returns success otherwise it goes on to fail if

>>> the iova or msi cookie is not set.  So really what we're trying to flag

>>> is that the MSI geometry participates in the IOMMU-MSI API you've

>>> created and we should pick a feature name that says that rather than

>>> something as vague a "programmable".  Perhaps simply iommu_msi_api

>>> rather than programmable.  

>> Yes I had the same questioning too when I wrote this code. So if my

>> understanding is correct we would have

>> - programmable: tells whether the MSI range is fixed or pogrammable and,

>> - mapping_required (new boolean field): indicating whether MSIs need to

>> be mapped in the IOMMU

> 

> Let's say we had a flag "iommu_msi_supported", doesn't that already

> tell us whether the MSI range is programmable and the API to use to do

> it?  Can't we figure out if mapping is required based on whether

> iommu_msi_supported is set and aperture_start and aperture_end indicate

> a valid range?  It seems like what we want on this code path is to

> simply know whether the IOMMU domain is relevant to the IOMMU MSI API,

> which would be abundantly clear with such a flag.

OK I now get your point. Thanks for the clarification. I renamed
programmable into iommu_msi_supported.
> 

>>>

>>> BTW, I don't see that you ever set aperture_start/end once

>>> iommu_msi_set_aperture() has been called.  It seems like doing so would

>>> add some consistency to that MSI geometry attribute.  

>> Indeed currently I mentionned:

>> /* In case the aperture is programmable, start/end are set to 0 */

> 

> Which is confusing, if a user sets an aperture, I would think they'd

> like to see the MSI geometry updated to reflect that.

> 

>> If I set start/end in iommu_msi_set_aperture then I think I should also

>> return the actual values in iommu_domain_get_attr. I thought the extra

>> care to handle the possible race between the set_aperture (msi_iommu)

>> and the get_attr (iommu) was maybe not worth the benefits:

>> is_aperture_set is not visible to get_attr so I would be forced to

>> introduce a mutex at iommu_domain level or call an msi-iommu function

>> from iommu implementation. So I preferred to keep start/end as read-only

>> info initialized by the iommu driver.

> 

> Seems like a race between iommu_msi_set_aperture() and

> iommu_domain_get_attr() is the caller's problem.  We can always define

> that start >= end is invalid and set them in the right order that

> iommu_domain_get_attr() may get different versions of invalid, but it

> will always be invalid or valid.  A helper function could tell us if we

> have a valid range rather than using is_aperture_set.  Thanks,



Agreed; I added an iommu_domain_msi_aperture_valid helper function.

Thanks!

Eric
> 

> Alex

>