diff mbox series

[1/2] lib: utils/ipi: Fix size check in aclint_mswi_cold_init()

Message ID 20220108040336.14414-2-apatel@ventanamicro.com
State Superseded
Headers show
Series Misc driver improvements | expand

Commit Message

Anup Patel Jan. 8, 2022, 4:03 a.m. UTC 
Currently, the ACLINT MSWI size check is forcing size to be at least
0x4000. This is inappropriate check because most systems will never
utilize full 16KB for a single ACLINT MSWI device so instead we should
check that ACLINT MSWI size is enough for on the associated HARTs.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 lib/utils/ipi/aclint_mswi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Dong Du Jan. 8, 2022, 8:34 a.m. UTC | #1 
On Jan 8, 2022, at 12:03 PM, Anup Patel apatel@ventanamicro.com wrote:

> Currently, the ACLINT MSWI size check is forcing size to be at least
> 0x4000. This is inappropriate check because most systems will never
> utilize full 16KB for a single ACLINT MSWI device so instead we should
> check that ACLINT MSWI size is enough for on the associated HARTs.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
> lib/utils/ipi/aclint_mswi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/utils/ipi/aclint_mswi.c b/lib/utils/ipi/aclint_mswi.c
> index a3de2f5..832e223 100644
> --- a/lib/utils/ipi/aclint_mswi.c
> +++ b/lib/utils/ipi/aclint_mswi.c
> @@ -74,7 +74,7 @@ int aclint_mswi_cold_init(struct aclint_mswi_data *mswi)
> 
> 	/* Sanity checks */
> 	if (!mswi || (mswi->addr & (ACLINT_MSWI_ALIGN - 1)) ||
> -	    (mswi->size < ACLINT_MSWI_SIZE) ||
> +	    (mswi->size < (mswi->hart_count * sizeof(u32))) ||

Hi Anup,
 
    Shouldn't we also change ACLINT_MSWI_SIZE in lib/utils/ipi/fdt_ipi_mswi.c?

Regards,
Dong

> 	    (mswi->first_hartid >= SBI_HARTMASK_MAX_BITS) ||
> 	    (mswi->hart_count > ACLINT_MSWI_MAX_HARTS))
> 		return SBI_EINVAL;
> --
> 2.25.1
> 
> 
> --
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
Anup Patel Jan. 8, 2022, 11:35 a.m. UTC | #2 
On Sat, Jan 8, 2022 at 2:04 PM Dong Du <dd_nirvana@sjtu.edu.cn> wrote:
>
>
>
> On Jan 8, 2022, at 12:03 PM, Anup Patel apatel@ventanamicro.com wrote:
>
> > Currently, the ACLINT MSWI size check is forcing size to be at least
> > 0x4000. This is inappropriate check because most systems will never
> > utilize full 16KB for a single ACLINT MSWI device so instead we should
> > check that ACLINT MSWI size is enough for on the associated HARTs.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> > lib/utils/ipi/aclint_mswi.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/utils/ipi/aclint_mswi.c b/lib/utils/ipi/aclint_mswi.c
> > index a3de2f5..832e223 100644
> > --- a/lib/utils/ipi/aclint_mswi.c
> > +++ b/lib/utils/ipi/aclint_mswi.c
> > @@ -74,7 +74,7 @@ int aclint_mswi_cold_init(struct aclint_mswi_data *mswi)
> >
> >       /* Sanity checks */
> >       if (!mswi || (mswi->addr & (ACLINT_MSWI_ALIGN - 1)) ||
> > -         (mswi->size < ACLINT_MSWI_SIZE) ||
> > +         (mswi->size < (mswi->hart_count * sizeof(u32))) ||
>
> Hi Anup,
>
>     Shouldn't we also change ACLINT_MSWI_SIZE in lib/utils/ipi/fdt_ipi_mswi.c?

ACLINT_MSWI_SIZE represents the upper-limit because one MSWI
device can be associated with up to 4095 HARTs.

I think we also should fail on "mswi->size > ACLINT_MSWI_SIZE".

Regards,
Anup

>
> Regards,
> Dong
>
> >           (mswi->first_hartid >= SBI_HARTMASK_MAX_BITS) ||
> >           (mswi->hart_count > ACLINT_MSWI_MAX_HARTS))
> >               return SBI_EINVAL;
> > --
> > 2.25.1
> >
> >
> > --
> > opensbi mailing list
> > opensbi@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
Dong Du Jan. 8, 2022, 2:50 p.m. UTC | #3 
On Jan 8, 2022, at 7:35 PM, Anup Patel apatel@ventanamicro.com wrote:

