[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • From: Jürgen Groß <jgross@xxxxxxxx>
  • Date: Wed, 12 Feb 2025 12:15:07 +0100
  • Autocrypt: addr=jgross@xxxxxxxx; keydata= xsBNBFOMcBYBCACgGjqjoGvbEouQZw/ToiBg9W98AlM2QHV+iNHsEs7kxWhKMjrioyspZKOB ycWxw3ie3j9uvg9EOB3aN4xiTv4qbnGiTr3oJhkB1gsb6ToJQZ8uxGq2kaV2KL9650I1SJve dYm8Of8Zd621lSmoKOwlNClALZNew72NjJLEzTalU1OdT7/i1TXkH09XSSI8mEQ/ouNcMvIJ NwQpd369y9bfIhWUiVXEK7MlRgUG6MvIj6Y3Am/BBLUVbDa4+gmzDC9ezlZkTZG2t14zWPvx XP3FAp2pkW0xqG7/377qptDmrk42GlSKN4z76ELnLxussxc7I2hx18NUcbP8+uty4bMxABEB AAHNH0p1ZXJnZW4gR3Jvc3MgPGpncm9zc0BzdXNlLmNvbT7CwHkEEwECACMFAlOMcK8CGwMH CwkIBwMCAQYVCAIJCgsEFgIDAQIeAQIXgAAKCRCw3p3WKL8TL8eZB/9G0juS/kDY9LhEXseh mE9U+iA1VsLhgDqVbsOtZ/S14LRFHczNd/Lqkn7souCSoyWsBs3/wO+OjPvxf7m+Ef+sMtr0 G5lCWEWa9wa0IXx5HRPW/ScL+e4AVUbL7rurYMfwCzco+7TfjhMEOkC+va5gzi1KrErgNRHH kg3PhlnRY0Udyqx++UYkAsN4TQuEhNN32MvN0Np3WlBJOgKcuXpIElmMM5f1BBzJSKBkW0Jc Wy3h2Wy912vHKpPV/Xv7ZwVJ27v7KcuZcErtptDevAljxJtE7aJG6WiBzm+v9EswyWxwMCIO RoVBYuiocc51872tRGywc03xaQydB+9R7BHPzsBNBFOMcBYBCADLMfoA44MwGOB9YT1V4KCy vAfd7E0BTfaAurbG+Olacciz3yd09QOmejFZC6AnoykydyvTFLAWYcSCdISMr88COmmCbJzn sHAogjexXiif6ANUUlHpjxlHCCcELmZUzomNDnEOTxZFeWMTFF9Rf2k2F0Tl4E5kmsNGgtSa aMO0rNZoOEiD/7UfPP3dfh8JCQ1VtUUsQtT1sxos8Eb/HmriJhnaTZ7Hp3jtgTVkV0ybpgFg w6WMaRkrBh17mV0z2ajjmabB7SJxcouSkR0hcpNl4oM74d2/VqoW4BxxxOD1FcNCObCELfIS auZx+XT6s+CE7Qi/c44ibBMR7hyjdzWbABEBAAHCwF8EGAECAAkFAlOMcBYCGwwACgkQsN6d 1ii/Ey9D+Af/WFr3q+bg/8v5tCknCtn92d5lyYTBNt7xgWzDZX8G6/pngzKyWfedArllp0Pn fgIXtMNV+3t8Li1Tg843EXkP7+2+CQ98MB8XvvPLYAfW8nNDV85TyVgWlldNcgdv7nn1Sq8g HwB2BHdIAkYce3hEoDQXt/mKlgEGsLpzJcnLKimtPXQQy9TxUaLBe9PInPd+Ohix0XOlY+Uk QFEx50Ki3rSDl2Zt2tnkNYKUCvTJq7jvOlaPd6d/W0tZqpyy7KVay+K4aMobDsodB3dvEAs6 ScCnh03dDAFgIq5nsB11j3KPKdVoPlfucX2c7kGNH+LUMbzqV6beIENfNexkOfxHfw==
  • Cc: linux-kernel@xxxxxxxxxxxxxxx, x86@xxxxxxxxxx, iommu@xxxxxxxxxxxxxxx, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>, Thomas Gleixner <tglx@xxxxxxxxxxxxx>, Ingo Molnar <mingo@xxxxxxxxxx>, Borislav Petkov <bp@xxxxxxxxx>, Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>, "H. Peter Anvin" <hpa@xxxxxxxxx>, Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Wed, 12 Feb 2025 11:15:11 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 12.02.25 02:30, Stefano Stabellini wrote:
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.

The PageXenRemapped bit is used only for determining whether
xen_destroy_contiguous_region() should be called. And by not setting the bit
I'm avoiding to call xen_destroy_contiguous_region() later. So I don't see any
unfortunate side effect.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.