Re: xen-swiotlb vs phys_to_dma

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>]

On Fri, 2 Oct 2020, Christoph Hellwig wrote:
> Hi Stefano,
> 
> I've looked over xen-swiotlb in linux-next, that is with your recent
> changes to take dma offsets into account.  One thing that puzzles me
> is that xen_swiotlb_map_page passes virt_to_phys(xen_io_tlb_start) as
> the tbl_dma_addr argument to swiotlb_tbl_map_single, despite the fact
> that the argument is a dma_addr_t and both other callers translate
> from a physical to the dma address.  Was this an oversight?

Hi Christoph,

It was not an oversight, it was done on purpose, although maybe I could
have been wrong. There was a brief discussion on this topic here: 

https://marc.info/?l=linux-kernel&m=159011972107683&w=2
https://marc.info/?l=linux-kernel&m=159018047129198&w=2

I'll repeat and summarize here for convenience. 

swiotlb_init_with_tbl is called by xen_swiotlb_init, passing a virtual
address (xen_io_tlb_start), which gets converted to phys and stored in
io_tlb_start as a physical address at the beginning of swiotlb_init_with_tbl.

Afterwards, xen_swiotlb_map_page calls swiotlb_tbl_map_single. The
second parameter, dma_addr_t tbl_dma_addr, is used to calculate the
right slot in the swiotlb buffer to use, comparing it against
io_tlb_start.

Thus, I think it makes sense for xen_swiotlb_map_page to call
swiotlb_tbl_map_single passing an address meant to be compared with
io_tlb_start, which is __pa(xen_io_tlb_start), so
virt_to_phys(xen_io_tlb_start) seems to be what we want.

However, you are right that it is strange that tbl_dma_addr is a
dma_addr_t, and maybe it shouldn't be? Maybe the tbl_dma_addr parameter
to swiotlb_tbl_map_single should be a phys address instead?
Or it could be swiotlb_init_with_tbl to be wrong and it should take a
dma address to initialize the swiotlb buffer.