[PATCH 3/3] lib: utils/timer: check if addr exists
Nikita Shubin
nikita.shubin at maquefel.me
Fri Feb 18 00:45:00 PST 2022
Hello Anup!
On Fri, 18 Feb 2022 10:59:56 +0530
Anup Patel <anup at brainfault.org> wrote:
Thas't a bit more complicated than i thought, if we remove the region
bounds checks in aclint_mtimer driver we run into following:
```
aclint_mtimer_add_regions : addr=0x200bff8, size=0x4008
aclint_mtimer_add_regions : pos=0x200bff8, rsize=0x8
aclint_mtimer_add_regions : reg->base=0x200bff8, size=0x7
aclint_mtimer_add_regions : pos=0x200c000, rsize=0x1000
aclint_mtimer_add_regions : reg->base=0x200c000, size=0xfff
aclint_mtimer_add_regions : pos=0x200d000, rsize=0x1000
aclint_mtimer_add_regions : reg->base=0x200d000, size=0xfff
aclint_mtimer_add_regions : pos=0x200e000, rsize=0x1000
aclint_mtimer_add_regions : reg->base=0x200e000, size=0xfff
aclint_mtimer_add_regions : pos=0x200f000, rsize=0x1000
aclint_mtimer_add_regions : reg->base=0x200f000, size=0xfff
aclint_mtimer_add_regions : addr=0x2004000, size=0x7ff8
aclint_mtimer_add_regions : pos=0x2004000, rsize=0x1000
aclint_mtimer_add_regions : reg->base=0x2004000, size=0xfff
aclint_mtimer_add_regions : pos=0x2005000, rsize=0x1000
aclint_mtimer_add_regions : reg->base=0x2005000, size=0xfff
aclint_mtimer_add_regions : pos=0x2006000, rsize=0x1000
aclint_mtimer_add_regions : reg->base=0x2006000, size=0xfff
aclint_mtimer_add_regions : pos=0x2007000, rsize=0x1000
aclint_mtimer_add_regions : reg->base=0x2007000, size=0xfff
aclint_mtimer_add_regions : pos=0x2008000, rsize=0x1000
aclint_mtimer_add_regions : reg->base=0x2008000, size=0xfff
aclint_mtimer_add_regions : pos=0x2009000, rsize=0x1000
aclint_mtimer_add_regions : reg->base=0x2009000, size=0xfff
aclint_mtimer_add_regions : pos=0x200a000, rsize=0x1000
aclint_mtimer_add_regions : reg->base=0x200a000, size=0xfff
aclint_mtimer_add_regions : pos=0x200b000, rsize=0xff8
aclint_mtimer_add_regions : reg->base=0x200b000, size=0xfff
sbi_domain_root_add_memregion: is_region_conflict check failed
0x200b000 conflicts existing 0x200bff8
init_coldboot: timer init failed (error -6)
```
QEMU virt machine dts with aclint=on:
mtimer at 2004000 {
interrupts-extended = <0x04 0x07 0x02 0x07>;
reg = <0x00 0x200bff8 0x00 0x4008
0x00 0x2004000 0x00 0x7ff8>;
compatible = "riscv,aclint-mtimer";
};
Indeed:
```
(gdb) p/x 0x2004000 + 0x7ff8 + 0x4008
$8 = 0x2010000
```
Give's us proper aligned region, while adding separate mtime/mtimecmp
gives us an overlap on adding, so i have to think about this.
> On Mon, Feb 14, 2022 at 6:40 PM Nikita Shubin
> <nikita.shubin at maquefel.me> wrote:
> >
> > On Mon, 14 Feb 2022 17:57:44 +0530
> > Anup Patel <anup at brainfault.org> wrote:
> >
> > > 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 ?
> >
> > That's a bit modified example from your presentation on plumbers:
> >
> > mtimer0: mtimer at 30000000 {
> > compatible = "riscv,aclint-mtimer";
> > reg = <0x3a000000 0x8>,
> > /* MTIME */
> > <0x30000000 0x20>; /* MTIMECMPs */
> > interrupts-extended = <0x04 7>;
> > };
> >
> > mtimer1: mtimer at 31000000 {
> > compatible = "riscv,aclint-mtimer";
> > reg = <0x3a000000 0x8>,
> > /* MTIME */
> > <0x31000000 0x20>; /* MTIMECMPs */
> > interrupts-extended = <0x02 7>;
> > };
> >
> > The regions are not adjusted and this will fail.
> >
> > Adjusted regions like i described:
> >
> > mtimer0: mtimer at 30000000 {
> > compatible = "riscv,aclint-mtimer";
> > reg = <0x30000000 0x1000>,
> > /* MTIME */
> > <0x2ffff000 0x1000>; /* MTIMECMPs */
> > interrupts-extended = <0x04 7>;
> > };
> >
> > mtimer1: mtimer at 31000000 {
> > compatible = "riscv,aclint-mtimer";
> > reg = <0x30000000 0x1000>,
> > /* MTIME */
> > <0x2fffe000 0x1000>; /* MTIMECMPs */
> > interrupts-extended = <0x02 7>;
> > };
> >
> > The main reason why earlier worked becouse qemu generates mtimer
> > bindings like this:
> >
> > reg = <0x00 0x200bff8 0x00 0x4008 0x00 0x2004000 0x00 0x7ff8>;
> >
> > In other words everything bigger than 0x1000 will succeed becouse of
> > sbi_domain bug corrected in patch 1.
>
> I understand the problem now.
>
> I agree with your suggestion. We should let sbi_domain do all the
> region merging so we should drop following checks:
> f (mt->mtime_addr == (mt->mtimecmp_addr + mt->mtimecmp_size)) {
> ...
> } else if (mt->mtimecmp_addr == (mt->mtime_addr + mt->mtime_size)) {
>
> Also, we should only add mtime region if it is not already added by
> other mtimer devices.
>
> Can you send a v2 patch ?
>
> Regards,
> Anup
>
> >
> >
> > >
> > > 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
> > > >
> >
More information about the opensbi
mailing list