Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1390603 > unrolled thread

Re: [PATCH v8 6/8] iommu/msi-iommu: iommu_msi_domain

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v8 6/8] iommu/msi-iommu: iommu_msi_domain Alex Williamson <alex.williamson@redhat.com> - 2016-04-29 00:30 +0200
    Re: [PATCH v8 6/8] iommu/msi-iommu: iommu_msi_domain Eric Auger <eric.auger@linaro.org> - 2016-05-02 17:50 +0200
      Re: [PATCH v8 6/8] iommu/msi-iommu: iommu_msi_domain Alex Williamson <alex.williamson@redhat.com> - 2016-05-02 22:30 +0200
        Re: [PATCH v8 6/8] iommu/msi-iommu: iommu_msi_domain Eric Auger <eric.auger@linaro.org> - 2016-05-03 14:30 +0200

#1390603 — Re: [PATCH v8 6/8] iommu/msi-iommu: iommu_msi_domain

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.

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.

Nice series overall.  Thanks,

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 */

[toc] | [next] | [standalone]

#1392322

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 */
> 

[toc] | [prev] | [next] | [standalone]

#1392500

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.

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

Alex

[toc] | [prev] | [next] | [standalone]

#1393384

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
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web