> On Sat, Jan 8, 2022 at 2:04 PM Dong Du <dd_nirvana@sjtu.edu.cn> wrote:
>>
>>
>>
>> On Jan 8, 2022, at 12:03 PM, Anup Patel apatel@ventanamicro.com wrote:
>>
>> > Currently, the ACLINT MSWI size check is forcing size to be at least
>> > 0x4000. This is inappropriate check because most systems will never
>> > utilize full 16KB for a single ACLINT MSWI device so instead we should
>> > check that ACLINT MSWI size is enough for on the associated HARTs.
>> >
>> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
>> > ---
>> > lib/utils/ipi/aclint_mswi.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/lib/utils/ipi/aclint_mswi.c b/lib/utils/ipi/aclint_mswi.c
>> > index a3de2f5..832e223 100644
>> > --- a/lib/utils/ipi/aclint_mswi.c
>> > +++ b/lib/utils/ipi/aclint_mswi.c
>> > @@ -74,7 +74,7 @@ int aclint_mswi_cold_init(struct aclint_mswi_data *mswi)
>> >
>> >       /* Sanity checks */
>> >       if (!mswi || (mswi->addr & (ACLINT_MSWI_ALIGN - 1)) ||
>> > -         (mswi->size < ACLINT_MSWI_SIZE) ||
>> > +         (mswi->size < (mswi->hart_count * sizeof(u32))) ||
>>
>> Hi Anup,
>>
>>     Shouldn't we also change ACLINT_MSWI_SIZE in lib/utils/ipi/fdt_ipi_mswi.c?
> 
> ACLINT_MSWI_SIZE represents the upper-limit because one MSWI
> device can be associated with up to 4095 HARTs.
> 
> I think we also should fail on "mswi->size > ACLINT_MSWI_SIZE".

Hi Anup,

  What I see is "if ((ms->size - offset) < ACLINT_MSWI_SIZE)" at ipi_mswi_cold_init() (fdt_ipi_mswi.c),
  which treats the ACLINT_MSWI_SIZE as the lower bound.

Regards,
Dong

> 
> Regards,
> Anup
> 
>>
>> Regards,
>> Dong
>>
>> >           (mswi->first_hartid >= SBI_HARTMASK_MAX_BITS) ||
>> >           (mswi->hart_count > ACLINT_MSWI_MAX_HARTS))
>> >               return SBI_EINVAL;
>> > --
>> > 2.25.1
>> >
>> >
>> > --
>> > opensbi mailing list
>> > opensbi@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/opensbi
Anup Patel Jan. 8, 2022, 5:09 p.m. UTC | #4 
On Sat, Jan 8, 2022 at 8:20 PM Dong Du <dd_nirvana@sjtu.edu.cn> wrote:
>
> On Jan 8, 2022, at 7:35 PM, Anup Patel apatel@ventanamicro.com wrote:
>
> > On Sat, Jan 8, 2022 at 2:04 PM Dong Du <dd_nirvana@sjtu.edu.cn> wrote:
> >>
> >>
> >>
> >> On Jan 8, 2022, at 12:03 PM, Anup Patel apatel@ventanamicro.com wrote:
> >>
> >> > Currently, the ACLINT MSWI size check is forcing size to be at least
> >> > 0x4000. This is inappropriate check because most systems will never
> >> > utilize full 16KB for a single ACLINT MSWI device so instead we should
> >> > check that ACLINT MSWI size is enough for on the associated HARTs.
> >> >
> >> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> >> > ---
> >> > lib/utils/ipi/aclint_mswi.c | 2 +-
> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/lib/utils/ipi/aclint_mswi.c b/lib/utils/ipi/aclint_mswi.c
> >> > index a3de2f5..832e223 100644
> >> > --- a/lib/utils/ipi/aclint_mswi.c
> >> > +++ b/lib/utils/ipi/aclint_mswi.c
> >> > @@ -74,7 +74,7 @@ int aclint_mswi_cold_init(struct aclint_mswi_data *mswi)
> >> >
> >> >       /* Sanity checks */
> >> >       if (!mswi || (mswi->addr & (ACLINT_MSWI_ALIGN - 1)) ||
> >> > -         (mswi->size < ACLINT_MSWI_SIZE) ||
> >> > +         (mswi->size < (mswi->hart_count * sizeof(u32))) ||
> >>
> >> Hi Anup,
> >>
> >>     Shouldn't we also change ACLINT_MSWI_SIZE in lib/utils/ipi/fdt_ipi_mswi.c?
> >
> > ACLINT_MSWI_SIZE represents the upper-limit because one MSWI
> > device can be associated with up to 4095 HARTs.
> >
> > I think we also should fail on "mswi->size > ACLINT_MSWI_SIZE".
>
> Hi Anup,
>
>   What I see is "if ((ms->size - offset) < ACLINT_MSWI_SIZE)" at ipi_mswi_cold_init() (fdt_ipi_mswi.c),
>   which treats the ACLINT_MSWI_SIZE as the lower bound.

That's only SiFive CLINT backward compatibility
because match->data != NULL only for SiFive CLINT.

Regards,
Anup

>
> Regards,
> Dong
>
> >
> > Regards,
> > Anup
> >
> >>
> >> Regards,
> >> Dong
> >>
> >> >           (mswi->first_hartid >= SBI_HARTMASK_MAX_BITS) ||
> >> >           (mswi->hart_count > ACLINT_MSWI_MAX_HARTS))
> >> >               return SBI_EINVAL;
> >> > --
> >> > 2.25.1
> >> >
> >> >
> >> > --
> >> > opensbi mailing list
> >> > opensbi@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/opensbi
Dong Du Jan. 9, 2022, 4:45 a.m. UTC | #5 
On Jan 9, 2022, at 1:09 AM, Anup Patel apatel@ventanamicro.com wrote:

