[RFC PATCH 0/2] apple: dart: Add driver specific instance of LMB

[RFC PATCH 0/2] apple: dart: Add driver specific instance of LMB

[Sughosh Ganu]

Sughosh Ganu (2):
  lmb: refactor lmb push and pop functions
  apple: dart: use driver specific instance of LMB

 drivers/iommu/apple_dart.c | 67 +++++++++++++++++++++++++++++++++++++-
 include/lmb.h              |  6 +++-
 lib/lmb.c                  | 21 +++++++++---
 test/lib/lmb.c             | 18 +++++-----
 4 files changed, 96 insertions(+), 16 deletions(-)

-- 
2.34.1

[RFC PATCH 1/2] lmb: refactor lmb push and pop functions

[Sughosh Ganu]

The LMB module uses a separate instance of the lmb structure when
running tests. Setting up the correct instance is done using the
lmb_push() and lmb_pop() functions. In addition to pushing and popping
the lmb instances, these functions are also doing some test specific
operations. Move these operations to lmb_test_push() and
lmb_test_pop() functions, so that lmb_push() and lmb_pop() can be
called from elsewhere.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 include/lmb.h  |  4 +++-
 lib/lmb.c      | 20 ++++++++++++++++----
 test/lib/lmb.c | 18 +++++++++---------
 3 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/include/lmb.h b/include/lmb.h
index e46abf400c6..72d7093023a 100644
--- a/include/lmb.h
+++ b/include/lmb.h
@@ -171,8 +171,10 @@ void lmb_dump_all_force(void);
 void lmb_arch_add_memory(void);
 
 struct lmb *lmb_get(void);
-int lmb_push(struct lmb *store);
+void lmb_push(struct lmb *store);
 void lmb_pop(struct lmb *store);
+int lmb_test_push(struct lmb *store);
+void lmb_test_pop(struct lmb *store);
 
 static inline int lmb_read_check(phys_addr_t addr, phys_size_t len)
 {
diff --git a/lib/lmb.c b/lib/lmb.c
index eec99c185ee..2960c05abfc 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -892,12 +892,23 @@ struct lmb *lmb_get(void)
 	return &lmb;
 }
 
+void lmb_push(struct lmb *store)
+{
+	*store = lmb;
+}
+
+void lmb_pop(struct lmb *store)
+{
+	lmb = *store;
+}
+
 #if CONFIG_IS_ENABLED(UNIT_TEST)
-int lmb_push(struct lmb *store)
+int lmb_test_push(struct lmb *store)
 {
 	int ret;
 
-	*store = lmb;
+	lmb_push(store);
+
 	ret = lmb_setup(true);
 	if (ret)
 		return ret;
@@ -905,10 +916,11 @@ int lmb_push(struct lmb *store)
 	return 0;
 }
 
-void lmb_pop(struct lmb *store)
+void lmb_test_pop(struct lmb *store)
 {
 	alist_uninit(&lmb.free_mem);
 	alist_uninit(&lmb.used_mem);
-	lmb = *store;
+
+	lmb_pop(store);
 }
 #endif /* UNIT_TEST */
diff --git a/test/lib/lmb.c b/test/lib/lmb.c
index b2c54fb4bcb..64e619eff22 100644
--- a/test/lib/lmb.c
+++ b/test/lib/lmb.c
@@ -63,7 +63,7 @@ static int setup_lmb_test(struct unit_test_state *uts, struct lmb *store,
 {
 	struct lmb *lmb;
 
-	ut_assertok(lmb_push(store));
+	ut_assertok(lmb_test_push(store));
 	lmb = lmb_get();
 	*mem_lstp = &lmb->free_mem;
 	*used_lstp = &lmb->used_mem;
@@ -194,7 +194,7 @@ static int test_multi_alloc(struct unit_test_state *uts, const phys_addr_t ram,
 		ut_asserteq(mem[0].size, ram_size);
 	}
 
-	lmb_pop(&store);
+	lmb_test_pop(&store);
 
 	return 0;
 }
@@ -293,7 +293,7 @@ static int test_bigblock(struct unit_test_state *uts, const phys_addr_t ram)
 	ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, alloc_64k_addr, 0x10000,
 		   0, 0, 0, 0);
 
-	lmb_pop(&store);
+	lmb_test_pop(&store);
 
 	return 0;
 }
@@ -373,7 +373,7 @@ static int test_noreserved(struct unit_test_state *uts, const phys_addr_t ram,
 	ut_asserteq(ret, 0);
 	ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 0, 0, 0, 0, 0, 0, 0);
 
-	lmb_pop(&store);
+	lmb_test_pop(&store);
 
 	return 0;
 }
@@ -446,7 +446,7 @@ static int lib_test_lmb_at_0(struct unit_test_state *uts)
 	ut_asserteq(ret, 0);
 	ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 0, 0, 0, 0, 0, 0, 0);
 
-	lmb_pop(&store);
+	lmb_test_pop(&store);
 
 	return 0;
 }
@@ -499,7 +499,7 @@ static int lib_test_lmb_overlapping_reserve(struct unit_test_state *uts)
 	ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40000000, 0x40000,
 		   0, 0, 0, 0);
 
-	lmb_pop(&store);
+	lmb_test_pop(&store);
 
 	return 0;
 }
@@ -619,7 +619,7 @@ static int test_alloc_addr(struct unit_test_state *uts, const phys_addr_t ram)
 		ut_asserteq(ret, 0);
 	}
 
