Re: [PATCH 2/2] xen/swiotlb: don't destroy contiguous region in all cases

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 11 Feb 2025, Juergen Gross wrote:
> In case xen_swiotlb_alloc_coherent() needed to create a contiguous
> region only for other reason than the memory not being compliant with
> the device's DMA mask, there is no reason why this contiguous region
> should be destroyed by xen_swiotlb_free_coherent() later. Destroying
> this region should be done only, if the memory of the region was
> allocated with more stringent placement requirements than the memory
> it did replace.
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
>  arch/x86/include/asm/xen/swiotlb-xen.h |  5 +++--
>  arch/x86/xen/mmu_pv.c                  | 18 ++++++++++++------
>  drivers/xen/swiotlb-xen.c              | 11 +++++++----
>  3 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/include/asm/xen/swiotlb-xen.h 
> b/arch/x86/include/asm/xen/swiotlb-xen.h
> index abde0f44df57..a353f20c7e79 100644
> --- a/arch/x86/include/asm/xen/swiotlb-xen.h
> +++ b/arch/x86/include/asm/xen/swiotlb-xen.h
> @@ -4,8 +4,9 @@
>  
>  int xen_swiotlb_fixup(void *buf, unsigned long nslabs);
>  int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
> -                             unsigned int address_bits,
> -                             dma_addr_t *dma_handle);
> +                              unsigned int address_bits,
> +                              dma_addr_t *dma_handle,
> +                              unsigned int *address_bits_in);
>  void xen_destroy_contiguous_region(phys_addr_t pstart, unsigned int order);
>  
>  #endif /* _ASM_X86_SWIOTLB_XEN_H */
> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
> index 2c70cd35e72c..fb586238f7c4 100644
> --- a/arch/x86/xen/mmu_pv.c
> +++ b/arch/x86/xen/mmu_pv.c
> @@ -2208,19 +2208,22 @@ void __init xen_init_mmu_ops(void)
>  static unsigned long discontig_frames[1<<MAX_CONTIG_ORDER];
>  
>  #define VOID_PTE (mfn_pte(0, __pgprot(0)))
> -static void xen_zap_pfn_range(unsigned long vaddr, unsigned int order,
> -                             unsigned long *in_frames,
> -                             unsigned long *out_frames)
> +static int xen_zap_pfn_range(unsigned long vaddr, unsigned int order,
> +                          unsigned long *in_frames,
> +                          unsigned long *out_frames)
>  {
>       int i;
> +     u64 address_bits = 0;
>       struct multicall_space mcs;
>  
>       xen_mc_batch();
>       for (i = 0; i < (1UL<<order); i++, vaddr += PAGE_SIZE) {
>               mcs = __xen_mc_entry(0);
>  
> -             if (in_frames)
> +             if (in_frames) {
>                       in_frames[i] = virt_to_mfn((void *)vaddr);
> +                     address_bits |= in_frames[i] << PAGE_SHIFT;
> +             }
>  
>               MULTI_update_va_mapping(mcs.mc, vaddr, VOID_PTE, 0);
>               __set_phys_to_machine(virt_to_pfn((void *)vaddr), 
> INVALID_P2M_ENTRY);
> @@ -2229,6 +2232,8 @@ static void xen_zap_pfn_range(unsigned long vaddr, 
> unsigned int order,
>                       out_frames[i] = virt_to_pfn((void *)vaddr);
>       }
>       xen_mc_issue(0);
> +
> +     return fls64(address_bits);
>  }
>  
>  /*
> @@ -2321,7 +2326,8 @@ static int xen_exchange_memory(unsigned long 
> extents_in, unsigned int order_in,
>  
>  int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
>                                unsigned int address_bits,
> -                              dma_addr_t *dma_handle)
> +                              dma_addr_t *dma_handle,
> +                              unsigned int *address_bits_in)
>  {
>       unsigned long *in_frames = discontig_frames, out_frame;
>       unsigned long  flags;
> @@ -2336,7 +2342,7 @@ int xen_create_contiguous_region(phys_addr_t pstart, 
> unsigned int order,
>       spin_lock_irqsave(&xen_reservation_lock, flags);
>  
>       /* 1. Zap current PTEs, remembering MFNs. */
> -     xen_zap_pfn_range(vstart, order, in_frames, NULL);
> +     *address_bits_in = xen_zap_pfn_range(vstart, order, in_frames, NULL);
>  
>       /* 2. Get a new contiguous memory extent. */
>       out_frame = virt_to_pfn((void *)vstart);
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 26c62e0d34e9..3f3724f53914 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -118,6 +118,7 @@ int xen_swiotlb_fixup(void *buf, unsigned long nslabs)
>       int rc;
>       unsigned int order = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT);
>       unsigned int i, dma_bits = order + PAGE_SHIFT;
> +     unsigned int dummy;
>       dma_addr_t dma_handle;
>       phys_addr_t p = virt_to_phys(buf);
>  
> @@ -129,7 +130,7 @@ int xen_swiotlb_fixup(void *buf, unsigned long nslabs)
>               do {
>                       rc = xen_create_contiguous_region(
>                               p + (i << IO_TLB_SHIFT), order,
> -                             dma_bits, &dma_handle);
> +                             dma_bits, &dma_handle, &dummy);
>               } while (rc && dma_bits++ < MAX_DMA_BITS);
>               if (rc)
>                       return rc;
> @@ -144,6 +145,7 @@ xen_swiotlb_alloc_coherent(struct device *dev, size_t 
> size,
>               dma_addr_t *dma_handle, gfp_t flags, unsigned long attrs)
>  {
>       u64 dma_mask = dev->coherent_dma_mask;
> +     unsigned int address_bits = fls64(dma_mask), address_bits_in;
>       int order = get_order(size);
>       phys_addr_t phys;
>       void *ret;
> @@ -160,10 +162,11 @@ xen_swiotlb_alloc_coherent(struct device *dev, size_t 
> size,
>       if (*dma_handle + size - 1 > dma_mask ||
>           range_straddles_page_boundary(phys, size) ||
>           range_requires_alignment(phys, size)) {
> -             if (xen_create_contiguous_region(phys, order, fls64(dma_mask),
> -                             dma_handle) != 0)
> +             if (xen_create_contiguous_region(phys, order, address_bits,
> +                                              dma_handle, &address_bits_in))
>                       goto out_free_pages;
> -             SetPageXenRemapped(virt_to_page(ret));
> +             if (address_bits_in > address_bits)
> +                     SetPageXenRemapped(virt_to_page(ret));

This has the unfortunate side effect of making "PageXenRemapped"
unreliable as an indicator of whether a page has been remapped. A page
could still be remapped without the "PageXenRemapped" bit being set.  

I recommend adding an in-code comment to clarify this behavior.



>       }
>  
>       memset(ret, 0, size);
> -- 
> 2.43.0
>