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

Groups > linux.kernel > #1382709 > unrolled thread

[PATCH v7 00/10] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 1/3: iommu changes

Back to article view | Back to linux.kernel

Contents

  [PATCH v7 00/10] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 1/3: iommu changes Eric Auger <eric.auger@linaro.org> - 2016-04-19 19:00 +0200
    [PATCH v7 05/10] iommu/dma-reserved-iommu: reserved binding rb-tree and helpers Eric Auger <eric.auger@linaro.org> - 2016-04-19 19:00 +0200
      Re: [PATCH v7 05/10] iommu/dma-reserved-iommu: reserved binding  rb-tree and helpers Robin Murphy <robin.murphy@arm.com> - 2016-04-20 15:20 +0200
        Re: [PATCH v7 05/10] iommu/dma-reserved-iommu: reserved binding  rb-tree and helpers Eric Auger <eric.auger@linaro.org> - 2016-04-20 18:30 +0200
          Re: [PATCH v7 05/10] iommu/dma-reserved-iommu: reserved binding  rb-tree and helpers Robin Murphy <robin.murphy@arm.com> - 2016-04-22 15:10 +0200
    [PATCH v7 10/10] iommu/arm-smmu: call iommu_free_reserved_iova_domain on domain destruction Eric Auger <eric.auger@linaro.org> - 2016-04-19 19:00 +0200
      Re: [PATCH v7 10/10] iommu/arm-smmu: call  iommu_free_reserved_iova_domain on domain destruction Robin Murphy <robin.murphy@arm.com> - 2016-04-20 19:40 +0200
        Re: [PATCH v7 10/10] iommu/arm-smmu: call  iommu_free_reserved_iova_domain on domain destruction Eric Auger <eric.auger@linaro.org> - 2016-04-21 10:50 +0200
    [PATCH v7 09/10] iommu/dma-reserved-iommu: iommu_msi_mapping_translate_msg Eric Auger <eric.auger@linaro.org> - 2016-04-19 19:00 +0200
      Re: [PATCH v7 09/10] iommu/dma-reserved-iommu:  iommu_msi_mapping_translate_msg Marc Zyngier <marc.zyngier@arm.com> - 2016-04-20 11:40 +0200
        Re: [PATCH v7 09/10] iommu/dma-reserved-iommu:  iommu_msi_mapping_translate_msg Eric Auger <eric.auger@linaro.org> - 2016-04-20 15:00 +0200
      Re: [PATCH v7 09/10] iommu/dma-reserved-iommu:  iommu_msi_mapping_translate_msg Robin Murphy <robin.murphy@arm.com> - 2016-04-20 19:30 +0200
        Re: [PATCH v7 09/10] iommu/dma-reserved-iommu:  iommu_msi_mapping_translate_msg Eric Auger <eric.auger@linaro.org> - 2016-04-21 10:50 +0200
    Re: [PATCH v7 00/10] KVM PCIe/MSI passthrough on ARM/ARM64: kernel  part 1/3: iommu changes Eric Auger <eric.auger@linaro.org> - 2016-04-21 14:20 +0200
      Re: [PATCH v7 00/10] KVM PCIe/MSI passthrough on ARM/ARM64: kernel  part 1/3: iommu changes Alex Williamson <alex.williamson@redhat.com> - 2016-04-21 21:40 +0200
        Re: [PATCH v7 00/10] KVM PCIe/MSI passthrough on ARM/ARM64: kernel  part 1/3: iommu changes Eric Auger <eric.auger@linaro.org> - 2016-04-22 14:40 +0200
          Re: [PATCH v7 00/10] KVM PCIe/MSI passthrough on ARM/ARM64: kernel  part 1/3: iommu changes Alex Williamson <alex.williamson@redhat.com> - 2016-04-22 21:10 +0200

#1382709 — [PATCH v7 00/10] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 1/3: iommu changes

This series introduces the dma-reserved-iommu api used to:

- create/destroy an iova domain dedicated to reserved iova bindings
- map/unmap physical addresses onto reserved IOVAs.
- search for an existing reserved iova mapping matching a PA window
- determine whether an msi needs to be iommu mapped
- translate an msi_msg PA address into its IOVA counterpart

Currently reserved IOVAs are meant to map MSI physical doorbells. A single
reserved domain does exit per domain.

Also a new domain attribute is introduced to signal whether the MSI
addresses must be mapped in the IOMMU.

In current usage:
VFIO subsystem is supposed to create/destroy the iommu reserved domain.
The MSI layer is supposed to allocate/free iova mappings

Since several drivers are likely to use the same doorbell, a reference
counting takes place on the bindings. An RB-tree indexed by PA is used
to easily lookup for existing mappings at MSI message composition time.

More details & context can be found at:
http://www.linaro.org/blog/core-dump/kvm-pciemsi-passthrough-armarm64/

Best Regards

Eric

Git: complete series available at
https://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.6-rc4-pcie-passthrough-v7

History:

v6 -> v7:
- fixed known lock bugs and multiple page sized slots matching
  (I currently only have a single MSI frame made of a single page)
- reserved_iova_cookie now pointing to a struct that encapsulates the
  iova domain handle + protection attribute passed from VFIO (Alex' req)
- 2 new functions exposed: iommu_msi_mapping_translate_msg,
  iommu_msi_mapping_desc_to_domain: not sure this is the right location/proto
  though
- iommu_put_reserved_iova now takes a phys_addr_t
- everything now is cleanup on iommu_domain destruction

RFC v5 -> patch v6:
- split to ease the review process
- in dma-reserved-api use a spin lock instead of a mutex (reported by
  Jean-Philippe)
- revisit iommu_get_reserved_iova API to pass a size parameter upon
  Marc's request
- Consistently use the page order passed when creating the iova domain.
- init reserved_binding_list (reported by Julien)

RFC v4 -> RFC v5:
- take into account Thomas' comments on MSI related patches
  - split "msi: IOMMU map the doorbell address when needed"
  - increase readability and add comments
  - fix style issues
 - split "iommu: Add DOMAIN_ATTR_MSI_MAPPING attribute"
 - platform ITS now advertises IOMMU_CAP_INTR_REMAP
 - fix compilation issue with CONFIG_IOMMU API unset
 - arm-smmu-v3 now advertises DOMAIN_ATTR_MSI_MAPPING

RFC v3 -> v4:
- Move doorbell mapping/unmapping in msi.c
- fix ref count issue on set_affinity: in case of a change in the address
  the previous address is decremented
- doorbell map/unmap now is done on msi composition. Should allow the use
  case for platform MSI controllers
- create dma-reserved-iommu.h/c exposing/implementing a new API dedicated
  to reserved IOVA management (looking like dma-iommu glue)
- series reordering to ease the review:
  - first part is related to IOMMU
  - second related to MSI sub-system
  - third related to VFIO (except arm-smmu IOMMU_CAP_INTR_REMAP removal)
- expose the number of requested IOVA pages through VFIO_IOMMU_GET_INFO
  [this partially addresses Marc's comments on iommu_get/put_single_reserved
   size/alignment problematic - which I did not ignore - but I don't know
   how much I can do at the moment]

RFC v2 -> RFC v3:
- should fix wrong handling of some CONFIG combinations:
  CONFIG_IOVA, CONFIG_IOMMU_API, CONFIG_PCI_MSI_IRQ_DOMAIN
- fix MSI_FLAG_IRQ_REMAPPING setting in GICv3 ITS (although not tested)

PATCH v1 -> RFC v2:
- reverted to RFC since it looks more reasonable ;-) the code is split
  between VFIO, IOMMU, MSI controller and I am not sure I did the right
  choices. Also API need to be further discussed.
- iova API usage in arm-smmu.c.
- MSI controller natively programs the MSI addr with either the PA or IOVA.
  This is not done anymore in vfio-pci driver as suggested by Alex.
- check irq remapping capability of the group

RFC v1 [2] -> PATCH v1:
- use the existing dma map/unmap ioctl interface with a flag to register a
  reserved IOVA range. Use the legacy Rb to store this special vfio_dma.
- a single reserved IOVA contiguous region now is allowed
- use of an RB tree indexed by PA to store allocated reserved slots
- use of a vfio_domain iova_domain to manage iova allocation within the
  window provided by the userspace
- vfio alloc_map/unmap_free take a vfio_group handle
- vfio_group handle is cached in vfio_pci_device
- add ref counting to bindings
- user modality enabled at the end of the series


Eric Auger (10):
  iommu: Add DOMAIN_ATTR_MSI_MAPPING attribute
  iommu/arm-smmu: advertise DOMAIN_ATTR_MSI_MAPPING attribute
  iommu: introduce a reserved iova cookie
  iommu/dma-reserved-iommu: alloc/free_reserved_iova_domain
  iommu/dma-reserved-iommu: reserved binding rb-tree and helpers
  iommu/dma-reserved-iommu: iommu_get/put_reserved_iova
  iommu/dma-reserved-iommu: delete bindings in
    iommu_free_reserved_iova_domain
  iommu/dma-reserved_iommu: iommu_msi_mapping_desc_to_domain
  iommu/dma-reserved-iommu: iommu_msi_mapping_translate_msg
  iommu/arm-smmu: call iommu_free_reserved_iova_domain on domain
    destruction

 drivers/iommu/Kconfig              |   8 +
 drivers/iommu/Makefile             |   1 +
 drivers/iommu/arm-smmu-v3.c        |   4 +
 drivers/iommu/arm-smmu.c           |   4 +
 drivers/iommu/dma-reserved-iommu.c | 422 +++++++++++++++++++++++++++++++++++++
 drivers/iommu/iommu.c              |   2 +
 include/linux/dma-reserved-iommu.h | 142 +++++++++++++
 include/linux/iommu.h              |   7 +
 8 files changed, 590 insertions(+)
 create mode 100644 drivers/iommu/dma-reserved-iommu.c
 create mode 100644 include/linux/dma-reserved-iommu.h

-- 
1.9.1

[toc] | [next] | [standalone]

#1382710 — [PATCH v7 05/10] iommu/dma-reserved-iommu: reserved binding rb-tree and helpers

we will need to track which host physical addresses are mapped to
reserved IOVA. In that prospect we introduce a new RB tree indexed
by physical address. This RB tree only is used for reserved IOVA
bindings.

It is expected this RB tree will contain very few bindings. Those
generally correspond to single page mapping one MSI frame (GICv2m
frame or ITS GITS_TRANSLATER frame).

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