-	lmb_pop(&store);
+	lmb_test_pop(&store);
 
 	return 0;
 }
@@ -691,7 +691,7 @@ static int test_get_unreserved_size(struct unit_test_state *uts,
 	s = lmb_get_free_size(ram_end - 4);
 	ut_asserteq(s, 4);
 
-	lmb_pop(&store);
+	lmb_test_pop(&store);
 
 	return 0;
 }
@@ -797,7 +797,7 @@ static int lib_test_lmb_flags(struct unit_test_state *uts)
 	ut_asserteq(lmb_is_nomap(&used[1]), 0);
 	ut_asserteq(lmb_is_nomap(&used[2]), 1);
 
-	lmb_pop(&store);
+	lmb_test_pop(&store);
 
 	return 0;
 }
-- 
2.34.1

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

[Sughosh Ganu]

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.

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);
+}
+
+static void apple_dart_lmb_pop(struct apple_dart_priv *priv)
+{
+	struct lmb *almb = &priv->apple_dart_lmb;
+	struct lmb *rlmb = &priv->ram_lmb;
+
+	lmb_push(almb);
+
+	lmb_pop(rlmb);
+}
+
 static void apple_dart_t8020_flush_tlb(struct apple_dart_priv *priv)
 {
 	dsb();
@@ -123,7 +176,9 @@ 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);
 
+	apple_dart_lmb_setup(priv);
 	dva = lmb_alloc(psize, DART_PAGE_SIZE);
+	apple_dart_lmb_pop(priv);
 
 	idx = dva / DART_PAGE_SIZE;
 	for (i = 0; i < psize / DART_PAGE_SIZE; i++) {
@@ -159,7 +214,9 @@ 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);
 
+	apple_dart_lmb_setup(priv);
 	lmb_free(dva, psize);
+	apple_dart_lmb_pop(priv);
 }
 
 static struct iommu_ops apple_dart_ops = {
@@ -173,7 +230,7 @@ static int apple_dart_probe(struct udevice *dev)
 	dma_addr_t addr;
 	phys_addr_t l2;
 	int ntte, nl1, nl2;
-	int sid, i;
+	int sid, i, ret;
 	u32 params2, params4;
 
 	priv->base = dev_read_addr_ptr(dev);
@@ -212,7 +269,13 @@ static int apple_dart_probe(struct udevice *dev)
 	priv->dvabase = DART_PAGE_SIZE;
 	priv->dvaend = SZ_4G - DART_PAGE_SIZE;
 
+	ret = apple_dart_lmb_init(priv);
+	if (ret)
+		return ret;
+
+	apple_dart_lmb_setup(priv);
 	lmb_add(priv->dvabase, priv->dvaend - priv->dvabase);
+	apple_dart_lmb_pop(priv);
 
 	/* Disable translations. */
 	for (sid = 0; sid < priv->nsid; sid++)
@@ -294,6 +357,8 @@ static int apple_dart_remove(struct udevice *dev)
 	}
 	priv->flush_tlb(priv);
 
+	apple_dart_lmb_uninit(priv);
+
 	return 0;
 }
 
diff --git a/include/lmb.h b/include/lmb.h
index 72d7093023a..a39b48a9a97 100644
--- a/include/lmb.h
+++ b/include/lmb.h
@@ -14,6 +14,8 @@
  * Copyright (C) 2001 Peter Bergner, IBM Corp.
  */
 
+#define LMB_ALIST_INITIAL_SIZE	4
+
 /**
  * enum lmb_flags - definition of memory region attributes
  * @LMB_NONE: no special request
diff --git a/lib/lmb.c b/lib/lmb.c
index 2960c05abfc..959236f6e5a 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -28,7 +28,6 @@ DECLARE_GLOBAL_DATA_PTR;
 #define MAP_OP_ADD		(u8)0x3
 
 #define LMB_ALLOC_ANYWHERE	0
-#define LMB_ALIST_INITIAL_SIZE	4
 
 static struct lmb lmb;
 
-- 
2.34.1

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

[Janne Grunau]

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

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

[Sughosh Ganu]

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

Good to know. Thanks for testing the changes.

>
> > 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.

Yes, even I was trying to avoid having another instance of LMB as much
as possible, but given that this is breaking the platform, I thought
this would be the quickest to fix the break. Personally, I would
prefer LMB to have a consistent window of RAM addresses for all
platforms, i.e. ram_base to ram_top. Also, apart from this peculiar
scenario, I am not sure if there would be other uses of having to
allocate anything other than RAM addresses by LMB.

-sughosh


>
> Janne

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

[Janne Grunau]

On Tue, Oct 29, 2024 at 02:22:53PM +0530, Sughosh Ganu wrote:
> On Tue, 29 Oct 2024 at 14:02, Janne Grunau <j@jannau.net> wrote:
> >
> > 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.
> 
> Good to know. Thanks for testing the changes.

It's SError-ing on `dc zva` of an invalid address which somehow got into
the main lmb. I don't see how but it went away with my minimal io_lmb
implemenmtation exposing lmb_setup, lmb_add, lmb_alloc, lmb_free.

I'm not too happy about this approach since it's a little fragile with
respect to the global struct lmb. to be somewhat safe we would need to
reorder lmb.c to avoid `static struct lmb lmb;` is visible in "shared"
code to avoid using it by surprise in the middle of functions like in
_lmb_alloc_base().

I'll post patches later today.

Janne