| Message ID | 20200603222247.11681-8-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 42ea275a85a6ee17a99b719446c07a47498c7a24 |
| Headers | show |
| Series | [v2,01/11] swiotlb-xen: use vmalloc_to_page on vmalloc virt addresses | expand |
On 6/3/20 6:22 PM, Stefano Stabellini wrote: > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 0a6cb67f0fc4..60ef07440905 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -64,16 +64,16 @@ static inline dma_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t paddr) Weren't you going to rename this to xen_phys_to_dma()? (And the the one below to xen_dma_to_phys())? -boris > > dma |= paddr & ~XEN_PAGE_MASK; > > - return dma; > + return phys_to_dma(dev, dma); > } > > -static inline phys_addr_t xen_bus_to_phys(struct device *dev, dma_addr_t baddr) > +static inline phys_addr_t xen_bus_to_phys(struct device *dev, > + dma_addr_t dma_addr) > { > + phys_addr_t baddr = dma_to_phys(dev, dma_addr); > unsigned long xen_pfn = bfn_to_pfn(XEN_PFN_DOWN(baddr)); > - dma_addr_t dma = (dma_addr_t)xen_pfn << XEN_PAGE_SHIFT; > - phys_addr_t paddr = dma; > - > - paddr |= baddr & ~XEN_PAGE_MASK; > + phys_addr_t paddr = (xen_pfn << XEN_PAGE_SHIFT) | > + (baddr & ~XEN_PAGE_MASK); >
On 6/4/20 1:46 PM, Boris Ostrovsky wrote: > On 6/3/20 6:22 PM, Stefano Stabellini wrote: >> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c >> index 0a6cb67f0fc4..60ef07440905 100644 >> --- a/drivers/xen/swiotlb-xen.c >> +++ b/drivers/xen/swiotlb-xen.c >> @@ -64,16 +64,16 @@ static inline dma_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t paddr) > > Weren't you going to rename this to xen_phys_to_dma()? (And the the one > below to xen_dma_to_phys())? Nevermind, I see you did that in the next patch. -boris
On Wed, Jun 03, 2020 at 03:22:44PM -0700, Stefano Stabellini wrote: > From: Stefano Stabellini <stefano.stabellini@xilinx.com> > > With some devices physical addresses are different than dma addresses. > To be able to deal with these cases, we need to call phys_to_dma on > physical addresses (including machine addresses in Xen terminology) > before returning them from xen_swiotlb_alloc_coherent and > xen_swiotlb_map_page. > > We also need to convert dma addresses back to physical addresses using > dma_to_phys in xen_swiotlb_free_coherent and xen_swiotlb_unmap_page if > we want to do any operations on them. > > Call dma_to_phys in is_xen_swiotlb_buffer. > Call phys_to_dma in xen_phys_to_bus. > Call dma_to_phys in xen_bus_to_phys. > > Everything is taken care of by these changes except for > xen_swiotlb_alloc_coherent and xen_swiotlb_free_coherent, which need a > few explicit phys_to_dma/dma_to_phys calls. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > Tested-by: Corey Minyard <cminyard@mvista.com> > Tested-by: Roman Shaposhnik <roman@zededa.com> > --- > Changes in v2: > - improve commit message > --- > drivers/xen/swiotlb-xen.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 0a6cb67f0fc4..60ef07440905 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -64,16 +64,16 @@ static inline dma_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t paddr) > > dma |= paddr & ~XEN_PAGE_MASK; > > - return dma; > + return phys_to_dma(dev, dma); So looking at this function: The dma name for something passed to phys_to_dma is really weird. The fact that the comments says don't use XEN_PFN_PHYS beause of the type mismatch while nothing but swiotlb-xen is the only user of XEN_PFN_PHYS is also weird. I think XEN_PFN_PHYS needs to move to swiotlb-xen first, then use a hardcoded u64 for the size, and the split the function into a phys_to_xen_phys (or so) function where the result gets passed to phys_to_dma. Similar for the reverse direction.
On Mon, 8 Jun 2020, Christoph Hellwig wrote: > On Wed, Jun 03, 2020 at 03:22:44PM -0700, Stefano Stabellini wrote: > > From: Stefano Stabellini <stefano.stabellini@xilinx.com> > > > > With some devices physical addresses are different than dma addresses. > > To be able to deal with these cases, we need to call phys_to_dma on > > physical addresses (including machine addresses in Xen terminology) > > before returning them from xen_swiotlb_alloc_coherent and > > xen_swiotlb_map_page. > > > > We also need to convert dma addresses back to physical addresses using > > dma_to_phys in xen_swiotlb_free_coherent and xen_swiotlb_unmap_page if > > we want to do any operations on them. > > > > Call dma_to_phys in is_xen_swiotlb_buffer. > > Call phys_to_dma in xen_phys_to_bus. > > Call dma_to_phys in xen_bus_to_phys. > > > > Everything is taken care of by these changes except for > > xen_swiotlb_alloc_coherent and xen_swiotlb_free_coherent, which need a > > few explicit phys_to_dma/dma_to_phys calls. > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > Tested-by: Corey Minyard <cminyard@mvista.com> > > Tested-by: Roman Shaposhnik <roman@zededa.com> > > --- > > Changes in v2: > > - improve commit message > > --- > > drivers/xen/swiotlb-xen.c | 22 ++++++++++++---------- > > 1 file changed, 12 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > > index 0a6cb67f0fc4..60ef07440905 100644 > > --- a/drivers/xen/swiotlb-xen.c > > +++ b/drivers/xen/swiotlb-xen.c > > @@ -64,16 +64,16 @@ static inline dma_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t paddr) > > > > dma |= paddr & ~XEN_PAGE_MASK; > > > > - return dma; > > + return phys_to_dma(dev, dma); > > So looking at this function: > > The dma name for something passed to phys_to_dma is really > weird. Yeah, that is true, I am not sure why I chose that confusing name. I'll rename it. > The fact that the comments says don't use XEN_PFN_PHYS > beause of the type mismatch while nothing but swiotlb-xen is the only > user of XEN_PFN_PHYS is also weird. I think XEN_PFN_PHYS needs to move > to swiotlb-xen first, then use a hardcoded u64 for the size, and the > split the function into a phys_to_xen_phys (or so) function where > the result gets passed to phys_to_dma. I understand what you are suggesting about having something like: xen_phys_to_dma(...) { phys_addr_t phys = xen_phys_to_bus(dev, paddr) return phys_to_dma(phys); } I thought about it myself. I'll do it. But I don't think I understood the comment about XEN_PFN_PHYS. > Similar for the reverse direction. OK
On Mon, 8 Jun 2020, Stefano Stabellini wrote: > On Mon, 8 Jun 2020, Christoph Hellwig wrote: > > On Wed, Jun 03, 2020 at 03:22:44PM -0700, Stefano Stabellini wrote: > > > From: Stefano Stabellini <stefano.stabellini@xilinx.com> > > > > > > With some devices physical addresses are different than dma addresses. > > > To be able to deal with these cases, we need to call phys_to_dma on > > > physical addresses (including machine addresses in Xen terminology) > > > before returning them from xen_swiotlb_alloc_coherent and > > > xen_swiotlb_map_page. > > > > > > We also need to convert dma addresses back to physical addresses using > > > dma_to_phys in xen_swiotlb_free_coherent and xen_swiotlb_unmap_page if > > > we want to do any operations on them. > > > > > > Call dma_to_phys in is_xen_swiotlb_buffer. > > > Call phys_to_dma in xen_phys_to_bus. > > > Call dma_to_phys in xen_bus_to_phys. > > > > > > Everything is taken care of by these changes except for > > > xen_swiotlb_alloc_coherent and xen_swiotlb_free_coherent, which need a > > > few explicit phys_to_dma/dma_to_phys calls. > > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > > Tested-by: Corey Minyard <cminyard@mvista.com> > > > Tested-by: Roman Shaposhnik <roman@zededa.com> > > > --- > > > Changes in v2: > > > - improve commit message > > > --- > > > drivers/xen/swiotlb-xen.c | 22 ++++++++++++---------- > > > 1 file changed, 12 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > > > index 0a6cb67f0fc4..60ef07440905 100644 > > > --- a/drivers/xen/swiotlb-xen.c > > > +++ b/drivers/xen/swiotlb-xen.c > > > @@ -64,16 +64,16 @@ static inline dma_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t paddr) > > > > > > dma |= paddr & ~XEN_PAGE_MASK; > > > > > > - return dma; > > > + return phys_to_dma(dev, dma); > > > > So looking at this function: > > > > The dma name for something passed to phys_to_dma is really > > weird. > > Yeah, that is true, I am not sure why I chose that confusing name. I'll > rename it. > > > > The fact that the comments says don't use XEN_PFN_PHYS > > beause of the type mismatch while nothing but swiotlb-xen is the only > > user of XEN_PFN_PHYS is also weird. I think XEN_PFN_PHYS needs to move > > to swiotlb-xen first, then use a hardcoded u64 for the size, and the > > split the function into a phys_to_xen_phys (or so) function where > > the result gets passed to phys_to_dma. > > I understand what you are suggesting about having something like: > > xen_phys_to_dma(...) > { > phys_addr_t phys = xen_phys_to_bus(dev, paddr) > return phys_to_dma(phys); > } > > I thought about it myself. I'll do it. > > But I don't think I understood the comment about XEN_PFN_PHYS. You meant to move the #define from the header to swiotlb-xen.c, didn't you, and to use a cast to u64 instead of phys_addr_t?
On Mon, Jun 08, 2020 at 04:06:57PM -0700, Stefano Stabellini wrote: > I understand what you are suggesting about having something like: > > xen_phys_to_dma(...) > { > phys_addr_t phys = xen_phys_to_bus(dev, paddr) > return phys_to_dma(phys); > } > > I thought about it myself. I'll do it. "something", yes. Except that I think the bus is a little confusing, isn't it? What is the Xen term for these addresses? Also we probably don't need the extra local variable. > But I don't think I understood the comment about XEN_PFN_PHYS. There is a comment above xen_phys_to_bus that it avoids using XEN_PFN_PHYS because XEN_PFN_PHYS of the phys_addr_t vs dma_addr_t mismatch. But XEN_PFN_PHYS could just use a u64 instead of the phys_addr_t and then we could use it. Especially as XEN_PFN_PHYS isn't actually used anywhere except in swiotlb-xen.c. Or we could remove XEN_PFN_PHYS enirely, as it isn't all that useful to start with.
On Mon, 8 Jun 2020, Christoph Hellwig wrote: > On Mon, Jun 08, 2020 at 04:06:57PM -0700, Stefano Stabellini wrote: > > I understand what you are suggesting about having something like: > > > > xen_phys_to_dma(...) > > { > > phys_addr_t phys = xen_phys_to_bus(dev, paddr) > > return phys_to_dma(phys); > > } > > > > I thought about it myself. I'll do it. > > "something", yes. Except that I think the bus is a little confusing, > isn't it? What is the Xen term for these addresses? Xen reasons in terms of frame numbers. Xen calls them "dfn" for device frame number. They were supposed to be called "bfn" but eventually they settled for a different name when the series was committed. I could s/bfn/dfn/g to match the terminology, if that helps. > Also we probably don't need the extra local variable. Sure > > But I don't think I understood the comment about XEN_PFN_PHYS. > > There is a comment above xen_phys_to_bus that it avoids using > XEN_PFN_PHYS because XEN_PFN_PHYS of the phys_addr_t vs dma_addr_t > mismatch. But XEN_PFN_PHYS could just use a u64 instead of the > phys_addr_t and then we could use it. Especially as XEN_PFN_PHYS > isn't actually used anywhere except in swiotlb-xen.c. Or we could > remove XEN_PFN_PHYS enirely, as it isn't all that useful to start > with. I'll remove it.
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 0a6cb67f0fc4..60ef07440905 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -64,16 +64,16 @@ static inline dma_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t paddr) dma |= paddr & ~XEN_PAGE_MASK; - return dma; + return phys_to_dma(dev, dma); } -static inline phys_addr_t xen_bus_to_phys(struct device *dev, dma_addr_t baddr) +static inline phys_addr_t xen_bus_to_phys(struct device *dev, + dma_addr_t dma_addr) { + phys_addr_t baddr = dma_to_phys(dev, dma_addr); unsigned long xen_pfn = bfn_to_pfn(XEN_PFN_DOWN(baddr)); - dma_addr_t dma = (dma_addr_t)xen_pfn << XEN_PAGE_SHIFT; - phys_addr_t paddr = dma; - - paddr |= baddr & ~XEN_PAGE_MASK; + phys_addr_t paddr = (xen_pfn << XEN_PAGE_SHIFT) | + (baddr & ~XEN_PAGE_MASK); return paddr; } @@ -99,7 +99,7 @@ static inline int range_straddles_page_boundary(phys_addr_t p, size_t size) static int is_xen_swiotlb_buffer(struct device *dev, dma_addr_t dma_addr) { - unsigned long bfn = XEN_PFN_DOWN(dma_addr); + unsigned long bfn = XEN_PFN_DOWN(dma_to_phys(dev, dma_addr)); unsigned long xen_pfn = bfn_to_local_pfn(bfn); phys_addr_t paddr = XEN_PFN_PHYS(xen_pfn); @@ -304,11 +304,11 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, if (hwdev && hwdev->coherent_dma_mask) dma_mask = hwdev->coherent_dma_mask; - /* At this point dma_handle is the physical address, next we are + /* At this point dma_handle is the dma address, next we are * going to set it to the machine address. * Do not use virt_to_phys(ret) because on ARM it doesn't correspond * to *dma_handle. */ - phys = *dma_handle; + phys = dma_to_phys(hwdev, *dma_handle); dev_addr = xen_phys_to_bus(hwdev, phys); if (((dev_addr + size - 1 <= dma_mask)) && !range_straddles_page_boundary(phys, size)) @@ -319,6 +319,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); return NULL; } + *dma_handle = phys_to_dma(hwdev, *dma_handle); SetPageXenRemapped(virt_to_page(ret)); } memset(ret, 0, size); @@ -351,7 +352,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, TestClearPageXenRemapped(pg)) xen_destroy_contiguous_region(phys, order); - xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); + xen_free_coherent_pages(hwdev, size, vaddr, phys_to_dma(hwdev, phys), + attrs); } /*