IOMMU reserved region tweaks
Subsystems (4)
Robin Murphy
maintainerv2 cover IOMMU reserved region tweaks
Hi Joerg,
Here's v2 incorporating Eric's feedback and R-b tags, with patch #4 added to finish the job. Whilst presented as a logical series because it's all touching the same code, it doesn't necessarily need merging as such - I'd be happier with patch #1 going into 4.11 before anyone starts using the sysfs region info in anger, but it's your call; #2 is really a standalone fix (other than context conflicts) but is non-urgent; #3 and #4 can happily wait for 4.12.
Robin.
Robin Murphy (4):
iommu: Disambiguate MSI region types
iommu/dma: Don't reserve PCI I/O windows
iommu/dma: Handle IOMMU API reserved regions
iommu/dma: Make PCI window reservation generic
drivers/iommu/amd_iommu.c | 2 +- drivers/iommu/arm-smmu-v3.c | 4 +- drivers/iommu/arm-smmu.c | 4 +- drivers/iommu/dma-iommu.c | 111 ++++++++++++++++++++++++++++++++++------ drivers/iommu/intel-iommu.c | 2 +- drivers/iommu/iommu.c | 5 +- drivers/vfio/vfio_iommu_type1.c | 7 ++- include/linux/dma-iommu.h | 5 ++ include/linux/iommu.h | 18 +++++-- 9 files changed, 127 insertions(+), 31 deletions(-)
Signature · 1 line
2.11.0.dirty
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v2 1/4 iommu: Disambiguate MSI region types
The introduction of reserved regions has left a couple of rough edges which we could do with sorting out sooner rather than later. Since we are not yet addressing the potential dynamic aspect of software-managed reservations and presenting them at arbitrary fixed addresses, it is incongruous that we end up displaying hardware vs. software-managed MSI regions to userspace differently, especially since ARM-based systems may actually require one or the other, or even potentially both at once, (which iommu-dma currently has no hope of dealing with at all). Let's resolve the former user-visible inconsistency ASAP before the ABI has been baked into a kernel release, in a way that also lays the groundwork for the latter shortcoming to be addressed by follow-up patches.
For clarity, rename the software-managed type to IOMMU_RESV_SW_MSI, use IOMMU_RESV_MSI to describe the hardware type, and document everything a little bit. Since the x86 MSI remapping hardware falls squarely under this meaning of IOMMU_RESV_MSI, apply that type to their regions as well, so that we tell the same story to userspace across all platforms.
Secondly, as the various region types require quite different handling, and it really makes little sense to ever try combining them, convert the bitfield-esque #defines to a plain enum in the process before anyone gets the wrong impression.
---
Notes:
v2:
- Reword commit message
- Convert type to an enum [Eric]
- Rename vfio_iommu_has_resv_msi() for clarity [Eric]
drivers/iommu/amd_iommu.c | 2 +- drivers/iommu/arm-smmu-v3.c | 2 +- drivers/iommu/arm-smmu.c | 2 +- drivers/iommu/intel-iommu.c | 2 +- drivers/iommu/iommu.c | 5 +++-- drivers/vfio/vfio_iommu_type1.c | 7 +++---- include/linux/iommu.h | 18 +++++++++++++----- 7 files changed, 23 insertions(+), 15 deletions(-)
drivers/iommu/amd_iommu.c +1 -1
@@ -3202,7 +3202,7 @@ static void amd_iommu_get_resv_regions(struct device *dev, region =iommu_alloc_resv_region (MSI_RANGE_START ,MSI_RANGE_END -MSI_RANGE_START +1 , -0 ,IOMMU_RESV_RESERVED ); +0 ,IOMMU_RESV_MSI );if (!region)return ;list_add_tail (®ion->list, head);
drivers/iommu/arm-smmu-v3.c +1 -1
@@ -1888,7 +1888,7 @@ static void arm_smmu_get_resv_regions(struct device *dev,int prot =IOMMU_WRITE |IOMMU_NOEXEC |IOMMU_MMIO ; region =iommu_alloc_resv_region (MSI_IOVA_BASE ,MSI_IOVA_LENGTH , - prot,IOMMU_RESV_MSI ); + prot,IOMMU_RESV_SW_MSI );if (!region)return ;
drivers/iommu/arm-smmu.c +1 -1
@@ -1608,7 +1608,7 @@ static void arm_smmu_get_resv_regions(struct device *dev,int prot =IOMMU_WRITE |IOMMU_NOEXEC |IOMMU_MMIO ; region =iommu_alloc_resv_region (MSI_IOVA_BASE ,MSI_IOVA_LENGTH , - prot,IOMMU_RESV_MSI ); + prot,IOMMU_RESV_SW_MSI );if (!region)return ;
drivers/iommu/intel-iommu.c +1 -1
@@ -5249,7 +5249,7 @@ static void intel_iommu_get_resv_regions(struct device *device, reg =iommu_alloc_resv_region (IOAPIC_RANGE_START ,IOAPIC_RANGE_END -IOAPIC_RANGE_START +1 , -0 ,IOMMU_RESV_RESERVED ); +0 ,IOMMU_RESV_MSI );if (!reg)return ;list_add_tail (®->list, head);
drivers/iommu/iommu.c +3 -2
@@ -72,6 +72,7 @@ static const char * const iommu_group_resv_type_string[] = { [IOMMU_RESV_DIRECT] = "direct", [IOMMU_RESV_RESERVED] = "reserved", [IOMMU_RESV_MSI] = "msi", + [IOMMU_RESV_SW_MSI] = "msi", };#define IOMMU_GROUP_ATTR (_name, _mode, _show, _store) \
@@ -1743,8 +1744,8 @@ void iommu_put_resv_regions(struct device *dev, struct list_head *list) }struct iommu_resv_region *iommu_alloc_resv_region (phys_addr_t start, -size_t length, -int prot,int type) +size_t length,int prot, +enum iommu_resv_type type) {struct iommu_resv_region *region;
drivers/vfio/vfio_iommu_type1.c +3 -4
@@ -1182,8 +1182,7 @@ static struct vfio_group *find_iommu_group(struct vfio_domain *domain,return NULL ; } -static bool vfio_iommu_has_resv_msi (struct iommu_group *group, -phys_addr_t *base) +static bool vfio_iommu_has_sw_msi (struct iommu_group *group,phys_addr_t *base) {struct list_head group_resv_regions;struct iommu_resv_region *region, *next;
@@ -1192,7 +1191,7 @@ static bool vfio_iommu_has_resv_msi(struct iommu_group *group,INIT_LIST_HEAD (&group_resv_regions);iommu_get_group_resv_regions (group, &group_resv_regions);list_for_each_entry (region, &group_resv_regions, list) { -if (region->type &IOMMU_RESV_MSI ) { +if (region->type ==IOMMU_RESV_SW_MSI ) { *base = region->start; ret = true; goto out;
@@ -1283,7 +1282,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,if (ret) goto out_domain; - resv_msi =vfio_iommu_has_resv_msi (iommu_group, &resv_msi_base); + resv_msi =vfio_iommu_has_sw_msi (iommu_group, &resv_msi_base);INIT_LIST_HEAD (&domain->group_list);list_add (&group->next, &domain->group_list);
include/linux/iommu.h +13 -5
@@ -125,9 +125,16 @@ enum iommu_attr { };/* These are the possible reserved region types */ -#define IOMMU_RESV_DIRECT (1 << 0) -#define IOMMU_RESV_RESERVED (1 << 1) -#define IOMMU_RESV_MSI (1 << 2) +enum iommu_resv_type { +/* Memory regions which must be mapped 1:1 at all times */ +IOMMU_RESV_DIRECT , +/* Arbitrary "never map this or give it to a device" address ranges */ +IOMMU_RESV_RESERVED , +/* Hardware MSI region (untranslated) */ +IOMMU_RESV_MSI , +/* Software-managed MSI translation window */ +IOMMU_RESV_SW_MSI , +};/** * struct iommu_resv_region - descriptor for a reserved memory region
@@ -142,7 +149,7 @@ struct iommu_resv_region {phys_addr_t start;size_t length;int prot; -int type; +enum iommu_resv_type type; };#ifdef CONFIG_IOMMU_API
@@ -288,7 +295,8 @@ extern void iommu_get_resv_regions(struct device *dev, struct list_head *list);extern void iommu_put_resv_regions (struct device *dev,struct list_head *list);extern int iommu_request_dm_for_dev (struct device *dev);extern struct iommu_resv_region * -iommu_alloc_resv_region (phys_addr_t start,size_t length,int prot,int type); +iommu_alloc_resv_region (phys_addr_t start,size_t length,int prot, +enum iommu_resv_type type);extern int iommu_get_group_resv_regions (struct iommu_group *group,struct list_head *head);
Signature · 1 line
2.11.0.dirty
A message that is not in the archive (sent off-list or not yet received): a56c76f7276a12120ecd0675be4c69de58aec8ff.1489683129.git.robin.murphy-5wv7dgnIgG8@public.gmane.org
Joerg Roedel
AppliedRe: [PATCH v2 1/4] iommu: Disambiguate MSI region types
Quoted 23 lines · Robin Murphy
The introduction of reserved regions has left a couple of rough edges which we could do with sorting out sooner rather than later. Since we are not yet addressing the potential dynamic aspect of software-managed reservations and presenting them at arbitrary fixed addresses, it is incongruous that we end up displaying hardware vs. software-managed MSI regions to userspace differently, especially since ARM-based systems may actually require one or the other, or even potentially both at once, (which iommu-dma currently has no hope of dealing with at all). Let's resolve the former user-visible inconsistency ASAP before the ABI has been baked into a kernel release, in a way that also lays the groundwork for the latter shortcoming to be addressed by follow-up patches.
For clarity, rename the software-managed type to
IOMMU_RESV_SW_MSI, useIOMMU_RESV_MSIto describe the hardware type, and document everything a little bit. Since the x86 MSI remapping hardware falls squarely under this meaning ofIOMMU_RESV_MSI, apply that type to their regions as well, so that we tell the same story to userspace across all platforms.Secondly, as the various region types require quite different handling, and it really makes little sense to ever try combining them, convert the bitfield-esque #defines to a plain enum in the process before anyone gets the wrong impression.
Quoted 6 lines
Fixes d30ddcaa7b02 ("iommu: Add a new type field in iommu_resv_region")Reviewed-by Eric Auger <[email protected]>CC Alex Williamson <[email protected]>CC David Woodhouse <[email protected]>Signed-off-by Robin Murphy <[email protected]>
Applied this one to iommu/fixes.
Robin Murphy
maintainerPATCH v2 2/4 iommu/dma: Don't reserve PCI I/O windows
In reply to Robin MurphyEven if a host controller's CPU-side MMIO windows into PCI I/O space do happen to leak into PCI memory space such that it might treat them as peer addresses, trying to reserve the corresponding I/O space addresses doesn't do anything to help solve that problem. Stop doing a silly thing.
---
Notes:
v2:
- No change
drivers/iommu/dma-iommu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/iommu/dma-iommu.c +1 -2
@@ -175,8 +175,7 @@ static void iova_reserve_pci_windows(struct pci_dev *dev,unsigned long lo, hi;resource_list_for_each_entry (window, &bridge->windows) { -if (resource_type (window->res) !=IORESOURCE_MEM && -resource_type (window->res) !=IORESOURCE_IO ) +if (resource_type (window->res) !=IORESOURCE_MEM )continue ; lo =iova_pfn (iovad, window->res->start - window->offset);
Signature · 1 line
2.11.0.dirty
Robin Murphy
maintainerPATCH v2 3/4 iommu/dma: Handle IOMMU API reserved regions
In reply to Robin MurphyNow that it's simple to discover the necessary reservations for a given device/IOMMU combination, let's wire up the appropriate handling. Basic reserved regions and direct-mapped regions we simply have to carve out of IOVA space (the IOMMU core having already mapped the latter before attaching the device). For hardware MSI regions, we also pre-populate the cookie with matching msi_pages. That way, irqchip drivers which normally assume MSIs to require mapping at the IOMMU can keep working without having to special-case their iommu_dma_map_msi_msg() hook, or indeed be aware at all of quirks preventing the IOMMU from translating certain addresses.
---
Notes:
v2:
- Make the IOVA alignment logic safer [Eric]
- Don't duplicate direct region mapping [Eric]
- Don't wait for the subsequent patch to start using the return value [Eric]
- Fix the sneaky off-by-one for pfn_hi
- Minor cosmetic tweaks
drivers/iommu/dma-iommu.c | 76 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 69 insertions(+), 7 deletions(-)
drivers/iommu/dma-iommu.c +69 -7
@@ -184,6 +184,66 @@ static void iova_reserve_pci_windows(struct pci_dev *dev, } } +static int cookie_init_hw_msi_region (struct iommu_dma_cookie *cookie, +phys_addr_t start,phys_addr_t end) +{ +struct iova_domain *iovad = &cookie->iovad; +struct iommu_dma_msi_page *msi_page; +int i, num_pages; + + start -=iova_offset (iovad, start); + num_pages =iova_align (iovad, end - start) >>iova_shift (iovad); + + msi_page =kcalloc (num_pages,sizeof (*msi_page),GFP_KERNEL ); +if (!msi_page) +return -ENOMEM ; + +for (i =0 ; i < num_pages; i++) { + msi_page[i].phys = start; + msi_page[i].iova = start; +INIT_LIST_HEAD (&msi_page[i].list); +list_add (&msi_page[i].list, &cookie->msi_page_list); + start += iovad->granule; + } + +return 0 ; +} + +static int iova_reserve_iommu_regions (struct device *dev, +struct iommu_domain *domain) +{ +struct iommu_dma_cookie *cookie = domain->iova_cookie; +struct iova_domain *iovad = &cookie->iovad; +struct iommu_resv_region *region; +LIST_HEAD (resv_regions); +int ret =0 ; + +if (dev_is_pci (dev)) +iova_reserve_pci_windows (to_pci_dev (dev), iovad); + +iommu_get_resv_regions (dev, &resv_regions); +list_for_each_entry (region, &resv_regions, list) { +unsigned long lo, hi; + +/* We ARE the software that manages these! */ +if (region->type ==IOMMU_RESV_SW_MSI ) +continue ; + + lo =iova_pfn (iovad, region->start); + hi =iova_pfn (iovad, region->start + region->length -1 ); +reserve_iova (iovad, lo, hi); + +if (region->type ==IOMMU_RESV_MSI ) + ret =cookie_init_hw_msi_region (cookie, region->start, + region->start + region->length); +if (ret) +break ; + } +iommu_put_resv_regions (dev, &resv_regions); + +return ret; +} +/** * iommu_dma_init_domain - Initialise a DMA mapping domain * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
@@ -202,7 +262,6 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,struct iommu_dma_cookie *cookie = domain->iova_cookie;struct iova_domain *iovad = &cookie->iovad;unsigned long order, base_pfn, end_pfn; -bool pci = dev &&dev_is_pci (dev);if (!cookie || cookie->type !=IOMMU_DMA_IOVA_COOKIE )return -EINVAL ;
@@ -232,7 +291,7 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,* leave the cache limit at the top of their range to save an rb_last() * traversal on every allocation. */ -if (pci) +if (dev &&dev_is_pci (dev)) end_pfn &=DMA_BIT_MASK (32 ) >> order;/* start_pfn is always nonzero for an already-initialised domain */
@@ -247,12 +306,15 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,* area cache limit down for the benefit of the smaller one. */ iovad->dma_32bit_pfn =min (end_pfn, iovad->dma_32bit_pfn); - }else { -init_iova_domain (iovad,1UL << order, base_pfn, end_pfn); -if (pci) -iova_reserve_pci_windows (to_pci_dev (dev), iovad); + +return 0 ; } -return 0 ; + +init_iova_domain (iovad,1UL << order, base_pfn, end_pfn); +if (!dev) +return 0 ; + +return iova_reserve_iommu_regions (dev, domain); }EXPORT_SYMBOL (iommu_dma_init_domain);
Signature · 1 line
2.11.0.dirty
Robin Murphy
maintainerPATCH v2 4/4 iommu/dma: Make PCI window reservation generic
In reply to Robin MurphyNow that we're applying the IOMMU API reserved regions to our IOVA domains, we shouldn't need to privately special-case PCI windows, or indeed anything else which isn't specific to our iommu-dma layer. However, since those aren't IOMMU-specific either, rather than start duplicating code into IOMMU drivers let's transform the existing function into an iommu_get_resv_regions() helper that they can share.
---
Notes:
v2:
- New
drivers/iommu/arm-smmu-v3.c | 2 ++ drivers/iommu/arm-smmu.c | 2 ++ drivers/iommu/dma-iommu.c | 38 ++++++++++++++++++++++++++++---------- include/linux/dma-iommu.h | 5 +++++ 4 files changed, 37 insertions(+), 10 deletions(-)
drivers/iommu/arm-smmu-v3.c +2 -0
@@ -1893,6 +1893,8 @@ static void arm_smmu_get_resv_regions(struct device *dev,return ;list_add_tail (®ion->list, head); + +iommu_dma_get_resv_regions (dev, head); } static void arm_smmu_put_resv_regions(struct device *dev,
drivers/iommu/arm-smmu.c +2 -0
@@ -1613,6 +1613,8 @@ static void arm_smmu_get_resv_regions(struct device *dev,return ;list_add_tail (®ion->list, head); + +iommu_dma_get_resv_regions (dev, head); } static void arm_smmu_put_resv_regions(struct device *dev,
drivers/iommu/dma-iommu.c +28 -10
@@ -167,22 +167,43 @@ void iommu_put_dma_cookie(struct iommu_domain *domain) }EXPORT_SYMBOL (iommu_put_dma_cookie); -static void iova_reserve_pci_windows (struct pci_dev *dev, -struct iova_domain *iovad) +/** +* iommu_dma_get_resv_regions - Reserved region driver helper +* @dev: Device from iommu_get_resv_regions() +* @list: Reserved region list from iommu_get_resv_regions() +* +* IOMMU drivers can use this to implement their .get_resv_regions callback +* for general non-IOMMU-specific reservations. Currently, this covers host +* bridge windows for PCI devices. +*/ +void iommu_dma_get_resv_regions (struct device *dev,struct list_head *list) { -struct pci_host_bridge *bridge =pci_find_host_bridge (dev->bus); +struct pci_host_bridge *bridge;struct resource_entry *window; -unsigned long lo, hi; +if (!dev_is_pci (dev)) +return ; + + bridge =pci_find_host_bridge (to_pci_dev (dev)->bus);resource_list_for_each_entry (window, &bridge->windows) { +struct iommu_resv_region *region; +phys_addr_t start; +size_t length; +if (resource_type (window->res) !=IORESOURCE_MEM )continue ; - lo =iova_pfn (iovad, window->res->start - window->offset); - hi =iova_pfn (iovad, window->res->end - window->offset); -reserve_iova (iovad, lo, hi); + start = window->res->start - window->offset; + length = window->res->end - window->res->start +1 ; + region =iommu_alloc_resv_region (start, length,0 , +IOMMU_RESV_RESERVED ); +if (!region) +return ; + +list_add_tail (®ion->list, list); } } +EXPORT_SYMBOL (iommu_dma_get_resv_regions);static int cookie_init_hw_msi_region (struct iommu_dma_cookie *cookie,phys_addr_t start,phys_addr_t end)
@@ -218,9 +239,6 @@ static int iova_reserve_iommu_regions(struct device *dev,LIST_HEAD (resv_regions);int ret =0 ; -if (dev_is_pci (dev)) -iova_reserve_pci_windows (to_pci_dev (dev), iovad); -iommu_get_resv_regions (dev, &resv_regions);list_for_each_entry (region, &resv_regions, list) {unsigned long lo, hi;
include/linux/dma-iommu.h +5 -0
@@ -71,6 +71,7 @@ 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); +void iommu_dma_get_resv_regions (struct device *dev,struct list_head *list);#else
@@ -100,6 +101,10 @@ static inline void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg) { } +static inline void iommu_dma_get_resv_regions (struct device *dev,struct list_head *list) +{ +} +#endif /* CONFIG_IOMMU_DMA */ #endif /* __KERNEL__ */ #endif /* __DMA_IOMMU_H */
Signature · 1 line
2.11.0.dirty
PATCH v2 4/4 iommu/dma: Make PCI window reservation generic
Quoted 14 lines · Robin Murphy
Now that we're applying the IOMMU API reserved regions to our IOVA domains, we shouldn't need to privately special-case PCI windows, or indeed anything else which isn't specific to our iommu-dma layer. However, since those aren't IOMMU-specific either, rather than start duplicating code into IOMMU drivers let's transform the existing function into an
iommu_get_resv_regions()helper that they can share.Signed-off-by Robin Murphy <[email protected]>---
Notes:
v2:
- New
Quoted 5 lines
drivers/iommu/arm-smmu-v3.c | 2 ++ drivers/iommu/arm-smmu.c | 2 ++ drivers/iommu/dma-iommu.c | 38 ++++++++++++++++++++++++++++---------- include/linux/dma-iommu.h | 5 +++++ 4 files changed, 37 insertions(+), 10 deletions(-)
Applied patches 2-4 to my arm/core branch, thanks.