---
v5 -> v6:
- add comment about @d->reserved_lock to be held

v3 -> v4:
- that code was formerly in "iommu/arm-smmu: add a reserved binding RB tree"
---
 drivers/iommu/dma-reserved-iommu.c | 63 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/iommu/dma-reserved-iommu.c b/drivers/iommu/dma-reserved-iommu.c
index 2562af0..f6fa18e 100644
--- a/drivers/iommu/dma-reserved-iommu.c
+++ b/drivers/iommu/dma-reserved-iommu.c
@@ -23,6 +23,69 @@ struct reserved_iova_domain {
 	int prot; /* iommu protection attributes to be obeyed */
 };
 
+struct iommu_reserved_binding {
+	struct kref		kref;
+	struct rb_node		node;
+	struct iommu_domain	*domain;
+	phys_addr_t		addr;
+	dma_addr_t		iova;
+	size_t			size;
+};
+
+/* Reserved binding RB-tree manipulation */
+
+/* @d->reserved_lock must be held */
+static struct iommu_reserved_binding *find_reserved_binding(
+				    struct iommu_domain *d,
+				    phys_addr_t start, size_t size)
+{
+	struct rb_node *node = d->reserved_binding_list.rb_node;
+
+	while (node) {
+		struct iommu_reserved_binding *binding =
+			rb_entry(node, struct iommu_reserved_binding, node);
+
+		if (start + size <= binding->addr)
+			node = node->rb_left;
+		else if (start >= binding->addr + binding->size)
+			node = node->rb_right;
+		else
+			return binding;
+	}
+
+	return NULL;
+}
+
+/* @d->reserved_lock must be held */
+static void link_reserved_binding(struct iommu_domain *d,
+				  struct iommu_reserved_binding *new)
+{
+	struct rb_node **link = &d->reserved_binding_list.rb_node;
+	struct rb_node *parent = NULL;
+	struct iommu_reserved_binding *binding;
+
+	while (*link) {
+		parent = *link;
+		binding = rb_entry(parent, struct iommu_reserved_binding,
+				   node);
+
+		if (new->addr + new->size <= binding->addr)
+			link = &(*link)->rb_left;
+		else
+			link = &(*link)->rb_right;
+	}
+
+	rb_link_node(&new->node, parent, link);
+	rb_insert_color(&new->node, &d->reserved_binding_list);
+}
+
+/* @d->reserved_lock must be held */
+static void unlink_reserved_binding(struct iommu_domain *d,
+				    struct iommu_reserved_binding *old)
+{
+	rb_erase(&old->node, &d->reserved_binding_list);
+}
+
 int iommu_alloc_reserved_iova_domain(struct iommu_domain *domain,
 				     dma_addr_t iova, size_t size, int prot,
 				     unsigned long order)
-- 
1.9.1

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

#1383364 — Re: [PATCH v7 05/10] iommu/dma-reserved-iommu: reserved binding rb-tree and helpers

On 19/04/16 17:56, Eric Auger wrote:
> we will need to track which host physical addresses are mapped to
> reserved IOVA. In that prospect we introduce a new RB tree indexed
> by physical address. This RB tree only is used for reserved IOVA
> bindings.
>
> It is expected this RB tree will contain very few bindings.

Sounds like a good reason in favour of using a list, and thus having 
rather less code here ;)

>  Those
> generally correspond to single page mapping one MSI frame (GICv2m
> frame or ITS GITS_TRANSLATER frame).
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
> v5 -> v6:
> - add comment about @d->reserved_lock to be held
>
> v3 -> v4:
> - that code was formerly in "iommu/arm-smmu: add a reserved binding RB tree"
> ---
>   drivers/iommu/dma-reserved-iommu.c | 63 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 63 insertions(+)
>
> diff --git a/drivers/iommu/dma-reserved-iommu.c b/drivers/iommu/dma-reserved-iommu.c
> index 2562af0..f6fa18e 100644
> --- a/drivers/iommu/dma-reserved-iommu.c
> +++ b/drivers/iommu/dma-reserved-iommu.c
> @@ -23,6 +23,69 @@ struct reserved_iova_domain {
>   	int prot; /* iommu protection attributes to be obeyed */
>   };
>
> +struct iommu_reserved_binding {
> +	struct kref		kref;
> +	struct rb_node		node;
> +	struct iommu_domain	*domain;

Hang on, the tree these are in is already embedded in a domain. Ergo we 
can't look them up without first knowing the domain they belong to, so 
what purpose does this guy serve?

Robin.

> +	phys_addr_t		addr;
> +	dma_addr_t		iova;
> +	size_t			size;
> +};
> +
> +/* Reserved binding RB-tree manipulation */
> +
> +/* @d->reserved_lock must be held */
> +static struct iommu_reserved_binding *find_reserved_binding(
> +				    struct iommu_domain *d,
> +				    phys_addr_t start, size_t size)
> +{
> +	struct rb_node *node = d->reserved_binding_list.rb_node;
> +
> +	while (node) {
> +		struct iommu_reserved_binding *binding =
> +			rb_entry(node, struct iommu_reserved_binding, node);
> +
> +		if (start + size <= binding->addr)
> +			node = node->rb_left;
> +		else if (start >= binding->addr + binding->size)
> +			node = node->rb_right;
> +		else
> +			return binding;
> +	}
> +
> +	return NULL;
> +}
> +
> +/* @d->reserved_lock must be held */
> +static void link_reserved_binding(struct iommu_domain *d,
> +				  struct iommu_reserved_binding *new)
> +{
> +	struct rb_node **link = &d->reserved_binding_list.rb_node;
> +	struct rb_node *parent = NULL;
> +	struct iommu_reserved_binding *binding;
> +
> +	while (*link) {
> +		parent = *link;
> +		binding = rb_entry(parent, struct iommu_reserved_binding,
> +				   node);
> +
> +		if (new->addr + new->size <= binding->addr)
> +			link = &(*link)->rb_left;
> +		else
> +			link = &(*link)->rb_right;
> +	}
> +
> +	rb_link_node(&new->node, parent, link);
> +	rb_insert_color(&new->node, &d->reserved_binding_list);
> +}
> +
> +/* @d->reserved_lock must be held */
> +static void unlink_reserved_binding(struct iommu_domain *d,
> +				    struct iommu_reserved_binding *old)
> +{
> +	rb_erase(&old->node, &d->reserved_binding_list);
> +}
> +
>   int iommu_alloc_reserved_iova_domain(struct iommu_domain *domain,
>   				     dma_addr_t iova, size_t size, int prot,
>   				     unsigned long order)
>

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

#1383549 — Re: [PATCH v7 05/10] iommu/dma-reserved-iommu: reserved binding rb-tree and helpers

Robin,
On 04/20/2016 03:12 PM, Robin Murphy wrote:
> On 19/04/16 17:56, Eric Auger wrote:
>> we will need to track which host physical addresses are mapped to
>> reserved IOVA. In that prospect we introduce a new RB tree indexed
>> by physical address. This RB tree only is used for reserved IOVA
>> bindings.
>>
>> It is expected this RB tree will contain very few bindings.
> 
> Sounds like a good reason in favour of using a list, and thus having
> rather less code here ;)