> On Sat, Jan 8, 2022 at 8:20 PM Dong Du <dd_nirvana@sjtu.edu.cn> wrote:
>>
>> On Jan 8, 2022, at 7:35 PM, Anup Patel apatel@ventanamicro.com wrote:
>>
>> > On Sat, Jan 8, 2022 at 2:04 PM Dong Du <dd_nirvana@sjtu.edu.cn> wrote:
>> >>
>> >>
>> >>
>> >> On Jan 8, 2022, at 12:03 PM, Anup Patel apatel@ventanamicro.com wrote:
>> >>
>> >> > Currently, the ACLINT MSWI size check is forcing size to be at least
>> >> > 0x4000. This is inappropriate check because most systems will never
>> >> > utilize full 16KB for a single ACLINT MSWI device so instead we should
>> >> > check that ACLINT MSWI size is enough for on the associated HARTs.
>> >> >
>> >> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
>> >> > ---
>> >> > lib/utils/ipi/aclint_mswi.c | 2 +-
>> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/lib/utils/ipi/aclint_mswi.c b/lib/utils/ipi/aclint_mswi.c
>> >> > index a3de2f5..832e223 100644
>> >> > --- a/lib/utils/ipi/aclint_mswi.c
>> >> > +++ b/lib/utils/ipi/aclint_mswi.c
>> >> > @@ -74,7 +74,7 @@ int aclint_mswi_cold_init(struct aclint_mswi_data *mswi)
>> >> >
>> >> >       /* Sanity checks */
>> >> >       if (!mswi || (mswi->addr & (ACLINT_MSWI_ALIGN - 1)) ||
>> >> > -         (mswi->size < ACLINT_MSWI_SIZE) ||
>> >> > +         (mswi->size < (mswi->hart_count * sizeof(u32))) ||
>> >>
>> >> Hi Anup,
>> >>
>> >>     Shouldn't we also change ACLINT_MSWI_SIZE in lib/utils/ipi/fdt_ipi_mswi.c?
>> >
>> > ACLINT_MSWI_SIZE represents the upper-limit because one MSWI
>> > device can be associated with up to 4095 HARTs.
>> >
>> > I think we also should fail on "mswi->size > ACLINT_MSWI_SIZE".
>>
>> Hi Anup,
>>
>>   What I see is "if ((ms->size - offset) < ACLINT_MSWI_SIZE)" at
>>   ipi_mswi_cold_init() (fdt_ipi_mswi.c),
>>   which treats the ACLINT_MSWI_SIZE as the lower bound.
> 
> That's only SiFive CLINT backward compatibility
> because match->data != NULL only for SiFive CLINT.

  Thanks for the clarification.

  Reviewed-by: Dong Du <Dd_nirvana@sjtu.edu.cn>

Regards,
Dong

> 
> Regards,
> Anup
> 
>>
>> Regards,
>> Dong
>>
>> >
>> > Regards,
>> > Anup
>> >
>> >>
>> >> Regards,
>> >> Dong
>> >>
>> >> >           (mswi->first_hartid >= SBI_HARTMASK_MAX_BITS) ||
>> >> >           (mswi->hart_count > ACLINT_MSWI_MAX_HARTS))
>> >> >               return SBI_EINVAL;
>> >> > --
>> >> > 2.25.1
>> >> >
>> >> >
>> >> > --
>> >> > opensbi mailing list
>> >> > opensbi@lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/opensbi
diff mbox series

Patch

diff --git a/lib/utils/ipi/aclint_mswi.c b/lib/utils/ipi/aclint_mswi.c
index a3de2f5..832e223 100644
--- a/lib/utils/ipi/aclint_mswi.c
+++ b/lib/utils/ipi/aclint_mswi.c
@@ -74,7 +74,7 @@  int aclint_mswi_cold_init(struct aclint_mswi_data *mswi)
 
 	/* Sanity checks */
 	if (!mswi || (mswi->addr & (ACLINT_MSWI_ALIGN - 1)) ||
-	    (mswi->size < ACLINT_MSWI_SIZE) ||
+	    (mswi->size < (mswi->hart_count * sizeof(u32))) ||
 	    (mswi->first_hartid >= SBI_HARTMASK_MAX_BITS) ||
 	    (mswi->hart_count > ACLINT_MSWI_MAX_HARTS))
 		return SBI_EINVAL;