[v13,03/15] iommu/dma: Allow MSI-only cookies

Commit Message

Eric Auger Oct. 6, 2016, 8:45 a.m. UTC

From: Robin Murphy <[e-mail]>

IOMMU domain users such as VFIO face a similar problem to DMA API ops
with regard to mapping MSI messages in systems where the MSI write is
subject to IOMMU translation. With the relevant infrastructure now in
place for managed DMA domains, it's actually really simple for other
users to piggyback off that and reap the benefits without giving up
their own IOVA management, and without having to reinvent their own
wheel in the MSI layer.

Allow such users to opt into automatic MSI remapping by dedicating a
region of their IOVA space to a managed cookie.

Signed-off-by: Robin Murphy <[e-mail]>
Signed-off-by: Eric Auger <[e-mail]>

---

v1 -> v2:
- compared to Robin's version
- add NULL last param to iommu_dma_init_domain
- set the msi_geometry aperture
- I removed
  if (base < U64_MAX - size)
     reserve_iova(iovad, iova_pfn(iovad, base + size), ULONG_MAX);
  don't get why we would reserve something out of the scope of the iova domain?
  what do I miss?
---
 drivers/iommu/dma-iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/dma-iommu.h |  9 +++++++++
 2 files changed, 49 insertions(+)

Comments

Alex Williamson Oct. 6, 2016, 8:17 p.m. UTC | #1

On Thu,  6 Oct 2016 08:45:19 +0000
Eric Auger <[e-mail]> wrote:

> From: Robin Murphy <[e-mail]>
> 
> IOMMU domain users such as VFIO face a similar problem to DMA API ops
> with regard to mapping MSI messages in systems where the MSI write is
> subject to IOMMU translation. With the relevant infrastructure now in
> place for managed DMA domains, it's actually really simple for other
> users to piggyback off that and reap the benefits without giving up
> their own IOVA management, and without having to reinvent their own
> wheel in the MSI layer.
> 
> Allow such users to opt into automatic MSI remapping by dedicating a
> region of their IOVA space to a managed cookie.
> 
> Signed-off-by: Robin Murphy <[e-mail]>
> Signed-off-by: Eric Auger <[e-mail]>
> 
> ---
> 
> v1 -> v2:
> - compared to Robin's version
> - add NULL last param to iommu_dma_init_domain
> - set the msi_geometry aperture
> - I removed
>   if (base < U64_MAX - size)
>      reserve_iova(iovad, iova_pfn(iovad, base + size), ULONG_MAX);
>   don't get why we would reserve something out of the scope of the iova domain?
>   what do I miss?
> ---
>  drivers/iommu/dma-iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/dma-iommu.h |  9 +++++++++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index c5ab866..11da1a0 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -716,3 +716,43 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>  		msg->address_lo += lower_32_bits(msi_page->iova);
>  	}
>  }
> +
> +/**
> + * iommu_get_dma_msi_region_cookie - Configure a domain for MSI remapping only

Should this perhaps be iommu_setup_dma_msi_region_cookie, or something
along those lines.  I'm not sure what we're get'ing.  Thanks,

Alex

> + * @domain: IOMMU domain to prepare
> + * @base: Base address of IOVA region to use as the MSI remapping aperture
> + * @size: Size of the desired MSI aperture
> + *
> + * Users who manage their own IOVA allocation and do not want DMA API support,
> + * but would still like to take advantage of automatic MSI remapping, can use
> + * this to initialise their own domain appropriately.
> + */
> +int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
> +		dma_addr_t base, u64 size)
> +{
> +	struct iommu_dma_cookie *cookie;
> +	struct iova_domain *iovad;
> +	int ret;
> +
> +	if (domain->type == IOMMU_DOMAIN_DMA)
> +		return -EINVAL;
> +
> +	ret = iommu_get_dma_cookie(domain);
> +	if (ret)
> +		return ret;
> +
> +	ret = iommu_dma_init_domain(domain, base, size, NULL);
> +	if (ret) {
> +		iommu_put_dma_cookie(domain);
> +		return ret;
> +	}
> +
> +	domain->msi_geometry.aperture_start = base;
> +	domain->msi_geometry.aperture_end = base + size - 1;
> +
> +	cookie = domain->iova_cookie;
> +	iovad = &cookie->iovad;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(iommu_get_dma_msi_region_cookie);
> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
> index 32c5890..1c55413 100644
> --- a/include/linux/dma-iommu.h
> +++ b/include/linux/dma-iommu.h
> @@ -67,6 +67,9 @@ int iommu_dma_mapping_error(struct device *dev, dma_addr_t dma_addr);
>  /* The DMA API isn't _quite_ the whole story, though... */
>  void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg);
>  
> +int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
> +		dma_addr_t base, u64 size);
> +
>  #else
>  
>  struct iommu_domain;
> @@ -90,6 +93,12 @@ static inline void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>  {
>  }
>  
> +static inline int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
> +		dma_addr_t base, u64 size)
> +{
> +	return -ENODEV;
> +}
> +
>  #endif	/* CONFIG_IOMMU_DMA */
>  #endif	/* __KERNEL__ */
>  #endif	/* __DMA_IOMMU_H */

Eric Auger Oct. 7, 2016, 5:14 p.m. UTC | #2

Hi Alex,

On 06/10/2016 22:17, Alex Williamson wrote:
> On Thu,  6 Oct 2016 08:45:19 +0000
> Eric Auger <[e-mail]> wrote:
> 
>> From: Robin Murphy <[e-mail]>
>>
>> IOMMU domain users such as VFIO face a similar problem to DMA API ops
>> with regard to mapping MSI messages in systems where the MSI write is
>> subject to IOMMU translation. With the relevant infrastructure now in
>> place for managed DMA domains, it's actually really simple for other
>> users to piggyback off that and reap the benefits without giving up
>> their own IOVA management, and without having to reinvent their own
>> wheel in the MSI layer.
>>
>> Allow such users to opt into automatic MSI remapping by dedicating a
>> region of their IOVA space to a managed cookie.
>>
>> Signed-off-by: Robin Murphy <[e-mail]>
>> Signed-off-by: Eric Auger <[e-mail]>
>>
>> ---
>>
>> v1 -> v2:
>> - compared to Robin's version
>> - add NULL last param to iommu_dma_init_domain
>> - set the msi_geometry aperture
>> - I removed
>>   if (base < U64_MAX - size)
>>      reserve_iova(iovad, iova_pfn(iovad, base + size), ULONG_MAX);
>>   don't get why we would reserve something out of the scope of the iova domain?
>>   what do I miss?
>> ---
>>  drivers/iommu/dma-iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>  include/linux/dma-iommu.h |  9 +++++++++
>>  2 files changed, 49 insertions(+)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index c5ab866..11da1a0 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -716,3 +716,43 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>>  		msg->address_lo += lower_32_bits(msi_page->iova);
>>  	}
>>  }
>> +
>> +/**
>> + * iommu_get_dma_msi_region_cookie - Configure a domain for MSI remapping only
> 
> Should this perhaps be iommu_setup_dma_msi_region_cookie, or something
> along those lines.  I'm not sure what we're get'ing.  Thanks,
This was chosen by analogy with legacy iommu_get_dma_cookie/
iommu_put_dma_cookie. But in practice it does both get &
iommu_dma_init_domain.

I plan to rename into iommu_setup_dma_msi_region if no objection

Thanks

Eric

> 
> Alex
> 
>> + * @domain: IOMMU domain to prepare
>> + * @base: Base address of IOVA region to use as the MSI remapping aperture
>> + * @size: Size of the desired MSI aperture
>> + *
>> + * Users who manage their own IOVA allocation and do not want DMA API support,
>> + * but would still like to take advantage of automatic MSI remapping, can use
>> + * this to initialise their own domain appropriately.
>> + */
>> +int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
>> +		dma_addr_t base, u64 size)
>> +{
>> +	struct iommu_dma_cookie *cookie;
>> +	struct iova_domain *iovad;
>> +	int ret;
>> +
>> +	if (domain->type == IOMMU_DOMAIN_DMA)
>> +		return -EINVAL;
>> +
>> +	ret = iommu_get_dma_cookie(domain);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = iommu_dma_init_domain(domain, base, size, NULL);
>> +	if (ret) {
>> +		iommu_put_dma_cookie(domain);
>> +		return ret;
>> +	}
>> +
>> +	domain->msi_geometry.aperture_start = base;
>> +	domain->msi_geometry.aperture_end = base + size - 1;
>> +
>> +	cookie = domain->iova_cookie;
>> +	iovad = &cookie->iovad;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(iommu_get_dma_msi_region_cookie);
>> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
>> index 32c5890..1c55413 100644
>> --- a/include/linux/dma-iommu.h
>> +++ b/include/linux/dma-iommu.h
>> @@ -67,6 +67,9 @@ int iommu_dma_mapping_error(struct device *dev, dma_addr_t dma_addr);
>>  /* The DMA API isn't _quite_ the whole story, though... */
>>  void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg);
>>  
>> +int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
>> +		dma_addr_t base, u64 size);
>> +
>>  #else
>>  
>>  struct iommu_domain;
>> @@ -90,6 +93,12 @@ static inline void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>>  {
>>  }
>>  
>> +static inline int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
>> +		dma_addr_t base, u64 size)
>> +{
>> +	return -ENODEV;
>> +}
>> +
>>  #endif	/* CONFIG_IOMMU_DMA */
>>  #endif	/* __KERNEL__ */
>>  #endif	/* __DMA_IOMMU_H */
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Robin Murphy Oct. 10, 2016, 2:26 p.m. UTC | #3

