[PATCH v2] iommu/io-pgtable-arm: Don't use dma_to_phys()

[Yong Wu]

On Fri, 2015-09-18 at 12:04 +0100, Robin Murphy wrote:
> On 18/09/15 09:55, Yong Wu wrote:
> > On Thu, 2015-09-17 at 17:42 +0100, Robin Murphy wrote:
> [...]
> >> the appropriate course of action. Further care (and ugliness) is also
> >> necessary in the comparison to avoid truncation if phys_addr_t and
> >> dma_addr_t differ in size.
> [...]
> >>   		/*
> >>   		 * We depend on the IOMMU being able to work with any physical
> >> -		 * address directly, so if the DMA layer suggests it can't by
> >> -		 * giving us back some translation, that bodes very badly...
> >> +		 * address directly, so if the DMA layer suggests otherwise by
> >> +		 * translating or truncating them, that bodes very badly...
> >>   		 */
> >> -		if (dma != __arm_lpae_dma_addr(dev, pages))
> >> +		if (dma != virt_to_phys(pages))
> >
> > Could I ask why not use __arm_lpae_dma_addr(pages) here?
> > dma is dma_addr_t.
> 
> Specifically, the problem case for that is when phys_addr_t is 64-bit 
> but dma_addr_t is 32-bit. The cast in __arm_lpae_dma_addr is necessary 
> to avoid a truncation warning when we make the DMA API calls, but we 
> actually need the opposite in the comparison here - comparing the 
> different types directly allows integer promotion to kick in 
> appropriately so we don't lose the top half of the larger address. 
> Otherwise, you'd never spot the difference between, say, your original 
> page at 0x88c0000000 and a bounce-buffered copy that happened to end up 
> mapped to 0xc0000000.

Thanks.

About here:
> @@ -629,6 +626,11 @@ arm_lpae_alloc_pgtable(struct io_pgtable_cfg
*cfg)
>       if (cfg->oas > ARM_LPAE_MAX_ADDR_BITS)
>               return NULL;
>  
> +     if (cfg->iommu_dev->dma_pfn_offset) {

Do we need change to :
	if (!selftest_running && cfg->iommu_dev->dma_pfn_offset) {

cfg->iommu_dev will be null while self test.

> +             dev_err(cfg->iommu_dev, "Cannot accommodate DMA offset
for IOMMU page tables\n");
> +             return NULL;
> +     }
> +
>       data = kmalloc(sizeof(*data), GFP_KERNEL);
>       if (!data)
>               return NULL;

> 
> Robin.
>