[5/5] iommu/tegra-smmu: Add resv_regions ops

Add support for get/put reserved regions.

Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
---
 drivers/iommu/tegra-smmu.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

On 16/01/2019 20:50, Navneet Kumar wrote:
> Add support for get/put reserved regions.

For any particular reason? If you're aiming to get 
VFIO-passthrough-with-MSIs working on Tegra, this probably won't be 
correct anyway...

> Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
> ---
>   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 <soc/tegra/ahb.h>
>   #include <soc/tegra/mc.h>
>   
> +#define MSI_IOVA_BASE	0x8000000

...because this arbitrary IOVA relies on writes to the MSI doorbell 
undergoing address translation at the IOMMU just like any other access. 
Looking at Tegra 134, that seems to implement an MSI doorbell internal 
to the PCIe root complex, far upstream of translation, therefore at the 
very least you'd need to reserve whatever IOVA region is shadowed by 
that doorbell in PCI memory space...

> +#define MSI_IOVA_LENGTH	0x100000
> +
>   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);

...and as an untranslatable hardware MSI region, not a software-managed one.

Robin.

> +	if (!region)
> +		return;
> +
> +	list_add_tail(&region->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)
>

Hi all,

Based on comments from Robin, do we really need to reserve an MSI region if the MSI doorbell is implemented
completely within the PCIe root complex?
Is MSI doorbell still internal to the PCIe root complex on T210 etc.?

I'm in favor of dropping this patch unless someone here has a counter argument.

On 1/17/19 9:06 AM, Robin Murphy wrote:
> On 16/01/2019 20:50, Navneet Kumar wrote:
>> Add support for get/put reserved regions.
> 
> For any particular reason? If you're aiming to get VFIO-passthrough-with-MSIs working on Tegra, this probably won't be correct anyway...
> 
>> Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
>> ---
>>   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 <soc/tegra/ahb.h>
>>   #include <soc/tegra/mc.h>
>>   +#define MSI_IOVA_BASE    0x8000000
> 
> ...because this arbitrary IOVA relies on writes to the MSI doorbell undergoing address translation at the IOMMU just like any other access. Looking at Tegra 134, that seems to implement an MSI doorbell internal to the PCIe root complex, far upstream of translation, therefore at the very least you'd need to reserve whatever IOVA region is shadowed by that doorbell in PCI memory space...
> 
>> +#define MSI_IOVA_LENGTH    0x100000
>> +
>>   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);
> 
> ...and as an untranslatable hardware MSI region, not a software-managed one.
> 
> Robin.
> 
>> +    if (!region)
>> +        return;
>> +
>> +    list_add_tail(&region->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)
>>

+Manikanta, Sagar from PCIe team.

-----Original Message-----
From: Navneet Kumar 
Sent: Friday, January 25, 2019 2:45 PM
To: SW-Mobile-Memory-Core <SW-Mobile-Memory-Core@exchange.nvidia.com>
Cc: Thierry Reding <treding@nvidia.com>; linux-tegra@vger.kernel.org; thierry.reding@gmail.com; Jonathan Hunter <jonathanh@nvidia.com>
Subject: Re: [PATCH 5/5] iommu/tegra-smmu: Add resv_regions ops

Hi all,

Based on comments from Robin, do we really need to reserve an MSI region if the MSI doorbell is implemented completely within the PCIe root complex?
Is MSI doorbell still internal to the PCIe root complex on T210 etc.?

I'm in favor of dropping this patch unless someone here has a counter argument.

On 1/17/19 9:06 AM, Robin Murphy wrote:
> On 16/01/2019 20:50, Navneet Kumar wrote:
>> Add support for get/put reserved regions.
> 
> For any particular reason? If you're aiming to get VFIO-passthrough-with-MSIs working on Tegra, this probably won't be correct anyway...
> 
>> Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
>> ---
>>   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 <soc/tegra/ahb.h>
>>   #include <soc/tegra/mc.h>
>>   +#define MSI_IOVA_BASE    0x8000000
> 
> ...because this arbitrary IOVA relies on writes to the MSI doorbell undergoing address translation at the IOMMU just like any other access. Looking at Tegra 134, that seems to implement an MSI doorbell internal to the PCIe root complex, far upstream of translation, therefore at the very least you'd need to reserve whatever IOVA region is shadowed by that doorbell in PCI memory space...
> 
>> +#define MSI_IOVA_LENGTH    0x100000
>> +
>>   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);
> 
> ...and as an untranslatable hardware MSI region, not a software-managed one.
> 
> Robin.
> 
>> +    if (!region)
>> +        return;
>> +
>> +    list_add_tail(&region->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)
>>