Hi Alex, Eric,

On 06/10/16 21:17, Alex Williamson wrote:
> On Thu,  6 Oct 2016 08:45:19 +0000
> Eric Auger <[e-mail]> wrote:
> 
>> From: Robin Murphy <[e-mail]>
>>
>> IOMMU domain users such as VFIO face a similar problem to DMA API ops
>> with regard to mapping MSI messages in systems where the MSI write is
>> subject to IOMMU translation. With the relevant infrastructure now in
>> place for managed DMA domains, it's actually really simple for other
>> users to piggyback off that and reap the benefits without giving up
>> their own IOVA management, and without having to reinvent their own
>> wheel in the MSI layer.
>>
>> Allow such users to opt into automatic MSI remapping by dedicating a
>> region of their IOVA space to a managed cookie.
>>
>> Signed-off-by: Robin Murphy <[e-mail]>
>> Signed-off-by: Eric Auger <[e-mail]>
>>
>> ---
>>
>> v1 -> v2:
>> - compared to Robin's version
>> - add NULL last param to iommu_dma_init_domain
>> - set the msi_geometry aperture
>> - I removed
>>   if (base < U64_MAX - size)
>>      reserve_iova(iovad, iova_pfn(iovad, base + size), ULONG_MAX);
>>   don't get why we would reserve something out of the scope of the iova domain?
>>   what do I miss?
>> ---
>>  drivers/iommu/dma-iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>  include/linux/dma-iommu.h |  9 +++++++++
>>  2 files changed, 49 insertions(+)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index c5ab866..11da1a0 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -716,3 +716,43 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>>  		msg->address_lo += lower_32_bits(msi_page->iova);
>>  	}
>>  }
>> +
>> +/**
>> + * iommu_get_dma_msi_region_cookie - Configure a domain for MSI remapping only
> 
> Should this perhaps be iommu_setup_dma_msi_region_cookie, or something
> along those lines.  I'm not sure what we're get'ing.  Thanks,

What we're getting is private third-party resources for the iommu_domain
given in the argument. It's a get/put rather than alloc/free model since
we operate opaquely on the domain as a container, rather than on the
actual resource in question (an IOVA allocator).

Since this particular use case is slightly different from the normal
flow and has special initialisation requirements, it seemed a lot
cleaner to simply combine that initialisation operation with the
prerequisite "get" into a single call. Especially as it helps emphasise
that this is not 'normal' DMA cookie usage.

> 
> Alex
> 
>> + * @domain: IOMMU domain to prepare
>> + * @base: Base address of IOVA region to use as the MSI remapping aperture
>> + * @size: Size of the desired MSI aperture
>> + *
>> + * Users who manage their own IOVA allocation and do not want DMA API support,
>> + * but would still like to take advantage of automatic MSI remapping, can use
>> + * this to initialise their own domain appropriately.
>> + */
>> +int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
>> +		dma_addr_t base, u64 size)
>> +{
>> +	struct iommu_dma_cookie *cookie;
>> +	struct iova_domain *iovad;
>> +	int ret;
>> +
>> +	if (domain->type == IOMMU_DOMAIN_DMA)
>> +		return -EINVAL;
>> +
>> +	ret = iommu_get_dma_cookie(domain);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = iommu_dma_init_domain(domain, base, size, NULL);
>> +	if (ret) {
>> +		iommu_put_dma_cookie(domain);
>> +		return ret;
>> +	}

It *is* necessary to explicitly reserve the upper part of the IOVA
domain here - the aforementioned "special initialisation" - because
dma_32bit_pfn is only an optimisation hint to prevent the allocator
walking down from the very top of the the tree every time when devices
with different DMA masks share a domain (I'm in two minds as to whether
to tweak the way the iommu-dma code uses it in this respect, now that I
fully understand things). The only actual upper limit to allocation is
the DMA mask passed into each alloc_iova() call, so if we want to ensure
IOVAs are really allocated within this specific region, we have to carve
out everything above it.

Robin.

>> +
>> +	domain->msi_geometry.aperture_start = base;
>> +	domain->msi_geometry.aperture_end = base + size - 1;
>> +
>> +	cookie = domain->iova_cookie;
>> +	iovad = &cookie->iovad;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(iommu_get_dma_msi_region_cookie);
>> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
>> index 32c5890..1c55413 100644
>> --- a/include/linux/dma-iommu.h
>> +++ b/include/linux/dma-iommu.h
>> @@ -67,6 +67,9 @@ int iommu_dma_mapping_error(struct device *dev, dma_addr_t dma_addr);
>>  /* The DMA API isn't _quite_ the whole story, though... */
>>  void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg);
>>  
>> +int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
>> +		dma_addr_t base, u64 size);
>> +
>>  #else
>>  
>>  struct iommu_domain;
>> @@ -90,6 +93,12 @@ static inline void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>>  {
>>  }
>>  
>> +static inline int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
>> +		dma_addr_t base, u64 size)
>> +{
>> +	return -ENODEV;
>> +}
>> +
>>  #endif	/* CONFIG_IOMMU_DMA */
>>  #endif	/* __KERNEL__ */
>>  #endif	/* __DMA_IOMMU_H */
>

Eric Auger Oct. 10, 2016, 2:47 p.m. UTC | #4

Hi Robin,

On 10/10/2016 16:26, Robin Murphy wrote:
> Hi Alex, Eric,
> 
> On 06/10/16 21:17, Alex Williamson wrote:
>> On Thu,  6 Oct 2016 08:45:19 +0000
>> Eric Auger <[e-mail]> wrote:
>>
>>> From: Robin Murphy <[e-mail]>
>>>
>>> IOMMU domain users such as VFIO face a similar problem to DMA API ops
>>> with regard to mapping MSI messages in systems where the MSI write is
>>> subject to IOMMU translation. With the relevant infrastructure now in
>>> place for managed DMA domains, it's actually really simple for other
>>> users to piggyback off that and reap the benefits without giving up
>>> their own IOVA management, and without having to reinvent their own
>>> wheel in the MSI layer.
>>>
>>> Allow such users to opt into automatic MSI remapping by dedicating a
>>> region of their IOVA space to a managed cookie.
>>>
>>> Signed-off-by: Robin Murphy <[e-mail]>
>>> Signed-off-by: Eric Auger <[e-mail]>
>>>
>>> ---
>>>
>>> v1 -> v2:
>>> - compared to Robin's version
>>> - add NULL last param to iommu_dma_init_domain
>>> - set the msi_geometry aperture
>>> - I removed
>>>   if (base < U64_MAX - size)
>>>      reserve_iova(iovad, iova_pfn(iovad, base + size), ULONG_MAX);
>>>   don't get why we would reserve something out of the scope of the iova domain?
>>>   what do I miss?
>>> ---
>>>  drivers/iommu/dma-iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/dma-iommu.h |  9 +++++++++
>>>  2 files changed, 49 insertions(+)
>>>
>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>> index c5ab866..11da1a0 100644
>>> --- a/drivers/iommu/dma-iommu.c
>>> +++ b/drivers/iommu/dma-iommu.c
>>> @@ -716,3 +716,43 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>>>  		msg->address_lo += lower_32_bits(msi_page->iova);
>>>  	}
>>>  }
>>> +
>>> +/**
>>> + * iommu_get_dma_msi_region_cookie - Configure a domain for MSI remapping only
>>
>> Should this perhaps be iommu_setup_dma_msi_region_cookie, or something
>> along those lines.  I'm not sure what we're get'ing.  Thanks,
> 
> What we're getting is private third-party resources for the iommu_domain
> given in the argument. It's a get/put rather than alloc/free model since
> we operate opaquely on the domain as a container, rather than on the
> actual resource in question (an IOVA allocator).
> 
> Since this particular use case is slightly different from the normal
> flow and has special initialisation requirements, it seemed a lot
> cleaner to simply combine that initialisation operation with the
> prerequisite "get" into a single call. Especially as it helps emphasise
> that this is not 'normal' DMA cookie usage.

I renamed iommu_get_dma_msi_region_cookie into
iommu_setup_dma_msi_region. Is it a problem for you?
> 
>>
>> Alex
>>
>>> + * @domain: IOMMU domain to prepare
>>> + * @base: Base address of IOVA region to use as the MSI remapping aperture
>>> + * @size: Size of the desired MSI aperture
>>> + *
>>> + * Users who manage their own IOVA allocation and do not want DMA API support,
>>> + * but would still like to take advantage of automatic MSI remapping, can use
>>> + * this to initialise their own domain appropriately.
>>> + */
>>> +int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
>>> +		dma_addr_t base, u64 size)
>>> +{
>>> +	struct iommu_dma_cookie *cookie;
>>> +	struct iova_domain *iovad;
>>> +	int ret;
>>> +
>>> +	if (domain->type == IOMMU_DOMAIN_DMA)
>>> +		return -EINVAL;
>>> +
>>> +	ret = iommu_get_dma_cookie(domain);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = iommu_dma_init_domain(domain, base, size, NULL);
>>> +	if (ret) {
>>> +		iommu_put_dma_cookie(domain);
>>> +		return ret;
>>> +	}
> 
> It *is* necessary to explicitly reserve the upper part of the IOVA
> domain here - the aforementioned "special initialisation" - because
> dma_32bit_pfn is only an optimisation hint to prevent the allocator
> walking down from the very top of the the tree every time when devices
> with different DMA masks share a domain (I'm in two minds as to whether
> to tweak the way the iommu-dma code uses it in this respect, now that I
> fully understand things). The only actual upper limit to allocation is
> the DMA mask passed into each alloc_iova() call, so if we want to ensure
> IOVAs are really allocated within this specific region, we have to carve
> out everything above it.

thank you for the explanation. So I will restore the reserve then.

Thanks

Eric
> 
> Robin.
> 
>>> +
>>> +	domain->msi_geometry.aperture_start = base;
>>> +	domain->msi_geometry.aperture_end = base + size - 1;
>>> +
>>> +	cookie = domain->iova_cookie;
>>> +	iovad = &cookie->iovad;
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL(iommu_get_dma_msi_region_cookie);
>>> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
>>> index 32c5890..1c55413 100644
>>> --- a/include/linux/dma-iommu.h
>>> +++ b/include/linux/dma-iommu.h
>>> @@ -67,6 +67,9 @@ int iommu_dma_mapping_error(struct device *dev, dma_addr_t dma_addr);
>>>  /* The DMA API isn't _quite_ the whole story, though... */
>>>  void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg);
>>>  
>>> +int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
>>> +		dma_addr_t base, u64 size);
>>> +
>>>  #else
>>>  
>>>  struct iommu_domain;
>>> @@ -90,6 +93,12 @@ static inline void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>>>  {
>>>  }
>>>  
>>> +static inline int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
>>> +		dma_addr_t base, u64 size)
>>> +{
>>> +	return -ENODEV;
>>> +}
>>> +
>>>  #endif	/* CONFIG_IOMMU_DMA */
>>>  #endif	/* __KERNEL__ */
>>>  #endif	/* __DMA_IOMMU_H */
>>
>

