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

["Jürgen Groß" <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 6262 bytes --]

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@suse.com>
>> ---
>>   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 #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]