Hi all,
This series is a collection of fixes to get Linux running on the RPi4 as
dom0.
Conceptually there are only two significant changes:
- make sure not to call virt_to_page on vmalloc virt addresses (patch
#1)
- use phys_to_dma and dma_to_phys to translate phys to/from dma
addresses (all other patches)
In particular in regards to the second part, the RPi4 is the first
board where Xen can run that has the property that dma addresses are
different from physical addresses, and swiotlb-xen was written with the
assumption that phys addr == dma addr.
This series adds the phys_to_dma and dma_to_phys calls to make it work.
Cheers,
Stefano
The following changes since commit b85051e755b0e9d6dd8f17ef1da083851b83287d:
Merge tag 'fixes-for-5.7-rc6' of git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux (2020-05-20 13:23:55 -0700)
are available in the Git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/sstabellini/xen.git fix-rpi4-v2
for you to fetch changes up to 49783ba67f75da3490d2c01ed9b445d8a89bbb0d:
xen/arm: call dma_to_phys on the dma_addr_t parameter of dma_cache_maint (2020-06-03 15:05:53 -0700)
----------------------------------------------------------------
Boris Ostrovsky (1):
swiotlb-xen: use vmalloc_to_page on vmalloc virt addresses
Stefano Stabellini (10):
swiotlb-xen: remove start_dma_addr
swiotlb-xen: add struct device* parameter to xen_phys_to_bus
swiotlb-xen: add struct device* parameter to xen_bus_to_phys
swiotlb-xen: add struct device* parameter to xen_dma_sync_for_cpu
swiotlb-xen: add struct device* parameter to xen_dma_sync_for_device
swiotlb-xen: add struct device* parameter to is_xen_swiotlb_buffer
swiotlb-xen: introduce phys_to_dma/dma_to_phys translations
swiotlb-xen: rename xen_phys_to_bus to xen_phys_to_dma and xen_bus_to_phys to xen_dma_to_phys
xen/arm: introduce phys/dma translations in xen_dma_sync_for_*
xen/arm: call dma_to_phys on the dma_addr_t parameter of dma_cache_maint
arch/arm/xen/mm.c | 32 +++++++++++++++++++-------------
drivers/xen/swiotlb-xen.c | 72 +++++++++++++++++++++++++++++++++++++-----------------------------------
include/xen/swiotlb-xen.h | 10 ++++++----
3 files changed, 62 insertions(+), 52 deletions(-)
From: Stefano Stabellini <[email protected]>
The parameter is unused in this patch.
No functional changes.
Signed-off-by: Stefano Stabellini <[email protected]>
Tested-by: Corey Minyard <[email protected]>
Tested-by: Roman Shaposhnik <[email protected]>
---
drivers/xen/swiotlb-xen.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index ed09f8ac34c5..1c44dbc69b70 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -57,7 +57,7 @@ static unsigned long xen_io_tlb_nslabs;
* can be 32bit when dma_addr_t is 64bit leading to a loss in
* information if the shift is done before casting to 64bit.
*/
-static inline dma_addr_t xen_phys_to_bus(phys_addr_t paddr)
+static inline dma_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t paddr)
{
unsigned long bfn = pfn_to_bfn(XEN_PFN_DOWN(paddr));
dma_addr_t dma = (dma_addr_t)bfn << XEN_PAGE_SHIFT;
@@ -78,9 +78,9 @@ static inline phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
return paddr;
}
-static inline dma_addr_t xen_virt_to_bus(void *address)
+static inline dma_addr_t xen_virt_to_bus(struct device *dev, void *address)
{
- return xen_phys_to_bus(virt_to_phys(address));
+ return xen_phys_to_bus(dev, virt_to_phys(address));
}
static inline int range_straddles_page_boundary(phys_addr_t p, size_t size)
@@ -309,7 +309,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size,
* Do not use virt_to_phys(ret) because on ARM it doesn't correspond
* to *dma_handle. */
phys = *dma_handle;
- dev_addr = xen_phys_to_bus(phys);
+ dev_addr = xen_phys_to_bus(hwdev, phys);
if (((dev_addr + size - 1 <= dma_mask)) &&
!range_straddles_page_boundary(phys, size))
*dma_handle = dev_addr;
@@ -367,7 +367,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
unsigned long attrs)
{
phys_addr_t map, phys = page_to_phys(page) + offset;
- dma_addr_t dev_addr = xen_phys_to_bus(phys);
+ dma_addr_t dev_addr = xen_phys_to_bus(dev, phys);
BUG_ON(dir == DMA_NONE);
/*
@@ -392,7 +392,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
return DMA_MAPPING_ERROR;
phys = map;
- dev_addr = xen_phys_to_bus(map);
+ dev_addr = xen_phys_to_bus(dev, map);
/*
* Ensure that the address returned is DMA'ble
@@ -536,7 +536,7 @@ xen_swiotlb_sync_sg_for_device(struct device *dev, struct scatterlist *sgl,
static int
xen_swiotlb_dma_supported(struct device *hwdev, u64 mask)
{
- return xen_virt_to_bus(xen_io_tlb_end - 1) <= mask;
+ return xen_virt_to_bus(hwdev, xen_io_tlb_end - 1) <= mask;
}
const struct dma_map_ops xen_swiotlb_dma_ops = {
--
2.17.1
From: Stefano Stabellini <[email protected]>
The parameter is unused in this patch.
No functional changes.
Signed-off-by: Stefano Stabellini <[email protected]>
Tested-by: Corey Minyard <[email protected]>
Tested-by: Roman Shaposhnik <[email protected]>
---
arch/arm/xen/mm.c | 5 +++--
drivers/xen/swiotlb-xen.c | 4 ++--
include/xen/swiotlb-xen.h | 5 +++--
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index 1a00e8003c64..f2414ea40a79 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -81,8 +81,9 @@ void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
}
-void xen_dma_sync_for_device(dma_addr_t handle, phys_addr_t paddr, size_t size,
- enum dma_data_direction dir)
+void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
+ phys_addr_t paddr, size_t size,
+ enum dma_data_direction dir)
{
if (pfn_valid(PFN_DOWN(handle)))
arch_sync_dma_for_device(paddr, size, dir);
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index d9b3d9f2a7d1..9b846c143c90 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -405,7 +405,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
done:
if (!dev_is_dma_coherent(dev) && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
- xen_dma_sync_for_device(dev_addr, phys, size, dir);
+ xen_dma_sync_for_device(dev, dev_addr, phys, size, dir);
return dev_addr;
}
@@ -455,7 +455,7 @@ xen_swiotlb_sync_single_for_device(struct device *dev, dma_addr_t dma_addr,
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_DEVICE);
if (!dev_is_dma_coherent(dev))
- xen_dma_sync_for_device(dma_addr, paddr, size, dir);
+ xen_dma_sync_for_device(dev, dma_addr, paddr, size, dir);
}
/*
diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
index f62d1854780b..6d235fe2b92d 100644
--- a/include/xen/swiotlb-xen.h
+++ b/include/xen/swiotlb-xen.h
@@ -7,8 +7,9 @@
void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
phys_addr_t paddr, size_t size,
enum dma_data_direction dir);
-void xen_dma_sync_for_device(dma_addr_t handle, phys_addr_t paddr, size_t size,
- enum dma_data_direction dir);
+void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
+ phys_addr_t paddr, size_t size,
+ enum dma_data_direction dir);
extern int xen_swiotlb_init(int verbose, bool early);
extern const struct dma_map_ops xen_swiotlb_dma_ops;
--
2.17.1
From: Stefano Stabellini <[email protected]>
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 <[email protected]>
Tested-by: Corey Minyard <[email protected]>
Tested-by: Roman Shaposhnik <[email protected]>
---
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);
}
-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);
}
/*
--
2.17.1
From: Stefano Stabellini <[email protected]>
dma_cache_maint is getting called passing a dma address which could be
different from a physical address.
Add a struct device* parameter to dma_cache_maint.
Translate the dma_addr_t parameter of dma_cache_maint by calling
dma_to_phys. Do it for the first page and all the following pages, in
case of multipage handling.
Signed-off-by: Stefano Stabellini <[email protected]>
Tested-by: Corey Minyard <[email protected]>
Tested-by: Roman Shaposhnik <[email protected]>
---
Changes in v2:
- improve commit message
---
arch/arm/xen/mm.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index bbad712a890d..1dc20f4bdc33 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -43,15 +43,18 @@ unsigned long xen_get_swiotlb_free_pages(unsigned int order)
static bool hypercall_cflush = false;
/* buffers in highmem or foreign pages cannot cross page boundaries */
-static void dma_cache_maint(dma_addr_t handle, size_t size, u32 op)
+static void dma_cache_maint(struct device *dev, dma_addr_t handle,
+ size_t size, u32 op)
{
struct gnttab_cache_flush cflush;
- cflush.a.dev_bus_addr = handle & XEN_PAGE_MASK;
cflush.offset = xen_offset_in_page(handle);
cflush.op = op;
+ handle &= XEN_PAGE_MASK;
do {
+ cflush.a.dev_bus_addr = dma_to_phys(dev, handle);
+
if (size + cflush.offset > XEN_PAGE_SIZE)
cflush.length = XEN_PAGE_SIZE - cflush.offset;
else
@@ -60,7 +63,7 @@ static void dma_cache_maint(dma_addr_t handle, size_t size, u32 op)
HYPERVISOR_grant_table_op(GNTTABOP_cache_flush, &cflush, 1);
cflush.offset = 0;
- cflush.a.dev_bus_addr += cflush.length;
+ handle += cflush.length;
size -= cflush.length;
} while (size);
}
@@ -79,7 +82,7 @@ void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle))))
arch_sync_dma_for_cpu(paddr, size, dir);
else if (dir != DMA_TO_DEVICE)
- dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
+ dma_cache_maint(dev, handle, size, GNTTAB_CACHE_INVAL);
}
void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
@@ -89,9 +92,9 @@ void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle))))
arch_sync_dma_for_device(paddr, size, dir);
else if (dir == DMA_FROM_DEVICE)
- dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
+ dma_cache_maint(dev, handle, size, GNTTAB_CACHE_INVAL);
else
- dma_cache_maint(handle, size, GNTTAB_CACHE_CLEAN);
+ dma_cache_maint(dev, handle, size, GNTTAB_CACHE_CLEAN);
}
bool xen_arch_need_swiotlb(struct device *dev,
--
2.17.1
so that their names can better describe their behavior.
No functional changes.
Signed-off-by: Stefano Stabellini <[email protected]>
---
drivers/xen/swiotlb-xen.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 60ef07440905..41129c02d59a 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -57,7 +57,7 @@ static unsigned long xen_io_tlb_nslabs;
* can be 32bit when dma_addr_t is 64bit leading to a loss in
* information if the shift is done before casting to 64bit.
*/
-static inline dma_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t paddr)
+static inline dma_addr_t xen_phys_to_dma(struct device *dev, phys_addr_t paddr)
{
unsigned long bfn = pfn_to_bfn(XEN_PFN_DOWN(paddr));
dma_addr_t dma = (dma_addr_t)bfn << XEN_PAGE_SHIFT;
@@ -67,7 +67,7 @@ static inline dma_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t paddr)
return phys_to_dma(dev, dma);
}
-static inline phys_addr_t xen_bus_to_phys(struct device *dev,
+static inline phys_addr_t xen_dma_to_phys(struct device *dev,
dma_addr_t dma_addr)
{
phys_addr_t baddr = dma_to_phys(dev, dma_addr);
@@ -80,7 +80,7 @@ static inline phys_addr_t xen_bus_to_phys(struct device *dev,
static inline dma_addr_t xen_virt_to_bus(struct device *dev, void *address)
{
- return xen_phys_to_bus(dev, virt_to_phys(address));
+ return xen_phys_to_dma(dev, virt_to_phys(address));
}
static inline int range_straddles_page_boundary(phys_addr_t p, size_t size)
@@ -309,7 +309,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size,
* Do not use virt_to_phys(ret) because on ARM it doesn't correspond
* to *dma_handle. */
phys = dma_to_phys(hwdev, *dma_handle);
- dev_addr = xen_phys_to_bus(hwdev, phys);
+ dev_addr = xen_phys_to_dma(hwdev, phys);
if (((dev_addr + size - 1 <= dma_mask)) &&
!range_straddles_page_boundary(phys, size))
*dma_handle = dev_addr;
@@ -340,7 +340,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
/* do not use virt_to_phys because on ARM it doesn't return you the
* physical address */
- phys = xen_bus_to_phys(hwdev, dev_addr);
+ phys = xen_dma_to_phys(hwdev, dev_addr);
/* Convert the size to actually allocated. */
size = 1UL << (order + XEN_PAGE_SHIFT);
@@ -369,7 +369,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
unsigned long attrs)
{
phys_addr_t map, phys = page_to_phys(page) + offset;
- dma_addr_t dev_addr = xen_phys_to_bus(dev, phys);
+ dma_addr_t dev_addr = xen_phys_to_dma(dev, phys);
BUG_ON(dir == DMA_NONE);
/*
@@ -394,7 +394,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
return DMA_MAPPING_ERROR;
phys = map;
- dev_addr = xen_phys_to_bus(dev, map);
+ dev_addr = xen_phys_to_dma(dev, map);
/*
* Ensure that the address returned is DMA'ble
@@ -422,7 +422,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
static void xen_swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
size_t size, enum dma_data_direction dir, unsigned long attrs)
{
- phys_addr_t paddr = xen_bus_to_phys(hwdev, dev_addr);
+ phys_addr_t paddr = xen_dma_to_phys(hwdev, dev_addr);
BUG_ON(dir == DMA_NONE);
@@ -438,7 +438,7 @@ static void
xen_swiotlb_sync_single_for_cpu(struct device *dev, dma_addr_t dma_addr,
size_t size, enum dma_data_direction dir)
{
- phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
+ phys_addr_t paddr = xen_dma_to_phys(dev, dma_addr);
if (!dev_is_dma_coherent(dev))
xen_dma_sync_for_cpu(dev, dma_addr, paddr, size, dir);
@@ -451,7 +451,7 @@ static void
xen_swiotlb_sync_single_for_device(struct device *dev, dma_addr_t dma_addr,
size_t size, enum dma_data_direction dir)
{
- phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
+ phys_addr_t paddr = xen_dma_to_phys(dev, dma_addr);
if (is_xen_swiotlb_buffer(dev, dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_DEVICE);
--
2.17.1
From: Stefano Stabellini <[email protected]>
The parameter is unused in this patch.
No functional changes.
Signed-off-by: Stefano Stabellini <[email protected]>
Tested-by: Corey Minyard <[email protected]>
Tested-by: Roman Shaposhnik <[email protected]>
---
arch/arm/xen/mm.c | 5 +++--
drivers/xen/swiotlb-xen.c | 4 ++--
include/xen/swiotlb-xen.h | 5 +++--
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index d40e9e5fc52b..1a00e8003c64 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -71,8 +71,9 @@ static void dma_cache_maint(dma_addr_t handle, size_t size, u32 op)
* pfn_valid returns true the pages is local and we can use the native
* dma-direct functions, otherwise we call the Xen specific version.
*/
-void xen_dma_sync_for_cpu(dma_addr_t handle, phys_addr_t paddr, size_t size,
- enum dma_data_direction dir)
+void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
+ phys_addr_t paddr, size_t size,
+ enum dma_data_direction dir)
{
if (pfn_valid(PFN_DOWN(handle)))
arch_sync_dma_for_cpu(paddr, size, dir);
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index e38a1cce4100..d9b3d9f2a7d1 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -425,7 +425,7 @@ static void xen_swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
BUG_ON(dir == DMA_NONE);
if (!dev_is_dma_coherent(hwdev) && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
- xen_dma_sync_for_cpu(dev_addr, paddr, size, dir);
+ xen_dma_sync_for_cpu(hwdev, dev_addr, paddr, size, dir);
/* NOTE: We use dev_addr here, not paddr! */
if (is_xen_swiotlb_buffer(dev_addr))
@@ -439,7 +439,7 @@ xen_swiotlb_sync_single_for_cpu(struct device *dev, dma_addr_t dma_addr,
phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
if (!dev_is_dma_coherent(dev))
- xen_dma_sync_for_cpu(dma_addr, paddr, size, dir);
+ xen_dma_sync_for_cpu(dev, dma_addr, paddr, size, dir);
if (is_xen_swiotlb_buffer(dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_CPU);
diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
index ffc0d3902b71..f62d1854780b 100644
--- a/include/xen/swiotlb-xen.h
+++ b/include/xen/swiotlb-xen.h
@@ -4,8 +4,9 @@
#include <linux/swiotlb.h>
-void xen_dma_sync_for_cpu(dma_addr_t handle, phys_addr_t paddr, size_t size,
- enum dma_data_direction dir);
+void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
+ phys_addr_t paddr, size_t size,
+ enum dma_data_direction dir);
void xen_dma_sync_for_device(dma_addr_t handle, phys_addr_t paddr, size_t size,
enum dma_data_direction dir);
--
2.17.1
From: Stefano Stabellini <[email protected]>
It is not strictly needed. Call virt_to_phys on xen_io_tlb_start
instead. It will be useful not to have a start_dma_addr around with the
next patches.
Note that virt_to_phys is not the same as xen_virt_to_bus but actually
it is used to compared again __pa(xen_io_tlb_start) as passed to
swiotlb_init_with_tbl, so virt_to_phys is actually what we want.
Signed-off-by: Stefano Stabellini <[email protected]>
Tested-by: Corey Minyard <[email protected]>
Tested-by: Roman Shaposhnik <[email protected]>
---
Changes in v2:
- update commit message
---
---
drivers/xen/swiotlb-xen.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index a42129cba36e..ed09f8ac34c5 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -52,8 +52,6 @@ static unsigned long xen_io_tlb_nslabs;
* Quick lookup value of the bus address of the IOTLB.
*/
-static u64 start_dma_addr;
-
/*
* Both of these functions should avoid XEN_PFN_PHYS because phys_addr_t
* can be 32bit when dma_addr_t is 64bit leading to a loss in
@@ -241,7 +239,6 @@ int __ref xen_swiotlb_init(int verbose, bool early)
m_ret = XEN_SWIOTLB_EFIXUP;
goto error;
}
- start_dma_addr = xen_virt_to_bus(xen_io_tlb_start);
if (early) {
if (swiotlb_init_with_tbl(xen_io_tlb_start, xen_io_tlb_nslabs,
verbose))
@@ -389,8 +386,8 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
*/
trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force);
- map = swiotlb_tbl_map_single(dev, start_dma_addr, phys,
- size, size, dir, attrs);
+ map = swiotlb_tbl_map_single(dev, virt_to_phys(xen_io_tlb_start),
+ phys, size, size, dir, attrs);
if (map == (phys_addr_t)DMA_MAPPING_ERROR)
return DMA_MAPPING_ERROR;
--
2.17.1
From: Stefano Stabellini <[email protected]>
The parameter is unused in this patch.
No functional changes.
Signed-off-by: Stefano Stabellini <[email protected]>
Tested-by: Corey Minyard <[email protected]>
Tested-by: Roman Shaposhnik <[email protected]>
---
drivers/xen/swiotlb-xen.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 9b846c143c90..0a6cb67f0fc4 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -97,7 +97,7 @@ static inline int range_straddles_page_boundary(phys_addr_t p, size_t size)
return 0;
}
-static int is_xen_swiotlb_buffer(dma_addr_t dma_addr)
+static int is_xen_swiotlb_buffer(struct device *dev, dma_addr_t dma_addr)
{
unsigned long bfn = XEN_PFN_DOWN(dma_addr);
unsigned long xen_pfn = bfn_to_local_pfn(bfn);
@@ -428,7 +428,7 @@ static void xen_swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
xen_dma_sync_for_cpu(hwdev, dev_addr, paddr, size, dir);
/* NOTE: We use dev_addr here, not paddr! */
- if (is_xen_swiotlb_buffer(dev_addr))
+ if (is_xen_swiotlb_buffer(hwdev, dev_addr))
swiotlb_tbl_unmap_single(hwdev, paddr, size, size, dir, attrs);
}
@@ -441,7 +441,7 @@ xen_swiotlb_sync_single_for_cpu(struct device *dev, dma_addr_t dma_addr,
if (!dev_is_dma_coherent(dev))
xen_dma_sync_for_cpu(dev, dma_addr, paddr, size, dir);
- if (is_xen_swiotlb_buffer(dma_addr))
+ if (is_xen_swiotlb_buffer(dev, dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_CPU);
}
@@ -451,7 +451,7 @@ xen_swiotlb_sync_single_for_device(struct device *dev, dma_addr_t dma_addr,
{
phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
- if (is_xen_swiotlb_buffer(dma_addr))
+ if (is_xen_swiotlb_buffer(dev, dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_DEVICE);
if (!dev_is_dma_coherent(dev))
--
2.17.1
From: Boris Ostrovsky <[email protected]>
xen_alloc_coherent_pages might return pages for which virt_to_phys and
virt_to_page don't work, e.g. ioremap'ed pages.
So in xen_swiotlb_free_coherent we can't assume that virt_to_page works.
Instead add a is_vmalloc_addr check and use vmalloc_to_page on vmalloc
virt addresses.
This patch fixes the following crash at boot on RPi4:
https://marc.info/?l=xen-devel&m=158862573216800
Signed-off-by: Boris Ostrovsky <[email protected]>
Signed-off-by: Stefano Stabellini <[email protected]>
Tested-by: Corey Minyard <[email protected]>
Tested-by: Roman Shaposhnik <[email protected]>
---
Changes in v2:
- update commit message
---
drivers/xen/swiotlb-xen.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index b6d27762c6f8..a42129cba36e 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -335,6 +335,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
int order = get_order(size);
phys_addr_t phys;
u64 dma_mask = DMA_BIT_MASK(32);
+ struct page *pg;
if (hwdev && hwdev->coherent_dma_mask)
dma_mask = hwdev->coherent_dma_mask;
@@ -346,9 +347,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
/* Convert the size to actually allocated. */
size = 1UL << (order + XEN_PAGE_SHIFT);
+ pg = is_vmalloc_addr(vaddr) ? vmalloc_to_page(vaddr) :
+ virt_to_page(vaddr);
if (!WARN_ON((dev_addr + size - 1 > dma_mask) ||
range_straddles_page_boundary(phys, size)) &&
- TestClearPageXenRemapped(virt_to_page(vaddr)))
+ TestClearPageXenRemapped(pg))
xen_destroy_contiguous_region(phys, order);
xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs);
--
2.17.1
From: Stefano Stabellini <[email protected]>
xen_dma_sync_for_cpu, xen_dma_sync_for_device, xen_arch_need_swiotlb are
getting called passing dma addresses. On some platforms dma addresses
could be different from physical addresses. Before doing any operations
on these addresses we need to convert them back to physical addresses
using dma_to_phys.
Add dma_to_phys calls to xen_dma_sync_for_cpu, xen_dma_sync_for_device,
and xen_arch_need_swiotlb.
dma_cache_maint is fixed by the next patch.
Signed-off-by: Stefano Stabellini <[email protected]>
Tested-by: Corey Minyard <[email protected]>
Tested-by: Roman Shaposhnik <[email protected]>
---
Changes in v2:
- improve commit message
- don't use pfn_valid
---
arch/arm/xen/mm.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index f2414ea40a79..bbad712a890d 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
#include <linux/cpu.h>
+#include <linux/dma-direct.h>
#include <linux/dma-noncoherent.h>
#include <linux/gfp.h>
#include <linux/highmem.h>
@@ -75,7 +76,7 @@ void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
phys_addr_t paddr, size_t size,
enum dma_data_direction dir)
{
- if (pfn_valid(PFN_DOWN(handle)))
+ if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle))))
arch_sync_dma_for_cpu(paddr, size, dir);
else if (dir != DMA_TO_DEVICE)
dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
@@ -85,7 +86,7 @@ void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
phys_addr_t paddr, size_t size,
enum dma_data_direction dir)
{
- if (pfn_valid(PFN_DOWN(handle)))
+ if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle))))
arch_sync_dma_for_device(paddr, size, dir);
else if (dir == DMA_FROM_DEVICE)
dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
@@ -98,7 +99,7 @@ bool xen_arch_need_swiotlb(struct device *dev,
dma_addr_t dev_addr)
{
unsigned int xen_pfn = XEN_PFN_DOWN(phys);
- unsigned int bfn = XEN_PFN_DOWN(dev_addr);
+ unsigned int bfn = XEN_PFN_DOWN(dma_to_phys(dev, dev_addr));
/*
* The swiotlb buffer should be used if
--
2.17.1
From: Stefano Stabellini <[email protected]>
The parameter is unused in this patch.
No functional changes.
Signed-off-by: Stefano Stabellini <[email protected]>
Tested-by: Corey Minyard <[email protected]>
Tested-by: Roman Shaposhnik <[email protected]>
---
drivers/xen/swiotlb-xen.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 1c44dbc69b70..e38a1cce4100 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -67,7 +67,7 @@ static inline dma_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t paddr)
return dma;
}
-static inline phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
+static inline phys_addr_t xen_bus_to_phys(struct device *dev, dma_addr_t baddr)
{
unsigned long xen_pfn = bfn_to_pfn(XEN_PFN_DOWN(baddr));
dma_addr_t dma = (dma_addr_t)xen_pfn << XEN_PAGE_SHIFT;
@@ -339,7 +339,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
/* do not use virt_to_phys because on ARM it doesn't return you the
* physical address */
- phys = xen_bus_to_phys(dev_addr);
+ phys = xen_bus_to_phys(hwdev, dev_addr);
/* Convert the size to actually allocated. */
size = 1UL << (order + XEN_PAGE_SHIFT);
@@ -420,7 +420,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
static void xen_swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
size_t size, enum dma_data_direction dir, unsigned long attrs)
{
- phys_addr_t paddr = xen_bus_to_phys(dev_addr);
+ phys_addr_t paddr = xen_bus_to_phys(hwdev, dev_addr);
BUG_ON(dir == DMA_NONE);
@@ -436,7 +436,7 @@ static void
xen_swiotlb_sync_single_for_cpu(struct device *dev, dma_addr_t dma_addr,
size_t size, enum dma_data_direction dir)
{
- phys_addr_t paddr = xen_bus_to_phys(dma_addr);
+ phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
if (!dev_is_dma_coherent(dev))
xen_dma_sync_for_cpu(dma_addr, paddr, size, dir);
@@ -449,7 +449,7 @@ static void
xen_swiotlb_sync_single_for_device(struct device *dev, dma_addr_t dma_addr,
size_t size, enum dma_data_direction dir)
{
- phys_addr_t paddr = xen_bus_to_phys(dma_addr);
+ phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
if (is_xen_swiotlb_buffer(dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_DEVICE);
--
2.17.1
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 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/3/20 6:22 PM, Stefano Stabellini wrote:
> Hi all,
>
> This series is a collection of fixes to get Linux running on the RPi4 as
> dom0.
>
> Conceptually there are only two significant changes:
>
> - make sure not to call virt_to_page on vmalloc virt addresses (patch
> #1)
> - use phys_to_dma and dma_to_phys to translate phys to/from dma
> addresses (all other patches)
>
> In particular in regards to the second part, the RPi4 is the first
> board where Xen can run that has the property that dma addresses are
> different from physical addresses, and swiotlb-xen was written with the
> assumption that phys addr == dma addr.
>
> This series adds the phys_to_dma and dma_to_phys calls to make it work.
>
> Cheers,
>
> Stefano
(+ Julien)
Reviewed-by: Boris Ostrovsky <[email protected]>
Well, this isn't just RPi4, but basically any ARM or ARM64 system
with non-coherent DMA (which is most of them).
> + struct page *pg;
Please spell out page.
>
> if (hwdev && hwdev->coherent_dma_mask)
> dma_mask = hwdev->coherent_dma_mask;
> @@ -346,9 +347,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
> /* Convert the size to actually allocated. */
> size = 1UL << (order + XEN_PAGE_SHIFT);
>
> + pg = is_vmalloc_addr(vaddr) ? vmalloc_to_page(vaddr) :
> + virt_to_page(vaddr);
Please use plain old if/else to make this more readable.
On Wed, Jun 03, 2020 at 03:22:39PM -0700, Stefano Stabellini wrote:
> From: Stefano Stabellini <[email protected]>
>
> The parameter is unused in this patch.
> No functional changes.
This looks weird. I'm pretty sure you are going to use it later, but
why not just add the argument when it actually is used?
Same comment as for the last one. Also in the subject a whitespace
is missing after "device" and before "*".
On Wed, Jun 03, 2020 at 03:22:45PM -0700, Stefano Stabellini wrote:
> so that their names can better describe their behavior.
>
> No functional changes.
I think this should go with the actual change, and adding the
parameters. Touching this function piecemail in three patches for
what really is a single logical change is rather strange.
On Wed, Jun 03, 2020 at 03:22:44PM -0700, Stefano Stabellini wrote:
> From: Stefano Stabellini <[email protected]>
>
> 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 <[email protected]>
> Tested-by: Corey Minyard <[email protected]>
> Tested-by: Roman Shaposhnik <[email protected]>
> ---
> 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 Wed, Jun 03, 2020 at 03:22:46PM -0700, Stefano Stabellini wrote:
> From: Stefano Stabellini <[email protected]>
>
> xen_dma_sync_for_cpu, xen_dma_sync_for_device, xen_arch_need_swiotlb are
> getting called passing dma addresses. On some platforms dma addresses
> could be different from physical addresses. Before doing any operations
> on these addresses we need to convert them back to physical addresses
> using dma_to_phys.
>
> Add dma_to_phys calls to xen_dma_sync_for_cpu, xen_dma_sync_for_device,
> and xen_arch_need_swiotlb.
>
> dma_cache_maint is fixed by the next patch.
The calling conventions because really weird now because
xen_dma_sync_for_{device,cpu} already get both a phys_addr_t and
a dma_addr_t.
>
> - if (pfn_valid(PFN_DOWN(handle)))
> + if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle))))
But here we translate the dma address to a phys addr
> arch_sync_dma_for_cpu(paddr, size, dir);
While this still uses the passed in paddr. I think the uses of
addresses in this code really needs a major rethink.
On Mon, 8 Jun 2020, Christoph Hellwig wrote:
> On Wed, Jun 03, 2020 at 03:22:39PM -0700, Stefano Stabellini wrote:
> > From: Stefano Stabellini <[email protected]>
> >
> > The parameter is unused in this patch.
> > No functional changes.
>
> This looks weird. I'm pretty sure you are going to use it later, but
> why not just add the argument when it actually is used?
It is just a matter of taste. Xen reviewers tend to ask for splitting
patches into small chunks, especially large verbose non-functional
changes like renaming or adding parameters. It is supposed to make it
easier to review, to make it easier not to get distracted by
renaming/non-functional changes while looking at the important changes.
As a contributor, I am happy either way.
Hi Christoph,
Thanks you for the review.
On Mon, 8 Jun 2020, Christoph Hellwig wrote:
> Well, this isn't just RPi4, but basically any ARM or ARM64 system
> with non-coherent DMA (which is most of them).
Well... yes :-)
> > + struct page *pg;
>
> Please spell out page.
OK
> >
> > if (hwdev && hwdev->coherent_dma_mask)
> > dma_mask = hwdev->coherent_dma_mask;
> > @@ -346,9 +347,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
> > /* Convert the size to actually allocated. */
> > size = 1UL << (order + XEN_PAGE_SHIFT);
> >
> > + pg = is_vmalloc_addr(vaddr) ? vmalloc_to_page(vaddr) :
> > + virt_to_page(vaddr);
>
> Please use plain old if/else to make this more readable.
Sure
On Mon, 8 Jun 2020, Christoph Hellwig wrote:
> On Wed, Jun 03, 2020 at 03:22:44PM -0700, Stefano Stabellini wrote:
> > From: Stefano Stabellini <[email protected]>
> >
> > 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 <[email protected]>
> > Tested-by: Corey Minyard <[email protected]>
> > Tested-by: Roman Shaposhnik <[email protected]>
> > ---
> > 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, Christoph Hellwig wrote:
> On Wed, Jun 03, 2020 at 03:22:46PM -0700, Stefano Stabellini wrote:
> > From: Stefano Stabellini <[email protected]>
> >
> > xen_dma_sync_for_cpu, xen_dma_sync_for_device, xen_arch_need_swiotlb are
> > getting called passing dma addresses. On some platforms dma addresses
> > could be different from physical addresses. Before doing any operations
> > on these addresses we need to convert them back to physical addresses
> > using dma_to_phys.
> >
> > Add dma_to_phys calls to xen_dma_sync_for_cpu, xen_dma_sync_for_device,
> > and xen_arch_need_swiotlb.
> >
> > dma_cache_maint is fixed by the next patch.
>
> The calling conventions because really weird now because
> xen_dma_sync_for_{device,cpu} already get both a phys_addr_t and
> a dma_addr_t.
>
> >
> > - if (pfn_valid(PFN_DOWN(handle)))
> > + if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle))))
>
> But here we translate the dma address to a phys addr
>
> > arch_sync_dma_for_cpu(paddr, size, dir);
>
> While this still uses the passed in paddr. I think the uses of
> addresses in this code really needs a major rethink.
Yeah, the pfn_valid check is a bit weird by definition because we are
using it to understand whether the address belong to us or to another
VM. To do the pfn_valid check we need to translate the dma address into
something the CPU understands, hence, the dma_to_phys call.
Why can't we use the already-provided paddr? Because paddr has been
translated twice:
1) from dma to maybe-foreign phys address (could be ours, or another VM)
2) from maybe-foreign address to local (using our local mapping of the foreign page)
In fact, it would be clearer if we had all three addresses as parameters
of xen_dma_sync_for_cpu: the dma address, the maybe-foreign physical
address (we tend to call it xenbus address, baddr), the local physical
address. Something like:
void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
phys_addr_t baddr, phys_addr_t paddr, size_t size,
enum dma_data_direction dir)
{
if (pfn_valid(baddr))
arch_sync_dma_for_cpu(paddr, size, dir);
else if (dir != DMA_TO_DEVICE)
dma_cache_maint(dev, handle, size, GNTTAB_CACHE_INVAL);
}
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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Tested-by: Corey Minyard <[email protected]>
> > > Tested-by: Roman Shaposhnik <[email protected]>
> > > ---
> > > 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, Jun 08, 2020 at 05:38:28PM -0700, Stefano Stabellini wrote:
> Yeah, the pfn_valid check is a bit weird by definition because we are
> using it to understand whether the address belong to us or to another
> VM. To do the pfn_valid check we need to translate the dma address into
> something the CPU understands, hence, the dma_to_phys call.
>
> Why can't we use the already-provided paddr? Because paddr has been
> translated twice:
> 1) from dma to maybe-foreign phys address (could be ours, or another VM)
> 2) from maybe-foreign address to local (using our local mapping of the foreign page)
>
> In fact, it would be clearer if we had all three addresses as parameters
> of xen_dma_sync_for_cpu: the dma address, the maybe-foreign physical
> address (we tend to call it xenbus address, baddr), the local physical
> address. Something like:
I think instead we should move the arch_sync_dma_for_{device,cpu}
calls from xen_dma_sync_for_{device,cpu} into the callers, as they
are provided by the generic dma-noncoherent.h and optimized out for
coherent architectures like x86. Then the swiotlb-xen.c code only
need to call dma_cache_maint as the interface (which would have to
grow a better name), which should then only need a single kind of
address.
On Mon, Jun 08, 2020 at 10:38:02PM -0700, Christoph Hellwig wrote:
> On Mon, Jun 08, 2020 at 05:38:28PM -0700, Stefano Stabellini wrote:
> > Yeah, the pfn_valid check is a bit weird by definition because we are
> > using it to understand whether the address belong to us or to another
> > VM. To do the pfn_valid check we need to translate the dma address into
> > something the CPU understands, hence, the dma_to_phys call.
> >
> > Why can't we use the already-provided paddr? Because paddr has been
> > translated twice:
> > 1) from dma to maybe-foreign phys address (could be ours, or another VM)
> > 2) from maybe-foreign address to local (using our local mapping of the foreign page)
> >
> > In fact, it would be clearer if we had all three addresses as parameters
> > of xen_dma_sync_for_cpu: the dma address, the maybe-foreign physical
> > address (we tend to call it xenbus address, baddr), the local physical
> > address. Something like:
>
> I think instead we should move the arch_sync_dma_for_{device,cpu}
> calls from xen_dma_sync_for_{device,cpu} into the callers, as they
> are provided by the generic dma-noncoherent.h and optimized out for
> coherent architectures like x86. Then the swiotlb-xen.c code only
> need to call dma_cache_maint as the interface (which would have to
> grow a better name), which should then only need a single kind of
> address.
... actually I'd keep the xen_dma_sync_for_{device,cpu} names for the
low-level interface, just move the arch_sync_dma_for_{device,cpu}
calls up.
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.
On Mon, 8 Jun 2020, Christoph Hellwig wrote:
> On Mon, Jun 08, 2020 at 10:38:02PM -0700, Christoph Hellwig wrote:
> > On Mon, Jun 08, 2020 at 05:38:28PM -0700, Stefano Stabellini wrote:
> > > Yeah, the pfn_valid check is a bit weird by definition because we are
> > > using it to understand whether the address belong to us or to another
> > > VM. To do the pfn_valid check we need to translate the dma address into
> > > something the CPU understands, hence, the dma_to_phys call.
> > >
> > > Why can't we use the already-provided paddr? Because paddr has been
> > > translated twice:
> > > 1) from dma to maybe-foreign phys address (could be ours, or another VM)
> > > 2) from maybe-foreign address to local (using our local mapping of the foreign page)
> > >
> > > In fact, it would be clearer if we had all three addresses as parameters
> > > of xen_dma_sync_for_cpu: the dma address, the maybe-foreign physical
> > > address (we tend to call it xenbus address, baddr), the local physical
> > > address. Something like:
> >
> > I think instead we should move the arch_sync_dma_for_{device,cpu}
> > calls from xen_dma_sync_for_{device,cpu} into the callers, as they
> > are provided by the generic dma-noncoherent.h and optimized out for
> > coherent architectures like x86. Then the swiotlb-xen.c code only
> > need to call dma_cache_maint as the interface (which would have to
> > grow a better name), which should then only need a single kind of
> > address.
>
> ... actually I'd keep the xen_dma_sync_for_{device,cpu} names for the
> low-level interface, just move the arch_sync_dma_for_{device,cpu}
> calls up.
I can do that.