Robin Murphy Oct. 10, 2016, 3:52 p.m. UTC | #5

On 10/10/16 15:47, Auger Eric wrote:
> Hi Robin,
> 
> On 10/10/2016 16:26, Robin Murphy wrote:
>> Hi Alex, Eric,
>>
>> On 06/10/16 21:17, Alex Williamson wrote:
>>> On Thu,  6 Oct 2016 08:45:19 +0000
>>> Eric Auger <[e-mail]> wrote:
>>>
>>>> From: Robin Murphy <[e-mail]>
>>>>
>>>> IOMMU domain users such as VFIO face a similar problem to DMA API ops
>>>> with regard to mapping MSI messages in systems where the MSI write is
>>>> subject to IOMMU translation. With the relevant infrastructure now in
>>>> place for managed DMA domains, it's actually really simple for other
>>>> users to piggyback off that and reap the benefits without giving up
>>>> their own IOVA management, and without having to reinvent their own
>>>> wheel in the MSI layer.
>>>>
>>>> Allow such users to opt into automatic MSI remapping by dedicating a
>>>> region of their IOVA space to a managed cookie.
>>>>
>>>> Signed-off-by: Robin Murphy <[e-mail]>
>>>> Signed-off-by: Eric Auger <[e-mail]>
>>>>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>> - compared to Robin's version
>>>> - add NULL last param to iommu_dma_init_domain
>>>> - set the msi_geometry aperture
>>>> - I removed
>>>>   if (base < U64_MAX - size)
>>>>      reserve_iova(iovad, iova_pfn(iovad, base + size), ULONG_MAX);
>>>>   don't get why we would reserve something out of the scope of the iova domain?
>>>>   what do I miss?
>>>> ---
>>>>  drivers/iommu/dma-iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/dma-iommu.h |  9 +++++++++
>>>>  2 files changed, 49 insertions(+)
>>>>
>>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>>> index c5ab866..11da1a0 100644
>>>> --- a/drivers/iommu/dma-iommu.c
>>>> +++ b/drivers/iommu/dma-iommu.c
>>>> @@ -716,3 +716,43 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>>>>  		msg->address_lo += lower_32_bits(msi_page->iova);
>>>>  	}
>>>>  }
>>>> +
>>>> +/**
>>>> + * iommu_get_dma_msi_region_cookie - Configure a domain for MSI remapping only
>>>
>>> Should this perhaps be iommu_setup_dma_msi_region_cookie, or something
>>> along those lines.  I'm not sure what we're get'ing.  Thanks,
>>
>> What we're getting is private third-party resources for the iommu_domain
>> given in the argument. It's a get/put rather than alloc/free model since
>> we operate opaquely on the domain as a container, rather than on the
>> actual resource in question (an IOVA allocator).
>>
>> Since this particular use case is slightly different from the normal
>> flow and has special initialisation requirements, it seemed a lot
>> cleaner to simply combine that initialisation operation with the
>> prerequisite "get" into a single call. Especially as it helps emphasise
>> that this is not 'normal' DMA cookie usage.
> 
> I renamed iommu_get_dma_msi_region_cookie into
> iommu_setup_dma_msi_region. Is it a problem for you?

I'd still prefer not to completely disguise the fact that it's
performing a get_cookie(), which ultimately still needs to be matched by
a put_cookie() somewhere. Really, VFIO should be doing the latter itself
before freeing the domain, as there's not strictly any guarantee that
the underlying IOMMU driver knows anything about this.

Robin.

>>
>>>
>>> Alex
>>>
>>>> + * @domain: IOMMU domain to prepare
>>>> + * @base: Base address of IOVA region to use as the MSI remapping aperture
>>>> + * @size: Size of the desired MSI aperture
>>>> + *
>>>> + * Users who manage their own IOVA allocation and do not want DMA API support,
>>>> + * but would still like to take advantage of automatic MSI remapping, can use
>>>> + * this to initialise their own domain appropriately.
>>>> + */
>>>> +int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
>>>> +		dma_addr_t base, u64 size)
>>>> +{
>>>> +	struct iommu_dma_cookie *cookie;
>>>> +	struct iova_domain *iovad;
>>>> +	int ret;
>>>> +
>>>> +	if (domain->type == IOMMU_DOMAIN_DMA)
>>>> +		return -EINVAL;
>>>> +
>>>> +	ret = iommu_get_dma_cookie(domain);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	ret = iommu_dma_init_domain(domain, base, size, NULL);
>>>> +	if (ret) {
>>>> +		iommu_put_dma_cookie(domain);
>>>> +		return ret;
>>>> +	}
>>
>> It *is* necessary to explicitly reserve the upper part of the IOVA
>> domain here - the aforementioned "special initialisation" - because
>> dma_32bit_pfn is only an optimisation hint to prevent the allocator
>> walking down from the very top of the the tree every time when devices
>> with different DMA masks share a domain (I'm in two minds as to whether
>> to tweak the way the iommu-dma code uses it in this respect, now that I
>> fully understand things). The only actual upper limit to allocation is
>> the DMA mask passed into each alloc_iova() call, so if we want to ensure
>> IOVAs are really allocated within this specific region, we have to carve
>> out everything above it.
> 
> thank you for the explanation. So I will restore the reserve then.
> 
> Thanks
> 
> Eric
>>
>> Robin.
>>
>>>> +
>>>> +	domain->msi_geometry.aperture_start = base;
>>>> +	domain->msi_geometry.aperture_end = base + size - 1;
>>>> +
>>>> +	cookie = domain->iova_cookie;
>>>> +	iovad = &cookie->iovad;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(iommu_get_dma_msi_region_cookie);
>>>> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
>>>> index 32c5890..1c55413 100644
>>>> --- a/include/linux/dma-iommu.h
>>>> +++ b/include/linux/dma-iommu.h
>>>> @@ -67,6 +67,9 @@ int iommu_dma_mapping_error(struct device *dev, dma_addr_t dma_addr);
>>>>  /* The DMA API isn't _quite_ the whole story, though... */
>>>>  void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg);
>>>>  
>>>> +int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
>>>> +		dma_addr_t base, u64 size);
>>>> +
>>>>  #else
>>>>  
>>>>  struct iommu_domain;
>>>> @@ -90,6 +93,12 @@ static inline void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>>>>  {
>>>>  }
>>>>  
>>>> +static inline int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
>>>> +		dma_addr_t base, u64 size)
>>>> +{
>>>> +	return -ENODEV;
>>>> +}
>>>> +
>>>>  #endif	/* CONFIG_IOMMU_DMA */
>>>>  #endif	/* __KERNEL__ */
>>>>  #endif	/* __DMA_IOMMU_H */
>>>
>>
>

