| Message ID | 20241029071846.514559-3-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | apple: dart: Add driver specific instance of LMB | expand |
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
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;
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(-)