> On Fri, Sep 08, 2023 at 04:38:34PM +0800, Alvin Chang 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 connot 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 <alvi...@andestech.com> > > --- > > Changes from v1: Convert ePMP over to Smepmp. > > > > target/riscv/pmp.c | 51 > > ++++++++++++++++++++++++++++++++++++++-------- > > 1 file changed, 42 insertions(+), 9 deletions(-) > > > > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c index > > 9d8db493e6..d1c3fc1e4f 100644 > > --- a/target/riscv/pmp.c > > +++ b/target/riscv/pmp.c > > @@ -98,16 +98,49 @@ 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) { > > - locked = false; > > - } > > - /* shared region and not adding X bit */ > > - if ((val & PMP_LOCK) != PMP_LOCK && > > - (val & 0x7) != (PMP_WRITE | 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)) { > > + /* > > + * Convert the PMP permissions to match the truth > table in the > > + * ePMP spec. > > + */ > > + const uint8_t epmp_operation = > > + ((val & PMP_LOCK) >> 4) | ((val & PMP_READ) << > 2) | > > + (val & PMP_WRITE) | ((val & PMP_EXEC) >> 2); > > + > > + switch (epmp_operation) { > > + /* pmpcfg.L = 0. Neither M-mode-only nor locked > Shared-Region */ > > + case 0: > > + case 1: > > + case 2: > > + case 3: > > + case 4: > > + case 5: > > + case 6: > > + case 7: > > + /* pmpcfg.L = 1 and pmpcfg.X = 0 (but case 10 is not > allowed) */ > > + case 8: > > case 0 ... 8: > OK, will apply case ranges. > > + case 12: > > + case 14: > > + /* pmpcfg.LRWX = 1111 */ > > + case 15: /* Read-only locked Shared-Region on all > > + modes */ > > locked = false; > > + break; > > + /* Other rules which add new code regions are not > allowed */ > > + case 9: > > + case 10: /* Execute-only locked Shared-Region on all > modes */ > > + case 11: > > case 9 ... 11: > > And why not put these cases in numerical order? > Agree, I will put them in numerical order. > > + case 13: > > + break; > > + default: > > + g_assert_not_reached(); > > } > > } > > } else { > > -- > > 2.34.1 > > > > > > It looks like this patch has overlap with > > https://lore.kernel.org/all/20230907062440.1174224-1-mchitale@ventanamicr <https://lore.kernel.org/all/20230907062440.1174224-1-mchit...@ventanamicro.com/> > o.com/ <https://lore.kernel.org/all/20230907062440.1174224-1-mchit...@ventanamicro.com/> > > Maybe you and Mayuresh can work together on a final patch. > It seems Mayuresh's patch is to reset PMP entries and mseccfg when CPU resets. This patch is to check the valid setting of pmpcfg at runtime, when CPU supports Smepmp. I think they are two independent patches. > Thanks, > drew