[PATCH 3/3] lib: utils/timer: check if addr exists

[Anup Patel]

On Mon, Feb 14, 2022 at 5:39 PM Nikita Shubin <nikita.shubin at maquefel.me> wrote:
>
> Hello Anup!
>
> On Mon, 14 Feb 2022 17:08:36 +0530
> Anup Patel <anup at brainfault.org> wrote:
>
> > On Fri, Feb 11, 2022 at 4:42 PM Nikita Shubin
> > <nikita.shubin at maquefel.me> wrote:
> > >
> > > From: Nikita Shubin <n.shubin at yadro.com>
> > >
> > > In case we want shared time addr register for different mtimer's,
> > > we need to check if region was already added to domain, otherwise
> > > sbi_domain_root_add_memregion will fail and will couse init to fail
> > > with:
> > > init_coldboot: timer init failed (error -3)
> >
> > A mtimer device has two separate base addresses:
> > 1) mtime base address
> > 2) mtimecmp base address
> >
> > Two separate mtimer devices can share the same mtime register
> > but cannot share mtimecmp registers.
>
> In case mtimecmp is less than mtime it is possible to run in following
> situation:
>
> - mtimecmp 0 - N - 1 and mtime are not adjusted and treated separately,
>   mtime exists check succeed
> - mtimecmp N is adjusted to mtime and gets merged to mt->mtimecmp_addr
>   + mt->mtime_size + mt->mtimecmp_size and fails with region conflict
>   with existing mtime region added from previous steps
>
> How about dropping these two checks:
>
> if (mt->mtime_addr == (mt->mtimecmp_addr + mt->mtimecmp_size)) {
> ...
> } else if (mt->mtimecmp_addr == (mt->mtime_addr + mt->mtime_size)) {
>
> sbi_domain will take care of and merge these separate regions, so we
> can check only mtime addr.
>
> What do you think about this ?

I am still trying to understand the issue.

Can you share example DT nodes ?

Regards,
Anup

>
>
> >
> > Regards,
> > Anup
> >
> > >
> > > Signed-off-by: Nikita Shubin <n.shubin at yadro.com>
> > > ---
> > >  lib/utils/timer/aclint_mtimer.c | 51
> > > ++++++++++++++++++++++++++------- 1 file changed, 40 insertions(+),
> > > 11 deletions(-)
> > >
> > > diff --git a/lib/utils/timer/aclint_mtimer.c
> > > b/lib/utils/timer/aclint_mtimer.c index a957b1c..2c90e22 100644
> > > --- a/lib/utils/timer/aclint_mtimer.c
> > > +++ b/lib/utils/timer/aclint_mtimer.c
> > > @@ -175,6 +175,9 @@ int aclint_mtimer_cold_init(struct
> > > aclint_mtimer_data *mt, {
> > >         u32 i;
> > >         int rc;
> > > +       struct aclint_mtimer_data *last_mt = NULL;
> > > +       bool skip_time_addr = false;
> > > +       bool skip_timecmp_addr = false;
> > >
> > >         /* Sanity checks */
> > >         if (!mt || !mt->mtime_size ||
> > > @@ -206,27 +209,53 @@ int aclint_mtimer_cold_init(struct
> > > aclint_mtimer_data *mt, for (i = 0; i < mt->hart_count; i++)
> > >                 mtimer_hartid2data[mt->first_hartid + i] = mt;
> > >
> > > +       /* Check if timer or timercmp addr already exists
> > > +        * otherwise aclint_mtimer_add_regions will fail
> > > +        * becouse of regions conflict.
> > > +        */
> > > +       for (i = 0; i < mt->first_hartid; i++) {
> > > +               if (last_mt && last_mt == mtimer_hartid2data[i])
> > > +                       continue;
> > > +
> > > +               if (mtimer_hartid2data[i]->mtime_addr ==
> > > mt->mtime_addr)
> > > +                       skip_time_addr = true;
> > > +
> > > +               if (mtimer_hartid2data[i]->mtimecmp_addr ==
> > > mt->mtimecmp_addr)
> > > +                       skip_timecmp_addr = true;
> > > +
> > > +               if (skip_time_addr && skip_timecmp_addr)
> > > +                       break;
> > > +
> > > +               last_mt = mtimer_hartid2data[i];
> > > +       }
> > > +
> > >         /* Add MTIMER regions to the root domain */
> > > -       if (mt->mtime_addr == (mt->mtimecmp_addr +
> > > mt->mtimecmp_size)) {
> > > +       if ((mt->mtime_addr == (mt->mtimecmp_addr +
> > > mt->mtimecmp_size)) &&
> > > +               !skip_timecmp_addr) {
> > >                 rc = aclint_mtimer_add_regions(mt->mtimecmp_addr,
> > >                                         mt->mtime_size +
> > > mt->mtimecmp_size); if (rc)
> > >                         return rc;
> > > -       } else if (mt->mtimecmp_addr == (mt->mtime_addr +
> > > mt->mtime_size)) {
> > > +       } else if (mt->mtimecmp_addr == (mt->mtime_addr +
> > > mt->mtime_size) &&
> > > +               !skip_time_addr) {
> > >                 rc = aclint_mtimer_add_regions(mt->mtime_addr,
> > >                                         mt->mtime_size +
> > > mt->mtimecmp_size); if (rc)
> > >                         return rc;
> > >         } else {
> > > -               rc = aclint_mtimer_add_regions(mt->mtime_addr,
> > > -                                               mt->mtime_size);
> > > -               if (rc)
> > > -                       return rc;
> > > -
> > > -               rc = aclint_mtimer_add_regions(mt->mtimecmp_addr,
> > > -                                               mt->mtimecmp_size);
> > > -               if (rc)
> > > -                       return rc;
> > > +               if (!skip_time_addr) {
> > > +                       rc =
> > > aclint_mtimer_add_regions(mt->mtime_addr,
> > > +
> > > mt->mtime_size);
> > > +                       if (rc)
> > > +                               return rc;
> > > +               }
> > > +
> > > +               if (!skip_timecmp_addr) {
> > > +                       rc =
> > > aclint_mtimer_add_regions(mt->mtimecmp_addr,
> > > +
> > > mt->mtimecmp_size);
> > > +                       if (rc)
> > > +                               return rc;
> > > +               }
> > >         }
> > >
> > >         mtimer.timer_freq = mt->mtime_freq;
> > > --
> > > 2.31.1
> > >
> > >
> > > --
> > > opensbi mailing list
> > > opensbi at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/opensbi
>