[PATCH, RFC] swiotlb-xen: fix dma to physical address translation for cache operations

[PATCH, RFC] swiotlb-xen: fix dma to physical address translation for cache operations

[Christoph Hellwig]

All other places in swiotlb-xen got from PFN to BFN and then call
phys_to_dma on the result or vice versa, but the reverse mapping used
for cache maintenance skips the BFN to PFN mapping.

[Note: only found by code inspection, please review very carefully!]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 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 67aa74d201627d..e4620303138b4d 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -234,7 +234,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)) {
-		if (pfn_valid(PFN_DOWN(dma_to_phys(dev, dev_addr))))
+		if (pfn_valid(PFN_DOWN(phys)))
 			arch_sync_dma_for_device(phys, size, dir);
 		else
 			xen_dma_sync_for_device(dev, dev_addr, size, dir);
@@ -258,7 +258,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)) {
-		if (pfn_valid(PFN_DOWN(dma_to_phys(hwdev, dev_addr))))
+		if (pfn_valid(PFN_DOWN(paddr)))
 			arch_sync_dma_for_cpu(paddr, size, dir);
 		else
 			xen_dma_sync_for_cpu(hwdev, dev_addr, size, dir);
@@ -276,7 +276,7 @@ xen_swiotlb_sync_single_for_cpu(struct device *dev, dma_addr_t dma_addr,
 	phys_addr_t paddr = xen_dma_to_phys(dev, dma_addr);
 
 	if (!dev_is_dma_coherent(dev)) {
-		if (pfn_valid(PFN_DOWN(dma_to_phys(dev, dma_addr))))
+		if (pfn_valid(PFN_DOWN(paddr)))
 			arch_sync_dma_for_cpu(paddr, size, dir);
 		else
 			xen_dma_sync_for_cpu(dev, dma_addr, size, dir);
@@ -296,7 +296,7 @@ xen_swiotlb_sync_single_for_device(struct device *dev, dma_addr_t dma_addr,
 		swiotlb_sync_single_for_device(dev, paddr, size, dir);
 
 	if (!dev_is_dma_coherent(dev)) {
-		if (pfn_valid(PFN_DOWN(dma_to_phys(dev, dma_addr))))
+		if (pfn_valid(PFN_DOWN(paddr)))
 			arch_sync_dma_for_device(paddr, size, dir);
 		else
 			xen_dma_sync_for_device(dev, dma_addr, size, dir);
-- 
2.39.2

Re: [PATCH, RFC] swiotlb-xen: fix dma to physical address translation for cache operations

[Stefano Stabellini]

On Wed, 7 Jun 2023, Christoph Hellwig wrote:
> All other places in swiotlb-xen got from PFN to BFN and then call
> phys_to_dma on the result or vice versa, but the reverse mapping used
> for cache maintenance skips the BFN to PFN mapping.
> 
> [Note: only found by code inspection, please review very carefully!]
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Hi Christoph,

No, these checks are done on purpose differently. Let me explain.


xen_phys_to_dma does 2 translations:
1) guest physical to real physical (also called mfn in Xen)
2) real physical to dma

2) is not interesting in this discussion, 1) is.

Dom0 is mapped 1:1 guest physical == real physical by default on ARM.

For any Dom0 memory page:
- guest physical == real physical

Dom0 sometimes maps memory of other domains. By definitions, those are
not 1:1 mapped. For those:
- guest physical != real physical

Linux normally only knows about guest physical addresses, not real
physical addresses.


The checks below are meant to check if a given page is a regular Dom0
page, or a page of another domain. The check relies on the fact that if
it is a local Dom0 page then (guest physical == real physical) and
pfn_valid return true, otherwise if it not a local Dom0 page then (guest
physical != real physical) and pfn_valid return false.

Basically the check is doing:

  pfn_valid(real phys address)

In the local case:

  pfn_valid(real phys address) == pfn_valid(guest phys address) => true

In the foreign case:

  pfn_valid(real phys address) != pfn_valid(guest phys address) => false


Note that pfn_valid(guest phys address) would return true in both cases
because the foreign page is mapped over a valid Dom0 guest physical
address.


In short, it is a simple trick to detect if the address corresponds to a
regular Dom0 memory page or not.

Cheers,

Stefano

> ---
>  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 67aa74d201627d..e4620303138b4d 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -234,7 +234,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)) {
> -		if (pfn_valid(PFN_DOWN(dma_to_phys(dev, dev_addr))))
> +		if (pfn_valid(PFN_DOWN(phys)))
>  			arch_sync_dma_for_device(phys, size, dir);
>  		else
>  			xen_dma_sync_for_device(dev, dev_addr, size, dir);
> @@ -258,7 +258,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)) {
> -		if (pfn_valid(PFN_DOWN(dma_to_phys(hwdev, dev_addr))))
> +		if (pfn_valid(PFN_DOWN(paddr)))
>  			arch_sync_dma_for_cpu(paddr, size, dir);
>  		else
>  			xen_dma_sync_for_cpu(hwdev, dev_addr, size, dir);
> @@ -276,7 +276,7 @@ xen_swiotlb_sync_single_for_cpu(struct device *dev, dma_addr_t dma_addr,
>  	phys_addr_t paddr = xen_dma_to_phys(dev, dma_addr);
>  
>  	if (!dev_is_dma_coherent(dev)) {
> -		if (pfn_valid(PFN_DOWN(dma_to_phys(dev, dma_addr))))
> +		if (pfn_valid(PFN_DOWN(paddr)))
>  			arch_sync_dma_for_cpu(paddr, size, dir);
>  		else
>  			xen_dma_sync_for_cpu(dev, dma_addr, size, dir);
> @@ -296,7 +296,7 @@ xen_swiotlb_sync_single_for_device(struct device *dev, dma_addr_t dma_addr,
>  		swiotlb_sync_single_for_device(dev, paddr, size, dir);
>  
>  	if (!dev_is_dma_coherent(dev)) {
> -		if (pfn_valid(PFN_DOWN(dma_to_phys(dev, dma_addr))))
> +		if (pfn_valid(PFN_DOWN(paddr)))
>  			arch_sync_dma_for_device(paddr, size, dir);
>  		else
>  			xen_dma_sync_for_device(dev, dma_addr, size, dir);
> -- 
> 2.39.2
>