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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 23/05/2019 00:26, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefanos@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
>
> ---
>
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel