[RFC PATCH 0/2] apple: dart: Add driver specific instance of LMB
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(-)
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; }
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;
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
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
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
participants (2)
-
Janne Grunau -
Sughosh Ganu