Re: [PATCH v5] target/riscv: update checks on writing pmpcfg for Smepmp to version 1.0

[Alistair Francis]

On Wed, Dec 6, 2023 at 3:37 PM Alvin Chang <vivahavey@gmail.com> wrote:
>
> > -----Original Message-----
>
> > From: Alistair Francis <alistair23@gmail.com>
>
> > Sent: Wednesday, December 6, 2023 11:39 AM
>
> > To: Alvin Che-Chia Chang(張哲嘉) <alvinga@andestech.com>
>
> > Cc: qemu-riscv@nongnu.org; qemu-devel@nongnu.org;
>
> > alistair.francis@wdc.com; liweiwei@iscas.ac.cn
>
> > Subject: Re: [PATCH v5] target/riscv: update checks on writing pmpcfg for
>
> > Smepmp to version 1.0
>
> >
>
> > On Tue, Nov 14, 2023 at 12:24 PM Alvin Chang via <qemu-devel@nongnu.org>
>
> > wrote:
>
> > >
>
> > > Current checks on writing pmpcfg for Smepmp follows Smepmp version
>
> > > 0.9.1. However, Smepmp specification has already been ratified, and
>
> > > there are some differences between version 0.9.1 and 1.0. In this
>
> > > commit we update the checks of writing pmpcfg to follow Smepmp version
>
> > > 1.0.
>
> > >
>
> > > When mseccfg.MML is set, the constraints to modify PMP rules are:
>
> > > 1. Locked rules cannot be removed or modified until a PMP reset, unless
>
> > >    mseccfg.RLB is set.
>
> > > 2. From Smepmp specification version 1.0, chapter 2 section 4b:
>
> > >    Adding a rule with executable privileges that either is M-mode-only
>
> > >    or a locked Shared-Region is not possible and such pmpcfg writes are
>
> > >    ignored, leaving pmpcfg unchanged.
>
> > >
>
> > > The commit transfers the value of pmpcfg into the index of the Smepmp
>
> > > truth table, and checks the rules by aforementioned specification
>
> > > changes.
>
> > >
>
> > > Signed-off-by: Alvin Chang <alvinga@andestech.com>
>
> > > ---
>
> > > Changes from v4: Rebase on master.
>
> > >
>
> > > Changes from v3: Modify "epmp_operation" to "smepmp_operation".
>
> > >
>
> > > Changes from v2: Adopt switch case ranges and numerical order.
>
> > >
>
> > > Changes from v1: Convert ePMP over to Smepmp.
>
> > >
>
> > >  target/riscv/pmp.c | 40 ++++++++++++++++++++++++++++++++--------
>
> > >  1 file changed, 32 insertions(+), 8 deletions(-)
>
> > >
>
> > > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c index
>
> > > 162e88a90a..4069514069 100644
>
> > > --- a/target/riscv/pmp.c
>
> > > +++ b/target/riscv/pmp.c
>
> > > @@ -102,16 +102,40 @@ static bool pmp_write_cfg(CPURISCVState *env,
>
> > uint32_t pmp_index, uint8_t val)
>
> > >                  locked = false;
>
> > >              }
>
> > >
>
> > > -            /* mseccfg.MML is set */
>
> > > -            if (MSECCFG_MML_ISSET(env)) {
>
> > > -                /* not adding execute bit */
>
> > > -                if ((val & PMP_LOCK) != 0 && (val & PMP_EXEC) !=
>
> > PMP_EXEC) {
>
> > > +            /*
>
> > > +             * mseccfg.MML is set. Locked rules cannot be removed or
>
> > modified
>
> > > +             * until a PMP reset. Besides, from Smepmp specification
>
> > version 1.0
>
> > > +             * , chapter 2 section 4b says:
>
> > > +             * Adding a rule with executable privileges that either is
>
> > > +             * M-mode-only or a locked Shared-Region is not possible
>
> > and such
>
> > > +             * pmpcfg writes are ignored, leaving pmpcfg unchanged.
>
> > > +             */
>
> > > +            if (MSECCFG_MML_ISSET(env) && !pmp_is_locked(env,
>
> > > + pmp_index)) {
>
> >
>
> > This is tricky and took me a while to get my head around.
>
> >
>
> > From what I can tell, there is a bug in the spec.
>
> >
>
> > The spec specifically states that:
>
> >
>
> > """
>
> > The meaning of pmpcfg.L changes: Instead of marking a rule as locked and
>
> > enforced in all modes, it now marks a rule as M-mode-only when set and
>
> > S/U-mode-only when unset.
>
> > """
>
> >
>
> > So the check for !pmp_is_locked() sounds correct.
>
> >
>
> > But then they add:
>
> >
>
> > """
>
> > The formerly reserved encoding of pmpcfg.RW=01, and the encoding
>
> > pmpcfg.LRWX=1111, now encode a Shared-Region.
>
> > """
>
> >
>
> > Which contradicts what they just said.

In future can you please reply in plain text? Otherwise the formatting
gets a little funky

>
>
> Yes you are right, it seems there are some misleading words.
>
>
> >
>
> > I *think* we want to ignore the locked bit here. We don't actually care if 
> > it's
>
> > already set, instead we care if the region is an M-mode only region from the
>
> > 2.1 table
>
>
> The check for !pmp_is_locked() is because spec says (below table 2.1):
>
> "*Locked rules cannot be removed or modified until a PMP reset, unless 
> mseccfg.RLB is set."
>
> It is not related to M-mode-only or S/U-mode-only or Shared-Region.

Yes, but when mseccfg.MML is set

"The meaning of pmpcfg.L changes: Instead of marking a rule as locked
and enforced in all modes, it
now marks a rule as M-mode-only when set and S/U-mode-only when unset."

So the comment below table 2.1 no longer applies

>
>
> In other words, a pmpcfg where the pmpcfg.L bit was set can not be configured 
> anymore. Therefore, I think we should not ignore it here, since we are trying 
> to write a new value into the pmpcfg. If we ignore it, the locked pmpcfg will 
> be modified and it would violate the spec.
>
>
> If the pmpcfg was not locked, we also need to check the new value that the 
> user wants to write. Because chapter 2 section 4b says: "Adding a rule with 
> executable privileges that either is M-mode-only or a locked Shared-Region is 
> not possible and such pmpcfg writes are ignored, leaving pmpcfg unchanged". 
> This checking is implemented as that switch-case statement, based on table 
> 2.1 truth table.

Yeah, that sounds right.

Alistair

>
>
> Alvin Chang
>
>
> >
>
> > I think the best bet here is to create a helper function that takes a pmpcfg
>
> > value and returns if it is M-mode only. Then we should check if the current
>
> > pmp_index is M-mode only OR if we are adding one and then reject that.
>
> >
>
> > Does that make sense?
>
> >
>
> > Alistair
>
> >
>
> > > +                /*
>
> > > +                 * Convert the PMP permissions to match the truth
>
> > table in the
>
> > > +                 * Smepmp spec.
>
> > > +                 */
>
> > > +                const uint8_t smepmp_operation =
>
> > > +                    ((val & PMP_LOCK) >> 4) | ((val & PMP_READ) <<
>
> > 2) |
>
> > > +                    (val & PMP_WRITE) | ((val & PMP_EXEC) >> 2);
>
> > > +
>
> > > +                switch (smepmp_operation) {
>
> > > +                case 0 ... 8:
>
> > >                      locked = false;
>
> > > -                }
>
> > > -                /* shared region and not adding X bit */
>
> > > -                if ((val & PMP_LOCK) != PMP_LOCK &&
>
> > > -                    (val & 0x7) != (PMP_WRITE | PMP_EXEC)) {
>
> > > +                    break;
>
> > > +                case 9 ... 11:
>
> > > +                    break;
>
> > > +                case 12:
>
> > > +                    locked = false;
>
> > > +                    break;
>
> > > +                case 13:
>
> > > +                    break;
>
> > > +                case 14:
>
> > > +                case 15:
>
> > >                      locked = false;
>
> > > +                    break;
>
> > > +                default:
>
> > > +                    g_assert_not_reached();
>
> > >                  }
>
> > >              }
>
> > >          } else {
>
> > > --
>
> > > 2.34.1
>
> > >
>
> > >