The end address calculation for NA4 mode is wrong because the address
used isn't shifted.

That imply all NA4 setup are not applied by the PMP.

The solution is to use the shifted address calculated for start address
variable.

Modifications are tested on Zephyr OS userspace test suite which works
for other RISC-V boards (E31 and E34 core).

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 target/riscv/pmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 9418660f1b..2a2b9f5363 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -171,7 +171,7 @@ static void pmp_update_rule(CPURISCVState *env, uint32_t pmp_index)
 
     case PMP_AMATCH_NA4:
         sa = this_addr << 2; /* shift up from [xx:0] to [xx+2:2] */
-        ea = (this_addr + 4u) - 1u;
+        ea = (sa + 4u) - 1u;
         break;
 
     case PMP_AMATCH_NAPOT:
-- 
2.17.1

On Mon, Jul 6, 2020 at 2:45 AM Alexandre Mergnat <amergnat@baylibre.com> wrote:
>
> The end address calculation for NA4 mode is wrong because the address
> used isn't shifted.
>
> That imply all NA4 setup are not applied by the PMP.

I'm not sure what you mean here, can you clarify this?

>
> The solution is to use the shifted address calculated for start address
> variable.
>
> Modifications are tested on Zephyr OS userspace test suite which works
> for other RISC-V boards (E31 and E34 core).
>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

Otherwise:

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/pmp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 9418660f1b..2a2b9f5363 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -171,7 +171,7 @@ static void pmp_update_rule(CPURISCVState *env, uint32_t pmp_index)
>
>      case PMP_AMATCH_NA4:
>          sa = this_addr << 2; /* shift up from [xx:0] to [xx+2:2] */
> -        ea = (this_addr + 4u) - 1u;
> +        ea = (sa + 4u) - 1u;
>          break;
>
>      case PMP_AMATCH_NAPOT:
> --
> 2.17.1
>
>

Le ven. 10 juil. 2020 à 22:35, Alistair Francis <alistair23@gmail.com> a écrit :
>
> On Mon, Jul 6, 2020 at 2:45 AM Alexandre Mergnat <amergnat@baylibre.com> wrote:
> >
> > The end address calculation for NA4 mode is wrong because the address
> > used isn't shifted.
> >
> > That imply all NA4 setup are not applied by the PMP.
>
> I'm not sure what you mean here, can you clarify this?

I'm just saying NA4 configuration doesn't work properly on QEMU (It
doesn't watch 4byte but a huge range)
because the end address calculation is wrong.

>
> >
> > The solution is to use the shifted address calculated for start address
> > variable.
> >
> > Modifications are tested on Zephyr OS userspace test suite which works
> > for other RISC-V boards (E31 and E34 core).
> >
> > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
>
> Otherwise:
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> Alistair
>
> > ---
> >  target/riscv/pmp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> > index 9418660f1b..2a2b9f5363 100644
> > --- a/target/riscv/pmp.c
> > +++ b/target/riscv/pmp.c
> > @@ -171,7 +171,7 @@ static void pmp_update_rule(CPURISCVState *env, uint32_t pmp_index)
> >
> >      case PMP_AMATCH_NA4:
> >          sa = this_addr << 2; /* shift up from [xx:0] to [xx+2:2] */
> > -        ea = (this_addr + 4u) - 1u;
> > +        ea = (sa + 4u) - 1u;
> >          break;
> >
> >      case PMP_AMATCH_NAPOT:
> > --
> > 2.17.1
> >
> >

On Mon, Jul 13, 2020 at 3:10 AM Alexandre Mergnat <amergnat@baylibre.com> wrote:
>
> Le ven. 10 juil. 2020 à 22:35, Alistair Francis <alistair23@gmail.com> a écrit :
> >
> > On Mon, Jul 6, 2020 at 2:45 AM Alexandre Mergnat <amergnat@baylibre.com> wrote:
> > >
> > > The end address calculation for NA4 mode is wrong because the address
> > > used isn't shifted.
> > >
> > > That imply all NA4 setup are not applied by the PMP.
> >
> > I'm not sure what you mean here, can you clarify this?
>
> I'm just saying NA4 configuration doesn't work properly on QEMU (It
> doesn't watch 4byte but a huge range)
> because the end address calculation is wrong.

Ok, I replaced the original sentence with:

It doesn't watch 4 bytes but a huge range because the end address
calculation is wrong.

and changed the title to: target/riscv: Fix pmp NA4 implementation

and applied it to the RISC-V tree.

Alistair

>
> >
> > >
> > > The solution is to use the shifted address calculated for start address
> > > variable.
> > >
> > > Modifications are tested on Zephyr OS userspace test suite which works
> > > for other RISC-V boards (E31 and E34 core).
> > >
> > > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> >
> > Otherwise:
> >
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> >
> > Alistair
> >
> > > ---
> > >  target/riscv/pmp.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> > > index 9418660f1b..2a2b9f5363 100644
> > > --- a/target/riscv/pmp.c
> > > +++ b/target/riscv/pmp.c
> > > @@ -171,7 +171,7 @@ static void pmp_update_rule(CPURISCVState *env, uint32_t pmp_index)
> > >
> > >      case PMP_AMATCH_NA4:
> > >          sa = this_addr << 2; /* shift up from [xx:0] to [xx+2:2] */
> > > -        ea = (this_addr + 4u) - 1u;
> > > +        ea = (sa + 4u) - 1u;
> > >          break;
> > >
> > >      case PMP_AMATCH_NAPOT:
> > > --
> > > 2.17.1
> > >
> > >