[v1] xen/swiotlb: don't initialize swiotlb twice on arm64

[Xen-devel] [PATCH] xen/swiotlb: don't initialize swiotlb twice on arm64

Posted by Stefano Stabellini 4 years, 11 months ago

From: Stefano Stabellini <stefanos@xilinx.com>

On arm64 swiotlb is already initialized by mem_init. We don't want to
initialize it twice, the memory is already allocated. Detect this
condition in swiotlb-xen and skip the second initialization.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>

---

There are other issues which I found recently affecting the swiotlb on
arm64 -- I'll send the other patches separately.

diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 877baf2..8fcda2bf4 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -206,6 +206,7 @@ int __ref xen_swiotlb_init(int verbose, bool early)
 	int rc = -ENOMEM;
 	enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN;
 	unsigned int repeat = 3;
+	bool pre_initialized = false;
 
 	xen_io_tlb_nslabs = swiotlb_nr_tbl();
 retry:
@@ -214,7 +215,10 @@ int __ref xen_swiotlb_init(int verbose, bool early)
 	/*
 	 * Get IO TLB memory from any location.
 	 */
-	if (early) {
+	if (io_tlb_start != 0) {
+		xen_io_tlb_start = phys_to_virt(io_tlb_start);
+		pre_initialized = true;
+	} else if (early) {
 		xen_io_tlb_start = memblock_alloc(PAGE_ALIGN(bytes),
 						  PAGE_SIZE);
 		if (!xen_io_tlb_start)
@@ -264,7 +268,7 @@ int __ref xen_swiotlb_init(int verbose, bool early)
 			 verbose))
 			panic("Cannot allocate SWIOTLB buffer");
 		rc = 0;
-	} else
+	} else if (!pre_initialized)
 		rc = swiotlb_late_init_with_tbl(xen_io_tlb_start, xen_io_tlb_nslabs);
 
 	if (!rc)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/swiotlb: don't initialize swiotlb twice on arm64

Posted by Julien Grall 4 years, 11 months ago

Hi,

On 23/05/2019 00:26, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefanos@xilinx.com>
> 
> On arm64 swiotlb is already initialized by mem_init. We don't want to

Arm64 will not always initialize the swiotlb. It will only be done if the user 
force it or there are memory above the DMA limit.

> initialize it twice, the memory is already allocated. Detect this
> condition in swiotlb-xen and skip the second initialization.

I understand that the memory allocated by swiotlb will be replaced with freeing 
memory. So you at least have a memory leak.

However, the logic to allocate the memory is quite different. For instance, 
AFAICT, swiotlb will allocate low pages while xen swiotlb will alloc any pages.

So I think your commit message should contain a bit more details on the 
implication. I vaguely remember that on Xilinx on needed to use low memory as 
much as possible. Is this patch actually trying to fix that?

> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> 
> ---
> 
> There are other issues which I found recently affecting the swiotlb on
> arm64 -- I'll send the other patches separately.
> 
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 877baf2..8fcda2bf4 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -206,6 +206,7 @@ int __ref xen_swiotlb_init(int verbose, bool early)
>   	int rc = -ENOMEM;
>   	enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN;
>   	unsigned int repeat = 3;
> +	bool pre_initialized = false;
>   
>   	xen_io_tlb_nslabs = swiotlb_nr_tbl();
>   retry:
> @@ -214,7 +215,10 @@ int __ref xen_swiotlb_init(int verbose, bool early)
>   	/*
>   	 * Get IO TLB memory from any location.
>   	 */
> -	if (early) {
> +	if (io_tlb_start != 0) {

Rather than adding an extra if in a already difficult code to read. Can we move 
the allocation in a separate function and only call it if necessary?

> +		xen_io_tlb_start = phys_to_virt(io_tlb_start);
> +		pre_initialized = true;
> +	} else if (early) {
>   		xen_io_tlb_start = memblock_alloc(PAGE_ALIGN(bytes),
>   						  PAGE_SIZE);
>   		if (!xen_io_tlb_start)
> @@ -264,7 +268,7 @@ int __ref xen_swiotlb_init(int verbose, bool early)
>   			 verbose))
>   			panic("Cannot allocate SWIOTLB buffer");
>   		rc = 0;
> -	} else
> +	} else if (!pre_initialized)
>   		rc = swiotlb_late_init_with_tbl(xen_io_tlb_start, xen_io_tlb_nslabs);
>   
>   	if (!rc)
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/swiotlb: don't initialize swiotlb twice on arm64

Posted by Stefano Stabellini 4 years, 11 months ago

On Thu, 23 May 2019, Julien Grall wrote:
> On 23/05/2019 00:26, Stefano Stabellini wrote:
> > From: Stefano Stabellini <stefanos@xilinx.com>
> > 
> > On arm64 swiotlb is already initialized by mem_init. We don't want to
> 
> Arm64 will not always initialize the swiotlb. It will only be done if the user
> force it or there are memory above the DMA limit.
> 
> > initialize it twice, the memory is already allocated. Detect this
> > condition in swiotlb-xen and skip the second initialization.
> 
> I understand that the memory allocated by swiotlb will be replaced with
> freeing memory. So you at least have a memory leak.
> 
> However, the logic to allocate the memory is quite different. For instance,
> AFAICT, swiotlb will allocate low pages while xen swiotlb will alloc any
> pages.

That's right.


> So I think your commit message should contain a bit more details on the
> implication. I vaguely remember that on Xilinx on needed to use low memory as
> much as possible. Is this patch actually trying to fix that?

Yes, as a side-effect. Aside from the fruitless endeavor of allocating
memory twice, we also end up trading good low-memory pages for
high-memory pages. So, a side effect of this patch is that low-memory
pages become available via swiotlb-xen.

 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > 
> > ---
> > 
> > There are other issues which I found recently affecting the swiotlb on
> > arm64 -- I'll send the other patches separately.
> > 
> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index 877baf2..8fcda2bf4 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -206,6 +206,7 @@ int __ref xen_swiotlb_init(int verbose, bool early)
> >   	int rc = -ENOMEM;
> >   	enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN;
> >   	unsigned int repeat = 3;
> > +	bool pre_initialized = false;
> >     	xen_io_tlb_nslabs = swiotlb_nr_tbl();
> >   retry:
> > @@ -214,7 +215,10 @@ int __ref xen_swiotlb_init(int verbose, bool early)
> >   	/*
> >   	 * Get IO TLB memory from any location.
> >   	 */
> > -	if (early) {
> > +	if (io_tlb_start != 0) {
> 
> Rather than adding an extra if in a already difficult code to read. Can we
> move the allocation in a separate function and only call it if necessary?

Maybe I have a better idea. If io_tlb_start != 0, we could skip
everything else in this function and basically just return.


> > +		xen_io_tlb_start = phys_to_virt(io_tlb_start);
> > +		pre_initialized = true;
> > +	} else if (early) {
> >   		xen_io_tlb_start = memblock_alloc(PAGE_ALIGN(bytes),
> >   						  PAGE_SIZE);
> >   		if (!xen_io_tlb_start)
> > @@ -264,7 +268,7 @@ int __ref xen_swiotlb_init(int verbose, bool early)
> >   			 verbose))
> >   			panic("Cannot allocate SWIOTLB buffer");
> >   		rc = 0;
> > -	} else
> > +	} else if (!pre_initialized)
> >   		rc = swiotlb_late_init_with_tbl(xen_io_tlb_start,
> > xen_io_tlb_nslabs);
> >     	if (!rc)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel