|
| From: | 張哲嘉 |
| Subject: | Re: [PATCH v2] target/riscv: update checks on writing pmpcfg for Smepmp version 1.0 |
| Date: | Fri, 15 Sep 2023 10:53:36 +0800 |
> 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 <alvinga@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
>
> 20230907062440.1174224-1-mchitale@ventanamicro.com/">https://lore.kernel.org/all/20230907062440.1174224-1-mchitale@ventanamicr
20230907062440.1174224-1-mchitale@ventanamicro.com/">> o.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
| [Prev in Thread] | Current Thread | [Next in Thread] |