[PATCH 4/7] lib: utils/sys: Add CLINT memregion in the root domain

[Anup Patel]

On Mon, Apr 12, 2021 at 11:12 AM Xiang W <wxjstz at 126.com> wrote:
>
> 在 2021-04-12一的 10:53 +0530，Anup Patel写道：
> > On Sun, Apr 11, 2021 at 2:35 PM Xiang W <wxjstz at 126.com> wrote:
> > > 在 2021-04-10六的 12:48 +0530，Anup Patel写道：
> > > > The CLINT memory should not be accessed by the supervisor-mode
> > > > software so let's protect it by adding CLINT memregion to the
> > > > root domain.
> > > >
> > > > Signed-off-by: Anup Patel <anup.patel at wdc.com>
> > > clint_cold_timer_init and clint_cold_ipi_init may use the same
> > > clint,
> > > which will call clint_add_root_memregion twice and generate an
> > > error.
> > > I suggest to separate
> > >
> > > In clint_cold_timer_init execute
> > > sbi_domain_memregion_init(clint->addr, 0x4000,
> > > SBI_DOMAIN_MEMREGION_MMIO, &reg);
> > >
> > > In clint_cold_ipi_init execute
> > > sbi_domain_memregion_init(clint->addr + 0x4000, 0x8000,
> > > SBI_DOMAIN_MEMREGION_MMIO, &reg);
> >
> > I had though about this but the only issue in having two
> > separate memregions for CLINT is that it will take-up more
> > PMP CSRs. The "skip_conflt" flag was added as parameter
> > to sbi_domain_root_add_memregion() so that CLINT driver
> > can call it multiple time.
> >
> > I think we can have a better implementation of the
> > sbi_domain_root_add_memregion() which will merge
> > consecutive regions with matching flags. This will allow
> > use to add separate regions for CLINT driver but
> > sbi_domain_root_add_memregion() will merge them
> > into single region.
> Merge regions is a good idea. I think the merger can be performed by
> default. So we can separate this side first, and then modify
> sbi_domain_root_add_memregion to add the merge function.

I will be updating both PATCH3 and PATCH4 (this patch) for this
so please review these patches again in next revision.

Regards,
Anup

>
> Regards,
> Xiang W
> >
> > Regards,
> > Anup
> >
> > > Regards,
> > > Xiang W
> > > > ---
> > > >  lib/utils/sys/clint.c | 17 +++++++++++++++--
> > > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/lib/utils/sys/clint.c b/lib/utils/sys/clint.c
> > > > index 7a392aa..fe98cc4 100644
> > > > --- a/lib/utils/sys/clint.c
> > > > +++ b/lib/utils/sys/clint.c
> > > > @@ -10,6 +10,7 @@
> > > >  #include <sbi/riscv_asm.h>
> > > >  #include <sbi/riscv_atomic.h>
> > > >  #include <sbi/riscv_io.h>
> > > > +#include <sbi/sbi_domain.h>
> > > >  #include <sbi/sbi_error.h>
> > > >  #include <sbi/sbi_hartmask.h>
> > > >  #include <sbi_utils/sys/clint.h>
> > > > @@ -17,9 +18,19 @@
> > > >  #define CLINT_IPI_OFF                0
> > > >  #define CLINT_TIME_CMP_OFF   0x4000
> > > >  #define CLINT_TIME_VAL_OFF   0xbff8
> > > > +#define CLINT_SIZE           0xc000
> > > >
> > > >  static struct clint_data
> > > > *clint_ipi_hartid2data[SBI_HARTMASK_MAX_BITS];
> > > >
> > > > +static int clint_add_root_memregion(struct clint_data *clint)
> > > > +{
> > > > +     struct sbi_domain_memregion reg;
> > > > +
> > > > +     sbi_domain_memregion_init(clint->addr, CLINT_SIZE,
> > > > +                               SBI_DOMAIN_MEMREGION_MMIO, &reg);
> > > > +     return sbi_domain_root_add_memregion(&reg, true);
> > > > +}
> > > > +
> > > >  void clint_ipi_send(u32 target_hart)
> > > >  {
> > > >       struct clint_data *clint;
> > > > @@ -70,7 +81,8 @@ int clint_cold_ipi_init(struct clint_data
> > > > *clint)
> > > >       for (i = 0; i < clint->hart_count; i++)
> > > >               clint_ipi_hartid2data[clint->first_hartid + i] =
> > > > clint;
> > > >
> > > > -     return 0;
> > > > +     /* Add CLINT region to the root domain */
> > > > +     return clint_add_root_memregion(clint);
> > > >  }
> > > >
> > > >  static struct clint_data
> > > > *clint_timer_hartid2data[SBI_HARTMASK_MAX_BITS];
> > > > @@ -199,5 +211,6 @@ int clint_cold_timer_init(struct clint_data
> > > > *clint,
> > > >       for (i = 0; i < clint->hart_count; i++)
> > > >               clint_timer_hartid2data[clint->first_hartid + i] =
> > > > clint;
> > > >
> > > > -     return 0;
> > > > +     /* Add CLINT region to the root domain */
> > > > +     return clint_add_root_memregion(clint);
> > > >  }
> > > > --
> > > > 2.25.1
> > > >
> > > >
>