Re: [RFC PATCH 2/7] swiotlb: Make io_tlb_start a physical addressinstead of a virtual address

[Konrad Rzeszutek Wilk]

> >> @@ -450,7 +451,7 @@ void *swiotlb_tbl_map_single(struct device *hwdev, dma_addr_t tbl_dma_addr,
> >>  				io_tlb_list[i] = 0;
> >>  			for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) != IO_TLB_SEGSIZE - 1) && io_tlb_list[i]; i--)
> >>  				io_tlb_list[i] = ++count;
> >> -			dma_addr = io_tlb_start + (index << IO_TLB_SHIFT);
> >> +			dma_addr = (char *)phys_to_virt(io_tlb_start) + (index << IO_TLB_SHIFT);
> > I think this is going to fall flat with the other user of
> > swiotlb_tbl_map_single - Xen SWIOTLB. When it allocates the io_tlb_start
> > and does it magic to make sure its under 4GB - the io_tlb_start swath
> > of memory, ends up consisting of 2MB chunks of contingous spaces. But each
> > chunk is not linearly in the DMA space (thought it is in the CPU space).
> >
> > Meaning the io_tlb_start region 0-2MB can fall within the DMA address space
> > of 2048MB->2032MB, and io_tlb_start offset 2MB->4MB, can fall within 1024MB->1026MB,
> > and so on (depending on the availability of memory under 4GB).
> >
> > There is a clear virt_to_phys(x) != virt_to_dma(x).
> 
> Just to be sure I understand you are talking about DMA address space,
> not physical address space correct?  I am fully aware that DMA address
> space can be all over the place.  When I was writing the patch set the
> big reason why I decided to stop at physical address space was because
> DMA address spaces are device specific.
> 
> I understand that virt_to_phys(x) != virt_to_dma(x) for many platforms,
> however that is not my assertion.  My assertion is (virt_to_phys(x) + y)
> == virt_to_phys(x + y).  This should be true for any large block of
> contiguous memory that is DMA accessible since the CPU and the device
> should be able to view the memory in the same layout.  If that wasn't

That is true mostly for x86 but not all platforms do this.

> true I don't think is_swiotlb_buffer would be working correctly since it
> is essentially operating on the same assumption prior to my patches.

There are two pieces here - the is_swiotlb_buffer and the swiotlb_tbl_[map|unmap]
functions.

The is_swiotlb_buffer is operating on that principle (and your change
to reflect that is OK). The swiotlb_tbl_[*] is not.
> 
> If you take a look at patches 4 and 5 I do address changes that end up
> needing to be made to Xen SWIOTLB since it makes use of
> swiotlb_tbl_map_single.  All that I effectively end up changing is that
> instead of messing with a void pointer we instead are dealing with a
> physical address, and instead of calling xen_virt_to_bus we end up
> calling xen_phys_to_bus and thereby drop one extra virt_to_phys call in
> the process.

Sure that is OK. All of those changes when we bypass the bounce
buffer look OK (thought I should double-check again the patch to make
sure and also just take it for a little test spin).

The issue is when we do _use_ the bounce buffer. At that point we
run into the allocation from the bounce buffer where the patches
assume that the 64MB swath of bounce buffer memory is bus (or DMA)
memory contingous. And that is not the case sadly.

> 
> Thanks,
> 
> Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/