[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 at 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 at 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 at 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