Patch

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index c5ab866..11da1a0 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -716,3 +716,43 @@  void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
 		msg->address_lo += lower_32_bits(msi_page->iova);
 	}
 }
+
+/**
+ * iommu_get_dma_msi_region_cookie - Configure a domain for MSI remapping only
+ * @domain: IOMMU domain to prepare
+ * @base: Base address of IOVA region to use as the MSI remapping aperture
+ * @size: Size of the desired MSI aperture
+ *
+ * Users who manage their own IOVA allocation and do not want DMA API support,
+ * but would still like to take advantage of automatic MSI remapping, can use
+ * this to initialise their own domain appropriately.
+ */
+int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
+		dma_addr_t base, u64 size)
+{
+	struct iommu_dma_cookie *cookie;
+	struct iova_domain *iovad;
+	int ret;
+
+	if (domain->type == IOMMU_DOMAIN_DMA)
+		return -EINVAL;
+
+	ret = iommu_get_dma_cookie(domain);
+	if (ret)
+		return ret;
+
+	ret = iommu_dma_init_domain(domain, base, size, NULL);
+	if (ret) {
+		iommu_put_dma_cookie(domain);
+		return ret;
+	}
+
+	domain->msi_geometry.aperture_start = base;
+	domain->msi_geometry.aperture_end = base + size - 1;
+
+	cookie = domain->iova_cookie;
+	iovad = &cookie->iovad;
+
+	return 0;
+}
+EXPORT_SYMBOL(iommu_get_dma_msi_region_cookie);
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 32c5890..1c55413 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -67,6 +67,9 @@  int iommu_dma_mapping_error(struct device *dev, dma_addr_t dma_addr);
 /* The DMA API isn't _quite_ the whole story, though... */
 void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg);
 
+int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
+		dma_addr_t base, u64 size);
+
 #else
 
 struct iommu_domain;
@@ -90,6 +93,12 @@  static inline void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
 {
 }
 
+static inline int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain,
+		dma_addr_t base, u64 size)
+{
+	return -ENODEV;
+}
+
 #endif	/* CONFIG_IOMMU_DMA */
 #endif	/* __KERNEL__ */
 #endif	/* __DMA_IOMMU_H */