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@xxxxxxxxxx>
>> ---
>> 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(®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)
>>
