Re: [PATCH 1/5] iommu/tegra-smmu: Fix domain_alloc

2019-01-24 Thread navneet kumar
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(>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(>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 >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(>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

Re: [PATCH 2/5] iommu/tegra-smmu: Use non-secure register for flushing

2019-01-24 Thread navneet kumar
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

[PATCH 3/5] iommu/tegra-smmu: Fix client enablement order

2019-01-16 Thread Navneet Kumar
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 = >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

2019-01-16 Thread Navneet Kumar
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, , 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(>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


[PATCH 2/5] iommu/tegra-smmu: Use non-secure register for flushing

2019-01-16 Thread 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)
-- 
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

2019-01-16 Thread Navneet Kumar
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(>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(>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(_bus_type, _smmu_ops);
+   if (err < 0) {
+   iommu_device_unregister(>iommu);
+   iommu_device_sysfs_remove(>iommu);
+   return err;
+   }
+
+#ifdef CONFIG_PCI
+   if (!iommu_present(_bus_type)) {
+   pci_request_acs();
+   err = bus_set_iommu(_bus_type, _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(_bus_type, _smmu_ops);
-   if (err < 0) {
-   iommu_device_unregister(>iommu);
-   iommu_device_sysfs_remove(>iommu);
+   err = tegra_smmu_iommu_bus_init(smmu);
+   if (err)
return ERR_PTR(err);
-   }
+   err = bus_set_iommu(_bus_type, _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 1/5] iommu/tegra-smmu: Fix domain_alloc

2019-01-16 Thread Navneet Kumar
* 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(>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 >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(>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