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

[Alistair Francis <alistair23@gmail.com>]

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.

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

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
>
>