Re: [PATCH v4 05/27] lmb: make LMB memory map persistent and global

[Janne Grunau <janne@jannau.net>]

On Mon, Aug 26, 2024 at 05:29:18PM +0530, Sughosh Ganu wrote:
> The current LMB API's for allocating and reserving memory use a
> per-caller based memory view. Memory allocated by a caller can then be
> overwritten by another caller. Make these allocations and reservations
> persistent using the alloced list data structure.
> 
> Two alloced lists are declared -- one for the available(free) memory,
> and one for the used memory. Once full, the list can then be extended
> at runtime.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> [sjg: Optimise the logic to add a region in lmb_add_region_flags()]
> [sjg: Use a stack to store pointer of lmb struct when running lmb tests]
> ---
> Changes since V3:
> * Fix checkpatch warnings of spaces between function name and
>   open parantheses.
> * s/uint64_t/u64 as suggested by checkpatch.
> * Remove unneccessary parantheses in lmb.c as suggested by checkpatch.
> * Fix alignment in test/cmd/bdinfo.c as suggested by checkpatch.

[...]
 
>  drivers/iommu/apple_dart.c           |   8 +-

[...]

> diff --git a/drivers/iommu/apple_dart.c b/drivers/iommu/apple_dart.c
> index 9327dea1e3..611ac7cd6d 100644
> --- a/drivers/iommu/apple_dart.c
> +++ b/drivers/iommu/apple_dart.c
> @@ -70,7 +70,6 @@
>  
>  struct apple_dart_priv {
>  	void *base;
> -	struct lmb lmb;
>  	u64 *l1, *l2;
>  	int bypass, shift;
>  
> @@ -124,7 +123,7 @@ static dma_addr_t apple_dart_map(struct udevice *dev, void *addr, size_t size)
>  	off = (phys_addr_t)addr - paddr;
>  	psize = ALIGN(size + off, DART_PAGE_SIZE);
>  
> -	dva = lmb_alloc(&priv->lmb, psize, DART_PAGE_SIZE);
> +	dva = lmb_alloc(psize, DART_PAGE_SIZE);
>  
>  	idx = dva / DART_PAGE_SIZE;
>  	for (i = 0; i < psize / DART_PAGE_SIZE; i++) {
> @@ -160,7 +159,7 @@ static void apple_dart_unmap(struct udevice *dev, dma_addr_t addr, size_t size)
>  			   (unsigned long)&priv->l2[idx + i]);
>  	priv->flush_tlb(priv);
>  
> -	lmb_free(&priv->lmb, dva, psize);
> +	lmb_free(dva, psize);
>  }
>  
>  static struct iommu_ops apple_dart_ops = {
> @@ -213,8 +212,7 @@ static int apple_dart_probe(struct udevice *dev)
>  	priv->dvabase = DART_PAGE_SIZE;
>  	priv->dvaend = SZ_4G - DART_PAGE_SIZE;
>  
> -	lmb_init(&priv->lmb);
> -	lmb_add(&priv->lmb, priv->dvabase, priv->dvaend - priv->dvabase);
> +	lmb_add(priv->dvabase, priv->dvaend - priv->dvabase);

no. This breaks everything. apple_dart is an iommu and used struct lmb
to manage the IO virtual address space. This has nothing to do
with system memory.
Depending on the allocation strategy this will cause all sorts of
problems since the IO and the physical memory address space does not
overlap on apple silicon devices. IOVA is for many devices only 32-bit
but physical memory starts at 0x10_0000_0000 or 0x100_0000_0000.
Every device has its own iommu / IOVA space so sharing the space between
all of them is another problem. I'd assume only theoretical due to the
limited driver support and memory use in u-boot.

My current plan to fix this is to resurrect the old lmb code under a
different name.

Not sure about the same change in sandbox_iommu but I guess it could be
ok as there is no sandbox.

Janne