OK will move to a simple list.
> 
>>  Those
>> generally correspond to single page mapping one MSI frame (GICv2m
>> frame or ITS GITS_TRANSLATER frame).
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>> v5 -> v6:
>> - add comment about @d->reserved_lock to be held
>>
>> v3 -> v4:
>> - that code was formerly in "iommu/arm-smmu: add a reserved binding RB
>> tree"
>> ---
>>   drivers/iommu/dma-reserved-iommu.c | 63
>> ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 63 insertions(+)
>>
>> diff --git a/drivers/iommu/dma-reserved-iommu.c
>> b/drivers/iommu/dma-reserved-iommu.c
>> index 2562af0..f6fa18e 100644
>> --- a/drivers/iommu/dma-reserved-iommu.c
>> +++ b/drivers/iommu/dma-reserved-iommu.c
>> @@ -23,6 +23,69 @@ struct reserved_iova_domain {
>>       int prot; /* iommu protection attributes to be obeyed */
>>   };
>>
>> +struct iommu_reserved_binding {
>> +    struct kref        kref;
>> +    struct rb_node        node;
>> +    struct iommu_domain    *domain;
> 
> Hang on, the tree these are in is already embedded in a domain. Ergo we
> can't look them up without first knowing the domain they belong to, so
> what purpose does this guy serve?
this is used on the kref_put. The release function takes a kref; then we
get the container to retrieve the binding and storing the domain here
enables to unlink the node.

Best Regards

Eric
> 
> Robin.
> 
>> +    phys_addr_t        addr;
>> +    dma_addr_t        iova;
>> +    size_t            size;
>> +};
>> +
>> +/* Reserved binding RB-tree manipulation */
>> +
>> +/* @d->reserved_lock must be held */
>> +static struct iommu_reserved_binding *find_reserved_binding(
>> +                    struct iommu_domain *d,
>> +                    phys_addr_t start, size_t size)
>> +{
>> +    struct rb_node *node = d->reserved_binding_list.rb_node;
>> +
>> +    while (node) {
>> +        struct iommu_reserved_binding *binding =
>> +            rb_entry(node, struct iommu_reserved_binding, node);
>> +
>> +        if (start + size <= binding->addr)
>> +            node = node->rb_left;
>> +        else if (start >= binding->addr + binding->size)
>> +            node = node->rb_right;
>> +        else
>> +            return binding;
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +/* @d->reserved_lock must be held */
>> +static void link_reserved_binding(struct iommu_domain *d,
>> +                  struct iommu_reserved_binding *new)
>> +{
>> +    struct rb_node **link = &d->reserved_binding_list.rb_node;
>> +    struct rb_node *parent = NULL;
>> +    struct iommu_reserved_binding *binding;
>> +
>> +    while (*link) {
>> +        parent = *link;
>> +        binding = rb_entry(parent, struct iommu_reserved_binding,
>> +                   node);
>> +
>> +        if (new->addr + new->size <= binding->addr)
>> +            link = &(*link)->rb_left;
>> +        else
>> +            link = &(*link)->rb_right;
>> +    }
>> +
>> +    rb_link_node(&new->node, parent, link);
>> +    rb_insert_color(&new->node, &d->reserved_binding_list);
>> +}
>> +
>> +/* @d->reserved_lock must be held */
>> +static void unlink_reserved_binding(struct iommu_domain *d,
>> +                    struct iommu_reserved_binding *old)
>> +{
>> +    rb_erase(&old->node, &d->reserved_binding_list);
>> +}
>> +
>>   int iommu_alloc_reserved_iova_domain(struct iommu_domain *domain,
>>                        dma_addr_t iova, size_t size, int prot,
>>                        unsigned long order)
>>
> 

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

#1385151 — Re: [PATCH v7 05/10] iommu/dma-reserved-iommu: reserved binding rb-tree and helpers

On 20/04/16 17:18, Eric Auger wrote:
> Robin,
> On 04/20/2016 03:12 PM, Robin Murphy wrote:
>> On 19/04/16 17:56, Eric Auger wrote:
>>> we will need to track which host physical addresses are mapped to
>>> reserved IOVA. In that prospect we introduce a new RB tree indexed
>>> by physical address. This RB tree only is used for reserved IOVA
>>> bindings.
>>>
>>> It is expected this RB tree will contain very few bindings.
>>
>> Sounds like a good reason in favour of using a list, and thus having
>> rather less code here ;)
>
> OK will move to a simple list.
>>
>>>   Those
>>> generally correspond to single page mapping one MSI frame (GICv2m
>>> frame or ITS GITS_TRANSLATER frame).
>>>
>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>
>>> ---
>>> v5 -> v6:
>>> - add comment about @d->reserved_lock to be held
>>>
>>> v3 -> v4:
>>> - that code was formerly in "iommu/arm-smmu: add a reserved binding RB
>>> tree"
>>> ---
>>>    drivers/iommu/dma-reserved-iommu.c | 63
>>> ++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 63 insertions(+)
>>>
>>> diff --git a/drivers/iommu/dma-reserved-iommu.c
>>> b/drivers/iommu/dma-reserved-iommu.c
>>> index 2562af0..f6fa18e 100644
>>> --- a/drivers/iommu/dma-reserved-iommu.c
>>> +++ b/drivers/iommu/dma-reserved-iommu.c
>>> @@ -23,6 +23,69 @@ struct reserved_iova_domain {
>>>        int prot; /* iommu protection attributes to be obeyed */
>>>    };
>>>
>>> +struct iommu_reserved_binding {
>>> +    struct kref        kref;
>>> +    struct rb_node        node;
>>> +    struct iommu_domain    *domain;
>>
>> Hang on, the tree these are in is already embedded in a domain. Ergo we
>> can't look them up without first knowing the domain they belong to, so
>> what purpose does this guy serve?
> this is used on the kref_put. The release function takes a kref; then we
> get the container to retrieve the binding and storing the domain here
> enables to unlink the node.

Ah yes, I see now - that's annoyingly awkward. I think it could possibly 
be avoided in the list case (if the kref_put callback just did 
list_del_init(), the entry could then be checked for an empty list and 
disposed of outside the lock), but I'm not sure whether that's really 
worth the fuss. Oh well.

Robin.

> Best Regards
>
> Eric
>>
>> Robin.
>>
>>> +    phys_addr_t        addr;
>>> +    dma_addr_t        iova;
>>> +    size_t            size;
>>> +};
>>> +
>>> +/* Reserved binding RB-tree manipulation */
>>> +
>>> +/* @d->reserved_lock must be held */
>>> +static struct iommu_reserved_binding *find_reserved_binding(
>>> +                    struct iommu_domain *d,
>>> +                    phys_addr_t start, size_t size)
>>> +{
>>> +    struct rb_node *node = d->reserved_binding_list.rb_node;
>>> +
>>> +    while (node) {
>>> +        struct iommu_reserved_binding *binding =
>>> +            rb_entry(node, struct iommu_reserved_binding, node);
>>> +
>>> +        if (start + size <= binding->addr)
>>> +            node = node->rb_left;
>>> +        else if (start >= binding->addr + binding->size)
>>> +            node = node->rb_right;
>>> +        else
>>> +            return binding;
>>> +    }
>>> +
>>> +    return NULL;
>>> +}
>>> +
>>> +/* @d->reserved_lock must be held */
>>> +static void link_reserved_binding(struct iommu_domain *d,
>>> +                  struct iommu_reserved_binding *new)
>>> +{
>>> +    struct rb_node **link = &d->reserved_binding_list.rb_node;
>>> +    struct rb_node *parent = NULL;
>>> +    struct iommu_reserved_binding *binding;
>>> +
>>> +    while (*link) {
>>> +        parent = *link;
>>> +        binding = rb_entry(parent, struct iommu_reserved_binding,
>>> +                   node);
>>> +
>>> +        if (new->addr + new->size <= binding->addr)
>>> +            link = &(*link)->rb_left;
>>> +        else
>>> +            link = &(*link)->rb_right;
>>> +    }
>>> +
>>> +    rb_link_node(&new->node, parent, link);
>>> +    rb_insert_color(&new->node, &d->reserved_binding_list);
>>> +}
>>> +
>>> +/* @d->reserved_lock must be held */
>>> +static void unlink_reserved_binding(struct iommu_domain *d,
>>> +                    struct iommu_reserved_binding *old)
>>> +{
>>> +    rb_erase(&old->node, &d->reserved_binding_list);
>>> +}
>>> +
>>>    int iommu_alloc_reserved_iova_domain(struct iommu_domain *domain,
>>>                         dma_addr_t iova, size_t size, int prot,
>>>                         unsigned long order)
>>>
>>
>

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

#1382711 — [PATCH v7 10/10] iommu/arm-smmu: call iommu_free_reserved_iova_domain on domain destruction

When the domain gets destroyed, let's make sure all reserved iova
resources get released.

Choice is made to put that call in arm-smmu(-v3).c to do something similar
to what was done for iommu_put_dma_cookie.

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

---

v7: new
---
 drivers/iommu/arm-smmu-v3.c | 2 ++
 drivers/iommu/arm-smmu.c    | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index a077a35..afd0dac 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -22,6 +22,7 @@
 
 #include <linux/delay.h>
 #include <linux/dma-iommu.h>
+#include <linux/dma-reserved-iommu.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
 #include <linux/iommu.h>
@@ -1444,6 +1445,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 
 	iommu_put_dma_cookie(domain);
+	iommu_free_reserved_iova_domain(domain);
 	free_io_pgtable_ops(smmu_domain->pgtbl_ops);
 
 	/* Free the CD and ASID, if we allocated them */
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 8cd7b8a..492339f 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -30,6 +30,7 @@
 
 #include <linux/delay.h>
 #include <linux/dma-iommu.h>
+#include <linux/dma-reserved-iommu.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
@@ -1009,6 +1010,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
 	 * already been detached.
 	 */
 	iommu_put_dma_cookie(domain);
+	iommu_free_reserved_iova_domain(domain);
 	arm_smmu_destroy_domain_context(domain);
 	kfree(smmu_domain);
 }
-- 
1.9.1

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

#1383581 — Re: [PATCH v7 10/10] iommu/arm-smmu: call iommu_free_reserved_iova_domain on domain destruction

On 19/04/16 17:56, Eric Auger wrote:
> When the domain gets destroyed, let's make sure all reserved iova
> resources get released.
>
> Choice is made to put that call in arm-smmu(-v3).c to do something similar
> to what was done for iommu_put_dma_cookie.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
>
> v7: new
> ---
>   drivers/iommu/arm-smmu-v3.c | 2 ++
>   drivers/iommu/arm-smmu.c    | 2 ++
>   2 files changed, 4 insertions(+)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index a077a35..afd0dac 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -22,6 +22,7 @@
>
>   #include <linux/delay.h>
>   #include <linux/dma-iommu.h>
> +#include <linux/dma-reserved-iommu.h>
>   #include <linux/err.h>
>   #include <linux/interrupt.h>
>   #include <linux/iommu.h>
> @@ -1444,6 +1445,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
>   	struct arm_smmu_device *smmu = smmu_domain->smmu;
>
>   	iommu_put_dma_cookie(domain);
> +	iommu_free_reserved_iova_domain(domain);

Yikes! No, drivers shouldn't be randomly freeing things they didn't 
allocate - the owner of the domain, who presumably allocated the thing, 
can call that right _before_ they call iommu_domain_free().

>   	free_io_pgtable_ops(smmu_domain->pgtbl_ops);
>
>   	/* Free the CD and ASID, if we allocated them */
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 8cd7b8a..492339f 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -30,6 +30,7 @@
>
>   #include <linux/delay.h>
>   #include <linux/dma-iommu.h>
> +#include <linux/dma-reserved-iommu.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/err.h>
>   #include <linux/interrupt.h>
> @@ -1009,6 +1010,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
>   	 * already been detached.
>   	 */
>   	iommu_put_dma_cookie(domain);
> +	iommu_free_reserved_iova_domain(domain);

...which has the added bonus of preventing needless duplication everywhere.

Robin.

>   	arm_smmu_destroy_domain_context(domain);
>   	kfree(smmu_domain);
>   }
>

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

#1383948 — Re: [PATCH v7 10/10] iommu/arm-smmu: call iommu_free_reserved_iova_domain on domain destruction

Hi Robin,
On 04/20/2016 07:35 PM, Robin Murphy wrote:
> On 19/04/16 17:56, Eric Auger wrote:
>> When the domain gets destroyed, let's make sure all reserved iova
>> resources get released.
>>
>> Choice is made to put that call in arm-smmu(-v3).c to do something
>> similar
>> to what was done for iommu_put_dma_cookie.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v7: new
>> ---
>>   drivers/iommu/arm-smmu-v3.c | 2 ++
>>   drivers/iommu/arm-smmu.c    | 2 ++
>>   2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index a077a35..afd0dac 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -22,6 +22,7 @@
>>
>>   #include <linux/delay.h>
>>   #include <linux/dma-iommu.h>
>> +#include <linux/dma-reserved-iommu.h>
>>   #include <linux/err.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/iommu.h>
>> @@ -1444,6 +1445,7 @@ static void arm_smmu_domain_free(struct
>> iommu_domain *domain)
>>       struct arm_smmu_device *smmu = smmu_domain->smmu;
>>
>>       iommu_put_dma_cookie(domain);
>> +    iommu_free_reserved_iova_domain(domain);
> 
> Yikes! No, drivers shouldn't be randomly freeing things they didn't
> allocate - the owner of the domain, who presumably allocated the thing,
> can call that right _before_ they call iommu_domain_free().
OK I move that back to vfio_iommu_type1.c.

Thanks

Eric
> 
>>       free_io_pgtable_ops(smmu_domain->pgtbl_ops);
>>
>>       /* Free the CD and ASID, if we allocated them */
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 8cd7b8a..492339f 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -30,6 +30,7 @@
>>
>>   #include <linux/delay.h>
>>   #include <linux/dma-iommu.h>
>> +#include <linux/dma-reserved-iommu.h>
>>   #include <linux/dma-mapping.h>
>>   #include <linux/err.h>
>>   #include <linux/interrupt.h>
>> @@ -1009,6 +1010,7 @@ static void arm_smmu_domain_free(struct
>> iommu_domain *domain)
>>        * already been detached.
>>        */
>>       iommu_put_dma_cookie(domain);
>> +    iommu_free_reserved_iova_domain(domain);
> 
> ...which has the added bonus of preventing needless duplication everywhere.
> 
> Robin.
> 
>>       arm_smmu_destroy_domain_context(domain);
>>       kfree(smmu_domain);
>>   }
>>
> 

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

#1382712 — [PATCH v7 09/10] iommu/dma-reserved-iommu: iommu_msi_mapping_translate_msg

Introduce iommu_msi_mapping_translate_msg whose role consists in
detecting whether the device's MSIs must to be mapped into an IOMMU.
It case it must, the function overrides the MSI msg originally composed
and replaces the doorbell's PA by a pre-allocated and pre-mapped reserved
IOVA. In case the corresponding PA region is not found, the function
returns an error.

This function is likely to be called in some code that cannot sleep. This
is the reason why the allocation/mapping is done separately.

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

---

v7: creation
---
 drivers/iommu/dma-reserved-iommu.c | 69 ++++++++++++++++++++++++++++++++++++++
 include/linux/dma-reserved-iommu.h | 27 +++++++++++++++
 2 files changed, 96 insertions(+)

diff --git a/drivers/iommu/dma-reserved-iommu.c b/drivers/iommu/dma-reserved-iommu.c
index 907a17f..603ee45 100644
--- a/drivers/iommu/dma-reserved-iommu.c
+++ b/drivers/iommu/dma-reserved-iommu.c
@@ -18,6 +18,14 @@
 #include <linux/iommu.h>
 #include <linux/iova.h>
 #include <linux/msi.h>
+#include <linux/irq.h>
+
+#ifdef CONFIG_PHYS_ADDR_T_64BIT
+#define msg_to_phys_addr(msg) \
+	(((phys_addr_t)((msg)->address_hi) << 32) | (msg)->address_lo)
+#else
+#define msg_to_phys_addr(msg)	((msg)->address_lo)
+#endif
 
 struct reserved_iova_domain {
 	struct iova_domain *iovad;
@@ -351,3 +359,64 @@ struct iommu_domain *iommu_msi_mapping_desc_to_domain(struct msi_desc *desc)
 	return d;
 }
 EXPORT_SYMBOL_GPL(iommu_msi_mapping_desc_to_domain);
+
+static dma_addr_t iommu_find_reserved_iova(struct iommu_domain *domain,
+				    phys_addr_t addr, size_t size)
+{
+	unsigned long  base_pfn, end_pfn, nb_iommu_pages, order, flags;
+	size_t iommu_page_size, binding_size;
+	struct iommu_reserved_binding *b;
+	phys_addr_t aligned_base, offset;
+	dma_addr_t iova = DMA_ERROR_CODE;
+	struct iova_domain *iovad;
+
+	spin_lock_irqsave(&domain->reserved_lock, flags);
+
+	iovad = (struct iova_domain *)domain->reserved_iova_cookie;
+
+	if (!iovad)
+		goto unlock;
+
+	order = iova_shift(iovad);
+	base_pfn = addr >> order;
+	end_pfn = (addr + size - 1) >> order;
+	aligned_base = base_pfn << order;
+	offset = addr - aligned_base;
+	nb_iommu_pages = end_pfn - base_pfn + 1;
+	iommu_page_size = 1 << order;
+	binding_size = nb_iommu_pages * iommu_page_size;
+
+	b = find_reserved_binding(domain, aligned_base, binding_size);
+	if (b && (b->addr <= aligned_base) &&
+		(aligned_base + binding_size <=  b->addr + b->size))
+		iova = b->iova + offset + aligned_base - b->addr;
+unlock:
+	spin_unlock_irqrestore(&domain->reserved_lock, flags);
+	return iova;
+}
+
+int iommu_msi_mapping_translate_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct iommu_domain *d;
+	struct msi_desc *desc;
+	dma_addr_t iova;
+
+	desc = irq_data_get_msi_desc(data);
+	if (!desc)
+		return -EINVAL;
+
+	d = iommu_msi_mapping_desc_to_domain(desc);
+	if (!d)
+		return 0;
+
+	iova = iommu_find_reserved_iova(d, msg_to_phys_addr(msg),
+					sizeof(phys_addr_t));
+
+	if (iova == DMA_ERROR_CODE)
+		return -EINVAL;
+
+	msg->address_lo = lower_32_bits(iova);
+	msg->address_hi = upper_32_bits(iova);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iommu_msi_mapping_translate_msg);
diff --git a/include/linux/dma-reserved-iommu.h b/include/linux/dma-reserved-iommu.h
index 8373929..04e1912f 100644
--- a/include/linux/dma-reserved-iommu.h
+++ b/include/linux/dma-reserved-iommu.h
@@ -20,6 +20,8 @@
 
 struct iommu_domain;
 struct msi_desc;
+struct irq_data;
+struct msi_msg;
 
 #ifdef CONFIG_IOMMU_DMA_RESERVED
 
@@ -82,6 +84,25 @@ void iommu_put_reserved_iova(struct iommu_domain *domain, phys_addr_t addr);
  */
 struct iommu_domain *iommu_msi_mapping_desc_to_domain(struct msi_desc *desc);
 
+/**
+ * iommu_msi_mapping_translate_msg: in case the MSI transaction is translated
+ * by an IOMMU, the msg address must be an IOVA instead of a physical address.
+ * This function overwrites the original MSI message containing the doorbell
+ * physical address, result of the primary composition, with the doorbell IOVA.
+ *
+ * The doorbell physical address must be bound previously to an IOVA using
+ * iommu_get_reserved_iova
+ *
+ * @data: irq data handle
+ * @msg: original msi message containing the PA to be overwritten with
+ * the IOVA
+ *
+ * return 0 if the MSI does not need to be mapped or when the PA/IOVA
+ * were successfully swapped; return -EINVAL if the addresses need
+ * to be swapped but not IOMMU binding is found
+ */
+int iommu_msi_mapping_translate_msg(struct irq_data *data, struct msi_msg *msg);
+
 #else
 
 static inline int
@@ -111,5 +132,11 @@ iommu_msi_mapping_desc_to_domain(struct msi_desc *desc)
 	return NULL;
 }
 
+static inline int iommu_msi_mapping_translate_msg(struct irq_data *data,
+						  struct msi_msg *msg)
+{
+	return 0;
+}
+
 #endif	/* CONFIG_IOMMU_DMA_RESERVED */
 #endif	/* __DMA_RESERVED_IOMMU_H */
-- 
1.9.1

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

#1383208 — Re: [PATCH v7 09/10] iommu/dma-reserved-iommu: iommu_msi_mapping_translate_msg

On 19/04/16 17:56, Eric Auger wrote:
> Introduce iommu_msi_mapping_translate_msg whose role consists in
> detecting whether the device's MSIs must to be mapped into an IOMMU.
> It case it must, the function overrides the MSI msg originally composed
> and replaces the doorbell's PA by a pre-allocated and pre-mapped reserved
> IOVA. In case the corresponding PA region is not found, the function
> returns an error.
> 
> This function is likely to be called in some code that cannot sleep. This
> is the reason why the allocation/mapping is done separately.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v7: creation
> ---
>  drivers/iommu/dma-reserved-iommu.c | 69 ++++++++++++++++++++++++++++++++++++++
>  include/linux/dma-reserved-iommu.h | 27 +++++++++++++++
>  2 files changed, 96 insertions(+)
> 
> diff --git a/drivers/iommu/dma-reserved-iommu.c b/drivers/iommu/dma-reserved-iommu.c
> index 907a17f..603ee45 100644
> --- a/drivers/iommu/dma-reserved-iommu.c
> +++ b/drivers/iommu/dma-reserved-iommu.c
> @@ -18,6 +18,14 @@
>  #include <linux/iommu.h>
>  #include <linux/iova.h>
>  #include <linux/msi.h>
> +#include <linux/irq.h>
> +
> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
> +#define msg_to_phys_addr(msg) \
> +	(((phys_addr_t)((msg)->address_hi) << 32) | (msg)->address_lo)
> +#else
> +#define msg_to_phys_addr(msg)	((msg)->address_lo)
> +#endif
>  
>  struct reserved_iova_domain {
>  	struct iova_domain *iovad;
> @@ -351,3 +359,64 @@ struct iommu_domain *iommu_msi_mapping_desc_to_domain(struct msi_desc *desc)
>  	return d;
>  }
>  EXPORT_SYMBOL_GPL(iommu_msi_mapping_desc_to_domain);
> +
> +static dma_addr_t iommu_find_reserved_iova(struct iommu_domain *domain,
> +				    phys_addr_t addr, size_t size)
> +{
> +	unsigned long  base_pfn, end_pfn, nb_iommu_pages, order, flags;
> +	size_t iommu_page_size, binding_size;
> +	struct iommu_reserved_binding *b;
> +	phys_addr_t aligned_base, offset;
> +	dma_addr_t iova = DMA_ERROR_CODE;
> +	struct iova_domain *iovad;
> +
> +	spin_lock_irqsave(&domain->reserved_lock, flags);
> +
> +	iovad = (struct iova_domain *)domain->reserved_iova_cookie;
> +
> +	if (!iovad)
> +		goto unlock;
> +
> +	order = iova_shift(iovad);
> +	base_pfn = addr >> order;
> +	end_pfn = (addr + size - 1) >> order;
> +	aligned_base = base_pfn << order;
> +	offset = addr - aligned_base;
> +	nb_iommu_pages = end_pfn - base_pfn + 1;
> +	iommu_page_size = 1 << order;
> +	binding_size = nb_iommu_pages * iommu_page_size;
> +
> +	b = find_reserved_binding(domain, aligned_base, binding_size);
> +	if (b && (b->addr <= aligned_base) &&
> +		(aligned_base + binding_size <=  b->addr + b->size))
> +		iova = b->iova + offset + aligned_base - b->addr;
> +unlock:
> +	spin_unlock_irqrestore(&domain->reserved_lock, flags);
> +	return iova;
> +}
> +
> +int iommu_msi_mapping_translate_msg(struct irq_data *data, struct msi_msg *msg)
> +{

I'm really not keen on passing a full irq_data to something that doesn't
really care about it. What this really needs is the device that
generates the MSIs, which makes a lot more sense (you get the device and
the address from the msi_msg).

Or am I getting it wrong?

> +	struct iommu_domain *d;
> +	struct msi_desc *desc;
> +	dma_addr_t iova;
> +
> +	desc = irq_data_get_msi_desc(data);
> +	if (!desc)
> +		return -EINVAL;
> +
> +	d = iommu_msi_mapping_desc_to_domain(desc);
> +	if (!d)
> +		return 0;
> +
> +	iova = iommu_find_reserved_iova(d, msg_to_phys_addr(msg),
> +					sizeof(phys_addr_t));
> +
> +	if (iova == DMA_ERROR_CODE)
> +		return -EINVAL;
> +
> +	msg->address_lo = lower_32_bits(iova);
> +	msg->address_hi = upper_32_bits(iova);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_msi_mapping_translate_msg);
> diff --git a/include/linux/dma-reserved-iommu.h b/include/linux/dma-reserved-iommu.h
> index 8373929..04e1912f 100644
> --- a/include/linux/dma-reserved-iommu.h
> +++ b/include/linux/dma-reserved-iommu.h
> @@ -20,6 +20,8 @@
>  
>  struct iommu_domain;
>  struct msi_desc;
> +struct irq_data;
> +struct msi_msg;
>  
>  #ifdef CONFIG_IOMMU_DMA_RESERVED
>  
> @@ -82,6 +84,25 @@ void iommu_put_reserved_iova(struct iommu_domain *domain, phys_addr_t addr);
>   */
>  struct iommu_domain *iommu_msi_mapping_desc_to_domain(struct msi_desc *desc);
>  
> +/**
> + * iommu_msi_mapping_translate_msg: in case the MSI transaction is translated
> + * by an IOMMU, the msg address must be an IOVA instead of a physical address.
> + * This function overwrites the original MSI message containing the doorbell
> + * physical address, result of the primary composition, with the doorbell IOVA.
> + *
> + * The doorbell physical address must be bound previously to an IOVA using
> + * iommu_get_reserved_iova
> + *
> + * @data: irq data handle
> + * @msg: original msi message containing the PA to be overwritten with
> + * the IOVA
> + *
> + * return 0 if the MSI does not need to be mapped or when the PA/IOVA
> + * were successfully swapped; return -EINVAL if the addresses need
> + * to be swapped but not IOMMU binding is found
> + */
> +int iommu_msi_mapping_translate_msg(struct irq_data *data, struct msi_msg *msg);
> +
>  #else
>  
>  static inline int
> @@ -111,5 +132,11 @@ iommu_msi_mapping_desc_to_domain(struct msi_desc *desc)
>  	return NULL;
>  }
>  
> +static inline int iommu_msi_mapping_translate_msg(struct irq_data *data,
> +						  struct msi_msg *msg)
> +{
> +	return 0;
> +}
> +
>  #endif	/* CONFIG_IOMMU_DMA_RESERVED */
>  #endif	/* __DMA_RESERVED_IOMMU_H */
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

#1383352 — Re: [PATCH v7 09/10] iommu/dma-reserved-iommu: iommu_msi_mapping_translate_msg

On 04/20/2016 11:38 AM, Marc Zyngier wrote:
> On 19/04/16 17:56, Eric Auger wrote:
>> Introduce iommu_msi_mapping_translate_msg whose role consists in
>> detecting whether the device's MSIs must to be mapped into an IOMMU.
>> It case it must, the function overrides the MSI msg originally composed
>> and replaces the doorbell's PA by a pre-allocated and pre-mapped reserved
>> IOVA. In case the corresponding PA region is not found, the function
>> returns an error.
>>
>> This function is likely to be called in some code that cannot sleep. This
>> is the reason why the allocation/mapping is done separately.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v7: creation
>> ---
>>  drivers/iommu/dma-reserved-iommu.c | 69 ++++++++++++++++++++++++++++++++++++++
>>  include/linux/dma-reserved-iommu.h | 27 +++++++++++++++
>>  2 files changed, 96 insertions(+)
>>
>> diff --git a/drivers/iommu/dma-reserved-iommu.c b/drivers/iommu/dma-reserved-iommu.c
>> index 907a17f..603ee45 100644
>> --- a/drivers/iommu/dma-reserved-iommu.c
>> +++ b/drivers/iommu/dma-reserved-iommu.c
>> @@ -18,6 +18,14 @@
>>  #include <linux/iommu.h>
>>  #include <linux/iova.h>
>>  #include <linux/msi.h>
>> +#include <linux/irq.h>
>> +
>> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
>> +#define msg_to_phys_addr(msg) \
>> +	(((phys_addr_t)((msg)->address_hi) << 32) | (msg)->address_lo)
>> +#else
>> +#define msg_to_phys_addr(msg)	((msg)->address_lo)
>> +#endif
>>  
>>  struct reserved_iova_domain {
>>  	struct iova_domain *iovad;
>> @@ -351,3 +359,64 @@ struct iommu_domain *iommu_msi_mapping_desc_to_domain(struct msi_desc *desc)
>>  	return d;
>>  }
>>  EXPORT_SYMBOL_GPL(iommu_msi_mapping_desc_to_domain);
>> +
>> +static dma_addr_t iommu_find_reserved_iova(struct iommu_domain *domain,
>> +				    phys_addr_t addr, size_t size)
>> +{
>> +	unsigned long  base_pfn, end_pfn, nb_iommu_pages, order, flags;
>> +	size_t iommu_page_size, binding_size;
>> +	struct iommu_reserved_binding *b;
>> +	phys_addr_t aligned_base, offset;
>> +	dma_addr_t iova = DMA_ERROR_CODE;
>> +	struct iova_domain *iovad;
>> +
>> +	spin_lock_irqsave(&domain->reserved_lock, flags);
>> +
>> +	iovad = (struct iova_domain *)domain->reserved_iova_cookie;
>> +
>> +	if (!iovad)
>> +		goto unlock;
>> +
>> +	order = iova_shift(iovad);
>> +	base_pfn = addr >> order;
>> +	end_pfn = (addr + size - 1) >> order;
>> +	aligned_base = base_pfn << order;
>> +	offset = addr - aligned_base;
>> +	nb_iommu_pages = end_pfn - base_pfn + 1;
>> +	iommu_page_size = 1 << order;
>> +	binding_size = nb_iommu_pages * iommu_page_size;
>> +
>> +	b = find_reserved_binding(domain, aligned_base, binding_size);
>> +	if (b && (b->addr <= aligned_base) &&
>> +		(aligned_base + binding_size <=  b->addr + b->size))
>> +		iova = b->iova + offset + aligned_base - b->addr;
>> +unlock:
>> +	spin_unlock_irqrestore(&domain->reserved_lock, flags);
>> +	return iova;
>> +}
>> +
>> +int iommu_msi_mapping_translate_msg(struct irq_data *data, struct msi_msg *msg)
>> +{
> 
> I'm really not keen on passing a full irq_data to something that doesn't
> really care about it. What this really needs is the device that
> generates the MSIs, which makes a lot more sense (you get the device and
> the address from the msi_msg).
> 
> Or am I getting it wrong?

No I have no objection. I eventually decided to keep the irq_data*
parameter to be homegeneous with irq_chip_compose_msi_msg and I found
the function name understandable. However I also understand it looks
weird to get this irq-data incursion in this API

will change into iommu_msi_msg_pa_to_va, as you proposed.

Thank you for your time

Eric


> 
>> +	struct iommu_domain *d;
>> +	struct msi_desc *desc;
>> +	dma_addr_t iova;
>> +
>> +	desc = irq_data_get_msi_desc(data);
>> +	if (!desc)
>> +		return -EINVAL;
>> +
>> +	d = iommu_msi_mapping_desc_to_domain(desc);
>> +	if (!d)
>> +		return 0;
>> +
>> +	iova = iommu_find_reserved_iova(d, msg_to_phys_addr(msg),
>> +					sizeof(phys_addr_t));
>> +
>> +	if (iova == DMA_ERROR_CODE)
>> +		return -EINVAL;
>> +
>> +	msg->address_lo = lower_32_bits(iova);
>> +	msg->address_hi = upper_32_bits(iova);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_msi_mapping_translate_msg);
>> diff --git a/include/linux/dma-reserved-iommu.h b/include/linux/dma-reserved-iommu.h
>> index 8373929..04e1912f 100644
>> --- a/include/linux/dma-reserved-iommu.h
>> +++ b/include/linux/dma-reserved-iommu.h
>> @@ -20,6 +20,8 @@
>>  
>>  struct iommu_domain;
>>  struct msi_desc;
>> +struct irq_data;
>> +struct msi_msg;
>>  
>>  #ifdef CONFIG_IOMMU_DMA_RESERVED
>>  
>> @@ -82,6 +84,25 @@ void iommu_put_reserved_iova(struct iommu_domain *domain, phys_addr_t addr);
>>   */
>>  struct iommu_domain *iommu_msi_mapping_desc_to_domain(struct msi_desc *desc);
>>  
>> +/**
>> + * iommu_msi_mapping_translate_msg: in case the MSI transaction is translated
>> + * by an IOMMU, the msg address must be an IOVA instead of a physical address.
>> + * This function overwrites the original MSI message containing the doorbell
>> + * physical address, result of the primary composition, with the doorbell IOVA.
>> + *
>> + * The doorbell physical address must be bound previously to an IOVA using
>> + * iommu_get_reserved_iova
>> + *
>> + * @data: irq data handle
>> + * @msg: original msi message containing the PA to be overwritten with
>> + * the IOVA
>> + *
>> + * return 0 if the MSI does not need to be mapped or when the PA/IOVA
>> + * were successfully swapped; return -EINVAL if the addresses need
>> + * to be swapped but not IOMMU binding is found
>> + */
>> +int iommu_msi_mapping_translate_msg(struct irq_data *data, struct msi_msg *msg);
>> +
>>  #else
>>  
>>  static inline int
>> @@ -111,5 +132,11 @@ iommu_msi_mapping_desc_to_domain(struct msi_desc *desc)
>>  	return NULL;
>>  }
>>  
>> +static inline int iommu_msi_mapping_translate_msg(struct irq_data *data,
>> +						  struct msi_msg *msg)
>> +{
>> +	return 0;
>> +}
>> +
>>  #endif	/* CONFIG_IOMMU_DMA_RESERVED */
>>  #endif	/* __DMA_RESERVED_IOMMU_H */
>>
> 
> Thanks,
> 
> 	M.
> 

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

