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

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

[Janne Grunau]

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

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3686 bytes --]

On Mon, Oct 28, 2024 at 10:20:38PM +0100, Janne Grunau wrote:
> 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.

Oh damn, sorry. Perhaps it can be brought back in the iommu framework
more directly? I asked Caleb and they said snapdragon doesn't need
something like this.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Sughosh Ganu]

On Tue, 29 Oct 2024 at 02:50, Janne Grunau <janne@jannau.net> wrote:
>
> 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.

Firstly, apologies for breaking the driver. Looking at the code, it
looks like the driver works with a dva address range in the first 4GB.
I think what the driver expects is fairly straightforward, with a
top-down allocation scheme. I have sent a patch [1] which replaces the
LMB calls with a call to the allocation function. Can you please check
if this works on the board? Looking at how the driver is obtaining the
dva addresses, I think this logic should suffice. Thanks.

-sughosh

[1] - https://patchwork.ozlabs.org/project/uboot/patch/20241029033915.423211-1-sughosh.ganu@linaro.org/


-sughosh

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

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

[Sughosh Ganu]

On Tue, 29 Oct 2024 at 09:19, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Tue, 29 Oct 2024 at 02:50, Janne Grunau <janne@jannau.net> wrote:
> >
> > 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.
>
> Firstly, apologies for breaking the driver. Looking at the code, it
> looks like the driver works with a dva address range in the first 4GB.
> I think what the driver expects is fairly straightforward, with a
> top-down allocation scheme. I have sent a patch [1] which replaces the
> LMB calls with a call to the allocation function. Can you please check
> if this works on the board? Looking at how the driver is obtaining the
> dva addresses, I think this logic should suffice. Thanks.
>
> -sughosh
>
> [1] - https://patchwork.ozlabs.org/project/uboot/patch/20241029033915.423211-1-sughosh.ganu@linaro.org/

This would not work as it does not consider freeing of the va
addresses. So there might be a need for a parallel lmb instance for
this.

-sughosh

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

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

[Sughosh Ganu]

On Tue, 29 Oct 2024 at 10:02, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Tue, 29 Oct 2024 at 09:19, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > On Tue, 29 Oct 2024 at 02:50, Janne Grunau <janne@jannau.net> wrote:
> > >
> > > 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.
> >
> > Firstly, apologies for breaking the driver. Looking at the code, it
> > looks like the driver works with a dva address range in the first 4GB.
> > I think what the driver expects is fairly straightforward, with a
> > top-down allocation scheme. I have sent a patch [1] which replaces the
> > LMB calls with a call to the allocation function. Can you please check
> > if this works on the board? Looking at how the driver is obtaining the
> > dva addresses, I think this logic should suffice. Thanks.
> >
> > -sughosh
> >
> > [1] - https://patchwork.ozlabs.org/project/uboot/patch/20241029033915.423211-1-sughosh.ganu@linaro.org/
>
> This would not work as it does not consider freeing of the va
> addresses. So there might be a need for a parallel lmb instance for
> this.
>
> -sughosh
>
> >
> >
> > -sughosh
> >
> > >
> > > Not sure about the same change in sandbox_iommu but I guess it could be
> > > ok as there is no sandbox.

I have pushed the changes for the driver to one of my github branch
[1]. Can you please try it out and let me know. Thanks.

-sughosh

[1] - https://github.com/sughoshg/u-boot/tree/apple_dart_changes

> > >
> > > Janne