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

[Julien Grall <julien.grall@arm.com>]

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