On Fri, Jun 9, 2017 at 5:26 PM, Tuukka Toivonen
<tuukka.toivonen@xxxxxxxxx> wrote:
> Hi Tomasz,
>
> Couple of small comments below.
>
> On Wednesday, June 07, 2017 17:35:13 Tomasz Figa wrote:
>> >> +static void ipu3_mmu_domain_free(struct iommu_domain *dom)
>> >> +{
>> >> + struct ipu3_mmu_domain *mmu_dom =
>> >> + container_of(dom, struct ipu3_mmu_domain, domain);
>> >> + uint32_t l1_idx;
>> >> +
>> >> + for (l1_idx = 0; l1_idx < IPU3_MMU_L1PT_PTES; l1_idx++)
>> >> + if (mmu_dom->pgtbl[l1_idx] != mmu_dom-
>>dummy_l2_tbl)
>> >> + free_page((unsigned long)
>> >> + TBL_VIRT_ADDR(mmu_dom-
>>pgtbl[l1_idx]));
>> >> +
>> >> + free_page((unsigned long)TBL_VIRT_ADDR(mmu_dom-
>>dummy_page));
>> >> + free_page((unsigned long)TBL_VIRT_ADDR(mmu_dom-
>>dummy_l2_tbl));
>>
>> I might be overly paranoid, but reading back kernel virtual pointers
>> from device accessible memory doesn't seem safe to me. Other drivers
>> keep kernel pointers of page tables in a dedicated array (it's only 8K
>> of memory, but much better safety).
>
> They are accessible only to the IPU3 IOMMU, which can access whole
> system memory anyway and always does a read-only access to the MMU
> tables. So, I wouldn't worry too much, although extra copy for safety
> wouldn't necessarily harm too much.
Fair enough. Thanks for explanation.
>
> <...>
>
>> >> + ipu3_mmu_tlb_invalidate(mmu_dom->mmu->base);
>> >> +
>> >> + return unmapped << IPU3_MMU_PAGE_SHIFT;
>> >> +}
>> >> +
>> >> +static phys_addr_t ipu3_mmu_iova_to_phys(struct iommu_domain
> *domain,
>> >> + dma_addr_t iova)
>> >> +{
>> >> + struct ipu3_mmu_domain *d =
>> >> + container_of(domain, struct ipu3_mmu_domain,
> domain);
>> >> + uint32_t *l2_pt = TBL_VIRT_ADDR(d->pgtbl[iova >>
> IPU3_MMU_L1PT_SHIFT]);
>> >> +
>> >> + return (phys_addr_t)l2_pt[(iova & IPU3_MMU_L2PT_MASK)
>> >> + >> IPU3_MMU_L2PT_SHIFT] <<
> IPU3_MMU_PAGE_SHIFT;
>>
>> Could we avoid this TBL_VIRT_ADDR() here too? The memory cost to store
>> the page table CPU pointers is really small, but safety seems much
>> better. Moreover, it should make it possible to use the VT-d IOMMU to
>> further secure the system.
>
> IPU3 doesn't support VT-d and can't be enabled while VT-d is on.
Got it. Thanks.
Best regards,
Tomasz
