[PATCH 1/5] iommu/tegra-smmu: Fix domain_alloc
* Allocate dma iova cookie for a domain while adding dma iommu devices. * Perform a stricter check for domain type parameter. Signed-off-by: Navneet Kumar --- drivers/iommu/tegra-smmu.c | 43 +++ 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index 543f7c9..ee4d8a8 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -271,8 +272,10 @@ static bool tegra_smmu_capable(enum iommu_cap cap) static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) { struct tegra_smmu_as *as; + int ret; - if (type != IOMMU_DOMAIN_UNMANAGED) + if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA && + type != IOMMU_DOMAIN_IDENTITY) return NULL; as = kzalloc(sizeof(*as), GFP_KERNEL); @@ -281,26 +284,22 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE; + ret = (type == IOMMU_DOMAIN_DMA) ? iommu_get_dma_cookie(&as->domain) : + -ENODEV; + if (ret) + goto free_as; + as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO); - if (!as->pd) { - kfree(as); - return NULL; - } + if (!as->pd) + goto put_dma_cookie; as->count = kcalloc(SMMU_NUM_PDE, sizeof(u32), GFP_KERNEL); - if (!as->count) { - __free_page(as->pd); - kfree(as); - return NULL; - } + if (!as->count) + goto free_pd_range; as->pts = kcalloc(SMMU_NUM_PDE, sizeof(*as->pts), GFP_KERNEL); - if (!as->pts) { - kfree(as->count); - __free_page(as->pd); - kfree(as); - return NULL; - } + if (!as->pts) + goto free_pts; /* setup aperture */ as->domain.geometry.aperture_start = 0; @@ -308,6 +307,18 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) as->domain.geometry.force_aperture = true; return &as->domain; + +free_pts: + kfree(as->pts); +free_pd_range: + __free_page(as->pd); +put_dma_cookie: + if (type == IOMMU_DOMAIN_DMA) + iommu_put_dma_cookie(&as->domain); +free_as: + kfree(as); + + return NULL; } static void tegra_smmu_domain_free(struct iommu_domain *domain) -- 2.7.4 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 2/5] iommu/tegra-smmu: Use non-secure register for flushing
Use PTB_ASID instead of SMMU_CONFIG to flush smmu. PTB_ASID can be accessed from non-secure mode, SMMU_CONFIG cannot be. Using SMMU_CONFIG could pose a problem when kernel doesn't have secure mode access enabled from boot. Signed-off-by: Navneet Kumar --- drivers/iommu/tegra-smmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index ee4d8a8..fa175d9 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -235,7 +235,7 @@ static inline void smmu_flush_tlb_group(struct tegra_smmu *smmu, static inline void smmu_flush(struct tegra_smmu *smmu) { - smmu_readl(smmu, SMMU_CONFIG); + smmu_readl(smmu, SMMU_PTB_ASID); } static int tegra_smmu_alloc_asid(struct tegra_smmu *smmu, unsigned int *idp) -- 2.7.4 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4/5] iommu/tegra-smmu: Add PCI support
Add support for PCI devices. Signed-off-by: Navneet Kumar --- drivers/iommu/tegra-smmu.c | 47 +- 1 file changed, 34 insertions(+), 13 deletions(-) diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index 1a44cf6..4b43c63 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -17,6 +17,7 @@ #include #include #include +#include #include #include @@ -820,7 +821,8 @@ tegra_smmu_find_group(struct tegra_smmu *smmu, unsigned int swgroup) return NULL; } -static struct iommu_group *tegra_smmu_group_get(struct tegra_smmu *smmu, +static struct iommu_group *tegra_smmu_group_get(struct device *dev, + struct tegra_smmu *smmu, unsigned int swgroup) { const struct tegra_smmu_group_soc *soc; @@ -847,7 +849,11 @@ static struct iommu_group *tegra_smmu_group_get(struct tegra_smmu *smmu, INIT_LIST_HEAD(&group->list); group->soc = soc; - group->group = iommu_group_alloc(); + if (dev_is_pci(dev)) + group->group = pci_device_group(dev); + else + group->group = generic_device_group(dev); + if (IS_ERR(group->group)) { devm_kfree(smmu->dev, group); mutex_unlock(&smmu->lock); @@ -864,13 +870,8 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev) { struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); struct tegra_smmu *smmu = dev->archdata.iommu; - struct iommu_group *group; - group = tegra_smmu_group_get(smmu, fwspec->ids[0]); - if (!group) - group = generic_device_group(dev); - - return group; + return tegra_smmu_group_get(dev, smmu, fwspec->ids[0]); } static int tegra_smmu_of_xlate(struct device *dev, @@ -989,6 +990,28 @@ static void tegra_smmu_debugfs_exit(struct tegra_smmu *smmu) debugfs_remove_recursive(smmu->debugfs); } +static int tegra_smmu_iommu_bus_init(struct tegra_smmu *smmu) +{ + int err; + + err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops); + if (err < 0) { + iommu_device_unregister(&smmu->iommu); + iommu_device_sysfs_remove(&smmu->iommu); + return err; + } + +#ifdef CONFIG_PCI + if (!iommu_present(&pci_bus_type)) { + pci_request_acs(); + err = bus_set_iommu(&pci_bus_type, &tegra_smmu_ops); + if (err < 0) + return err; + } +#endif + return 0; +} + struct tegra_smmu *tegra_smmu_probe(struct device *dev, const struct tegra_smmu_soc *soc, struct tegra_mc *mc) @@ -1072,12 +1095,10 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev, return ERR_PTR(err); } - err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops); - if (err < 0) { - iommu_device_unregister(&smmu->iommu); - iommu_device_sysfs_remove(&smmu->iommu); + err = tegra_smmu_iommu_bus_init(smmu); + if (err) return ERR_PTR(err); - } + err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops); if (IS_ENABLED(CONFIG_DEBUG_FS)) tegra_smmu_debugfs_init(smmu); -- 2.7.4 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 3/5] iommu/tegra-smmu: Fix client enablement order
Enable clients' translation only after setting up the swgroups. Signed-off-by: Navneet Kumar --- drivers/iommu/tegra-smmu.c | 23 ++- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index fa175d9..1a44cf6 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -353,6 +353,20 @@ static void tegra_smmu_enable(struct tegra_smmu *smmu, unsigned int swgroup, unsigned int i; u32 value; + group = tegra_smmu_find_swgroup(smmu, swgroup); + if (group) { + value = smmu_readl(smmu, group->reg); + value &= ~SMMU_ASID_MASK; + value |= SMMU_ASID_VALUE(asid); + value |= SMMU_ASID_ENABLE; + smmu_writel(smmu, value, group->reg); + } else { + pr_warn("%s group from swgroup %u not found\n", __func__, + swgroup); + /* No point moving ahead if group was not found */ + return; + } + for (i = 0; i < smmu->soc->num_clients; i++) { const struct tegra_mc_client *client = &smmu->soc->clients[i]; @@ -363,15 +377,6 @@ static void tegra_smmu_enable(struct tegra_smmu *smmu, unsigned int swgroup, value |= BIT(client->smmu.bit); smmu_writel(smmu, value, client->smmu.reg); } - - group = tegra_smmu_find_swgroup(smmu, swgroup); - if (group) { - value = smmu_readl(smmu, group->reg); - value &= ~SMMU_ASID_MASK; - value |= SMMU_ASID_VALUE(asid); - value |= SMMU_ASID_ENABLE; - smmu_writel(smmu, value, group->reg); - } } static void tegra_smmu_disable(struct tegra_smmu *smmu, unsigned int swgroup, -- 2.7.4 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 5/5] iommu/tegra-smmu: Add resv_regions ops
Add support for get/put reserved regions. Signed-off-by: Navneet Kumar --- drivers/iommu/tegra-smmu.c | 31 +++ 1 file changed, 31 insertions(+) diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index 4b43c63..e848a45 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -22,6 +22,9 @@ #include #include +#define MSI_IOVA_BASE 0x800 +#define MSI_IOVA_LENGTH0x10 + struct tegra_smmu_group { struct list_head list; const struct tegra_smmu_group_soc *soc; @@ -882,6 +885,31 @@ static int tegra_smmu_of_xlate(struct device *dev, return iommu_fwspec_add_ids(dev, &id, 1); } +static void tegra_smmu_get_resv_regions(struct device *dev, + struct list_head *head) +{ + struct iommu_resv_region *region; + int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO; + + region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH, + prot, IOMMU_RESV_SW_MSI); + if (!region) + return; + + list_add_tail(®ion->list, head); + + iommu_dma_get_resv_regions(dev, head); +} + +static void tegra_smmu_put_resv_regions(struct device *dev, + struct list_head *head) +{ + struct iommu_resv_region *entry, *next; + + list_for_each_entry_safe(entry, next, head, list) + kfree(entry); +} + static const struct iommu_ops tegra_smmu_ops = { .capable = tegra_smmu_capable, .domain_alloc = tegra_smmu_domain_alloc, @@ -896,6 +924,9 @@ static const struct iommu_ops tegra_smmu_ops = { .iova_to_phys = tegra_smmu_iova_to_phys, .of_xlate = tegra_smmu_of_xlate, .pgsize_bitmap = SZ_4K, + + .get_resv_regions = tegra_smmu_get_resv_regions, + .put_resv_regions = tegra_smmu_put_resv_regions, }; static void tegra_smmu_ahb_enable(void) -- 2.7.4 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/5] iommu/tegra-smmu: Use non-secure register for flushing
On 1/17/19 7:25 AM, Dmitry Osipenko wrote: > 16.01.2019 23:50, Navneet Kumar пишет: >> Use PTB_ASID instead of SMMU_CONFIG to flush smmu. >> PTB_ASID can be accessed from non-secure mode, SMMU_CONFIG cannot be. >> Using SMMU_CONFIG could pose a problem when kernel doesn't have secure >> mode access enabled from boot. >> >> Signed-off-by: Navneet Kumar >> --- >> drivers/iommu/tegra-smmu.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c >> index ee4d8a8..fa175d9 100644 >> --- a/drivers/iommu/tegra-smmu.c >> +++ b/drivers/iommu/tegra-smmu.c >> @@ -235,7 +235,7 @@ static inline void smmu_flush_tlb_group(struct >> tegra_smmu *smmu, >> >> static inline void smmu_flush(struct tegra_smmu *smmu) >> { >> -smmu_readl(smmu, SMMU_CONFIG); >> +smmu_readl(smmu, SMMU_PTB_ASID); >> } >> >> static int tegra_smmu_alloc_asid(struct tegra_smmu *smmu, unsigned int *idp) >> > > Driver writes to SMMU_CONFIG during of the probing. Looks like it's better to > drop this patch for now and make it part of a complete solution that will add > proper support for a stricter insecure-mode platforms. > Thanks for the comment Dmitry. On secure platforms, writing to SMMU_CONFIG will be a no-op and will pose no harm. Having this patch is important because it fixes the flushing behavior on such platforms, which is critical. I propose to keep this patch as is, however, i can add more explanation to the commit message, stating the case of probe and how it will not have any ill effects. Pls ACK/NACK, and i shall post a V2. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/5] iommu/tegra-smmu: Fix domain_alloc
Thanks for the feedback, Robin, and, Dmitry. I mostly agree with all your comments, pls see my responses inline. I'll make these fixes in V2. On 1/17/19 8:50 AM, Robin Murphy wrote: > On 17/01/2019 15:13, Dmitry Osipenko wrote: >> 16.01.2019 23:50, Navneet Kumar пишет: >>> * Allocate dma iova cookie for a domain while adding dma iommu >>> devices. >>> * Perform a stricter check for domain type parameter. >>> >> >> Commit message should tell what exactly is getting "fixed". Apparently >> you're trying to support T132 ARM64 here. I'll fix it. >> >>> Signed-off-by: Navneet Kumar >>> --- >>> drivers/iommu/tegra-smmu.c | 43 >>> +++ >>> 1 file changed, 27 insertions(+), 16 deletions(-) >>> >>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c >>> index 543f7c9..ee4d8a8 100644 >>> --- a/drivers/iommu/tegra-smmu.c >>> +++ b/drivers/iommu/tegra-smmu.c >>> @@ -16,6 +16,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> #include >>> #include >>> @@ -271,8 +272,10 @@ static bool tegra_smmu_capable(enum iommu_cap cap) >>> static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) >>> { >>> struct tegra_smmu_as *as; >>> + int ret; >>> - if (type != IOMMU_DOMAIN_UNMANAGED) >>> + if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA && >>> + type != IOMMU_DOMAIN_IDENTITY) >>> return NULL; >> >> Should be better to write these lines like this for the sake of readability: >> >> if (type != IOMMU_DOMAIN_UNMANAGED && >> type != IOMMU_DOMAIN_DMA && >> type != IOMMU_DOMAIN_IDENTITY) >> return NULL; > > Furthermore, AFAICS there's no special handling being added for identity > domains - don't pretend to support them by giving back a regular translation > domain, that's just misleading and broken. Agreed. > >> >> >>> as = kzalloc(sizeof(*as), GFP_KERNEL); >>> @@ -281,26 +284,22 @@ static struct iommu_domain >>> *tegra_smmu_domain_alloc(unsigned type) >>> as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE; >>> + ret = (type == IOMMU_DOMAIN_DMA) ? iommu_get_dma_cookie(&as->domain) >>> : >>> + -ENODEV; >> >> This makes to fail allocation of domain that has type other than >> IOMMU_DOMAIN_DMA. >> >> Should be: >> >> if (type == IOMMU_DOMAIN_DMA) { >> err = iommu_get_dma_cookie(&as->domain); >> if (err) >> goto free_as; >> } >> Agreed. >> >>> + if (ret) >>> + goto free_as; >>> + >>> as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO); >>> - if (!as->pd) { >>> - kfree(as); >>> - return NULL; >>> - } >>> + if (!as->pd) >>> + goto put_dma_cookie; >>> as->count = kcalloc(SMMU_NUM_PDE, sizeof(u32), GFP_KERNEL); >>> - if (!as->count) { >>> - __free_page(as->pd); >>> - kfree(as); >>> - return NULL; >>> - } >>> + if (!as->count) >>> + goto free_pd_range; >>> as->pts = kcalloc(SMMU_NUM_PDE, sizeof(*as->pts), GFP_KERNEL); >>> - if (!as->pts) { >>> - kfree(as->count); >>> - __free_page(as->pd); >>> - kfree(as); >>> - return NULL; >>> - } >>> + if (!as->pts) >>> + goto free_pts; >>> /* setup aperture */ >>> as->domain.geometry.aperture_start = 0; >>> @@ -308,6 +307,18 @@ static struct iommu_domain >>> *tegra_smmu_domain_alloc(unsigned type) >>> as->domain.geometry.force_aperture = true; >>> return &as->domain; >>> + >>> +free_pts: >>> + kfree(as->pts); >>> +free_pd_range: >>> + __free_page(as->pd); >>> +put_dma_cookie: >>> + if (type == IOMMU_DOMAIN_DMA) > > FWIW you don't strictly need that check - since domain->iova_cookie won't be > set for other domain types anyway, iommu_put_dma_cookie() will simply ignore > them. > I'll still keep that check and not rely solely on the put_dma_cookie API to handle this case. This will keep the code robust even if the put_dma_cookie API changes in future (for whatever reason). It also looks like the canonical usage in other drivers. >>> + iommu_put_dma_cookie(&as->domain); >>> +free_as: >>> + kfree(as); >>> + >>> + return NULL; >>> } >>> static void tegra_smmu_domain_free(struct iommu_domain *domain) > > How about not leaking the cookie when you free a DMA ops domain? > Agreed. > Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu