Re: [RFC PATCH 2/2] apple: dart: use driver specific instance of LMB

[Janne Grunau <j@jannau.net>]

Hej,

On Tue, Oct 29, 2024 at 12:48:46PM +0530, Sughosh Ganu wrote:
> The LMB module is typically used for managing the platform's RAM
> memory. RAM memory gets added to the module's memory map, and
> subsequently allocated through various LMB API's.
> 
> The IOMMU driver for the apple platforms also uses the LMB API's for
> allocating device virtual addresses(VA). These addresses are different
> from the RAM addresses, and cannot be added to the RAM memory map. Add
> a separate instance of LMB memory map for the device VA's, which gets
> managed by the IOMMU driver. Use lmb_push() and lmb_pop() functions to
> set-up the relevant lmb instance.

thanks, this fixes the initial brokenness when setting up dma mappings
but I still see SError due to translation fault. I don't have time right
now to look into it so it could be unrelated to the iommu.

> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  drivers/iommu/apple_dart.c | 67 +++++++++++++++++++++++++++++++++++++-
>  include/lmb.h              |  2 ++
>  lib/lmb.c                  |  1 -
>  3 files changed, 68 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/apple_dart.c b/drivers/iommu/apple_dart.c
> index 611ac7cd6de..55f60287b0e 100644
> --- a/drivers/iommu/apple_dart.c
> +++ b/drivers/iommu/apple_dart.c
> @@ -3,6 +3,7 @@
>   * Copyright (C) 2021 Mark Kettenis <kettenis@openbsd.org>
>   */
>  
> +#include <alist.h>
>  #include <cpu_func.h>
>  #include <dm.h>
>  #include <iommu.h>
> @@ -70,6 +71,8 @@
>  
>  struct apple_dart_priv {
>  	void *base;
> +	struct lmb apple_dart_lmb;
> +	struct lmb ram_lmb;
>  	u64 *l1, *l2;
>  	int bypass, shift;
>  
> @@ -87,6 +90,56 @@ struct apple_dart_priv {
>  	void (*flush_tlb)(struct apple_dart_priv *priv);
>  };
>  
> +static int apple_dart_lmb_init(struct apple_dart_priv *priv)
> +{
> +	bool ret;
> +	struct lmb *almb = &priv->apple_dart_lmb;
> +
> +	ret = alist_init(&almb->free_mem, sizeof(struct lmb_region),
> +			 (uint)LMB_ALIST_INITIAL_SIZE);
> +	if (!ret) {
> +		log_debug("Unable to initialise the list for LMB free memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = alist_init(&almb->used_mem, sizeof(struct lmb_region),
> +			 (uint)LMB_ALIST_INITIAL_SIZE);
> +	if (!ret) {
> +		log_debug("Unable to initialise the list for LMB used memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static void apple_dart_lmb_uninit(struct apple_dart_priv *priv)
> +{
> +	struct lmb *almb = &priv->apple_dart_lmb;
> +
> +	alist_uninit(&almb->free_mem);
> +	alist_uninit(&almb->used_mem);
> +}
> +
> +static void apple_dart_lmb_setup(struct apple_dart_priv *priv)
> +{
> +	struct lmb *almb = &priv->apple_dart_lmb;
> +	struct lmb *rlmb = &priv->ram_lmb;
> +
> +	lmb_push(rlmb);
> +
> +	lmb_pop(almb);

This doesn't look look like a good solution to me. We're conflating two
different concepts into the global lmb. This looks error prone to me. I
was looking into adding iomb_* entry points into the lmb points which
take a pointer.

Janne