#1383579 — Re: [PATCH v7 09/10] iommu/dma-reserved-iommu: iommu_msi_mapping_translate_msg

On 19/04/16 17:56, Eric Auger wrote:
> Introduce iommu_msi_mapping_translate_msg whose role consists in
> detecting whether the device's MSIs must to be mapped into an IOMMU.
> It case it must, the function overrides the MSI msg originally composed
> and replaces the doorbell's PA by a pre-allocated and pre-mapped reserved
> IOVA. In case the corresponding PA region is not found, the function
> returns an error.
>
> This function is likely to be called in some code that cannot sleep. This
> is the reason why the allocation/mapping is done separately.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
>
> v7: creation
> ---
>   drivers/iommu/dma-reserved-iommu.c | 69 ++++++++++++++++++++++++++++++++++++++
>   include/linux/dma-reserved-iommu.h | 27 +++++++++++++++
>   2 files changed, 96 insertions(+)
>
> diff --git a/drivers/iommu/dma-reserved-iommu.c b/drivers/iommu/dma-reserved-iommu.c
> index 907a17f..603ee45 100644
> --- a/drivers/iommu/dma-reserved-iommu.c
> +++ b/drivers/iommu/dma-reserved-iommu.c
> @@ -18,6 +18,14 @@
>   #include <linux/iommu.h>
>   #include <linux/iova.h>
>   #include <linux/msi.h>
> +#include <linux/irq.h>
> +
> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
> +#define msg_to_phys_addr(msg) \
> +	(((phys_addr_t)((msg)->address_hi) << 32) | (msg)->address_lo)
> +#else
> +#define msg_to_phys_addr(msg)	((msg)->address_lo)
> +#endif
>
>   struct reserved_iova_domain {
>   	struct iova_domain *iovad;
> @@ -351,3 +359,64 @@ struct iommu_domain *iommu_msi_mapping_desc_to_domain(struct msi_desc *desc)
>   	return d;
>   }
>   EXPORT_SYMBOL_GPL(iommu_msi_mapping_desc_to_domain);
> +
> +static dma_addr_t iommu_find_reserved_iova(struct iommu_domain *domain,
> +				    phys_addr_t addr, size_t size)
> +{
> +	unsigned long  base_pfn, end_pfn, nb_iommu_pages, order, flags;
> +	size_t iommu_page_size, binding_size;
> +	struct iommu_reserved_binding *b;
> +	phys_addr_t aligned_base, offset;
> +	dma_addr_t iova = DMA_ERROR_CODE;
> +	struct iova_domain *iovad;
> +
> +	spin_lock_irqsave(&domain->reserved_lock, flags);
> +
> +	iovad = (struct iova_domain *)domain->reserved_iova_cookie;
> +
> +	if (!iovad)
> +		goto unlock;
> +
> +	order = iova_shift(iovad);
> +	base_pfn = addr >> order;
> +	end_pfn = (addr + size - 1) >> order;
> +	aligned_base = base_pfn << order;
> +	offset = addr - aligned_base;
> +	nb_iommu_pages = end_pfn - base_pfn + 1;
> +	iommu_page_size = 1 << order;
> +	binding_size = nb_iommu_pages * iommu_page_size;

This all looks rather familiar...

> +	b = find_reserved_binding(domain, aligned_base, binding_size);

...which implies that at least some of it should be factored into that guy.

> +	if (b && (b->addr <= aligned_base) &&
> +		(aligned_base + binding_size <=  b->addr + b->size))
> +		iova = b->iova + offset + aligned_base - b->addr;
> +unlock:
> +	spin_unlock_irqrestore(&domain->reserved_lock, flags);
> +	return iova;
> +}
> +
> +int iommu_msi_mapping_translate_msg(struct irq_data *data, struct msi_msg *msg)
> +{
> +	struct iommu_domain *d;
> +	struct msi_desc *desc;
> +	dma_addr_t iova;
> +
> +	desc = irq_data_get_msi_desc(data);
> +	if (!desc)
> +		return -EINVAL;
> +
> +	d = iommu_msi_mapping_desc_to_domain(desc);
> +	if (!d)
> +		return 0;
> +
> +	iova = iommu_find_reserved_iova(d, msg_to_phys_addr(msg),
> +					sizeof(phys_addr_t));
> +
> +	if (iova == DMA_ERROR_CODE)
> +		return -EINVAL;
> +
> +	msg->address_lo = lower_32_bits(iova);
> +	msg->address_hi = upper_32_bits(iova);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_msi_mapping_translate_msg);
> diff --git a/include/linux/dma-reserved-iommu.h b/include/linux/dma-reserved-iommu.h
> index 8373929..04e1912f 100644
> --- a/include/linux/dma-reserved-iommu.h
> +++ b/include/linux/dma-reserved-iommu.h
> @@ -20,6 +20,8 @@
>
>   struct iommu_domain;
>   struct msi_desc;
> +struct irq_data;
> +struct msi_msg;
>
>   #ifdef CONFIG_IOMMU_DMA_RESERVED
>
> @@ -82,6 +84,25 @@ void iommu_put_reserved_iova(struct iommu_domain *domain, phys_addr_t addr);
>    */
>   struct iommu_domain *iommu_msi_mapping_desc_to_domain(struct msi_desc *desc);
>
> +/**
> + * iommu_msi_mapping_translate_msg: in case the MSI transaction is translated
> + * by an IOMMU, the msg address must be an IOVA instead of a physical address.
> + * This function overwrites the original MSI message containing the doorbell
> + * physical address, result of the primary composition, with the doorbell IOVA.
> + *
> + * The doorbell physical address must be bound previously to an IOVA using
> + * iommu_get_reserved_iova
> + *
> + * @data: irq data handle
> + * @msg: original msi message containing the PA to be overwritten with
> + * the IOVA
> + *
> + * return 0 if the MSI does not need to be mapped or when the PA/IOVA
> + * were successfully swapped; return -EINVAL if the addresses need
> + * to be swapped but not IOMMU binding is found
> + */
> +int iommu_msi_mapping_translate_msg(struct irq_data *data, struct msi_msg *msg);
> +
>   #else
>
>   static inline int
> @@ -111,5 +132,11 @@ iommu_msi_mapping_desc_to_domain(struct msi_desc *desc)
>   	return NULL;
>   }
>
> +static inline int iommu_msi_mapping_translate_msg(struct irq_data *data,
> +						  struct msi_msg *msg)
> +{
> +	return 0;
> +}
> +
>   #endif	/* CONFIG_IOMMU_DMA_RESERVED */
>   #endif	/* __DMA_RESERVED_IOMMU_H */
>

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

#1383950 — Re: [PATCH v7 09/10] iommu/dma-reserved-iommu: iommu_msi_mapping_translate_msg

On 04/20/2016 07:28 PM, Robin Murphy wrote:
> On 19/04/16 17:56, Eric Auger wrote:
>> Introduce iommu_msi_mapping_translate_msg whose role consists in
>> detecting whether the device's MSIs must to be mapped into an IOMMU.
>> It case it must, the function overrides the MSI msg originally composed
>> and replaces the doorbell's PA by a pre-allocated and pre-mapped reserved
>> IOVA. In case the corresponding PA region is not found, the function
>> returns an error.
>>
>> This function is likely to be called in some code that cannot sleep. This
>> is the reason why the allocation/mapping is done separately.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v7: creation
>> ---
>>   drivers/iommu/dma-reserved-iommu.c | 69
>> ++++++++++++++++++++++++++++++++++++++
>>   include/linux/dma-reserved-iommu.h | 27 +++++++++++++++
>>   2 files changed, 96 insertions(+)
>>
>> diff --git a/drivers/iommu/dma-reserved-iommu.c
>> b/drivers/iommu/dma-reserved-iommu.c
>> index 907a17f..603ee45 100644
>> --- a/drivers/iommu/dma-reserved-iommu.c
>> +++ b/drivers/iommu/dma-reserved-iommu.c
>> @@ -18,6 +18,14 @@
>>   #include <linux/iommu.h>
>>   #include <linux/iova.h>
>>   #include <linux/msi.h>
>> +#include <linux/irq.h>
>> +
>> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
>> +#define msg_to_phys_addr(msg) \
>> +    (((phys_addr_t)((msg)->address_hi) << 32) | (msg)->address_lo)
>> +#else
>> +#define msg_to_phys_addr(msg)    ((msg)->address_lo)
>> +#endif
>>
>>   struct reserved_iova_domain {
>>       struct iova_domain *iovad;
>> @@ -351,3 +359,64 @@ struct iommu_domain
>> *iommu_msi_mapping_desc_to_domain(struct msi_desc *desc)
>>       return d;
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_msi_mapping_desc_to_domain);
>> +
>> +static dma_addr_t iommu_find_reserved_iova(struct iommu_domain *domain,
>> +                    phys_addr_t addr, size_t size)
>> +{
>> +    unsigned long  base_pfn, end_pfn, nb_iommu_pages, order, flags;
>> +    size_t iommu_page_size, binding_size;
>> +    struct iommu_reserved_binding *b;
>> +    phys_addr_t aligned_base, offset;
>> +    dma_addr_t iova = DMA_ERROR_CODE;
>> +    struct iova_domain *iovad;
>> +
>> +    spin_lock_irqsave(&domain->reserved_lock, flags);
>> +
>> +    iovad = (struct iova_domain *)domain->reserved_iova_cookie;
>> +
>> +    if (!iovad)
>> +        goto unlock;
>> +
>> +    order = iova_shift(iovad);
>> +    base_pfn = addr >> order;
>> +    end_pfn = (addr + size - 1) >> order;
>> +    aligned_base = base_pfn << order;
>> +    offset = addr - aligned_base;
>> +    nb_iommu_pages = end_pfn - base_pfn + 1;
>> +    iommu_page_size = 1 << order;
>> +    binding_size = nb_iommu_pages * iommu_page_size;
> 
> This all looks rather familiar...
> 
>> +    b = find_reserved_binding(domain, aligned_base, binding_size);
> 
> ...which implies that at least some of it should be factored into that guy.
ok. Besides, with your compact rewriting proposal, maybe it is not worth
anymore ;-)

Eric
> 
>> +    if (b && (b->addr <= aligned_base) &&
>> +        (aligned_base t + binding_size <=  b->addr + b->size))
>> +        iova = b->iova + offset + aligned_base - b->addr;
>> +unlock:
>> +    spin_unlock_irqrestore(&domain->reserved_lock, flags);
>> +    return iova;
>> +}
>> +
>> +int iommu_msi_mapping_translate_msg(struct irq_data *data, struct
>> msi_msg *msg)
>> +{
>> +    struct iommu_domain *d;
>> +    struct msi_desc *desc;
>> +    dma_addr_t iova;
>> +
>> +    desc = irq_data_get_msi_desc(data);
>> +    if (!desc)
>> +        return -EINVAL;
>> +
>> +    d = iommu_msi_mapping_desc_to_domain(desc);
>> +    if (!d)
>> +        return 0;
>> +
>> +    iova = iommu_find_reserved_iova(d, msg_to_phys_addr(msg),
>> +                    sizeof(phys_addr_t));
>> +
>> +    if (iova == DMA_ERROR_CODE)
>> +        return -EINVAL;
>> +
>> +    msg->address_lo = lower_32_bits(iova);
>> +    msg->address_hi = upper_32_bits(iova);
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_msi_mapping_translate_msg);
>> diff --git a/include/linux/dma-reserved-iommu.h
>> b/include/linux/dma-reserved-iommu.h
>> index 8373929..04e1912f 100644
>> --- a/include/linux/dma-reserved-iommu.h
>> +++ b/include/linux/dma-reserved-iommu.h
>> @@ -20,6 +20,8 @@
>>
>>   struct iommu_domain;
>>   struct msi_desc;
>> +struct irq_data;
>> +struct msi_msg;
>>
>>   #ifdef CONFIG_IOMMU_DMA_RESERVED
>>
>> @@ -82,6 +84,25 @@ void iommu_put_reserved_iova(struct iommu_domain
>> *domain, phys_addr_t addr);
>>    */
>>   struct iommu_domain *iommu_msi_mapping_desc_to_domain(struct
>> msi_desc *desc);
>>
>> +/**
>> + * iommu_msi_mapping_translate_msg: in case the MSI transaction is
>> translated
>> + * by an IOMMU, the msg address must be an IOVA instead of a physical
>> address.
>> + * This function overwrites the original MSI message containing the
>> doorbell
>> + * physical address, result of the primary composition, with the
>> doorbell IOVA.
>> + *
>> + * The doorbell physical address must be bound previously to an IOVA
>> using
>> + * iommu_get_reserved_iova
>> + *
>> + * @data: irq data handle
>> + * @msg: original msi message containing the PA to be overwritten with
>> + * the IOVA
>> + *
>> + * return 0 if the MSI does not need to be mapped or when the PA/IOVA
>> + * were successfully swapped; return -EINVAL if the addresses need
>> + * to be swapped but not IOMMU binding is found
>> + */
>> +int iommu_msi_mapping_translate_msg(struct irq_data *data, struct
>> msi_msg *msg);
>> +
>>   #else
>>
>>   static inline int
>> @@ -111,5 +132,11 @@ iommu_msi_mapping_desc_to_domain(struct msi_desc
>> *desc)
>>       return NULL;
>>   }
>>
>> +static inline int iommu_msi_mapping_translate_msg(struct irq_data *data,
>> +                          struct msi_msg *msg)
>> +{
>> +    return 0;
>> +}
>> +
>>   #endif    /* CONFIG_IOMMU_DMA_RESERVED */
>>   #endif    /* __DMA_RESERVED_IOMMU_H */
>>
> 

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

#1384142 — Re: [PATCH v7 00/10] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 1/3: iommu changes

Hi Alex, Robin,
On 04/19/2016 06:56 PM, Eric Auger wrote:
> This series introduces the dma-reserved-iommu api used to:
> 
> - create/destroy an iova domain dedicated to reserved iova bindings
> - map/unmap physical addresses onto reserved IOVAs.
> - search for an existing reserved iova mapping matching a PA window
> - determine whether an msi needs to be iommu mapped
> - translate an msi_msg PA address into its IOVA counterpart

Following Robin's review, I understand one important point we have to
clarify is how much this API has to be generic.

I agree with Robin on the fact there is quite a lot of duplication
between this dma-reserved-iommu implementation and dma-iommu
implementation. Maybe we could consider an msi-mapping API
implementation upon dma-iommu.c. This implementation would add MSI
doorbell binding list management, including, ref counting and locking.

We would need to add a map/unmap function taking an iova/pa/size as
parameters in current dma-iommu.c

An important assumption is that the dma-mapping API and the msi-mapping
API must not be used concurrently (be would be trying to use the same
cookie to store a different iova_domain).

Any thought/suggestion?

Best Regards

Eric


> 
> Currently reserved IOVAs are meant to map MSI physical doorbells. A single
> reserved domain does exit per domain.
> 
> Also a new domain attribute is introduced to signal whether the MSI
> addresses must be mapped in the IOMMU.
> 
> In current usage:
> VFIO subsystem is supposed to create/destroy the iommu reserved domain.
> The MSI layer is supposed to allocate/free iova mappings
> 
> Since several drivers are likely to use the same doorbell, a reference
> counting takes place on the bindings. An RB-tree indexed by PA is used
> to easily lookup for existing mappings at MSI message composition time.
> 
> More details & context can be found at:
> http://www.linaro.org/blog/core-dump/kvm-pciemsi-passthrough-armarm64/
> 
> Best Regards
> 
> Eric
> 
> Git: complete series available at
> https://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.6-rc4-pcie-passthrough-v7
> 
> History:
> 
> v6 -> v7:
> - fixed known lock bugs and multiple page sized slots matching
>   (I currently only have a single MSI frame made of a single page)
> - reserved_iova_cookie now pointing to a struct that encapsulates the
>   iova domain handle + protection attribute passed from VFIO (Alex' req)
> - 2 new functions exposed: iommu_msi_mapping_translate_msg,
>   iommu_msi_mapping_desc_to_domain: not sure this is the right location/proto
>   though
> - iommu_put_reserved_iova now takes a phys_addr_t
> - everything now is cleanup on iommu_domain destruction
> 
> RFC v5 -> patch v6:
> - split to ease the review process
> - in dma-reserved-api use a spin lock instead of a mutex (reported by
>   Jean-Philippe)
> - revisit iommu_get_reserved_iova API to pass a size parameter upon
>   Marc's request
> - Consistently use the page order passed when creating the iova domain.
> - init reserved_binding_list (reported by Julien)
> 
> RFC v4 -> RFC v5:
> - take into account Thomas' comments on MSI related patches
>   - split "msi: IOMMU map the doorbell address when needed"
>   - increase readability and add comments
>   - fix style issues
>  - split "iommu: Add DOMAIN_ATTR_MSI_MAPPING attribute"
>  - platform ITS now advertises IOMMU_CAP_INTR_REMAP
>  - fix compilation issue with CONFIG_IOMMU API unset
>  - arm-smmu-v3 now advertises DOMAIN_ATTR_MSI_MAPPING
> 
> RFC v3 -> v4:
> - Move doorbell mapping/unmapping in msi.c
> - fix ref count issue on set_affinity: in case of a change in the address
>   the previous address is decremented
> - doorbell map/unmap now is done on msi composition. Should allow the use
>   case for platform MSI controllers
> - create dma-reserved-iommu.h/c exposing/implementing a new API dedicated
>   to reserved IOVA management (looking like dma-iommu glue)
> - series reordering to ease the review:
>   - first part is related to IOMMU
>   - second related to MSI sub-system
>   - third related to VFIO (except arm-smmu IOMMU_CAP_INTR_REMAP removal)
> - expose the number of requested IOVA pages through VFIO_IOMMU_GET_INFO
>   [this partially addresses Marc's comments on iommu_get/put_single_reserved
>    size/alignment problematic - which I did not ignore - but I don't know
>    how much I can do at the moment]
> 
> RFC v2 -> RFC v3:
> - should fix wrong handling of some CONFIG combinations:
>   CONFIG_IOVA, CONFIG_IOMMU_API, CONFIG_PCI_MSI_IRQ_DOMAIN
> - fix MSI_FLAG_IRQ_REMAPPING setting in GICv3 ITS (although not tested)
> 
> PATCH v1 -> RFC v2:
> - reverted to RFC since it looks more reasonable ;-) the code is split
>   between VFIO, IOMMU, MSI controller and I am not sure I did the right
>   choices. Also API need to be further discussed.
> - iova API usage in arm-smmu.c.
> - MSI controller natively programs the MSI addr with either the PA or IOVA.
>   This is not done anymore in vfio-pci driver as suggested by Alex.
> - check irq remapping capability of the group
> 
> RFC v1 [2] -> PATCH v1:
> - use the existing dma map/unmap ioctl interface with a flag to register a
>   reserved IOVA range. Use the legacy Rb to store this special vfio_dma.
> - a single reserved IOVA contiguous region now is allowed
> - use of an RB tree indexed by PA to store allocated reserved slots
> - use of a vfio_domain iova_domain to manage iova allocation within the
>   window provided by the userspace
> - vfio alloc_map/unmap_free take a vfio_group handle
> - vfio_group handle is cached in vfio_pci_device
> - add ref counting to bindings
> - user modality enabled at the end of the series
> 
> 
> Eric Auger (10):
>   iommu: Add DOMAIN_ATTR_MSI_MAPPING attribute
>   iommu/arm-smmu: advertise DOMAIN_ATTR_MSI_MAPPING attribute
>   iommu: introduce a reserved iova cookie
>   iommu/dma-reserved-iommu: alloc/free_reserved_iova_domain
>   iommu/dma-reserved-iommu: reserved binding rb-tree and helpers
>   iommu/dma-reserved-iommu: iommu_get/put_reserved_iova
>   iommu/dma-reserved-iommu: delete bindings in
>     iommu_free_reserved_iova_domain
>   iommu/dma-reserved_iommu: iommu_msi_mapping_desc_to_domain
>   iommu/dma-reserved-iommu: iommu_msi_mapping_translate_msg
>   iommu/arm-smmu: call iommu_free_reserved_iova_domain on domain
>     destruction
> 
>  drivers/iommu/Kconfig              |   8 +
>  drivers/iommu/Makefile             |   1 +
>  drivers/iommu/arm-smmu-v3.c        |   4 +
>  drivers/iommu/arm-smmu.c           |   4 +
>  drivers/iommu/dma-reserved-iommu.c | 422 +++++++++++++++++++++++++++++++++++++
>  drivers/iommu/iommu.c              |   2 +
>  include/linux/dma-reserved-iommu.h | 142 +++++++++++++
>  include/linux/iommu.h              |   7 +
>  8 files changed, 590 insertions(+)
>  create mode 100644 drivers/iommu/dma-reserved-iommu.c
>  create mode 100644 include/linux/dma-reserved-iommu.h
> 

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

#1384532 — Re: [PATCH v7 00/10] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 1/3: iommu changes

On Thu, 21 Apr 2016 14:18:09 +0200
Eric Auger <eric.auger@linaro.org> wrote:

> Hi Alex, Robin,
> On 04/19/2016 06:56 PM, Eric Auger wrote:
> > This series introduces the dma-reserved-iommu api used to:
> > 
> > - create/destroy an iova domain dedicated to reserved iova bindings
> > - map/unmap physical addresses onto reserved IOVAs.
> > - search for an existing reserved iova mapping matching a PA window
> > - determine whether an msi needs to be iommu mapped
> > - translate an msi_msg PA address into its IOVA counterpart  
> 
> Following Robin's review, I understand one important point we have to
> clarify is how much this API has to be generic.
> 
> I agree with Robin on the fact there is quite a lot of duplication
> between this dma-reserved-iommu implementation and dma-iommu
> implementation. Maybe we could consider an msi-mapping API
> implementation upon dma-iommu.c. This implementation would add MSI
> doorbell binding list management, including, ref counting and locking.
> 
> We would need to add a map/unmap function taking an iova/pa/size as
> parameters in current dma-iommu.c
> 
> An important assumption is that the dma-mapping API and the msi-mapping
> API must not be used concurrently (be would be trying to use the same
> cookie to store a different iova_domain).
> 
> Any thought/suggestion?

Hi Eric,

I'm not attached to a generic interface, the important part for me is
that if we have an iommu domain with space reserved for MSI, the MSI
setup and allocation code should handle that so we don't need to play
the remapping tricks between vfio-pci and a vfio iommu driver that we
saw in early drafts of this.  My first inclination is always to try to
make a generic, re-usable interface, but I apologize if that's led us
astray here and we really do want the more simple, MSI specific
interface.

For the IOMMU API, rather than just a DOMAIN_ATTR_MSI_MAPPING flag,
what about DOMAIN_ATTR_MSI_GEOMETRY with both a get and set attribute?
Maybe something like:

struct iommu_domain_msi_geometry {
	dma_addr_t	aperture_start;
	dma_addr_t	aperture_end;
	bool		fixed; /* or 'programmable' depending on your polarity preference */
};

Calling \get\ on arm would return { 0, 0, false }, indicating it's
programmable, \set\ would allocate the iovad as specified.  That would
make it very easy to expand the API to x86 with reporting of the fixed
MSI range and it operates within the existing IOMMU API interfaces.
Thanks,

Alex

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

#1385125 — Re: [PATCH v7 00/10] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 1/3: iommu changes

Hi Alex,
On 04/21/2016 09:32 PM, Alex Williamson wrote:
> On Thu, 21 Apr 2016 14:18:09 +0200
> Eric Auger <eric.auger@linaro.org> wrote:
> 
>> Hi Alex, Robin,
>> On 04/19/2016 06:56 PM, Eric Auger wrote:
>>> This series introduces the dma-reserved-iommu api used to:
>>>
>>> - create/destroy an iova domain dedicated to reserved iova bindings
>>> - map/unmap physical addresses onto reserved IOVAs.
>>> - search for an existing reserved iova mapping matching a PA window
>>> - determine whether an msi needs to be iommu mapped
>>> - translate an msi_msg PA address into its IOVA counterpart  
>>
>> Following Robin's review, I understand one important point we have to
>> clarify is how much this API has to be generic.
>>
>> I agree with Robin on the fact there is quite a lot of duplication
>> between this dma-reserved-iommu implementation and dma-iommu
>> implementation. Maybe we could consider an msi-mapping API
>> implementation upon dma-iommu.c. This implementation would add MSI
>> doorbell binding list management, including, ref counting and locking.
>>
>> We would need to add a map/unmap function taking an iova/pa/size as
>> parameters in current dma-iommu.c
>>
>> An important assumption is that the dma-mapping API and the msi-mapping
>> API must not be used concurrently (be would be trying to use the same
>> cookie to store a different iova_domain).
>>
>> Any thought/suggestion?
> 
> Hi Eric,
> 
> I'm not attached to a generic interface, the important part for me is
> that if we have an iommu domain with space reserved for MSI, the MSI
> setup and allocation code should handle that so we don't need to play
> the remapping tricks between vfio-pci and a vfio iommu driver that we
> saw in early drafts of this.  My first inclination is always to try to
> make a generic, re-usable interface, but I apologize if that's led us
> astray here and we really do want the more simple, MSI specific
> interface.
> 
> For the IOMMU API, rather than just a DOMAIN_ATTR_MSI_MAPPING flag,
> what about DOMAIN_ATTR_MSI_GEOMETRY with both a get and set attribute?
> Maybe something like:
> 
> struct iommu_domain_msi_geometry {
> 	dma_addr_t	aperture_start;
> 	dma_addr_t	aperture_end;
> 	bool		fixed; /* or 'programmable' depending on your polarity preference */
> };
> 
> Calling \get\ on arm would return { 0, 0, false }, indicating it's
> programmable, \set\ would allocate the iovad as specified.  That would
> make it very easy to expand the API to x86 with reporting of the fixed
> MSI range and it operates within the existing IOMMU API interfaces.
> Thanks,
Yes I would be happy to handle this x86 query requirement. I would be
more inclined to define it at "MSI mapping API" level since the IOMMU
API implementation does not handle iova allocation, as Robin argued as
the beginning. When "MSI MAPPING API" CONFIG is unset I would return
default x86 aperture.

Does it make sense?

Best Regards

Eric
> 
> Alex
> 

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

#1385450 — Re: [PATCH v7 00/10] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 1/3: iommu changes

On Fri, 22 Apr 2016 14:31:18 +0200
Eric Auger <eric.auger@linaro.org> wrote:

> Hi Alex,
> On 04/21/2016 09:32 PM, Alex Williamson wrote:
> > On Thu, 21 Apr 2016 14:18:09 +0200
> > Eric Auger <eric.auger@linaro.org> wrote:
> >   
> >> Hi Alex, Robin,
> >> On 04/19/2016 06:56 PM, Eric Auger wrote:  
> >>> This series introduces the dma-reserved-iommu api used to:
> >>>
> >>> - create/destroy an iova domain dedicated to reserved iova bindings
> >>> - map/unmap physical addresses onto reserved IOVAs.
> >>> - search for an existing reserved iova mapping matching a PA window
> >>> - determine whether an msi needs to be iommu mapped
> >>> - translate an msi_msg PA address into its IOVA counterpart    
> >>
> >> Following Robin's review, I understand one important point we have to
> >> clarify is how much this API has to be generic.
> >>
> >> I agree with Robin on the fact there is quite a lot of duplication
> >> between this dma-reserved-iommu implementation and dma-iommu
> >> implementation. Maybe we could consider an msi-mapping API
> >> implementation upon dma-iommu.c. This implementation would add MSI
> >> doorbell binding list management, including, ref counting and locking.
> >>
> >> We would need to add a map/unmap function taking an iova/pa/size as
> >> parameters in current dma-iommu.c
> >>
> >> An important assumption is that the dma-mapping API and the msi-mapping
> >> API must not be used concurrently (be would be trying to use the same
> >> cookie to store a different iova_domain).
> >>
> >> Any thought/suggestion?  
> > 
> > Hi Eric,
> > 
> > I'm not attached to a generic interface, the important part for me is
> > that if we have an iommu domain with space reserved for MSI, the MSI
> > setup and allocation code should handle that so we don't need to play
> > the remapping tricks between vfio-pci and a vfio iommu driver that we
> > saw in early drafts of this.  My first inclination is always to try to
> > make a generic, re-usable interface, but I apologize if that's led us
> > astray here and we really do want the more simple, MSI specific
> > interface.
> > 
> > For the IOMMU API, rather than just a DOMAIN_ATTR_MSI_MAPPING flag,
> > what about DOMAIN_ATTR_MSI_GEOMETRY with both a get and set attribute?
> > Maybe something like:
> > 
> > struct iommu_domain_msi_geometry {
> > 	dma_addr_t	aperture_start;
> > 	dma_addr_t	aperture_end;
> > 	bool		fixed; /* or 'programmable' depending on your polarity preference */
> > };
> > 
> > Calling \get\ on arm would return { 0, 0, false }, indicating it's
> > programmable, \set\ would allocate the iovad as specified.  That would
> > make it very easy to expand the API to x86 with reporting of the fixed
> > MSI range and it operates within the existing IOMMU API interfaces.
> > Thanks,  
> Yes I would be happy to handle this x86 query requirement. I would be
> more inclined to define it at "MSI mapping API" level since the IOMMU
> API implementation does not handle iova allocation, as Robin argued as
> the beginning. When "MSI MAPPING API" CONFIG is unset I would return
> default x86 aperture.
> 
> Does it make sense?

It's not entirely clear to me if x86 would be participating in this MSI
mapping API given the implicit handling within iommu/irq-remapping.
It might make sense if x86 iommus simply left a gap in their existing
geometry reporting through the iommu api.  I guess we'll see in your
next draft ;)  Thanks,

Alex

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web