[PATCH 00/12] swiotlb-xen: fixes and adjustments

[PATCH 00/12] swiotlb-xen: fixes and adjustments

[Jan Beulich]

The primary intention really was the last patch, there you go ...

01: swiotlb-xen: avoid double free
02: swiotlb-xen: fix late init retry
03: swiotlb-xen: maintain slab count properly
04: swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
05: swiotlb-xen: suppress certain init retries
06: swiotlb-xen: limit init retries
07: swiotlb-xen: drop leftover __ref
08: swiotlb-xen: arrange to have buffer info logged
09: swiotlb-xen: drop DEFAULT_NSLABS
10: xen-pcifront: this module is PV-only
11: xen/pci-swiotlb: reduce visibility of symbols
12: swiotlb-xen: this is PV-only on x86

Jan

[PATCH 01/12] swiotlb-xen: avoid double free

[Jan Beulich]

Of the two paths leading to the "error" label in xen_swiotlb_init() one
didn't allocate anything, while the other did already free what was
allocated.

Fixes: b82776005369 ("xen/swiotlb: Use the swiotlb_late_init_with_tbl to init Xen-SWIOTLB late when PV PCI is used")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: stable@vger.kernel.org

--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -216,7 +216,6 @@ error:
 		goto retry;
 	}
 	pr_err("%s (rc:%d)\n", xen_swiotlb_error(m_ret), rc);
-	free_pages((unsigned long)start, order);
 	return rc;
 }
 

Re: [PATCH 01/12] swiotlb-xen: avoid double free

[Christoph Hellwig]

On Tue, Sep 07, 2021 at 02:04:25PM +0200, Jan Beulich wrote:
> Of the two paths leading to the "error" label in xen_swiotlb_init() one
> didn't allocate anything, while the other did already free what was
> allocated.
> 
> Fixes: b82776005369 ("xen/swiotlb: Use the swiotlb_late_init_with_tbl to init Xen-SWIOTLB late when PV PCI is used")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: stable@vger.kernel.org

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 01/12] swiotlb-xen: avoid double free

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> Of the two paths leading to the "error" label in xen_swiotlb_init() one
> didn't allocate anything, while the other did already free what was
> allocated.
> 
> Fixes: b82776005369 ("xen/swiotlb: Use the swiotlb_late_init_with_tbl to init Xen-SWIOTLB late when PV PCI is used")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -216,7 +216,6 @@ error:
>  		goto retry;
>  	}
>  	pr_err("%s (rc:%d)\n", xen_swiotlb_error(m_ret), rc);
> -	free_pages((unsigned long)start, order);
>  	return rc;
>  }
>  
> 

Re: [PATCH 01/12] swiotlb-xen: avoid double free

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> Of the two paths leading to the "error" label in xen_swiotlb_init() one
> didn't allocate anything, while the other did already free what was
> allocated.
> 
> Fixes: b82776005369 ("xen/swiotlb: Use the swiotlb_late_init_with_tbl to init Xen-SWIOTLB late when PV PCI is used")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -216,7 +216,6 @@ error:
>  		goto retry;
>  	}
>  	pr_err("%s (rc:%d)\n", xen_swiotlb_error(m_ret), rc);
> -	free_pages((unsigned long)start, order);
>  	return rc;
>  }
>  
> 

[PATCH 02/12] swiotlb-xen: fix late init retry

[Jan Beulich]

The commit referenced below removed the assignment of "bytes" from
xen_swiotlb_init() without - like done for xen_swiotlb_init_early() -
adding an assignment on the retry path, thus leading to excessively
sized allocations upon retries.

Fixes: 2d29960af0be ("swiotlb: dynamically allocate io_tlb_default_mem")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: stable@vger.kernel.org

--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -211,8 +211,8 @@ error:
 	if (repeat--) {
 		/* Min is 2MB */
 		nslabs = max(1024UL, (nslabs >> 1));
-		pr_info("Lowering to %luMB\n",
-			(nslabs << IO_TLB_SHIFT) >> 20);
+		bytes = nslabs << IO_TLB_SHIFT;
+		pr_info("Lowering to %luMB\n", bytes >> 20);
 		goto retry;
 	}
 	pr_err("%s (rc:%d)\n", xen_swiotlb_error(m_ret), rc);

Re: [PATCH 02/12] swiotlb-xen: fix late init retry

[Christoph Hellwig]

On Tue, Sep 07, 2021 at 02:04:47PM +0200, Jan Beulich wrote:
> The commit referenced below removed the assignment of "bytes" from
> xen_swiotlb_init() without - like done for xen_swiotlb_init_early() -
> adding an assignment on the retry path, thus leading to excessively
> sized allocations upon retries.
> 
> Fixes: 2d29960af0be ("swiotlb: dynamically allocate io_tlb_default_mem")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: stable@vger.kernel.org

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 03/12] swiotlb-xen: maintain slab count properly

[Jan Beulich]

Generic swiotlb code makes sure to keep the slab count a multiple of the
number of slabs per segment. Yet even without checking whether any such
assumption is made elsewhere, it is easy to see that xen_swiotlb_fixup()
might alter unrelated memory when calling xen_create_contiguous_region()
for the last segment, when that's not a full one - the function acts on
full order-N regions, not individual pages.

Align the slab count suitably when halving it for a retry. Add a build
time check and a runtime one. Replace the no longer useful local
variable "slabs" by an "order" one calculated just once, outside of the
loop. Re-use "order" for calculating "dma_bits", and change the type of
the latter as well as the one of "i" while touching this anyway.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -106,27 +106,26 @@ static int is_xen_swiotlb_buffer(struct
 
 static int xen_swiotlb_fixup(void *buf, unsigned long nslabs)
 {
-	int i, rc;
-	int dma_bits;
+	int rc;
+	unsigned int order = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT);
+	unsigned int i, dma_bits = order + PAGE_SHIFT;
 	dma_addr_t dma_handle;
 	phys_addr_t p = virt_to_phys(buf);
 
-	dma_bits = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT) + PAGE_SHIFT;
+	BUILD_BUG_ON(IO_TLB_SEGSIZE & (IO_TLB_SEGSIZE - 1));
+	BUG_ON(nslabs % IO_TLB_SEGSIZE);
 
 	i = 0;
 	do {
-		int slabs = min(nslabs - i, (unsigned long)IO_TLB_SEGSIZE);
-
 		do {
 			rc = xen_create_contiguous_region(
-				p + (i << IO_TLB_SHIFT),
-				get_order(slabs << IO_TLB_SHIFT),
+				p + (i << IO_TLB_SHIFT), order,
 				dma_bits, &dma_handle);
 		} while (rc && dma_bits++ < MAX_DMA_BITS);
 		if (rc)
 			return rc;
 
-		i += slabs;
+		i += IO_TLB_SEGSIZE;
 	} while (i < nslabs);
 	return 0;
 }
@@ -210,7 +209,7 @@ retry:
 error:
 	if (repeat--) {
 		/* Min is 2MB */
-		nslabs = max(1024UL, (nslabs >> 1));
+		nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
 		bytes = nslabs << IO_TLB_SHIFT;
 		pr_info("Lowering to %luMB\n", bytes >> 20);
 		goto retry;
@@ -245,7 +244,7 @@ retry:
 		memblock_free(__pa(start), PAGE_ALIGN(bytes));
 		if (repeat--) {
 			/* Min is 2MB */
-			nslabs = max(1024UL, (nslabs >> 1));
+			nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
 			bytes = nslabs << IO_TLB_SHIFT;
 			pr_info("Lowering to %luMB\n", bytes >> 20);
 			goto retry;

Re: [PATCH 03/12] swiotlb-xen: maintain slab count properly

[Christoph Hellwig]

On Tue, Sep 07, 2021 at 02:05:12PM +0200, Jan Beulich wrote:
> Generic swiotlb code makes sure to keep the slab count a multiple of the
> number of slabs per segment. Yet even without checking whether any such
> assumption is made elsewhere, it is easy to see that xen_swiotlb_fixup()
> might alter unrelated memory when calling xen_create_contiguous_region()
> for the last segment, when that's not a full one - the function acts on
> full order-N regions, not individual pages.
> 
> Align the slab count suitably when halving it for a retry. Add a build
> time check and a runtime one. Replace the no longer useful local
> variable "slabs" by an "order" one calculated just once, outside of the
> loop. Re-use "order" for calculating "dma_bits", and change the type of
> the latter as well as the one of "i" while touching this anyway.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 03/12] swiotlb-xen: maintain slab count properly

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> Generic swiotlb code makes sure to keep the slab count a multiple of the
> number of slabs per segment. Yet even without checking whether any such
> assumption is made elsewhere, it is easy to see that xen_swiotlb_fixup()
> might alter unrelated memory when calling xen_create_contiguous_region()
> for the last segment, when that's not a full one - the function acts on
> full order-N regions, not individual pages.
> 
> Align the slab count suitably when halving it for a retry. Add a build
> time check and a runtime one. Replace the no longer useful local
> variable "slabs" by an "order" one calculated just once, outside of the
> loop. Re-use "order" for calculating "dma_bits", and change the type of
> the latter as well as the one of "i" while touching this anyway.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -106,27 +106,26 @@ static int is_xen_swiotlb_buffer(struct
>  
>  static int xen_swiotlb_fixup(void *buf, unsigned long nslabs)
>  {
> -	int i, rc;
> -	int dma_bits;
> +	int rc;
> +	unsigned int order = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT);
> +	unsigned int i, dma_bits = order + PAGE_SHIFT;
>  	dma_addr_t dma_handle;
>  	phys_addr_t p = virt_to_phys(buf);
>  
> -	dma_bits = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT) + PAGE_SHIFT;
> +	BUILD_BUG_ON(IO_TLB_SEGSIZE & (IO_TLB_SEGSIZE - 1));
> +	BUG_ON(nslabs % IO_TLB_SEGSIZE);
>  
>  	i = 0;
>  	do {
> -		int slabs = min(nslabs - i, (unsigned long)IO_TLB_SEGSIZE);
> -
>  		do {
>  			rc = xen_create_contiguous_region(
> -				p + (i << IO_TLB_SHIFT),
> -				get_order(slabs << IO_TLB_SHIFT),
> +				p + (i << IO_TLB_SHIFT), order,
>  				dma_bits, &dma_handle);
>  		} while (rc && dma_bits++ < MAX_DMA_BITS);
>  		if (rc)
>  			return rc;
>  
> -		i += slabs;
> +		i += IO_TLB_SEGSIZE;
>  	} while (i < nslabs);
>  	return 0;
>  }
> @@ -210,7 +209,7 @@ retry:
>  error:
>  	if (repeat--) {
>  		/* Min is 2MB */
> -		nslabs = max(1024UL, (nslabs >> 1));
> +		nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
>  		bytes = nslabs << IO_TLB_SHIFT;
>  		pr_info("Lowering to %luMB\n", bytes >> 20);
>  		goto retry;
> @@ -245,7 +244,7 @@ retry:
>  		memblock_free(__pa(start), PAGE_ALIGN(bytes));
>  		if (repeat--) {
>  			/* Min is 2MB */
> -			nslabs = max(1024UL, (nslabs >> 1));
> +			nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
>  			bytes = nslabs << IO_TLB_SHIFT;
>  			pr_info("Lowering to %luMB\n", bytes >> 20);
>  			goto retry;
> 

Re: [PATCH 03/12] swiotlb-xen: maintain slab count properly

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> Generic swiotlb code makes sure to keep the slab count a multiple of the
> number of slabs per segment. Yet even without checking whether any such
> assumption is made elsewhere, it is easy to see that xen_swiotlb_fixup()
> might alter unrelated memory when calling xen_create_contiguous_region()
> for the last segment, when that's not a full one - the function acts on
> full order-N regions, not individual pages.
> 
> Align the slab count suitably when halving it for a retry. Add a build
> time check and a runtime one. Replace the no longer useful local
> variable "slabs" by an "order" one calculated just once, outside of the
> loop. Re-use "order" for calculating "dma_bits", and change the type of
> the latter as well as the one of "i" while touching this anyway.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -106,27 +106,26 @@ static int is_xen_swiotlb_buffer(struct
>  
>  static int xen_swiotlb_fixup(void *buf, unsigned long nslabs)
>  {
> -	int i, rc;
> -	int dma_bits;
> +	int rc;
> +	unsigned int order = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT);
> +	unsigned int i, dma_bits = order + PAGE_SHIFT;
>  	dma_addr_t dma_handle;
>  	phys_addr_t p = virt_to_phys(buf);
>  
> -	dma_bits = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT) + PAGE_SHIFT;
> +	BUILD_BUG_ON(IO_TLB_SEGSIZE & (IO_TLB_SEGSIZE - 1));
> +	BUG_ON(nslabs % IO_TLB_SEGSIZE);
>  
>  	i = 0;
>  	do {
> -		int slabs = min(nslabs - i, (unsigned long)IO_TLB_SEGSIZE);
> -
>  		do {
>  			rc = xen_create_contiguous_region(
> -				p + (i << IO_TLB_SHIFT),
> -				get_order(slabs << IO_TLB_SHIFT),
> +				p + (i << IO_TLB_SHIFT), order,
>  				dma_bits, &dma_handle);
>  		} while (rc && dma_bits++ < MAX_DMA_BITS);
>  		if (rc)
>  			return rc;
>  
> -		i += slabs;
> +		i += IO_TLB_SEGSIZE;
>  	} while (i < nslabs);
>  	return 0;
>  }
> @@ -210,7 +209,7 @@ retry:
>  error:
>  	if (repeat--) {
>  		/* Min is 2MB */
> -		nslabs = max(1024UL, (nslabs >> 1));
> +		nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
>  		bytes = nslabs << IO_TLB_SHIFT;
>  		pr_info("Lowering to %luMB\n", bytes >> 20);
>  		goto retry;
> @@ -245,7 +244,7 @@ retry:
>  		memblock_free(__pa(start), PAGE_ALIGN(bytes));
>  		if (repeat--) {
>  			/* Min is 2MB */
> -			nslabs = max(1024UL, (nslabs >> 1));
> +			nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
>  			bytes = nslabs << IO_TLB_SHIFT;
>  			pr_info("Lowering to %luMB\n", bytes >> 20);
>  			goto retry;
> 

[PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests

[Jan Beulich]

While the hypervisor hasn't been enforcing this, we would still better
avoid issuing requests with GFNs not aligned to the requested order.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I wonder how useful it is to include the alignment in the panic()
message. I further wonder how useful it is to wrap "bytes" in
PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
least was supposed to be, prior to "swiotlb-xen: maintain slab count
properly").

--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -231,10 +231,10 @@ retry:
 	/*
 	 * Get IO TLB memory from any location.
 	 */
-	start = memblock_alloc(PAGE_ALIGN(bytes), PAGE_SIZE);
+	start = memblock_alloc(PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
 	if (!start)
-		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
-		      __func__, PAGE_ALIGN(bytes), PAGE_SIZE);
+		panic("%s: Failed to allocate %lu bytes align=%#x\n",
+		      __func__, PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
 
 	/*
 	 * And replace that memory with pages under 4GB.

Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests

[Christoph Hellwig]

On Tue, Sep 07, 2021 at 02:05:32PM +0200, Jan Beulich wrote:
> While the hypervisor hasn't been enforcing this, we would still better
> avoid issuing requests with GFNs not aligned to the requested order.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I wonder how useful it is to include the alignment in the panic()
> message. I further wonder how useful it is to wrap "bytes" in
> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
> least was supposed to be, prior to "swiotlb-xen: maintain slab count
> properly").
> 
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -231,10 +231,10 @@ retry:
>  	/*
>  	 * Get IO TLB memory from any location.
>  	 */
> -	start = memblock_alloc(PAGE_ALIGN(bytes), PAGE_SIZE);
> +	start = memblock_alloc(PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
>  	if (!start)
> -		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> -		      __func__, PAGE_ALIGN(bytes), PAGE_SIZE);
> +		panic("%s: Failed to allocate %lu bytes align=%#x\n",
> +		      __func__, PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);

CAn you avoid the overly long lines here?  A good way to make it more
readable would be a variable to hold the byte count.

Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests

[Jan Beulich]

On 08.09.2021 08:57, Christoph Hellwig wrote:
> On Tue, Sep 07, 2021 at 02:05:32PM +0200, Jan Beulich wrote:
>> While the hypervisor hasn't been enforcing this, we would still better
>> avoid issuing requests with GFNs not aligned to the requested order.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I wonder how useful it is to include the alignment in the panic()
>> message. I further wonder how useful it is to wrap "bytes" in
>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
>> least was supposed to be, prior to "swiotlb-xen: maintain slab count
>> properly").
>>
>> --- a/drivers/xen/swiotlb-xen.c
>> +++ b/drivers/xen/swiotlb-xen.c
>> @@ -231,10 +231,10 @@ retry:
>>  	/*
>>  	 * Get IO TLB memory from any location.
>>  	 */
>> -	start = memblock_alloc(PAGE_ALIGN(bytes), PAGE_SIZE);
>> +	start = memblock_alloc(PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
>>  	if (!start)
>> -		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
>> -		      __func__, PAGE_ALIGN(bytes), PAGE_SIZE);
>> +		panic("%s: Failed to allocate %lu bytes align=%#x\n",
>> +		      __func__, PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
> 
> CAn you avoid the overly long lines here?  A good way to make it more
> readable would be a variable to hold the byte count.

There already is a variable for the byte count - bytes. Did you read
the post-commit-message remark? _That's_ what I'd prefer to shorten
things. Meanwhile I can of course wrap lines; I will admit that I
failed to pay attention to line length here.

Jan

Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> While the hypervisor hasn't been enforcing this, we would still better
> avoid issuing requests with GFNs not aligned to the requested order.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I wonder how useful it is to include the alignment in the panic()
> message.

Not very useful given that it is static. I don't mind either way but you
can go ahead and remove it if you prefer (and it would make the line
shorter.)


> I further wonder how useful it is to wrap "bytes" in
> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
> least was supposed to be, prior to "swiotlb-xen: maintain slab count
> properly").

This one I would keep, to make sure to print out the same amount passed
to memblock_alloc.


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -231,10 +231,10 @@ retry:
>  	/*
>  	 * Get IO TLB memory from any location.
>  	 */
> -	start = memblock_alloc(PAGE_ALIGN(bytes), PAGE_SIZE);
> +	start = memblock_alloc(PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
>  	if (!start)
> -		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> -		      __func__, PAGE_ALIGN(bytes), PAGE_SIZE);
> +		panic("%s: Failed to allocate %lu bytes align=%#x\n",
> +		      __func__, PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
>  
>  	/*
>  	 * And replace that memory with pages under 4GB.
> 

Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> While the hypervisor hasn't been enforcing this, we would still better
> avoid issuing requests with GFNs not aligned to the requested order.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I wonder how useful it is to include the alignment in the panic()
> message.

Not very useful given that it is static. I don't mind either way but you
can go ahead and remove it if you prefer (and it would make the line
shorter.)


> I further wonder how useful it is to wrap "bytes" in
> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
> least was supposed to be, prior to "swiotlb-xen: maintain slab count
> properly").

This one I would keep, to make sure to print out the same amount passed
to memblock_alloc.


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -231,10 +231,10 @@ retry:
>  	/*
>  	 * Get IO TLB memory from any location.
>  	 */
> -	start = memblock_alloc(PAGE_ALIGN(bytes), PAGE_SIZE);
> +	start = memblock_alloc(PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
>  	if (!start)
> -		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> -		      __func__, PAGE_ALIGN(bytes), PAGE_SIZE);
> +		panic("%s: Failed to allocate %lu bytes align=%#x\n",
> +		      __func__, PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
>  
>  	/*
>  	 * And replace that memory with pages under 4GB.
> 

Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests

[Jan Beulich]

On 11.09.2021 01:14, Stefano Stabellini wrote:
> On Tue, 7 Sep 2021, Jan Beulich wrote:
>> While the hypervisor hasn't been enforcing this, we would still better
>> avoid issuing requests with GFNs not aligned to the requested order.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I wonder how useful it is to include the alignment in the panic()
>> message.
> 
> Not very useful given that it is static. I don't mind either way but you
> can go ahead and remove it if you prefer (and it would make the line
> shorter.)
> 
> 
>> I further wonder how useful it is to wrap "bytes" in
>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
>> least was supposed to be, prior to "swiotlb-xen: maintain slab count
>> properly").
> 
> This one I would keep, to make sure to print out the same amount passed
> to memblock_alloc.

Oh - if I was to drop it from the printk(), I would have been meaning to
also drop it there. If it's useless, then it's useless everywhere.

Jan

Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests

[Stefano Stabellini]

On Mon, 13 Sep 2021, Jan Beulich wrote:
> On 11.09.2021 01:14, Stefano Stabellini wrote:
> > On Tue, 7 Sep 2021, Jan Beulich wrote:
> >> While the hypervisor hasn't been enforcing this, we would still better
> >> avoid issuing requests with GFNs not aligned to the requested order.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> I wonder how useful it is to include the alignment in the panic()
> >> message.
> > 
> > Not very useful given that it is static. I don't mind either way but you
> > can go ahead and remove it if you prefer (and it would make the line
> > shorter.)
> > 
> > 
> >> I further wonder how useful it is to wrap "bytes" in
> >> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
> >> least was supposed to be, prior to "swiotlb-xen: maintain slab count
> >> properly").
> > 
> > This one I would keep, to make sure to print out the same amount passed
> > to memblock_alloc.
> 
> Oh - if I was to drop it from the printk(), I would have been meaning to
> also drop it there. If it's useless, then it's useless everywhere.

That's fine too

Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests

[Stefano Stabellini]

On Mon, 13 Sep 2021, Jan Beulich wrote:
> On 11.09.2021 01:14, Stefano Stabellini wrote:
> > On Tue, 7 Sep 2021, Jan Beulich wrote:
> >> While the hypervisor hasn't been enforcing this, we would still better
> >> avoid issuing requests with GFNs not aligned to the requested order.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> I wonder how useful it is to include the alignment in the panic()
> >> message.
> > 
> > Not very useful given that it is static. I don't mind either way but you
> > can go ahead and remove it if you prefer (and it would make the line
> > shorter.)
> > 
> > 
> >> I further wonder how useful it is to wrap "bytes" in
> >> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
> >> least was supposed to be, prior to "swiotlb-xen: maintain slab count
> >> properly").
> > 
> > This one I would keep, to make sure to print out the same amount passed
> > to memblock_alloc.
> 
> Oh - if I was to drop it from the printk(), I would have been meaning to
> also drop it there. If it's useless, then it's useless everywhere.

That's fine too

Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests

[Jan Beulich]

On 13.09.2021 22:31, Stefano Stabellini wrote:
> On Mon, 13 Sep 2021, Jan Beulich wrote:
>> On 11.09.2021 01:14, Stefano Stabellini wrote:
>>> On Tue, 7 Sep 2021, Jan Beulich wrote:
>>>> While the hypervisor hasn't been enforcing this, we would still better
>>>> avoid issuing requests with GFNs not aligned to the requested order.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> I wonder how useful it is to include the alignment in the panic()
>>>> message.
>>>
>>> Not very useful given that it is static. I don't mind either way but you
>>> can go ahead and remove it if you prefer (and it would make the line
>>> shorter.)
>>>
>>>
>>>> I further wonder how useful it is to wrap "bytes" in
>>>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
>>>> least was supposed to be, prior to "swiotlb-xen: maintain slab count
>>>> properly").
>>>
>>> This one I would keep, to make sure to print out the same amount passed
>>> to memblock_alloc.
>>
>> Oh - if I was to drop it from the printk(), I would have been meaning to
>> also drop it there. If it's useless, then it's useless everywhere.
> 
> That's fine too

Thanks, I'll see about dropping that then.

Another Arm-related question has occurred to me: Do you actually
mind the higher-than-necessary alignment there? If so, a per-arch
definition of the needed alignment would need introducing. Maybe
that could default to PAGE_SIZE, allowing Arm and alike to get away
without explicitly specifying a value ...

Jan

Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests

[Stefano Stabellini]

On Tue, 14 Sep 2021, Jan Beulich wrote:
> On 13.09.2021 22:31, Stefano Stabellini wrote:
> > On Mon, 13 Sep 2021, Jan Beulich wrote:
> >> On 11.09.2021 01:14, Stefano Stabellini wrote:
> >>> On Tue, 7 Sep 2021, Jan Beulich wrote:
> >>>> While the hypervisor hasn't been enforcing this, we would still better
> >>>> avoid issuing requests with GFNs not aligned to the requested order.
> >>>>
> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>> ---
> >>>> I wonder how useful it is to include the alignment in the panic()
> >>>> message.
> >>>
> >>> Not very useful given that it is static. I don't mind either way but you
> >>> can go ahead and remove it if you prefer (and it would make the line
> >>> shorter.)
> >>>
> >>>
> >>>> I further wonder how useful it is to wrap "bytes" in
> >>>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
> >>>> least was supposed to be, prior to "swiotlb-xen: maintain slab count
> >>>> properly").
> >>>
> >>> This one I would keep, to make sure to print out the same amount passed
> >>> to memblock_alloc.
> >>
> >> Oh - if I was to drop it from the printk(), I would have been meaning to
> >> also drop it there. If it's useless, then it's useless everywhere.
> > 
> > That's fine too
> 
> Thanks, I'll see about dropping that then.
> 
> Another Arm-related question has occurred to me: Do you actually
> mind the higher-than-necessary alignment there? If so, a per-arch
> definition of the needed alignment would need introducing. Maybe
> that could default to PAGE_SIZE, allowing Arm and alike to get away
> without explicitly specifying a value ...

Certainly a patch like that could be good. Given that it is only one
allocation I was assuming that the higher-than-necessary alignment
wouldn't be a problem worth addressing (and I cannot completely rule out
that one day we might have to use XENMEM_exchange on ARM too).

Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests

[Stefano Stabellini]

On Tue, 14 Sep 2021, Jan Beulich wrote:
> On 13.09.2021 22:31, Stefano Stabellini wrote:
> > On Mon, 13 Sep 2021, Jan Beulich wrote:
> >> On 11.09.2021 01:14, Stefano Stabellini wrote:
> >>> On Tue, 7 Sep 2021, Jan Beulich wrote:
> >>>> While the hypervisor hasn't been enforcing this, we would still better
> >>>> avoid issuing requests with GFNs not aligned to the requested order.
> >>>>
> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>> ---
> >>>> I wonder how useful it is to include the alignment in the panic()
> >>>> message.
> >>>
> >>> Not very useful given that it is static. I don't mind either way but you
> >>> can go ahead and remove it if you prefer (and it would make the line
> >>> shorter.)
> >>>
> >>>
> >>>> I further wonder how useful it is to wrap "bytes" in
> >>>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
> >>>> least was supposed to be, prior to "swiotlb-xen: maintain slab count
> >>>> properly").
> >>>
> >>> This one I would keep, to make sure to print out the same amount passed
> >>> to memblock_alloc.
> >>
> >> Oh - if I was to drop it from the printk(), I would have been meaning to
> >> also drop it there. If it's useless, then it's useless everywhere.
> > 
> > That's fine too
> 
> Thanks, I'll see about dropping that then.
> 
> Another Arm-related question has occurred to me: Do you actually
> mind the higher-than-necessary alignment there? If so, a per-arch
> definition of the needed alignment would need introducing. Maybe
> that could default to PAGE_SIZE, allowing Arm and alike to get away
> without explicitly specifying a value ...

Certainly a patch like that could be good. Given that it is only one
allocation I was assuming that the higher-than-necessary alignment
wouldn't be a problem worth addressing (and I cannot completely rule out
that one day we might have to use XENMEM_exchange on ARM too).

Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests

[Stefano Stabellini]

On Tue, 14 Sep 2021, Stefano Stabellini wrote:
> On Tue, 14 Sep 2021, Jan Beulich wrote:
> > On 13.09.2021 22:31, Stefano Stabellini wrote:
> > > On Mon, 13 Sep 2021, Jan Beulich wrote:
> > >> On 11.09.2021 01:14, Stefano Stabellini wrote:
> > >>> On Tue, 7 Sep 2021, Jan Beulich wrote:
> > >>>> While the hypervisor hasn't been enforcing this, we would still better
> > >>>> avoid issuing requests with GFNs not aligned to the requested order.
> > >>>>
> > >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > >>>> ---
> > >>>> I wonder how useful it is to include the alignment in the panic()
> > >>>> message.
> > >>>
> > >>> Not very useful given that it is static. I don't mind either way but you
> > >>> can go ahead and remove it if you prefer (and it would make the line
> > >>> shorter.)
> > >>>
> > >>>
> > >>>> I further wonder how useful it is to wrap "bytes" in
> > >>>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
> > >>>> least was supposed to be, prior to "swiotlb-xen: maintain slab count
> > >>>> properly").
> > >>>
> > >>> This one I would keep, to make sure to print out the same amount passed
> > >>> to memblock_alloc.
> > >>
> > >> Oh - if I was to drop it from the printk(), I would have been meaning to
> > >> also drop it there. If it's useless, then it's useless everywhere.
> > > 
> > > That's fine too
> > 
> > Thanks, I'll see about dropping that then.
> > 
> > Another Arm-related question has occurred to me: Do you actually
> > mind the higher-than-necessary alignment there? If so, a per-arch
> > definition of the needed alignment would need introducing. Maybe
> > that could default to PAGE_SIZE, allowing Arm and alike to get away
> > without explicitly specifying a value ...
> 
> Certainly a patch like that could be good. Given that it is only one
> allocation I was assuming that the higher-than-necessary alignment
> wouldn't be a problem worth addressing (and I cannot completely rule out
> that one day we might have to use XENMEM_exchange on ARM too).

Also this code is currently #ifdef CONFIG_X86

Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests

[Stefano Stabellini]

On Tue, 14 Sep 2021, Stefano Stabellini wrote:
> On Tue, 14 Sep 2021, Jan Beulich wrote:
> > On 13.09.2021 22:31, Stefano Stabellini wrote:
> > > On Mon, 13 Sep 2021, Jan Beulich wrote:
> > >> On 11.09.2021 01:14, Stefano Stabellini wrote:
> > >>> On Tue, 7 Sep 2021, Jan Beulich wrote:
> > >>>> While the hypervisor hasn't been enforcing this, we would still better
> > >>>> avoid issuing requests with GFNs not aligned to the requested order.
> > >>>>
> > >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > >>>> ---
> > >>>> I wonder how useful it is to include the alignment in the panic()
> > >>>> message.
> > >>>
> > >>> Not very useful given that it is static. I don't mind either way but you
> > >>> can go ahead and remove it if you prefer (and it would make the line
> > >>> shorter.)
> > >>>
> > >>>
> > >>>> I further wonder how useful it is to wrap "bytes" in
> > >>>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
> > >>>> least was supposed to be, prior to "swiotlb-xen: maintain slab count
> > >>>> properly").
> > >>>
> > >>> This one I would keep, to make sure to print out the same amount passed
> > >>> to memblock_alloc.
> > >>
> > >> Oh - if I was to drop it from the printk(), I would have been meaning to
> > >> also drop it there. If it's useless, then it's useless everywhere.
> > > 
> > > That's fine too
> > 
> > Thanks, I'll see about dropping that then.
> > 
> > Another Arm-related question has occurred to me: Do you actually
> > mind the higher-than-necessary alignment there? If so, a per-arch
> > definition of the needed alignment would need introducing. Maybe
> > that could default to PAGE_SIZE, allowing Arm and alike to get away
> > without explicitly specifying a value ...
> 
> Certainly a patch like that could be good. Given that it is only one
> allocation I was assuming that the higher-than-necessary alignment
> wouldn't be a problem worth addressing (and I cannot completely rule out
> that one day we might have to use XENMEM_exchange on ARM too).

Also this code is currently #ifdef CONFIG_X86

Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests

[Jan Beulich]

On 15.09.2021 03:54, Stefano Stabellini wrote:
> On Tue, 14 Sep 2021, Stefano Stabellini wrote:
>> On Tue, 14 Sep 2021, Jan Beulich wrote:
>>> On 13.09.2021 22:31, Stefano Stabellini wrote:
>>>> On Mon, 13 Sep 2021, Jan Beulich wrote:
>>>>> On 11.09.2021 01:14, Stefano Stabellini wrote:
>>>>>> On Tue, 7 Sep 2021, Jan Beulich wrote:
>>>>>>> While the hypervisor hasn't been enforcing this, we would still better
>>>>>>> avoid issuing requests with GFNs not aligned to the requested order.
>>>>>>>
>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>> ---
>>>>>>> I wonder how useful it is to include the alignment in the panic()
>>>>>>> message.
>>>>>>
>>>>>> Not very useful given that it is static. I don't mind either way but you
>>>>>> can go ahead and remove it if you prefer (and it would make the line
>>>>>> shorter.)
>>>>>>
>>>>>>
>>>>>>> I further wonder how useful it is to wrap "bytes" in
>>>>>>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
>>>>>>> least was supposed to be, prior to "swiotlb-xen: maintain slab count
>>>>>>> properly").
>>>>>>
>>>>>> This one I would keep, to make sure to print out the same amount passed
>>>>>> to memblock_alloc.
>>>>>
>>>>> Oh - if I was to drop it from the printk(), I would have been meaning to
>>>>> also drop it there. If it's useless, then it's useless everywhere.
>>>>
>>>> That's fine too
>>>
>>> Thanks, I'll see about dropping that then.
>>>
>>> Another Arm-related question has occurred to me: Do you actually
>>> mind the higher-than-necessary alignment there? If so, a per-arch
>>> definition of the needed alignment would need introducing. Maybe
>>> that could default to PAGE_SIZE, allowing Arm and alike to get away
>>> without explicitly specifying a value ...
>>
>> Certainly a patch like that could be good. Given that it is only one
>> allocation I was assuming that the higher-than-necessary alignment
>> wouldn't be a problem worth addressing (and I cannot completely rule out
>> that one day we might have to use XENMEM_exchange on ARM too).
> 
> Also this code is currently #ifdef CONFIG_X86

Oh, good point. When writing the patch I did take this into consideration,
but I had managed to forget that aspect in the meantime. No adjustment to
this effect needed then.

Jan

[PATCH 05/12] swiotlb-xen: suppress certain init retries

[Jan Beulich]

Only on the 2nd of the paths leading to xen_swiotlb_init()'s "error"
label it is useful to retry the allocation; the first one did already
iterate through all possible order values.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I'm not convinced of the (lack of) indentation of the label, but I made
the new one matzch the existing one.

--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -184,7 +184,7 @@ retry:
 		order--;
 	}
 	if (!start)
-		goto error;
+		goto exit;
 	if (order != get_order(bytes)) {
 		pr_warn("Warning: only able to allocate %ld MB for software IO TLB\n",
 			(PAGE_SIZE << order) >> 20);
@@ -214,6 +214,7 @@ error:
 		pr_info("Lowering to %luMB\n", bytes >> 20);
 		goto retry;
 	}
+exit:
 	pr_err("%s (rc:%d)\n", xen_swiotlb_error(m_ret), rc);
 	return rc;
 }

Re: [PATCH 05/12] swiotlb-xen: suppress certain init retries

[Christoph Hellwig]

On Tue, Sep 07, 2021 at 02:05:54PM +0200, Jan Beulich wrote:
> Only on the 2nd of the paths leading to xen_swiotlb_init()'s "error"
> label it is useful to retry the allocation; the first one did already
> iterate through all possible order values.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 05/12] swiotlb-xen: suppress certain init retries

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> Only on the 2nd of the paths leading to xen_swiotlb_init()'s "error"
> label it is useful to retry the allocation; the first one did already
> iterate through all possible order values.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> I'm not convinced of the (lack of) indentation of the label, but I made
> the new one matzch the existing one.
> 
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -184,7 +184,7 @@ retry:
>  		order--;
>  	}
>  	if (!start)
> -		goto error;
> +		goto exit;
>  	if (order != get_order(bytes)) {
>  		pr_warn("Warning: only able to allocate %ld MB for software IO TLB\n",
>  			(PAGE_SIZE << order) >> 20);
> @@ -214,6 +214,7 @@ error:
>  		pr_info("Lowering to %luMB\n", bytes >> 20);
>  		goto retry;
>  	}
> +exit:
>  	pr_err("%s (rc:%d)\n", xen_swiotlb_error(m_ret), rc);
>  	return rc;
>  }
> 

Re: [PATCH 05/12] swiotlb-xen: suppress certain init retries

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> Only on the 2nd of the paths leading to xen_swiotlb_init()'s "error"
> label it is useful to retry the allocation; the first one did already
> iterate through all possible order values.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> I'm not convinced of the (lack of) indentation of the label, but I made
> the new one matzch the existing one.
> 
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -184,7 +184,7 @@ retry:
>  		order--;
>  	}
>  	if (!start)
> -		goto error;
> +		goto exit;
>  	if (order != get_order(bytes)) {
>  		pr_warn("Warning: only able to allocate %ld MB for software IO TLB\n",
>  			(PAGE_SIZE << order) >> 20);
> @@ -214,6 +214,7 @@ error:
>  		pr_info("Lowering to %luMB\n", bytes >> 20);
>  		goto retry;
>  	}
> +exit:
>  	pr_err("%s (rc:%d)\n", xen_swiotlb_error(m_ret), rc);
>  	return rc;
>  }
> 

[PATCH 06/12] swiotlb-xen: limit init retries

[Jan Beulich]

Due to the use of max(1024, ...) there's no point retrying (and issuing
bogus log messages) when the number of slabs is already no larger than
this minimum value.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -207,7 +207,7 @@ retry:
 	swiotlb_set_max_segment(PAGE_SIZE);
 	return 0;
 error:
-	if (repeat--) {
+	if (nslabs > 1024 && repeat--) {
 		/* Min is 2MB */
 		nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
 		bytes = nslabs << IO_TLB_SHIFT;
@@ -243,7 +243,7 @@ retry:
 	rc = xen_swiotlb_fixup(start, nslabs);
 	if (rc) {
 		memblock_free(__pa(start), PAGE_ALIGN(bytes));
-		if (repeat--) {
+		if (nslabs > 1024 && repeat--) {
 			/* Min is 2MB */
 			nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
 			bytes = nslabs << IO_TLB_SHIFT;

Re: [PATCH 06/12] swiotlb-xen: limit init retries

[Christoph Hellwig]

On Tue, Sep 07, 2021 at 02:06:37PM +0200, Jan Beulich wrote:
> Due to the use of max(1024, ...) there's no point retrying (and issuing
> bogus log messages) when the number of slabs is already no larger than
> this minimum value.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 06/12] swiotlb-xen: limit init retries

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> Due to the use of max(1024, ...) there's no point retrying (and issuing
> bogus log messages) when the number of slabs is already no larger than
> this minimum value.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -207,7 +207,7 @@ retry:
>  	swiotlb_set_max_segment(PAGE_SIZE);
>  	return 0;
>  error:
> -	if (repeat--) {
> +	if (nslabs > 1024 && repeat--) {
>  		/* Min is 2MB */
>  		nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
>  		bytes = nslabs << IO_TLB_SHIFT;
> @@ -243,7 +243,7 @@ retry:
>  	rc = xen_swiotlb_fixup(start, nslabs);
>  	if (rc) {
>  		memblock_free(__pa(start), PAGE_ALIGN(bytes));
> -		if (repeat--) {
> +		if (nslabs > 1024 && repeat--) {
>  			/* Min is 2MB */
>  			nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
>  			bytes = nslabs << IO_TLB_SHIFT;
> 

Re: [PATCH 06/12] swiotlb-xen: limit init retries

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> Due to the use of max(1024, ...) there's no point retrying (and issuing
> bogus log messages) when the number of slabs is already no larger than
> this minimum value.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -207,7 +207,7 @@ retry:
>  	swiotlb_set_max_segment(PAGE_SIZE);
>  	return 0;
>  error:
> -	if (repeat--) {
> +	if (nslabs > 1024 && repeat--) {
>  		/* Min is 2MB */
>  		nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
>  		bytes = nslabs << IO_TLB_SHIFT;
> @@ -243,7 +243,7 @@ retry:
>  	rc = xen_swiotlb_fixup(start, nslabs);
>  	if (rc) {
>  		memblock_free(__pa(start), PAGE_ALIGN(bytes));
> -		if (repeat--) {
> +		if (nslabs > 1024 && repeat--) {
>  			/* Min is 2MB */
>  			nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
>  			bytes = nslabs << IO_TLB_SHIFT;
> 

[PATCH 07/12] swiotlb-xen: drop leftover __ref

[Jan Beulich]

Commit a98f565462f0 ("xen-swiotlb: split xen_swiotlb_init") should not
only have added __init to the split off function, but also should have
dropped __ref from the one left.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -154,7 +154,7 @@ static const char *xen_swiotlb_error(enu
 
 #define DEFAULT_NSLABS		ALIGN(SZ_64M >> IO_TLB_SHIFT, IO_TLB_SEGSIZE)
 
-int __ref xen_swiotlb_init(void)
+int xen_swiotlb_init(void)
 {
 	enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN;
 	unsigned long bytes = swiotlb_size_or_default();

Re: [PATCH 07/12] swiotlb-xen: drop leftover __ref

[Christoph Hellwig]

On Tue, Sep 07, 2021 at 02:06:55PM +0200, Jan Beulich wrote:
> Commit a98f565462f0 ("xen-swiotlb: split xen_swiotlb_init") should not
> only have added __init to the split off function, but also should have
> dropped __ref from the one left.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 07/12] swiotlb-xen: drop leftover __ref

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> Commit a98f565462f0 ("xen-swiotlb: split xen_swiotlb_init") should not
> only have added __init to the split off function, but also should have
> dropped __ref from the one left.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -154,7 +154,7 @@ static const char *xen_swiotlb_error(enu
>  
>  #define DEFAULT_NSLABS		ALIGN(SZ_64M >> IO_TLB_SHIFT, IO_TLB_SEGSIZE)
>  
> -int __ref xen_swiotlb_init(void)
> +int xen_swiotlb_init(void)
>  {
>  	enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN;
>  	unsigned long bytes = swiotlb_size_or_default();
> 

Re: [PATCH 07/12] swiotlb-xen: drop leftover __ref

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> Commit a98f565462f0 ("xen-swiotlb: split xen_swiotlb_init") should not
> only have added __init to the split off function, but also should have
> dropped __ref from the one left.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -154,7 +154,7 @@ static const char *xen_swiotlb_error(enu
>  
>  #define DEFAULT_NSLABS		ALIGN(SZ_64M >> IO_TLB_SHIFT, IO_TLB_SEGSIZE)
>  
> -int __ref xen_swiotlb_init(void)
> +int xen_swiotlb_init(void)
>  {
>  	enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN;
>  	unsigned long bytes = swiotlb_size_or_default();
> 

[PATCH 08/12] swiotlb-xen: arrange to have buffer info logged

[Jan Beulich]

I consider it unhelpful that address and size of the buffer aren't put
in the log file; it makes diagnosing issues needlessly harder. The
majority of callers of swiotlb_init() also passes 1 for the "verbose"
parameter. 

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -253,7 +253,7 @@ retry:
 		panic("%s (rc:%d)", xen_swiotlb_error(XEN_SWIOTLB_EFIXUP), rc);
 	}
 
-	if (swiotlb_init_with_tbl(start, nslabs, false))
+	if (swiotlb_init_with_tbl(start, nslabs, true))
 		panic("Cannot allocate SWIOTLB buffer");
 	swiotlb_set_max_segment(PAGE_SIZE);
 }

Re: [PATCH 08/12] swiotlb-xen: arrange to have buffer info logged

[Christoph Hellwig]

On Tue, Sep 07, 2021 at 02:07:21PM +0200, Jan Beulich wrote:
> I consider it unhelpful that address and size of the buffer aren't put
> in the log file; it makes diagnosing issues needlessly harder. The
> majority of callers of swiotlb_init() also passes 1 for the "verbose"
> parameter. 
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 08/12] swiotlb-xen: arrange to have buffer info logged

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> I consider it unhelpful that address and size of the buffer aren't put
> in the log file; it makes diagnosing issues needlessly harder. The
> majority of callers of swiotlb_init() also passes 1 for the "verbose"
> parameter. 
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -253,7 +253,7 @@ retry:
>  		panic("%s (rc:%d)", xen_swiotlb_error(XEN_SWIOTLB_EFIXUP), rc);
>  	}
>  
> -	if (swiotlb_init_with_tbl(start, nslabs, false))
> +	if (swiotlb_init_with_tbl(start, nslabs, true))
>  		panic("Cannot allocate SWIOTLB buffer");
>  	swiotlb_set_max_segment(PAGE_SIZE);
>  }
> 

Re: [PATCH 08/12] swiotlb-xen: arrange to have buffer info logged

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> I consider it unhelpful that address and size of the buffer aren't put
> in the log file; it makes diagnosing issues needlessly harder. The
> majority of callers of swiotlb_init() also passes 1 for the "verbose"
> parameter. 
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -253,7 +253,7 @@ retry:
>  		panic("%s (rc:%d)", xen_swiotlb_error(XEN_SWIOTLB_EFIXUP), rc);
>  	}
>  
> -	if (swiotlb_init_with_tbl(start, nslabs, false))
> +	if (swiotlb_init_with_tbl(start, nslabs, true))
>  		panic("Cannot allocate SWIOTLB buffer");
>  	swiotlb_set_max_segment(PAGE_SIZE);
>  }
> 

[PATCH 09/12] swiotlb-xen: drop DEFAULT_NSLABS

[Jan Beulich]

It was introduced by 4035b43da6da ("xen-swiotlb: remove xen_set_nslabs")
and then not removed by 2d29960af0be ("swiotlb: dynamically allocate
io_tlb_default_mem").

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -152,8 +152,6 @@ static const char *xen_swiotlb_error(enu
 	return "";
 }
 
-#define DEFAULT_NSLABS		ALIGN(SZ_64M >> IO_TLB_SHIFT, IO_TLB_SEGSIZE)
-
 int xen_swiotlb_init(void)
 {
 	enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN;

Re: [PATCH 09/12] swiotlb-xen: drop DEFAULT_NSLABS

[Christoph Hellwig]

On Tue, Sep 07, 2021 at 02:07:47PM +0200, Jan Beulich wrote:
> It was introduced by 4035b43da6da ("xen-swiotlb: remove xen_set_nslabs")
> and then not removed by 2d29960af0be ("swiotlb: dynamically allocate
> io_tlb_default_mem").
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 09/12] swiotlb-xen: drop DEFAULT_NSLABS

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> It was introduced by 4035b43da6da ("xen-swiotlb: remove xen_set_nslabs")
> and then not removed by 2d29960af0be ("swiotlb: dynamically allocate
> io_tlb_default_mem").
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -152,8 +152,6 @@ static const char *xen_swiotlb_error(enu
>  	return "";
>  }
>  
> -#define DEFAULT_NSLABS		ALIGN(SZ_64M >> IO_TLB_SHIFT, IO_TLB_SEGSIZE)
> -
>  int xen_swiotlb_init(void)
>  {
>  	enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN;
> 

Re: [PATCH 09/12] swiotlb-xen: drop DEFAULT_NSLABS

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> It was introduced by 4035b43da6da ("xen-swiotlb: remove xen_set_nslabs")
> and then not removed by 2d29960af0be ("swiotlb: dynamically allocate
> io_tlb_default_mem").
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -152,8 +152,6 @@ static const char *xen_swiotlb_error(enu
>  	return "";
>  }
>  
> -#define DEFAULT_NSLABS		ALIGN(SZ_64M >> IO_TLB_SHIFT, IO_TLB_SEGSIZE)
> -
>  int xen_swiotlb_init(void)
>  {
>  	enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN;
> 

[PATCH 10/12] xen-pcifront: this module is PV-only

[Jan Beulich]

It's module init function does a xen_pv_domain() check first thing.
Hence there's no point building it in non-PV configurations.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -110,7 +110,7 @@ config PCI_PF_STUB
 
 config XEN_PCIDEV_FRONTEND
 	tristate "Xen PCI Frontend"
-	depends on X86 && XEN
+	depends on XEN_PV
 	select PCI_XEN
 	select XEN_XENBUS_FRONTEND
 	default y

Re: [PATCH 10/12] xen-pcifront: this module is PV-only

[Bjorn Helgaas]

Update subject to follow conventions (use "git log --oneline
drivers/pci/Kconfig").  Should say what this patch does.

Commit log below should also say what this patch does.  Currently it's
part of the rationale for the change, but doesn't say what the patch
does.

On Tue, Sep 07, 2021 at 02:10:41PM +0200, Jan Beulich wrote:
> It's module init function does a xen_pv_domain() check first thing.
> Hence there's no point building it in non-PV configurations.

s/It's/<name of function that calls xen_pv_domain()/   # pcifront_init()?
s/building it/building <name of module>/               # xen-pcifront.o?

I see that CONFIG_XEN_PV is only mentioned in arch/x86, so
CONFIG_XEN_PV=y cannot be set on other arches.  Is the current
"depends on X86" just a reflection of that, or is it because of some
other x86 dependency in the code?

The connection between xen_pv_domain() and CONFIG_XEN_PV is not
completely obvious.

If you only build xen-pcifront.o when CONFIG_XEN_PV=y, and
xen_pv_domain() is true if and only if CONFIG_XEN_PV=y, why bother
calling xen_pv_domain() at all?

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -110,7 +110,7 @@ config PCI_PF_STUB
>  
>  config XEN_PCIDEV_FRONTEND
>  	tristate "Xen PCI Frontend"
> -	depends on X86 && XEN
> +	depends on XEN_PV
>  	select PCI_XEN
>  	select XEN_XENBUS_FRONTEND
>  	default y
> 

Re: [PATCH 10/12] xen-pcifront: this module is PV-only

[Jan Beulich]

On 07.09.2021 17:30, Bjorn Helgaas wrote:
> Update subject to follow conventions (use "git log --oneline
> drivers/pci/Kconfig").  Should say what this patch does.

I can change that; I don't think it'll carry any different information.

> Commit log below should also say what this patch does.  Currently it's
> part of the rationale for the change, but doesn't say what the patch
> does.

"There's no point building ..." to me is as good as "Don't build ...".
But oh well, I can adjust ...

> On Tue, Sep 07, 2021 at 02:10:41PM +0200, Jan Beulich wrote:
>> It's module init function does a xen_pv_domain() check first thing.
>> Hence there's no point building it in non-PV configurations.
> 
> s/It's/<name of function that calls xen_pv_domain()/   # pcifront_init()?

I don't understand this - how is "module init function" not clear
enough?

> s/building it/building <name of module>/               # xen-pcifront.o?

The driver name is already part of the subject; I didn't think I
need to repeat that one here.

> I see that CONFIG_XEN_PV is only mentioned in arch/x86, so
> CONFIG_XEN_PV=y cannot be set on other arches.  Is the current
> "depends on X86" just a reflection of that, or is it because of some
> other x86 dependency in the code?
> 
> The connection between xen_pv_domain() and CONFIG_XEN_PV is not
> completely obvious.
> 
> If you only build xen-pcifront.o when CONFIG_XEN_PV=y, and
> xen_pv_domain() is true if and only if CONFIG_XEN_PV=y, why bother
> calling xen_pv_domain() at all?

Because XEN_PV=y only _enables_ the kernel to run in PV mode. It
may be enabled to also run in HVM and/or PVH modes. And it may
then _run_ in any of the enabled modes. IOW xen_pv_domain() will
always return false when !XEN_PV; no other implication is valid.
I don't think this basic concept needs explaining in a simple
patch like this. Instead I think the config option in question,
despite living in drivers/pci/Kconfig, should be under "XEN
HYPERVISOR INTERFACE" maintainership. I realize that's not even
expressable in ./MAINTAINERS. I wonder why the option was put
there in the first place, rather than in drivers/xen/Kconfig.

Jan

Re: [PATCH 10/12] xen-pcifront: this module is PV-only

[Bjorn Helgaas]

On Tue, Sep 07, 2021 at 06:14:16PM +0200, Jan Beulich wrote:
> On 07.09.2021 17:30, Bjorn Helgaas wrote:
> > Update subject to follow conventions (use "git log --oneline
> > drivers/pci/Kconfig").  Should say what this patch does.
> 
> I can change that; I don't think it'll carry any different information.

It might not be different information, but if you use the same
sentence structure and formatting as all the other subject lines,
they're easier to read as a group.

> > Commit log below should also say what this patch does.  Currently it's
> > part of the rationale for the change, but doesn't say what the patch
> > does.
> 
> "There's no point building ..." to me is as good as "Don't build ...".
> But oh well, I can adjust ...
> 
> > On Tue, Sep 07, 2021 at 02:10:41PM +0200, Jan Beulich wrote:
> >> It's module init function does a xen_pv_domain() check first thing.
> >> Hence there's no point building it in non-PV configurations.
> > 
> > s/It's/<name of function that calls xen_pv_domain()/   # pcifront_init()?
> 
> I don't understand this - how is "module init function" not clear
> enough?

Saying "module init function" makes the reader do extra work to
figure out what function you are referring to.  I had to look
at drivers/pci/Makefile to find the module name, then look at
drivers/pci/xen-pcifront.c to look for the init function.
Saying pcifront_init() makes it trivial to look *there*.

> > s/building it/building <name of module>/               # xen-pcifront.o?
> 
> The driver name is already part of the subject; I didn't think I
> need to repeat that one here.

Why be vague when it's so easy to be explicit and save everybody else
the effort?  Part of the disconnect here is that the subject line is
not *part* of the commit log, so the commit log should make sense even
if you can't see the subject line.  It's like an essay that should
make sense without its title.

Most of this *is* trivial, I agree.  Just minor hiccups in the process
of reading.

> > I see that CONFIG_XEN_PV is only mentioned in arch/x86, so
> > CONFIG_XEN_PV=y cannot be set on other arches.  Is the current
> > "depends on X86" just a reflection of that, or is it because of some
> > other x86 dependency in the code?
> > 
> > The connection between xen_pv_domain() and CONFIG_XEN_PV is not
> > completely obvious.
> > 
> > If you only build xen-pcifront.o when CONFIG_XEN_PV=y, and
> > xen_pv_domain() is true if and only if CONFIG_XEN_PV=y, why bother
> > calling xen_pv_domain() at all?
> 
> Because XEN_PV=y only _enables_ the kernel to run in PV mode. It
> may be enabled to also run in HVM and/or PVH modes. And it may
> then _run_ in any of the enabled modes. IOW xen_pv_domain() will
> always return false when !XEN_PV; no other implication is valid.
> I don't think this basic concept needs explaining in a simple
> patch like this. Instead I think the config option in question,
> despite living in drivers/pci/Kconfig, should be under "XEN
> HYPERVISOR INTERFACE" maintainership. I realize that's not even
> expressable in ./MAINTAINERS. I wonder why the option was put
> there in the first place, rather than in drivers/xen/Kconfig.

No doubt it's obvious to Xen experts.  Unfortunately I am not one.

Evidently there's no real dependency on the X86 arch, which makes me
wonder why the "depends on X86" was there in the first place.

Bjorn

Re: [PATCH 10/12] xen-pcifront: this module is PV-only

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> It's module init function does a xen_pv_domain() check first thing.
> Hence there's no point building it in non-PV configurations.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -110,7 +110,7 @@ config PCI_PF_STUB
>  
>  config XEN_PCIDEV_FRONTEND
>  	tristate "Xen PCI Frontend"
> -	depends on X86 && XEN
> +	depends on XEN_PV
>  	select PCI_XEN
>  	select XEN_XENBUS_FRONTEND
>  	default y
> 

Re: [PATCH 10/12] xen-pcifront: this module is PV-only

[Stefano Stabellini]

On Tue, 7 Sep 2021, Jan Beulich wrote:
> It's module init function does a xen_pv_domain() check first thing.
> Hence there's no point building it in non-PV configurations.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -110,7 +110,7 @@ config PCI_PF_STUB
>  
>  config XEN_PCIDEV_FRONTEND
>  	tristate "Xen PCI Frontend"
> -	depends on X86 && XEN
> +	depends on XEN_PV
>  	select PCI_XEN
>  	select XEN_XENBUS_FRONTEND
>  	default y
> 

[PATCH 11/12] xen/pci-swiotlb: reduce visibility of symbols

[Jan Beulich]

xen_swiotlb and pci_xen_swiotlb_init() are only used within the file
defining them, so make them static and remove the stubs. Otoh
pci_xen_swiotlb_detect() has a use (as function pointer) from the main
pci-swiotlb.c file - convert its stub to a #define to NULL.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/arch/x86/include/asm/xen/swiotlb-xen.h
+++ b/arch/x86/include/asm/xen/swiotlb-xen.h
@@ -3,14 +3,10 @@
 #define _ASM_X86_SWIOTLB_XEN_H
 
 #ifdef CONFIG_SWIOTLB_XEN
-extern int xen_swiotlb;
 extern int __init pci_xen_swiotlb_detect(void);
-extern void __init pci_xen_swiotlb_init(void);
 extern int pci_xen_swiotlb_init_late(void);
 #else
-#define xen_swiotlb (0)
-static inline int __init pci_xen_swiotlb_detect(void) { return 0; }
-static inline void __init pci_xen_swiotlb_init(void) { }
+#define pci_xen_swiotlb_detect NULL
 static inline int pci_xen_swiotlb_init_late(void) { return -ENXIO; }
 #endif
 
--- a/arch/x86/xen/pci-swiotlb-xen.c
+++ b/arch/x86/xen/pci-swiotlb-xen.c
@@ -18,7 +18,7 @@
 #endif
 #include <linux/export.h>
 
-int xen_swiotlb __read_mostly;
+static int xen_swiotlb __read_mostly;
 
 /*
  * pci_xen_swiotlb_detect - set xen_swiotlb to 1 if necessary
@@ -56,7 +56,7 @@ int __init pci_xen_swiotlb_detect(void)
 	return xen_swiotlb;
 }
 
-void __init pci_xen_swiotlb_init(void)
+static void __init pci_xen_swiotlb_init(void)
 {
 	if (xen_swiotlb) {
 		xen_swiotlb_init_early();

Re: [PATCH 11/12] xen/pci-swiotlb: reduce visibility of symbols

[Christoph Hellwig]

On Tue, Sep 07, 2021 at 02:11:14PM +0200, Jan Beulich wrote:
> xen_swiotlb and pci_xen_swiotlb_init() are only used within the file
> defining them, so make them static and remove the stubs. Otoh
> pci_xen_swiotlb_detect() has a use (as function pointer) from the main
> pci-swiotlb.c file - convert its stub to a #define to NULL.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 12/12] swiotlb-xen: this is PV-only on x86

[Jan Beulich]

The code is unreachable for HVM or PVH, and it also makes little sense
in auto-translated environments. On Arm, with
xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
time seeing what good the Xen specific variant does - the generic one
ought to be fine for all purposes there. Still Arm code explicitly
references symbols here, so the code will continue to be included there.

Instead of making PCI_XEN's "select" conditional, simply drop it -
SWIOTLB_XEN will be available unconditionally in the PV case anyway, and
is - as explained above - dead code in non-PV environments.

This in turn allows dropping the stubs for
xen_{create,destroy}_contiguous_region(), the former of which was broken
anyway - it failed to set the DMA handle output.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2605,7 +2605,6 @@ config PCI_OLPC
 config PCI_XEN
 	def_bool y
 	depends on PCI && XEN
-	select SWIOTLB_XEN
 
 config MMCONF_FAM10H
 	def_bool y
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -177,6 +177,7 @@ config XEN_GRANT_DMA_ALLOC
 
 config SWIOTLB_XEN
 	def_bool y
+	depends on XEN_PV || ARM || ARM64
 	select DMA_OPS
 	select SWIOTLB
 
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -46,19 +46,7 @@ extern unsigned long *xen_contiguous_bit
 int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
 				unsigned int address_bits,
 				dma_addr_t *dma_handle);
-
 void xen_destroy_contiguous_region(phys_addr_t pstart, unsigned int order);
-#else
-static inline int xen_create_contiguous_region(phys_addr_t pstart,
-					       unsigned int order,
-					       unsigned int address_bits,
-					       dma_addr_t *dma_handle)
-{
-	return 0;
-}
-
-static inline void xen_destroy_contiguous_region(phys_addr_t pstart,
-						 unsigned int order) { }
 #endif
 
 #if defined(CONFIG_XEN_PV)

Re: [PATCH 12/12] swiotlb-xen: this is PV-only on x86

[Christoph Hellwig]

On Tue, Sep 07, 2021 at 02:13:21PM +0200, Jan Beulich wrote:
> The code is unreachable for HVM or PVH, and it also makes little sense
> in auto-translated environments. On Arm, with
> xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
> time seeing what good the Xen specific variant does - the generic one
> ought to be fine for all purposes there. Still Arm code explicitly
> references symbols here, so the code will continue to be included there.

Can the Xen/arm folks look into that?  Getting ARM out of using
swiotlb-xen would be a huge step forward cleaning up some DMA APIs.

> 
> Instead of making PCI_XEN's "select" conditional, simply drop it -
> SWIOTLB_XEN will be available unconditionally in the PV case anyway, and
> is - as explained above - dead code in non-PV environments.
> 
> This in turn allows dropping the stubs for
> xen_{create,destroy}_contiguous_region(), the former of which was broken
> anyway - it failed to set the DMA handle output.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 12/12] swiotlb-xen: this is PV-only on x86

[Stefano Stabellini]

On Wed, 8 Sep 2021, Christoph Hellwig wrote:
> On Tue, Sep 07, 2021 at 02:13:21PM +0200, Jan Beulich wrote:
> > The code is unreachable for HVM or PVH, and it also makes little sense
> > in auto-translated environments. On Arm, with
> > xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
> > time seeing what good the Xen specific variant does - the generic one
> > ought to be fine for all purposes there. Still Arm code explicitly
> > references symbols here, so the code will continue to be included there.
> 
> Can the Xen/arm folks look into that?  Getting ARM out of using
> swiotlb-xen would be a huge step forward cleaning up some DMA APIs.

On ARM swiotlb-xen is used for a different purpose compared to x86.

Many ARM SoCs still don't have an IOMMU covering all DMA-mastering
devices (e.g. Raspberry Pi 4). As a consequence we map Dom0 1:1 (guest
physical == physical address).

Now if it was just for Dom0, thanks to the 1:1 mapping, we wouldn't need
swiotlb-xen. But when we start using PV drivers to share the network or
disk between Dom0 and DomU we are going to get DomU pages mapped in
Dom0, we call them "foreign pages".  They are not mapped 1:1. It can
happen that one of these foreign pages are used for DMA operations
(e.g. related to the NIC). swiotlb-xen is used to detect these
situations and translate the guest physical address to physical address
of foreign pages appropriately.

If an IOMMU is available and the DMA-mastering device is behind it, then
swiotlb-xen is not necessary. FYI there is community interest in
selectively disabling swiotlb-xen for devices that are behind an IOMMU.


> > Instead of making PCI_XEN's "select" conditional, simply drop it -
> > SWIOTLB_XEN will be available unconditionally in the PV case anyway, and
> > is - as explained above - dead code in non-PV environments.
> > 
> > This in turn allows dropping the stubs for
> > xen_{create,destroy}_contiguous_region(), the former of which was broken
> > anyway - it failed to set the DMA handle output.
> 
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [PATCH 12/12] swiotlb-xen: this is PV-only on x86

[Stefano Stabellini]

On Wed, 8 Sep 2021, Christoph Hellwig wrote:
> On Tue, Sep 07, 2021 at 02:13:21PM +0200, Jan Beulich wrote:
> > The code is unreachable for HVM or PVH, and it also makes little sense
> > in auto-translated environments. On Arm, with
> > xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
> > time seeing what good the Xen specific variant does - the generic one
> > ought to be fine for all purposes there. Still Arm code explicitly
> > references symbols here, so the code will continue to be included there.
> 
> Can the Xen/arm folks look into that?  Getting ARM out of using
> swiotlb-xen would be a huge step forward cleaning up some DMA APIs.

On ARM swiotlb-xen is used for a different purpose compared to x86.

Many ARM SoCs still don't have an IOMMU covering all DMA-mastering
devices (e.g. Raspberry Pi 4). As a consequence we map Dom0 1:1 (guest
physical == physical address).

Now if it was just for Dom0, thanks to the 1:1 mapping, we wouldn't need
swiotlb-xen. But when we start using PV drivers to share the network or
disk between Dom0 and DomU we are going to get DomU pages mapped in
Dom0, we call them "foreign pages".  They are not mapped 1:1. It can
happen that one of these foreign pages are used for DMA operations
(e.g. related to the NIC). swiotlb-xen is used to detect these
situations and translate the guest physical address to physical address
of foreign pages appropriately.

If an IOMMU is available and the DMA-mastering device is behind it, then
swiotlb-xen is not necessary. FYI there is community interest in
selectively disabling swiotlb-xen for devices that are behind an IOMMU.


> > Instead of making PCI_XEN's "select" conditional, simply drop it -
> > SWIOTLB_XEN will be available unconditionally in the PV case anyway, and
> > is - as explained above - dead code in non-PV environments.
> > 
> > This in turn allows dropping the stubs for
> > xen_{create,destroy}_contiguous_region(), the former of which was broken
> > anyway - it failed to set the DMA handle output.
> 
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [PATCH 12/12] swiotlb-xen: this is PV-only on x86

[Jan Beulich]

On 11.09.2021 01:48, Stefano Stabellini wrote:
> On Wed, 8 Sep 2021, Christoph Hellwig wrote:
>> On Tue, Sep 07, 2021 at 02:13:21PM +0200, Jan Beulich wrote:
>>> The code is unreachable for HVM or PVH, and it also makes little sense
>>> in auto-translated environments. On Arm, with
>>> xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
>>> time seeing what good the Xen specific variant does - the generic one
>>> ought to be fine for all purposes there. Still Arm code explicitly
>>> references symbols here, so the code will continue to be included there.
>>
>> Can the Xen/arm folks look into that?  Getting ARM out of using
>> swiotlb-xen would be a huge step forward cleaning up some DMA APIs.
> 
> On ARM swiotlb-xen is used for a different purpose compared to x86.
> 
> Many ARM SoCs still don't have an IOMMU covering all DMA-mastering
> devices (e.g. Raspberry Pi 4). As a consequence we map Dom0 1:1 (guest
> physical == physical address).
> 
> Now if it was just for Dom0, thanks to the 1:1 mapping, we wouldn't need
> swiotlb-xen. But when we start using PV drivers to share the network or
> disk between Dom0 and DomU we are going to get DomU pages mapped in
> Dom0, we call them "foreign pages".  They are not mapped 1:1. It can
> happen that one of these foreign pages are used for DMA operations
> (e.g. related to the NIC). swiotlb-xen is used to detect these
> situations and translate the guest physical address to physical address
> of foreign pages appropriately.

Hmm, you say "translate", which isn't my understanding of swiotlb's
purpose. As per my understanding swiotlb instead double buffers data
such that is becomes accessible, or suitably arranges underlying
machine addresses. The latter part is clearly a PV-only thing, unused
by Arm as can be seen by there not being any use of XENMEM_exchange.
So it must be the former part that you're talking about, but that's
also the purpose of the non-Xen swiotlb code. If only for my own
education and understanding, could you point me at the difference
between swiotlb-xen and generic swiotlb which addresses this specific
aspect of Arm behavior?

Jan

Re: [PATCH 12/12] swiotlb-xen: this is PV-only on x86

[Stefano Stabellini]

On Mon, 13 Sep 2021, Jan Beulich wrote:
> On 11.09.2021 01:48, Stefano Stabellini wrote:
> > On Wed, 8 Sep 2021, Christoph Hellwig wrote:
> >> On Tue, Sep 07, 2021 at 02:13:21PM +0200, Jan Beulich wrote:
> >>> The code is unreachable for HVM or PVH, and it also makes little sense
> >>> in auto-translated environments. On Arm, with
> >>> xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
> >>> time seeing what good the Xen specific variant does - the generic one
> >>> ought to be fine for all purposes there. Still Arm code explicitly
> >>> references symbols here, so the code will continue to be included there.
> >>
> >> Can the Xen/arm folks look into that?  Getting ARM out of using
> >> swiotlb-xen would be a huge step forward cleaning up some DMA APIs.
> > 
> > On ARM swiotlb-xen is used for a different purpose compared to x86.
> > 
> > Many ARM SoCs still don't have an IOMMU covering all DMA-mastering
> > devices (e.g. Raspberry Pi 4). As a consequence we map Dom0 1:1 (guest
> > physical == physical address).
> > 
> > Now if it was just for Dom0, thanks to the 1:1 mapping, we wouldn't need
> > swiotlb-xen. But when we start using PV drivers to share the network or
> > disk between Dom0 and DomU we are going to get DomU pages mapped in
> > Dom0, we call them "foreign pages".  They are not mapped 1:1. It can
> > happen that one of these foreign pages are used for DMA operations
> > (e.g. related to the NIC). swiotlb-xen is used to detect these
> > situations and translate the guest physical address to physical address
> > of foreign pages appropriately.
> 
> Hmm, you say "translate", which isn't my understanding of swiotlb's
> purpose. As per my understanding swiotlb instead double buffers data
> such that is becomes accessible, or suitably arranges underlying
> machine addresses. The latter part is clearly a PV-only thing, unused
> by Arm as can be seen by there not being any use of XENMEM_exchange.
> So it must be the former part that you're talking about, but that's
> also the purpose of the non-Xen swiotlb code. If only for my own
> education and understanding, could you point me at the difference
> between swiotlb-xen and generic swiotlb which addresses this specific
> aspect of Arm behavior?

If you look at xen_swiotlb_map_page, you'll see the call to
xen_phys_to_dma which eventually calls arch/arm/xen/p2m.c:__pfn_to_mfn.
If everything goes well and we only need to do translation we'll "goto
done". Otherwise, we'll fall back on a swiotlb buffer with
swiotlb_tbl_map_single, the result of which also needs to be translated,
see the second call to xen_phys_to_dma.

Re: [PATCH 12/12] swiotlb-xen: this is PV-only on x86

[Stefano Stabellini]

On Mon, 13 Sep 2021, Jan Beulich wrote:
> On 11.09.2021 01:48, Stefano Stabellini wrote:
> > On Wed, 8 Sep 2021, Christoph Hellwig wrote:
> >> On Tue, Sep 07, 2021 at 02:13:21PM +0200, Jan Beulich wrote:
> >>> The code is unreachable for HVM or PVH, and it also makes little sense
> >>> in auto-translated environments. On Arm, with
> >>> xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
> >>> time seeing what good the Xen specific variant does - the generic one
> >>> ought to be fine for all purposes there. Still Arm code explicitly
> >>> references symbols here, so the code will continue to be included there.
> >>
> >> Can the Xen/arm folks look into that?  Getting ARM out of using
> >> swiotlb-xen would be a huge step forward cleaning up some DMA APIs.
> > 
> > On ARM swiotlb-xen is used for a different purpose compared to x86.
> > 
> > Many ARM SoCs still don't have an IOMMU covering all DMA-mastering
> > devices (e.g. Raspberry Pi 4). As a consequence we map Dom0 1:1 (guest
> > physical == physical address).
> > 
> > Now if it was just for Dom0, thanks to the 1:1 mapping, we wouldn't need
> > swiotlb-xen. But when we start using PV drivers to share the network or
> > disk between Dom0 and DomU we are going to get DomU pages mapped in
> > Dom0, we call them "foreign pages".  They are not mapped 1:1. It can
> > happen that one of these foreign pages are used for DMA operations
> > (e.g. related to the NIC). swiotlb-xen is used to detect these
> > situations and translate the guest physical address to physical address
> > of foreign pages appropriately.
> 
> Hmm, you say "translate", which isn't my understanding of swiotlb's
> purpose. As per my understanding swiotlb instead double buffers data
> such that is becomes accessible, or suitably arranges underlying
> machine addresses. The latter part is clearly a PV-only thing, unused
> by Arm as can be seen by there not being any use of XENMEM_exchange.
> So it must be the former part that you're talking about, but that's
> also the purpose of the non-Xen swiotlb code. If only for my own
> education and understanding, could you point me at the difference
> between swiotlb-xen and generic swiotlb which addresses this specific
> aspect of Arm behavior?

If you look at xen_swiotlb_map_page, you'll see the call to
xen_phys_to_dma which eventually calls arch/arm/xen/p2m.c:__pfn_to_mfn.
If everything goes well and we only need to do translation we'll "goto
done". Otherwise, we'll fall back on a swiotlb buffer with
swiotlb_tbl_map_single, the result of which also needs to be translated,
see the second call to xen_phys_to_dma.

Re: [PATCH 12/12] swiotlb-xen: this is PV-only on x86

[Jan Beulich]

On 11.09.2021 01:48, Stefano Stabellini wrote:
> On Wed, 8 Sep 2021, Christoph Hellwig wrote:
>> On Tue, Sep 07, 2021 at 02:13:21PM +0200, Jan Beulich wrote:
>>> The code is unreachable for HVM or PVH, and it also makes little sense
>>> in auto-translated environments. On Arm, with
>>> xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
>>> time seeing what good the Xen specific variant does - the generic one
>>> ought to be fine for all purposes there. Still Arm code explicitly
>>> references symbols here, so the code will continue to be included there.
>>
>> Can the Xen/arm folks look into that?  Getting ARM out of using
>> swiotlb-xen would be a huge step forward cleaning up some DMA APIs.
> 
> On ARM swiotlb-xen is used for a different purpose compared to x86.
> 
> Many ARM SoCs still don't have an IOMMU covering all DMA-mastering
> devices (e.g. Raspberry Pi 4). As a consequence we map Dom0 1:1 (guest
> physical == physical address).
> 
> Now if it was just for Dom0, thanks to the 1:1 mapping, we wouldn't need
> swiotlb-xen. But when we start using PV drivers to share the network or
> disk between Dom0 and DomU we are going to get DomU pages mapped in
> Dom0, we call them "foreign pages".  They are not mapped 1:1. It can
> happen that one of these foreign pages are used for DMA operations
> (e.g. related to the NIC). swiotlb-xen is used to detect these
> situations and translate the guest physical address to physical address
> of foreign pages appropriately.

Thinking about this some more - if Dom0 is 1:1 mapped, why don't you
map foreign pages 1:1 as well then?

>>> Instead of making PCI_XEN's "select" conditional, simply drop it -
>>> SWIOTLB_XEN will be available unconditionally in the PV case anyway, and
>>> is - as explained above - dead code in non-PV environments.
>>>
>>> This in turn allows dropping the stubs for
>>> xen_{create,destroy}_contiguous_region(), the former of which was broken
>>> anyway - it failed to set the DMA handle output.
>>
>> Looks good:
>>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Thanks for this and the other reviews.

Jan

Re: [PATCH 12/12] swiotlb-xen: this is PV-only on x86

[Stefano Stabellini]

On Mon, 13 Sep 2021, Jan Beulich wrote:
> On 11.09.2021 01:48, Stefano Stabellini wrote:
> > On Wed, 8 Sep 2021, Christoph Hellwig wrote:
> >> On Tue, Sep 07, 2021 at 02:13:21PM +0200, Jan Beulich wrote:
> >>> The code is unreachable for HVM or PVH, and it also makes little sense
> >>> in auto-translated environments. On Arm, with
> >>> xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
> >>> time seeing what good the Xen specific variant does - the generic one
> >>> ought to be fine for all purposes there. Still Arm code explicitly
> >>> references symbols here, so the code will continue to be included there.
> >>
> >> Can the Xen/arm folks look into that?  Getting ARM out of using
> >> swiotlb-xen would be a huge step forward cleaning up some DMA APIs.
> > 
> > On ARM swiotlb-xen is used for a different purpose compared to x86.
> > 
> > Many ARM SoCs still don't have an IOMMU covering all DMA-mastering
> > devices (e.g. Raspberry Pi 4). As a consequence we map Dom0 1:1 (guest
> > physical == physical address).
> > 
> > Now if it was just for Dom0, thanks to the 1:1 mapping, we wouldn't need
> > swiotlb-xen. But when we start using PV drivers to share the network or
> > disk between Dom0 and DomU we are going to get DomU pages mapped in
> > Dom0, we call them "foreign pages".  They are not mapped 1:1. It can
> > happen that one of these foreign pages are used for DMA operations
> > (e.g. related to the NIC). swiotlb-xen is used to detect these
> > situations and translate the guest physical address to physical address
> > of foreign pages appropriately.
> 
> Thinking about this some more - if Dom0 is 1:1 mapped, why don't you
> map foreign pages 1:1 as well then?

That's because the foreign page, from Linux POV, would appear out of
thin air. It would just show up in a region not considered memory just
few moments before, so there would be no memblock, no struct page,
nothing. At least in the past that caused serious issues to the kernel.

This is the reason why the kernel is using ballooned-out pages to map
foreign pages even on x86:

drivers/block/xen-blkback/blkback.c:xen_blkbk_map -> gnttab_page_cache_get
drivers/xen/grant-table.c:gnttab_page_cache_get
drivers/xen/grant-table.c:gnttab_alloc_pages
drivers/xen/unpopulated-alloc.c:xen_alloc_unpopulated_pages
drivers/xen/balloon.c:alloc_xenballooned_pages

Re: [PATCH 12/12] swiotlb-xen: this is PV-only on x86

[Stefano Stabellini]

On Mon, 13 Sep 2021, Jan Beulich wrote:
> On 11.09.2021 01:48, Stefano Stabellini wrote:
> > On Wed, 8 Sep 2021, Christoph Hellwig wrote:
> >> On Tue, Sep 07, 2021 at 02:13:21PM +0200, Jan Beulich wrote:
> >>> The code is unreachable for HVM or PVH, and it also makes little sense
> >>> in auto-translated environments. On Arm, with
> >>> xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
> >>> time seeing what good the Xen specific variant does - the generic one
> >>> ought to be fine for all purposes there. Still Arm code explicitly
> >>> references symbols here, so the code will continue to be included there.
> >>
> >> Can the Xen/arm folks look into that?  Getting ARM out of using
> >> swiotlb-xen would be a huge step forward cleaning up some DMA APIs.
> > 
> > On ARM swiotlb-xen is used for a different purpose compared to x86.
> > 
> > Many ARM SoCs still don't have an IOMMU covering all DMA-mastering
> > devices (e.g. Raspberry Pi 4). As a consequence we map Dom0 1:1 (guest
> > physical == physical address).
> > 
> > Now if it was just for Dom0, thanks to the 1:1 mapping, we wouldn't need
> > swiotlb-xen. But when we start using PV drivers to share the network or
> > disk between Dom0 and DomU we are going to get DomU pages mapped in
> > Dom0, we call them "foreign pages".  They are not mapped 1:1. It can
> > happen that one of these foreign pages are used for DMA operations
> > (e.g. related to the NIC). swiotlb-xen is used to detect these
> > situations and translate the guest physical address to physical address
> > of foreign pages appropriately.
> 
> Thinking about this some more - if Dom0 is 1:1 mapped, why don't you
> map foreign pages 1:1 as well then?

That's because the foreign page, from Linux POV, would appear out of
thin air. It would just show up in a region not considered memory just
few moments before, so there would be no memblock, no struct page,
nothing. At least in the past that caused serious issues to the kernel.

This is the reason why the kernel is using ballooned-out pages to map
foreign pages even on x86:

drivers/block/xen-blkback/blkback.c:xen_blkbk_map -> gnttab_page_cache_get
drivers/xen/grant-table.c:gnttab_page_cache_get
drivers/xen/grant-table.c:gnttab_alloc_pages
drivers/xen/unpopulated-alloc.c:xen_alloc_unpopulated_pages
drivers/xen/balloon.c:alloc_xenballooned_pages

Re: [PATCH 00/12] swiotlb-xen: fixes and adjustments

[Juergen Gross]

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

On 07.09.21 14:03, Jan Beulich wrote:
> The primary intention really was the last patch, there you go ...
> 
> 01: swiotlb-xen: avoid double free
> 02: swiotlb-xen: fix late init retry
> 03: swiotlb-xen: maintain slab count properly
> 04: swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
> 05: swiotlb-xen: suppress certain init retries
> 06: swiotlb-xen: limit init retries
> 07: swiotlb-xen: drop leftover __ref
> 08: swiotlb-xen: arrange to have buffer info logged
> 09: swiotlb-xen: drop DEFAULT_NSLABS
> 10: xen-pcifront: this module is PV-only
> 11: xen/pci-swiotlb: reduce visibility of symbols
> 12: swiotlb-xen: this is PV-only on x86
> 
> Jan
> 

You should have Cc-ed Konrad being the maintainer and 
iommu@lists.linux-foundation.org. Doing so now.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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

Re: [PATCH 00/12] swiotlb-xen: fixes and adjustments

[Juergen Gross via iommu]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 811 bytes --]

On 07.09.21 14:03, Jan Beulich wrote:
> The primary intention really was the last patch, there you go ...
> 
> 01: swiotlb-xen: avoid double free
> 02: swiotlb-xen: fix late init retry
> 03: swiotlb-xen: maintain slab count properly
> 04: swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
> 05: swiotlb-xen: suppress certain init retries
> 06: swiotlb-xen: limit init retries
> 07: swiotlb-xen: drop leftover __ref
> 08: swiotlb-xen: arrange to have buffer info logged
> 09: swiotlb-xen: drop DEFAULT_NSLABS
> 10: xen-pcifront: this module is PV-only
> 11: xen/pci-swiotlb: reduce visibility of symbols
> 12: swiotlb-xen: this is PV-only on x86
> 
> Jan
> 

You should have Cc-ed Konrad being the maintainer and 
iommu@lists.linux-foundation.org. Doing so now.


Juergen

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 00/12] swiotlb-xen: fixes and adjustments

[Juergen Gross]

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

On 07.09.21 14:03, Jan Beulich wrote:
> The primary intention really was the last patch, there you go ...
> 
> 01: swiotlb-xen: avoid double free
> 02: swiotlb-xen: fix late init retry
> 03: swiotlb-xen: maintain slab count properly
> 04: swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
> 05: swiotlb-xen: suppress certain init retries
> 06: swiotlb-xen: limit init retries
> 07: swiotlb-xen: drop leftover __ref
> 08: swiotlb-xen: arrange to have buffer info logged
> 09: swiotlb-xen: drop DEFAULT_NSLABS
> 10: xen-pcifront: this module is PV-only
> 11: xen/pci-swiotlb: reduce visibility of symbols
> 12: swiotlb-xen: this is PV-only on x86

Pushed patches 1-3 and 5-9 to xen/tip.git for-linus-5.15


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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