Re: [PATCH 02/12] intel-ipu3: mmu: implement driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

<...>

> >> +       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.

Regards,
- Tuukka




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux